As StyleResolver::setupScopeStack has a bug that m_scopeStackParent is always set to be NULL, setupScopeStack is always invoked if scoped author styles exist.
Created attachment 145558 [details] Patch
Comment on attachment 145558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145558&action=review > Source/WebCore/ChangeLog:10 > + Reviewed by NOBODY (OOPS!). The "Reviewed By" line should be jut after the bug URL. See others. > Source/WebCore/css/StyleResolver.cpp:563 > for (; parent; parent = parent->parentOrHostNode()) { It would be better to introduce new local variable for this loop instead of reusing the parameter. It's generally a bad style to reuse it and that is a source of this bug. > Source/WebCore/css/StyleResolver.cpp:597 > + const ScopeStackFrame& lastFrame = m_scopeStack[m_scopeStack.size() - 1]; Vector::last() is preferred. Then it's easy to pack the conditional into the single if clause. Also, why do we need isEmpty() check?
Created attachment 146251 [details] Patch
(In reply to comment #2) > (From update of attachment 145558 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145558&action=review > > > Source/WebCore/ChangeLog:10 > > + Reviewed by NOBODY (OOPS!). > > The "Reviewed By" line should be jut after the bug URL. See others. Done. However, I think, this is the default order generated by prepare-ChangeLog script. > > Source/WebCore/css/StyleResolver.cpp:563 > > for (; parent; parent = parent->parentOrHostNode()) { > > It would be better to introduce new local variable for this loop instead of reusing the parameter. > It's generally a bad style to reuse it and that is a source of this bug. I see. Done. > > Source/WebCore/css/StyleResolver.cpp:597 > > + const ScopeStackFrame& lastFrame = m_scopeStack[m_scopeStack.size() - 1]; > > Vector::last() is preferred. Then it's easy to pack the conditional into the single if clause. Done. > Also, why do we need isEmpty() check? Yes, we need. Because pushScope doesn't add any ruleset if a scope doesn't have any rules, but m_scopeStackParent is always updated to avoid ruleset recalculation. And I also tested whether we could remove isEmpty() check or not and obtained many crash logs. Best regards, Takashi Sakamoto
Comment on attachment 146251 [details] Patch Okay, let's land this.
Comment on attachment 146251 [details] Patch Clearing flags on attachment: 146251 Committed r119782: <http://trac.webkit.org/changeset/119782>
All reviewed patches have been landed. Closing bug.