Re: [webkit-dev] Eliminate potential null pointer dereference?

[Andreas Kling]

On Sat, Apr 21, 2012 at 9:45 AM, Antti Koivisto  wrote:

> There is generally too much pointless drive-by refactoring going on in the
> project. I think we should take harder line against these "No new test /
> code cleanup only" type patches to reduce noise level.
>

 +1 to this.

Furthermore, I think part of the problem is drive-by reviews. Many of these
pseudo-cleanup patches may seem straightforward enough to basically
rubber-stamp, but if you don't know the code well enough it's better to
defer to someone who does. I doubt the appropriate reviewer(s) would have
reason to postpone trivial 2-line code reviews for too long anyway.

-Kling
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Maciej Stachowiak]

On Apr 21, 2012, at 9:45 AM, Antti Koivisto wrote:

> Sat, Apr 21, 2012 at 8:13 AM, John Yani  wrote:
> 2316if (selector->relation() != CSSSelector::SubSelector)
> 2317break;
> 2318selector = selector->tagHistory();
> 2319};
> 
> Now selector is null and we are trying to call tagHistory():
> 
> This is not possible. If selector->relation() == CSSSelector::SubSelector 
> then there will always be a subselector in tagHistory. 
>  
> 2321for (selector = selector->tagHistory(); selector; selector =
> 
> Which will result in segfault.
> 
> That would indicate a serious bug in CSS parser. The crash would allow us to 
> catch and fix the bug. Now the bug is hidden. We have also lost some 
> documentation (in form of code) on how our data structures look like. The 
> only sensible change here would have been ASSERT(selector) for documentation 
> purposes.

Or change the first loop to while(true) instead of while(selector) to make 
clear that it can't actually exit with selector being null.

 - Maciej

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Ryosuke Niwa]

Given Antti's response, I'm even less convinced that these coverity related
fixes are overall improvement (I have no doubt and am grateful that those
working on this effort is doing so on good intentions).

My suggestion is to require tests for any changes of this nature (perhaps
except uninitialized variables; although even those may have been
intentional and simply initializing variables just hide bugs) in the
future, and review any changes we've made in the past and revert them as
needed.

Also in general, let us remind ourselves of not reviewing changes we don't
understand (I'm not intending to blame any single reviewer here; just a
general comment). Had this patch been reviewed by antti on the first place,
we wouldn't be having this rather unpleasant conversation.

- Ryosuke
On Apr 21, 2012 9:45 AM, "Antti Koivisto"  wrote:

> Sat, Apr 21, 2012 at 8:13 AM, John Yani  wrote:
>
>> 2316if (selector->relation() != CSSSelector::SubSelector)
>> 2317break;
>> 2318selector = selector->tagHistory();
>> 2319};
>>
>> Now selector is null and we are trying to call tagHistory():
>>
>
> This is not possible. If selector->relation() == CSSSelector::SubSelector
> then there will always be a subselector in tagHistory.
>
>
>> 2321for (selector = selector->tagHistory(); selector; selector =
>>
>> Which will result in segfault.
>>
>
> That would indicate a serious bug in CSS parser. The crash would allow us
> to catch and fix the bug. Now the bug is hidden. We have also lost some
> documentation (in form of code) on how our data structures look like. The
> only sensible change here would have been ASSERT(selector) for
> documentation purposes.
>
> There is generally too much pointless drive-by refactoring going on in the
> project. I think we should take harder line against these "No new test /
> code cleanup only" type patches to reduce noise level.
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Antti Koivisto]

Sat, Apr 21, 2012 at 8:13 AM, John Yani  wrote:

> 2316if (selector->relation() != CSSSelector::SubSelector)
> 2317break;
> 2318selector = selector->tagHistory();
> 2319};
>
> Now selector is null and we are trying to call tagHistory():
>

This is not possible. If selector->relation() == CSSSelector::SubSelector
then there will always be a subselector in tagHistory.


> 2321for (selector = selector->tagHistory(); selector; selector =
>
> Which will result in segfault.
>

That would indicate a serious bug in CSS parser. The crash would allow us
to catch and fix the bug. Now the bug is hidden. We have also lost some
documentation (in form of code) on how our data structures look like. The
only sensible change here would have been ASSERT(selector) for
documentation purposes.

There is generally too much pointless drive-by refactoring going on in the
project. I think we should take harder line against these "No new test /
code cleanup only" type patches to reduce noise level.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[John Yani]

On 20 April 2012 08:18, Alexey Proskuryakov  wrote:
>
> I noticed a number of patches going in recently that add null checks, or 
> refactor the code under the premise of "eliminating potential null 
> dereference". What does that mean, and why are we allowing such patches to be 
> landed?
>
> For example, this change doesn't look nice: 
> .

It means that there might be a situation when selector is null after
first loop finishes:

2310 while (selector) {
2311// Allow certain common attributes (used in the default
style) in the selectors that match the current element.
2312if (selector->isAttributeSelector() &&
!isCommonAttributeSelectorAttribute(selector->attribute()))
2313return true;
2314if (selectorListContainsUncommonAttributeSelector(selector))
2315return true;
2316if (selector->relation() != CSSSelector::SubSelector)
2317break;
2318selector = selector->tagHistory();
2319};

Now selector is null and we are trying to call tagHistory():

2320
2321for (selector = selector->tagHistory(); selector; selector =
selector->tagHistory()) {
2322if (selector->isAttributeSelector())
2323return true;
2324if (selectorListContainsUncommonAttributeSelector(selector))
2325return true;
2326}

Which will result in segfault.

> We should not try to confuse ourselves with unusual for loop forms, and there 
> is no explanation at all of why these changes are needed. No regression tests 
> either.

To construct an obvious test it would be needed to move static inline
function "containsUncommonAttributeSelector" to either
CSSStyleSelector.h or a new file CSSStyleSelectorHelper.h

Then the test would be trivial:

WebCore::CSSSelector selector;
bool doesContain = WebCore::containsUncommonAttributeSelector(&selector);
ASSERT_TRUE(!doesContain);

Without the mentioned patch the above lines would result in a crash.

If your concern is only about missing explanation on why changes
required to add a test are not practical, I totally agree. Otherwise
I'm missing the point.

>
> - WBR, Alexey Proskuryakov
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Ryosuke Niwa]

On Fri, Apr 20, 2012 at 1:48 PM, Rachel Blum  wrote:

> I completely agree with Maciej here that if this is a reachable code, then
>> the patch author should put a reasonable effort into creating a test case. 
>> And
>> most importantly, these changes are clearly not "code cleanup".
>>
>
> I'm disagreeing here. (as far as NULL checks go).
>
> Unless there's a demonstrable reason that you _need_ a value
> uninitialized, why is the burden of proof on the person doing cleanup? Yes,
> at the point the code was written, it's well possible that the author was
> aware that the value would always be initialized for use. However, if code
> is added to a class, that invariant is not always checked again.
>

The burden of proof of correctness is always on the patch author in WebKit.
It doesn't matter whether the patch is a simple cleanup, adding a new
feature, or fixing a bug.

I think the confusion is over the intent of the person making the cleanup
> change. We (I speak as one of the people pushing static analysis) are not
> interested in *changing* WebKit behavior. We're interested in making sure
> behavior is deterministic. Requiring the construction of what amounts to an
> exploit for each fix for uninitialized variables seems a bit overkill :)
>
> I agree that the CHANGELOG entry should state that we deliberately didn't
> add tests. My personal policy is to propose those patches, complete with
> "No new tests/ cleanup only". If I get pushback on the review, I'm happy to
> abandon it.
>

As multiple reviewers have repeatedly stated in this thread, I don't think
"cleanup only" is an acceptable description of a change of this nature.
"cleanup only" implies that there is no behavioral change.

If we're modifying the code to make WebKit more deterministic, then there
is an observable behavior change, namely that WebKit is more deterministic
than it used to be. And in this particular case, WebKit may even have one
less crash bug. Such a behavioral change should ideally be tested. If it
cannot be tested or that coming up with a test case is too hard, then it
should be explained in the change log, and I don't consider "cleanup" as a
satisfactory explanation.

- Ryosuke
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Benjamin Poulain]

On Fri, Apr 20, 2012 at 1:48 PM, Rachel Blum  wrote:
> Unless there's a demonstrable reason that you _need_ a value uninitialized,
> why is the burden of proof on the person doing cleanup? Yes, at the point
> the code was written, it's well possible that the author was aware that the
> value would always be initialized for use. However, if code is added to a
> class, that invariant is not always checked again.

Unless totally trivial, a patch doing a "cleanup" is no different than
a patch adding a feature or fixing a bug. You should demonstrate your
change is correct.

The patch might be great, or it might be bad and blindly following a
tool. You show it is correct by adding tests or having a good
Changelog.

Benjamin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Maciej Stachowiak]

On Apr 20, 2012, at 1:48 PM, Rachel Blum wrote:

> I completely agree with Maciej here that if this is a reachable code, then 
> the patch author should put a reasonable effort into creating a test case. 
> And most importantly, these changes are clearly not "code cleanup".
> 
> I'm disagreeing here. (as far as NULL checks go).
> 
> Unless there's a demonstrable reason that you _need_ a value uninitialized, 
> why is the burden of proof on the person doing cleanup? Yes, at the point the 
> code was written, it's well possible that the author was aware that the value 
> would always be initialized for use. However, if code is added to a class, 
> that invariant is not always checked again.

I think there's a difference between null checks and initializing variables. I 
think it would be a reasonable style guideline to say that a constructor must 
initialize every data member, except possibly ones that have their own 
constructors or cases where the reason not to initialize is documented.

I don't know if I'd agree with a style guideline that says "add a null check 
everywhere that it seems like a value might be null".

> 
> I think the confusion is over the intent of the person making the cleanup 
> change. We (I speak as one of the people pushing static analysis) are not 
> interested in *changing* WebKit behavior. We're interested in making sure 
> behavior is deterministic. Requiring the construction of what amounts to an 
> exploit for each fix for uninitialized variables seems a bit overkill :)
> 
> I agree that the CHANGELOG entry should state that we deliberately didn't add 
> tests. My personal policy is to propose those patches, complete with "No new 
> tests/ cleanup only". If I get pushback on the review, I'm happy to abandon 
> it. 

I think given our current project policies, the best practice would be:

- Identify that the change was made in response to a static analyzer (and 
identify the tool)
- If the patch would cause a behavior change, make a reasonable attempt to make 
a test
- If the code path is in practice unreachable, then document that, and also 
think about whether the way it's addressed makes that clear.
- If you can't determine either way, then document that. "I don't know of a way 
to reach this code path but I can't prove it's impossible" is more accurate 
than "No new tests/cleanup only".

You mentioned in another email that you're not in favor of blindly doing 
everything the tool says, so I think that the above is a reasonable thing to 
expect as part of thinking about the tool's output.

To take the example of the last patch here, if the variable in question can't 
actually be null on exit from the first loop, the right fix would have been to 
change the loop condition rather than to add a null check. That would have made 
the code more readable for future programmers. If it is reachable, then the 
test would have helped people who do not have access to the relevant analysis 
tool, or the same level of understanding about which of its messages to obey 
and which to ignore. So I think the expectations here are reasonable.

Regards,
Maciej

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Rachel Blum]

>
> If this issue was spotted by a static analyzer, it would be good to
> mention that in the ChangeLog too.
>

We (the static analysis loons ;) usually try to do that. (The log will
contain CID= to identify the Coverity ID, and most of the time a
[Coverity] tag).

The bug that David mentioned above got Coverity a lot more attention on the
Chromium team, and quite a few people started working with it, most likely
without knowing we do that.

I apologize I didn't clearly communicate that's a best practice when I
pointed people there. Updating our documentation as we speak.

Rachel


> Regards,
> Maciej
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Rachel Blum]

>
> I completely agree with Maciej here that if this is a reachable code, then
> the patch author should put a reasonable effort into creating a test case. And
> most importantly, these changes are clearly not "code cleanup".
>

I'm disagreeing here. (as far as NULL checks go).

Unless there's a demonstrable reason that you _need_ a value uninitialized,
why is the burden of proof on the person doing cleanup? Yes, at the point
the code was written, it's well possible that the author was aware that the
value would always be initialized for use. However, if code is added to a
class, that invariant is not always checked again.

I think the confusion is over the intent of the person making the cleanup
change. We (I speak as one of the people pushing static analysis) are not
interested in *changing* WebKit behavior. We're interested in making sure
behavior is deterministic. Requiring the construction of what amounts to an
exploit for each fix for uninitialized variables seems a bit overkill :)

I agree that the CHANGELOG entry should state that we deliberately didn't
add tests. My personal policy is to propose those patches, complete with
"No new tests/ cleanup only". If I get pushback on the review, I'm happy to
abandon it.

Rachel


> On Thu, Apr 19, 2012 at 11:11 PM, David Levin  wrote:
>
>> I understand the other side as well that it would be good to figure out
>> if it is really an issue and find a test to prove it. I guess this is more
>> of what I think of as a BSD type of approach. It seems to be an area where
>> reasonable people can disagree.
>
>
> The WebKit contribution guide lists this as a requirement (
> http://www.webkit.org/coding/contributing.html):
> For any feature that affects the layout engine, a new regression test must
> be constructed. If you provide a patch that fixes a bug, that patch should
> also include the addition of a regression test that would fail without the
> patch and succeed with the patch. If no regression test is provided, the
> reviewer will ask you to revise the patch, so you can save time by
> constructing the test up front and making sure it's attached to the bug.
>
> So I don't think we can "disagree" on this topic. I'm sympathetic to the
> argument that it's hard to come up with a test case for things like this,
> but then the patch author should clearly state that in the change log, and
> most importantly the reviewer should be asking that during the review.
>
> - Ryosuke
>
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Rachel Blum]

>
> I completely agree with Maciej here that if this is a reachable code, then
> the patch author should put a reasonable effort into creating a test case. And
> most importantly, these changes are clearly not "code cleanup".
>

I'm disagreeing here. (as far as NULL checks go).

Unless there's a demonstrable reason that you _need_ a value uninitialized,
why is the burden of proof on the person doing cleanup? Yes, at the point
the code was written, it's well possible that the author was aware that the
value would always be initialized for use. However, if code is added to a
class, that invariant is not always checked again.

I think the confusion is over the intent of the person making the cleanup
change. We (I speak as one of the people pushing static analysis) are not
interested in *changing* WebKit behavior. We're not fixing existing bugs.
We're interested in making sure behavior is deterministic. Requiring the
construction of what amounts to an exploit for each fix for uninitialized
variables seems a bit overkill :)

I agree that the CHANGELOG entry should state that we deliberately didn't
add tests. My personal policy is to propose those patches, complete with
"No new tests/ cleanup only". If I get a lot of pushback on the review, I'm
happy to abandon it.

Rachel
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Rachel Blum]

> If we had a static analyzer that ran automatically as part of the WebKit
> development process, and a shared goal to get its complaints down to 0,
> then it might be reasonable to skip creating tests for issues that it
> diagnoses. But that doesn't seem to be the situation here.
>

If we ran a static analyzer as part of the process with the goal of having
cleaner code, we could have demonstrably avoided at least one bug with a
big enough impact to avoid a hot patch.

Mind, I'm not advocating doing that. I'm aware that false positives and a
lot of "noise" bugs make this a very difficult goal to achieve. What we
currently do strikes me as the better approach - we run the analyzer, and
people who actually care about that kind of stuff triage and fix the
remaining actual issues. (And believe me, we triage out quite a bit :)

Rachel
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Maciej Stachowiak]

On Apr 20, 2012, at 12:05 PM, Kentaro Hara wrote:

> +1 to the idea that we should try to add a test for each change. That
> being said, as rniwa mentioned, it is sometimes difficult to make a
> test because
> 
> (A) we do not come up with a test case
> (B) we know that the code is unreachable (e.g.
> https://bugs.webkit.org/show_bug.cgi?id=84377)
> 
> What is the consensus for such cases? As long as the change improves
> codebase and the benefit is explicitly described in ChangeLog, is it
> OK to commit the patch? Or shouldn't we try to fix a "potential" bug
> observed in static analysis?

FWIW the change cited above and its current ChangeLog seem ok to me.

In some cases, it's useful and important for a constructor to leave a data 
member uninitialized (e.g. for performance reasons), but those cases are 
exceptions and would need to be documented.

The ChangeLog also explains how the issue fixed here is unreachable in practice 
and also untestable.

If this issue was spotted by a static analyzer, it would be good to mention 
that in the ChangeLog too.

Regards,
Maciej

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Maciej Stachowiak]

Hi Luke,

I feel like you've been put on the defensive a bit here, which is unfortunate. 
I do agree with the value of readable code and "hackability" is one of the core 
values of the WebKit project. Code should indeed be meaningful to programmers. 
One thing to keep in mind though is that, by this definition, the standard of 
clean code is partly subjective. If other developers on the project do not in 
fact find changes to be more readable, then it's not actually a readability 
improvement.

As far as regression tests vs. static analyzers: the goal of regression tests 
is not only to prevent bugs from recurring but to have a shared way for the 
community as a whole to do so. A static analyzer might be an adequate 
substitute in some cases if others (a) knew about it and (b) agreed that it 
should become one of the project's testing tools. In other words, it's not the 
form of the test that's key, it's the knowledge and general availability of the 
relevant testing tool.

Finally, one additional comment on another email of yours:

On Apr 20, 2012, at 11:25 AM, Luke Macpherson wrote:
> On Fri, Apr 20, 2012 at 11:07 AM, Ryosuke Niwa  wrote:
>> Is the code reachable? It's quite possible that the code is unreachable and
>> therefore there is no way to hit that crash. Without a test, we can't answer
>> that question.
> 
> That is not rationally true. A test case can show that there is a code
> path leading to a null pointer dereference. A test cannot show that
> there are no possible code paths that lead to that state. This is
> exactly what I was getting at when explaining that the state space of
> webkit is too large to test. In this case we don't have a repro case
> that leads to that state, but that does not mean that it is not
> possible, or that the potential to crash should not be fixed.

The WebKit project's regression test policy does not require you to prove a 
universal negative, just to make a test that provides reasonable coverage, 
would have failed without the change, and passes with it. Historically as a 
project, we have not chosen to give up on testing even though perfect testing 
is infeasible.

Regards,
Maciej

On Apr 20, 2012, at 10:53 AM, Luke Macpherson wrote:

> Changing the two loops to use the same form improves readability
> because it makes it clear that the form of iteration is the same
> between the two loops. This is a very common C pattern when dealing
> with lists where the behavior changes after an element is encountered.
> The pattern is used instead of a flag because it eliminates branches
> in the code. In this case, not using the canonical two-for-loop form
> has lead the original writer to create code that is not obviously
> correct - you can’t look at this function and see that it does what it
> claims - in fact, the most obvious code path of exiting the first loop
> through the while condition directly results in a null pointer
> dereference.
> 
> Code should communicate meaning to other software engineers. It should
> allow us to reason together about the code, and to have certainty
> about what the code will do. I don’t think anyone could look at the
> existing code and be assured that it was correct. Fixing that problem
> and clearly communicating the correctness of the code is not “code
> churn” - on the contrary it is the kind of change that is vital to the
> long-term health of the codebase.
> 
> Tests are a good thing, but they are not the only thing. Consider the
> state-space of a large piece of software like webkit - it is
> essentially infinite. You can’t test every case and code path to
> ensure correctness. On the other hand, we don’t yet have the tools to
> produce a proof of correctness for webkit, and so we live somewhere
> between the two - some of our confidence comes from being able to
> reason about the correctness of the code in general, and some of our
> confidence comes from being able to test a small fraction of the state
> space.
> 
> What do we hope to achieve by adding a test when fixing a bug? To
> prevent a bug from being reintroduced into the codebase at a later
> date. This is a reasonable goal, so let’s remember that the goal is to
> prevent the bug from recurring, not to add a test for its own sake. In
> this case, the potential null pointer dereference was found using
> coverity, a static analysis tool that we run nightly. If the bug were
> to be reintroduced it is reasonable to expect that static analysis
> would be able to find it again.
> 
> Hope this helps you see where I'm coming from,
> Luke
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[David Levin]

On Fri, Apr 20, 2012 at 1:39 PM, Maciej Stachowiak  wrote:

>
> On Apr 19, 2012, at 11:11 PM, David Levin wrote:
>
> I think this all started with a lot of effort put into fixing an issue
> reported by a user where they said "the most popular online forum in
> Malaysia is broken." Then folks had to do a lot of builds (bisecting) to
> track down where the problem was introduced. Then they had to figure out
> what had broken, etc.
>
> It was mentioned (by gr...@chromium.org) that this very issue had already
> been flagged by own internal runs of coverity on chromium (including
> webkit). Now, it seemed a shame that we knew about issues in WebKit and
> were just ignoring them. It would be nice to be able to catch these issues
> faster rather than wait for a user to report it, etc. which makes the
> expense overall go up.
>
> So I believe there has been some effort invested in fixing some issues
> pointed out by coverity which is what these changes are and I believe
> coverity is mentioned in other changes of this sort.
>
> I understand the other side as well that it would be good to figure out if
> it is really an issue and find a test to prove it. I guess this is more of
> what I think of as a BSD type of approach. It seems to be an area where
> reasonable people can disagree.
>
> oth, regarding the style of this particular change, I find it unusual as
> well.
>
>
> If the change was attempting to fix an issue found by a specific static
> analyzer, then it should say so in the ChangeLog instead of "No new tests /
> code cleanup only". That goes double if a lot of such changes are being
> made, or people not in the know will be really confused (I don't think
> Alexey was the only one).
>

Totally agreed. I think that was a mistake here. (I haven't been involved
in this error but I did happen to notice other patches that mentioned
coverity. This would should have as well.)


>
> Also, the fact that it was found by a static analyzer does not exempt the
> contributor from making a reasonable effort to create a test case. We do
> accept that sometimes it's not practical and in that case it's ok to
> mention in the ChangeLog why it was not possible to make a test case.
> However, "code cleanup only" is not a reason that applies to changes made
> with the intent of producing a behavior change.
>

Yes there should have been a reasonable effort.


>
> If we had a static analyzer that ran automatically as part of the WebKit
> development process, and a shared goal to get its complaints down to 0,
> then it might be reasonable to skip creating tests for issues that it
> diagnoses. But that doesn't seem to be the situation here.
>

I wonder why there would need this criteria. Is it better to leave in
obvious wrong code?

In this case the code looked like approximately like this before the change:

while (foo) {
}

for (foo = foo->next();...)

Now either that while loop should be while (true) or else foo should be
null checked. Ideally, this code wouldn't have passed the 1st code review
(and it is unlikely that a test would have been required to do the fix at
that stage).

dave
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Maciej Stachowiak]

On Apr 19, 2012, at 11:11 PM, David Levin wrote:

> I think this all started with a lot of effort put into fixing an issue 
> reported by a user where they said "the most popular online forum in Malaysia 
> is broken." Then folks had to do a lot of builds (bisecting) to track down 
> where the problem was introduced. Then they had to figure out what had 
> broken, etc.
> 
> It was mentioned (by gr...@chromium.org) that this very issue had already 
> been flagged by own internal runs of coverity on chromium (including webkit). 
> Now, it seemed a shame that we knew about issues in WebKit and were just 
> ignoring them. It would be nice to be able to catch these issues faster 
> rather than wait for a user to report it, etc. which makes the expense 
> overall go up.
> 
> So I believe there has been some effort invested in fixing some issues 
> pointed out by coverity which is what these changes are and I believe 
> coverity is mentioned in other changes of this sort.
> 
> I understand the other side as well that it would be good to figure out if it 
> is really an issue and find a test to prove it. I guess this is more of what 
> I think of as a BSD type of approach. It seems to be an area where reasonable 
> people can disagree.
> 
> oth, regarding the style of this particular change, I find it unusual as well.

If the change was attempting to fix an issue found by a specific static 
analyzer, then it should say so in the ChangeLog instead of "No new tests / 
code cleanup only". That goes double if a lot of such changes are being made, 
or people not in the know will be really confused (I don't think Alexey was the 
only one).

Also, the fact that it was found by a static analyzer does not exempt the 
contributor from making a reasonable effort to create a test case. We do accept 
that sometimes it's not practical and in that case it's ok to mention in the 
ChangeLog why it was not possible to make a test case. However, "code cleanup 
only" is not a reason that applies to changes made with the intent of producing 
a behavior change.

If we had a static analyzer that ran automatically as part of the WebKit 
development process, and a shared goal to get its complaints down to 0, then it 
might be reasonable to skip creating tests for issues that it diagnoses. But 
that doesn't seem to be the situation here.

Regards,
Maciej


___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Shezan Baig]

On Fri, Apr 20, 2012 at 3:06 PM, Filip Pizlo  wrote:
> Someone in this thread said something about code readability. So let's
> consider that. If I see code like:
>
> if (!var) thingy();
>
> Then I will be under the impression that var might sometimes be zero and
> that thingy() might sometimes happen, and that the previous webkittens to
> touch this code had a good reason for this check.
>
> Coverity is no more accurate than testing; if it tells you that var might be
> zero than it cannot, will not, and does not give you 100% confidence that
> this is reachable.
>
> Hence if you add special cases for things that coverity warned you about,
> you are potentially doing a disservice to anyone looking at the code in the
> future. You are telling them that "var might be zero" when really you meant
> to say "I put in this check to get my tool to stop complaining".
>
> On the other hand, if you are able to construct a test that demonstrates
> reachability then you win. But if there is no test then see my previous
> paragraph.


Wouldn't an ASSERT be more helpful in this case, i.e. something like:

ASSERT(var);
thingy();

Does ASSERT make coverity stop giving out warnings?
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Filip Pizlo]

On Apr 20, 2012, at 11:40 AM, Benjamin Poulain  wrote:

> On Fri, Apr 20, 2012 at 10:53 AM, Luke Macpherson
>  wrote:
>> What do we hope to achieve by adding a test when fixing a bug? To
>> prevent a bug from being reintroduced into the codebase at a later
>> date. This is a reasonable goal, so let’s remember that the goal is to
>> prevent the bug from recurring, not to add a test for its own sake. In
>> this case, the potential null pointer dereference was found using
>> coverity, a static analysis tool that we run nightly. If the bug were
>> to be reintroduced it is reasonable to expect that static analysis
>> would be able to find it again.
> 
> I remember seeing similar patch based on Coverty, and when asked for a
> test, the author discovered the null check was useless in that place
> and the code was changed differently.
> 
> I wholeheartedly agree we should try to make tests for code changed
> based on static code analyzer. This will help making sure the change
> is meaningful, and the test would prevent regression.

+1

Someone in this thread said something about code readability. So let's consider 
that. If I see code like:

if (!var) thingy();

Then I will be under the impression that var might sometimes be zero and that 
thingy() might sometimes happen, and that the previous webkittens to touch this 
code had a good reason for this check. 

Coverity is no more accurate than testing; if it tells you that var might be 
zero than it cannot, will not, and does not give you 100% confidence that this 
is reachable. 

Hence if you add special cases for things that coverity warned you about, you 
are potentially doing a disservice to anyone looking at the code in the future. 
You are telling them that "var might be zero" when really you meant to say "I 
put in this check to get my tool to stop complaining". 

On the other hand, if you are able to construct a test that demonstrates 
reachability then you win. But if there is no test then see my previous 
paragraph. 

-F
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Kentaro Hara]

+1 to the idea that we should try to add a test for each change. That
being said, as rniwa mentioned, it is sometimes difficult to make a
test because

(A) we do not come up with a test case
(B) we know that the code is unreachable (e.g.
https://bugs.webkit.org/show_bug.cgi?id=84377)

What is the consensus for such cases? As long as the change improves
codebase and the benefit is explicitly described in ChangeLog, is it
OK to commit the patch? Or shouldn't we try to fix a "potential" bug
observed in static analysis?



On Fri, Apr 20, 2012 at 11:40 AM, Benjamin Poulain  wrote:
> On Fri, Apr 20, 2012 at 10:53 AM, Luke Macpherson
>  wrote:
>> What do we hope to achieve by adding a test when fixing a bug? To
>> prevent a bug from being reintroduced into the codebase at a later
>> date. This is a reasonable goal, so let’s remember that the goal is to
>> prevent the bug from recurring, not to add a test for its own sake. In
>> this case, the potential null pointer dereference was found using
>> coverity, a static analysis tool that we run nightly. If the bug were
>> to be reintroduced it is reasonable to expect that static analysis
>> would be able to find it again.
>
> I remember seeing similar patch based on Coverty, and when asked for a
> test, the author discovered the null check was useless in that place
> and the code was changed differently.
>
> I wholeheartedly agree we should try to make tests for code changed
> based on static code analyzer. This will help making sure the change
> is meaningful, and the test would prevent regression.
>
> Benjamin



-- 
Kentaro Hara, Tokyo, Japan (http://haraken.info)
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Benjamin Poulain]

On Fri, Apr 20, 2012 at 10:53 AM, Luke Macpherson
 wrote:
> What do we hope to achieve by adding a test when fixing a bug? To
> prevent a bug from being reintroduced into the codebase at a later
> date. This is a reasonable goal, so let’s remember that the goal is to
> prevent the bug from recurring, not to add a test for its own sake. In
> this case, the potential null pointer dereference was found using
> coverity, a static analysis tool that we run nightly. If the bug were
> to be reintroduced it is reasonable to expect that static analysis
> would be able to find it again.

I remember seeing similar patch based on Coverty, and when asked for a
test, the author discovered the null check was useless in that place
and the code was changed differently.

I wholeheartedly agree we should try to make tests for code changed
based on static code analyzer. This will help making sure the change
is meaningful, and the test would prevent regression.

Benjamin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Luke Macpherson]

On Fri, Apr 20, 2012 at 11:07 AM, Ryosuke Niwa  wrote:
> Is the code reachable? It's quite possible that the code is unreachable and
> therefore there is no way to hit that crash. Without a test, we can't answer
> that question.

That is not rationally true. A test case can show that there is a code
path leading to a null pointer dereference. A test cannot show that
there are no possible code paths that lead to that state. This is
exactly what I was getting at when explaining that the state space of
webkit is too large to test. In this case we don't have a repro case
that leads to that state, but that does not mean that it is not
possible, or that the potential to crash should not be fixed.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Ryosuke Niwa]

On Fri, Apr 20, 2012 at 10:53 AM, Luke Macpherson
wrote:
>
> Tests are a good thing, but they are not the only thing. Consider the
> state-space of a large piece of software like webkit - it is
> essentially infinite. You can’t test every case and code path to
> ensure correctness.


While I do understand where you're coming from, this is an agreed
policy. We should state why tests are absent in change logs or in bugs when
it's hard to create one putting reasonable efforts into creating one.

This is a reasonable goal, so let’s remember that the goal is to
> prevent the bug from recurring, not to add a test for its own sake. In
> this case, the potential null pointer dereference was found using
> coverity, a static analysis tool that we run nightly.


Is the code reachable? It's quite possible that the code is unreachable and
therefore there is no way to hit that crash. Without a test, we can't
answer that question.

If the bug were to be reintroduced it is reasonable to expect that static
> analysis would be able to find it again.


WebKit contributors are not required to use such a tool prior to committing
their changes at least for now, but we DO require contributors to run our
layout tests. And I don't think we want to require all contributors to run
coverity on webkit before committing their patches. Given that, there
is definitely a benefit in adding a test case for simple fixes like this.

On the other hand, I don't think it's realistic to force contributors to
come up with a test case if it's really hard since that could become a
significant development overhead as well. And in those cases where we
decide that it's too hard to create a test, we explicitly mention it in the
change log. It is this piece the particular patch ap brought up is missing.

- Ryosuke
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Luke Macpherson]

Changing the two loops to use the same form improves readability
because it makes it clear that the form of iteration is the same
between the two loops. This is a very common C pattern when dealing
with lists where the behavior changes after an element is encountered.
The pattern is used instead of a flag because it eliminates branches
in the code. In this case, not using the canonical two-for-loop form
has lead the original writer to create code that is not obviously
correct - you can’t look at this function and see that it does what it
claims - in fact, the most obvious code path of exiting the first loop
through the while condition directly results in a null pointer
dereference.

Code should communicate meaning to other software engineers. It should
allow us to reason together about the code, and to have certainty
about what the code will do. I don’t think anyone could look at the
existing code and be assured that it was correct. Fixing that problem
and clearly communicating the correctness of the code is not “code
churn” - on the contrary it is the kind of change that is vital to the
long-term health of the codebase.

Tests are a good thing, but they are not the only thing. Consider the
state-space of a large piece of software like webkit - it is
essentially infinite. You can’t test every case and code path to
ensure correctness. On the other hand, we don’t yet have the tools to
produce a proof of correctness for webkit, and so we live somewhere
between the two - some of our confidence comes from being able to
reason about the correctness of the code in general, and some of our
confidence comes from being able to test a small fraction of the state
space.

What do we hope to achieve by adding a test when fixing a bug? To
prevent a bug from being reintroduced into the codebase at a later
date. This is a reasonable goal, so let’s remember that the goal is to
prevent the bug from recurring, not to add a test for its own sake. In
this case, the potential null pointer dereference was found using
coverity, a static analysis tool that we run nightly. If the bug were
to be reintroduced it is reasonable to expect that static analysis
would be able to find it again.

Hope this helps you see where I'm coming from,
Luke
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[David Levin]

On Thu, Apr 19, 2012 at 11:36 PM, Ryosuke Niwa  wrote:

> On Thu, Apr 19, 2012 at 10:35 PM, David Barr  wrote:
>
>> Regarding style, the change homogenizes the loop constructs within that
>> method.
>
>
> I don't think that's necessary an improvement. e.g. we don't have a style
> guide that mandates it.
>
> I completely agree with Maciej here that if this is a reachable code, then
> the patch author should put a reasonable effort into creating a test case. And
> most importantly, these changes are clearly not "code cleanup".
>
> On Thu, Apr 19, 2012 at 11:11 PM, David Levin  wrote:
>
>> I understand the other side as well that it would be good to figure out
>> if it is really an issue and find a test to prove it. I guess this is more
>> of what I think of as a BSD type of approach. It seems to be an area where
>> reasonable people can disagree.
>
>
> The WebKit contribution guide lists this as a requirement (
> http://www.webkit.org/coding/contributing.html):
> For any feature that affects the layout engine, a new regression test must
> be constructed. If you provide a patch that fixes a bug, that patch should
> also include the addition of a regression test that would fail without the
> patch and succeed with the patch. If no regression test is provided, the
> reviewer will ask you to revise the patch, so you can save time by
> constructing the test up front and making sure it's attached to the bug.
>
> So I don't think we can "disagree" on this topic. I'm sympathetic to the
> argument that it's hard to come up with a test case for things like this,
> but then the patch author should clearly state that in the change log, and
> most importantly the reviewer should be asking that during the review.
>

You seem to be a bit confrontational here. I'm not sure why.

I was talking about about doing a patch for something where one wasn't able
to find a repro but it appeared like an issue might be there. Not whether
the changelog should say that or not. It may be good to ask a clarifying
question if something is unclear as opposed to responding like this.

dave
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Ryosuke Niwa]

On Thu, Apr 19, 2012 at 10:35 PM, David Barr  wrote:
>
> Regarding style, the change homogenizes the loop constructs within that
> method.


I don't think that's necessary an improvement. e.g. we don't have a style
guide that mandates it.

I completely agree with Maciej here that if this is a reachable code, then
the patch author should put a reasonable effort into creating a test case. And
most importantly, these changes are clearly not "code cleanup".

On Thu, Apr 19, 2012 at 11:11 PM, David Levin  wrote:

> I understand the other side as well that it would be good to figure out if
> it is really an issue and find a test to prove it. I guess this is more of
> what I think of as a BSD type of approach. It seems to be an area where
> reasonable people can disagree.


The WebKit contribution guide lists this as a requirement (
http://www.webkit.org/coding/contributing.html):
For any feature that affects the layout engine, a new regression test must
be constructed. If you provide a patch that fixes a bug, that patch should
also include the addition of a regression test that would fail without the
patch and succeed with the patch. If no regression test is provided, the
reviewer will ask you to revise the patch, so you can save time by
constructing the test up front and making sure it's attached to the bug.

So I don't think we can "disagree" on this topic. I'm sympathetic to the
argument that it's hard to come up with a test case for things like this,
but then the patch author should clearly state that in the change log, and
most importantly the reviewer should be asking that during the review.

- Ryosuke
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[David Levin]

I think this all started with a lot of effort put into fixing an issue
reported by a user where they said "the most popular online forum in
Malaysia is broken." Then folks had to do a lot of builds (bisecting) to
track down where the problem was introduced. Then they had to figure out
what had broken, etc.

It was mentioned (by gr...@chromium.org) that this very issue had already
been flagged by own internal runs of coverity on chromium (including
webkit). Now, it seemed a shame that we knew about issues in WebKit and
were just ignoring them. It would be nice to be able to catch these issues
faster rather than wait for a user to report it, etc. which makes the
expense overall go up.

So I believe there has been some effort invested in fixing some issues
pointed out by coverity which is what these changes are and I believe
coverity is mentioned in other changes of this sort.

I understand the other side as well that it would be good to figure out if
it is really an issue and find a test to prove it. I guess this is more of
what I think of as a BSD type of approach. It seems to be an area where
reasonable people can disagree.

oth, regarding the style of this particular change, I find it unusual as
well.

dave
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[Maciej Stachowiak]

On Apr 19, 2012, at 10:35 PM, David Barr wrote:

> On Fri, Apr 20, 2012 at 3:18 PM, Alexey Proskuryakov  wrote:
>> 
>> I noticed a number of patches going in recently that add null checks, or 
>> refactor the code under the premise of "eliminating potential null 
>> dereference". What does that mean, and why are we allowing such patches to 
>> be landed?
> 
> When there are known conditions for which a value is null that we
> would otherwise dereference, check first and take an alternate path in
> the null case.
> 
>> For example, this change doesn't look nice: 
>> .
>>  We should not try to confuse ourselves with unusual for loop forms, and 
>> there is no explanation at all of why these changes are needed. No 
>> regression tests either.
> 
> Style aside, it is quite clear that the one of the termination
> conditions for the first loop is selector == null.
> So the second loop ought to check selector != null before any
> dereference of selector.

If that code path is reachable, then it should be possible to construct a test, 
and the claim that there's no new tests because it's code cleanup only is 
wrong. If it is not reachable, then your argument does not apply

> 
> Regarding style, the change homogenizes the loop constructs within that 
> method.
> 

That seems subjective. Making a bunch of these tiny changes that are not tied 
to an actual change in behavior or a clear-cut broader goal has some downsides:
- Makes it harder to study history
- Causes needless extra build thrash for people and buildbots
- Creates risk of accidentally introducing a bug

I don't have a strong opinion on this one, but if a bunch of these changes are 
landing, then either they need tests if they fix real bugs, or they should be 
related to some more concrete goal than just "code cleanup".

Regards,
Maciej

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Eliminate potential null pointer dereference?

[David Barr]

On Fri, Apr 20, 2012 at 3:18 PM, Alexey Proskuryakov  wrote:
>
> I noticed a number of patches going in recently that add null checks, or 
> refactor the code under the premise of "eliminating potential null 
> dereference". What does that mean, and why are we allowing such patches to be 
> landed?

When there are known conditions for which a value is null that we
would otherwise dereference, check first and take an alternate path in
the null case.

> For example, this change doesn't look nice: 
> .
>  We should not try to confuse ourselves with unusual for loop forms, and 
> there is no explanation at all of why these changes are needed. No regression 
> tests either.

Style aside, it is quite clear that the one of the termination
conditions for the first loop is selector == null.
So the second loop ought to check selector != null before any
dereference of selector.

Regarding style, the change homogenizes the loop constructs within that method.

--
David Barr
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

[webkit-dev] Eliminate potential null pointer dereference?

[Alexey Proskuryakov]

I noticed a number of patches going in recently that add null checks, or 
refactor the code under the premise of "eliminating potential null 
dereference". What does that mean, and why are we allowing such patches to be 
landed?

For example, this change doesn't look nice: 
.
 We should not try to confuse ourselves with unusual for loop forms, and there 
is no explanation at all of why these changes are needed. No regression tests 
either.

- WBR, Alexey Proskuryakov

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev