[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" <koivisto at iki.fi> wrote:

> Sat, Apr 21, 2012 at 8:13 AM, John Yani <vanuan at gmail.com> wrote:
>
>> 2316            if (selector->relation() != CSSSelector::SubSelector)
>> 2317                break;
>> 2318            selector = 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.
>
>
>> 2321        for (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 at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20120421/98a655be/attachment.html>