On 20 April 2012 08:18, Alexey Proskuryakov <a...@webkit.org> 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 > 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