REGRESSION (r174245): Leaks of selector lists in CSS parsing
Created attachment 245524 [details] Patch
Comment on attachment 245524 [details] Patch Attachment 245524 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4614272839581696 New failing tests: fast/css/parsing-css-nth-child-of-3.html fast/css/parsing-css-nth-child-of-4.html fast/css/parsing-css-nth-last-child-of-3.html fast/css/parsing-css-nth-last-child-of-4.html
Created attachment 245526 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 245524 [details] Patch Attachment 245524 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4873772884557824 New failing tests: fast/css/parsing-css-nth-child-of-3.html fast/css/parsing-css-nth-last-child-of-4.html fast/css/parsing-css-nth-last-child-of-3.html fast/css/parsing-css-nth-child-of-4.html
Created attachment 245527 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 245546 [details] Patch
Created attachment 245547 [details] Patch
The magic invalidSelectorList value made this much harder than I expected. This is really messy, and we should look for a better solution eventually, but I think my latest patch does correctly fix the problem.
Comment on attachment 245547 [details] Patch Clearing flags on attachment: 245547 Committed r179258: <http://trac.webkit.org/changeset/179258>
All reviewed patches have been landed. Closing bug.
Comment on attachment 245547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245547&action=review > Source/WebCore/css/CSSGrammar.y.in:1509 > + if ($4 != invalidSelectorVector) { I don't understand this check. An invalid :not() should have a null selector list, not an invalid one.
Comment on attachment 245547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245547&action=review >> Source/WebCore/css/CSSGrammar.y.in:1509 >> + if ($4 != invalidSelectorVector) { > > I don't understand this check. An invalid :not() should have a null selector list, not an invalid one. You’re right. This check isn’t needed. Sorry! Feel free to remove it. I’d like to eventually follow this up with some more elegant scheme. I feel like the nth_selector_ending part of this is even more of a tightrope walk than the already-tricky lifetime management in this parser. Maybe we can require a newer version of yacc/bison and use fancier %union that works with C++ objects? Or find some other way to make this a little more foolproof. Maybe we should generate the parser with a completely different technology rather than yacc?
(In reply to comment #12) > Comment on attachment 245547 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245547&action=review > > >> Source/WebCore/css/CSSGrammar.y.in:1509 > >> + if ($4 != invalidSelectorVector) { > > > > I don't understand this check. An invalid :not() should have a null selector list, not an invalid one. > > You’re right. This check isn’t needed. Sorry! Feel free to remove it. > > I’d like to eventually follow this up with some more elegant scheme. I feel > like the nth_selector_ending part of this is even more of a tightrope walk > than the already-tricky lifetime management in this parser. Maybe we can > require a newer version of yacc/bison and use fancier %union that works with > C++ objects? Or find some other way to make this a little more foolproof. > Maybe we should generate the parser with a completely different technology > rather than yacc? "nth_selector_ending" was not my favorite option. I wanted to separate the two :nth-child() at the "pseudo" level but bison refused to shift that for an unknown reason. Modern bison supports real LR(1) in addition LARL. That would simplify a bunch of stuff but LR(1) also create giant state machines so it may be terrible for performance. Personally, I would like us to use an hybrid LL parser for various reasons. Google is writing a new CSS parser for blink, I am curious to see what they do.