[webkit-dev] Eliminate potential null pointer dereference?

[David Levin]

On Fri, Apr 20, 2012 at 1:39 PM, Maciej Stachowiak <mjs at apple.com> 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 groby at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20120420/477d3c86/attachment.html>