On 20 April 2012 08:18, Alexey Proskuryakov <[email protected]> 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:
> <http://trac.webkit.org/changeset/114678/trunk/Source/WebCore/css/CSSStyleSelector.cpp>.
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.
2312 if (selector->isAttributeSelector() &&
!isCommonAttributeSelectorAttribute(selector->attribute()))
2313 return true;
2314 if (selectorListContainsUncommonAttributeSelector(selector))
2315 return true;
2316 if (selector->relation() != CSSSelector::SubSelector)
2317 break;
2318 selector = selector->tagHistory();
2319 };
Now selector is null and we are trying to call tagHistory():
2320
2321 for (selector = selector->tagHistory(); selector; selector =
selector->tagHistory()) {
2322 if (selector->isAttributeSelector())
2323 return true;
2324 if (selectorListContainsUncommonAttributeSelector(selector))
2325 return 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
> [email protected]
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
_______________________________________________
webkit-dev mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
