REGRESSION (r173698): Leaks of selector lists in CSS parsing

• Component: CSS
• Status: RESOLVED FIXED

[Darin Adler]

REGRESSION (r174245): Leaks of selector lists in CSS parsing

[Darin Adler]

Created attachment 245524 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

Created attachment 245546 [details]
Patch

[Darin Adler]

Created attachment 245547 [details]
Patch

[Darin Adler]

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.

[WebKit Commit Bot]

Comment on attachment 245547 [details]
Patch

Clearing flags on attachment: 245547

Committed r179258: <http://trac.webkit.org/changeset/179258>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

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.

[Darin Adler]

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?

[Benjamin Poulain]

(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.