Bug 34067

Summary: CSS rule containing a string unclosed at the end of stylesheet is ignored.
Product: WebKit Reporter: Yuzo Fujishima <yuzo>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: abarth, ap, darin, dglazkov, eoconnor, eric, gustavo, hamaji, hayato, jasneet, phiw2, robert, rwlbuis, simon.fraser, tonikitoo, tonyg, webkit, webkit-ews, webkit.review.bot, xan.lopez, yuzo, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://test.csswg.org/suites/css2.1/20110111/html4/eof-003.htm
Bug Depends on:    
Bug Blocks: 47141, 6891    
Attachments:
Description Flags
Testcase
none
Change flex rule such that unclosed string is properly closed.
none
Fixed the test case - in standards mode now.
none
Tests equivalent to those in the patch
none
Parsing unclosed strings - fixed one test case and more cases
none
Change flex rule such that unclosed string is properly closed.
none
New testcase
none
Change flex rule such that unclosed string/url is properly handled.
none
Change flex rule such that unclosed string/url is properly handled.
none
Change flex rule such that unclosed string/url is properly handled.
none
Change flex rule such that unclosed string/url is properly handled.
none
Change flex rule such that unclosed string/url is properly handled.
none
Fix unclosed string/url handling.
none
Fix unclosed string/url handling.
none
Fix unclosed string/url handling.
none
Patch
none
Pre Source/WebCore move none

Description Yuzo Fujishima 2010-01-24 21:45:47 PST
http://www.w3.org/TR/CSS21/syndata.html#parsing-errors
specifies how unclosed strings must be handled.

For example,

<style type="text/css">
p:before {
    content: 'Has not 
</style>

must be handled as if it were:

<style type="text/css">
p:before {
    content: 'Has not ';
}
</style>
Comment 1 Yuzo Fujishima 2010-01-24 21:47:04 PST
Created attachment 47310 [details]
Testcase
Comment 2 Yuzo Fujishima 2010-01-24 21:57:00 PST
Created attachment 47312 [details]
Change flex rule such that unclosed string is properly closed.
Comment 3 Yuzo Fujishima 2010-01-24 22:00:09 PST
I think 6891 cannot be fixed until the unclosed string handling is fixed.

Yuzo
Comment 4 Eric Seidel (no email) 2010-01-26 15:01:07 PST
Comment on attachment 47312 [details]
Change flex rule such that unclosed string is properly closed.

How does our current behavior compare to FF and IE?
Comment 5 Philippe Wittenbergh 2010-01-26 16:21:01 PST
I think this bug is invalid.

The testcase is in quirks mode; once made into a standard mode test (with <!doctype html>), I get the following results

Firefox (Gecko 1.9.0 ~ 19.3a1pre) --> only show 'failed'
Opera 10.5a --> same as Gecko
IE8 --> same as Gecko.

testcase:
http://dev.l-c-n.com/CSS2_parse/parse_content2.html
(there are a few more tests where WebKit show different behaviour than Gecko & Opera in the same /CSS2_parse/ folder; I haven't fully pondered them )

See the bottom of the section, under the bullet 'Unexpected end of string'
http://www.w3.org/TR/CSS21/syndata.html#parsing-errors
Comment 6 Robert Blaut 2010-01-26 22:49:30 PST
Created attachment 47498 [details]
Fixed the test case - in standards mode now.
Comment 7 Robert Blaut 2010-01-26 22:56:12 PST
Yuzo, you are talking about unclosed strings but your test case shows a case with unexpected end of style sheets. 

Compare this clause:

"Unexpected end of style sheet.
User agents must close all open constructs (for example: blocks, parentheses, brackets, rules, strings, and comments) at the end of the style sheet. For example:

  @media screen {
    p:before { content: 'Hello
would be treated the same as:

  @media screen {
    p:before { content: 'Hello'; }
  }
in a conformant UA."



with this:


"Unexpected end of string.
User agents must close strings upon reaching the end of a line, but then drop the construct (declaration or rule) in which the string was found. For example:


      p {
        color: green;
        font-family: 'Courier New Times
        color: red;
        color: green;
      }
...would be treated the same as:


      p { color: green; color: green; }
...because the second declaration (from 'font-family' to the semicolon after 'color: red') is invalid and is dropped."

Both cases are different. 

So I assume the bug report title is incorrect.

The test case uploaded by me is rendered correctly by Opera 10.50 alfa only.

WebKit and Firefox 3.6 fails in this case.
Comment 8 Robert Blaut 2010-01-26 22:59:16 PST
(In reply to comment #5)
> testcase:
> http://dev.l-c-n.com/CSS2_parse/parse_content2.html

Philip, your test case is invalid now. Check mismatched selector div:before. I assume you mean p:before.
Comment 9 Philippe Wittenbergh 2010-01-26 23:11:50 PST
(In reply to comment #8)
> (In reply to comment #5)
> > testcase:
> > http://dev.l-c-n.com/CSS2_parse/parse_content2.html
> 
> Philip, your test case is invalid now. Check mismatched selector div:before. I
> assume you mean p:before.
Oopsie, uploaded the wrong one. Corrected. Thanks.
Comment 10 Yuzo Fujishima 2010-01-26 23:50:30 PST
Thanks. Changed the bug summary. Is this good enough?

BTW, I'd like to keep test 1 and 2 in the patch, to assure that
the patch doesn't break them, although only test 3 is for
"unclosed at the end of stylesheet" case.
Comment 11 Yuzo Fujishima 2010-01-26 23:53:19 PST
Eric,

The following are test results.

Safari 4.0 WITH this patch passes all of them.

The attached Testcases, both quirks and standards modes:
IE 8.0: FAIL
FF 3.5: FAIL
Opera 10.10: PASS
Safari 4.0 WITHOUT this patch: FAIL

Test 1 and 2 in the patch (as far as I can tell by watching the screen):
IE 8.0: FAIL
FF 3.5: PASS
Opera 10.10: PASS
Safari 4.0 WITHOUT this patch: PASS

Test 3 (ditto):
IE 8.0: FAIL
FF 3.5: FAIL
Opera 10.10: FAIL
Safari 4.0 WITHOUT this patch: FAIL
(Opera fails to handle unclosed string for font-family property but can handle it for content property. Hmm.)
Comment 12 Robert Blaut 2010-01-27 00:00:20 PST
(In reply to comment #11)
> The following are test results.

Yuzo, could you link to all mentioned test cases? I'd like to check them.
Comment 13 Yuzo Fujishima 2010-01-27 00:39:46 PST
Created attachment 47505 [details]
Tests equivalent to those in the patch

These tests should be meaningful for most browsers.
Comment 14 Yuzo Fujishima 2010-01-27 00:44:18 PST
Hi, Robert, Eric,

I've attached the tests ("Tests equivalent to those in the patch")
for other browsers.

Correction:
Test 3 passes for IE 8.0.

Test 3
IE 8.0: PASS
FF 3.5: FAIL
Opera 10.10: FAIL
Safari 4.0 WITHOUT this patch: FAIL

Yuzo
Comment 15 Robert Blaut 2010-01-27 04:31:51 PST
Created attachment 47517 [details]
Parsing unclosed strings - fixed one test case and more cases

Yuzo,

The test case 3 is incorrect and IE renders it incorrectly. Firefox and Opera are right in this case. Consider this fragment:

font-family: 'Arial
</style>

It means that there is not only 'Arial' word but also new line in a string. So above code is equivalent to:

font-family: 'Arial\';
} 

I've corrected your test cases and added 2 more.

Only Firefox 3.6 and Opera 10.50 renders it correctly. WebKit fails in tests: 3,4,5.
Comment 16 Robert Blaut 2010-01-27 04:34:51 PST
Also notice the other bug visible in WebKit in test 3.

WebKit treats:

font-family: 'Arial\';

as

font-family: serif;

Webkit should match no font-family in this case.
Comment 17 Yuzo Fujishima 2010-01-28 03:46:50 PST
Created attachment 47602 [details]
Change flex rule such that unclosed string is properly closed.
Comment 18 Yuzo Fujishima 2010-01-28 03:49:07 PST
Created attachment 47603 [details]
New testcase
Comment 19 WebKit Review Bot 2010-01-28 03:54:14 PST
Attachment 47602 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSParser.cpp:5344:  yy_more_flag is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/css/CSSParser.cpp:5345:  yy_more_len is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Yuzo Fujishima 2010-01-28 03:59:49 PST
I've changed the patch such that WebKit behaves similarly to other browsers.

New testcase results: (test 1, test 2, test 3, test 4)

IE 8.0:      (FAIL, FAIL, PASS, FAIL)
FF 3.6:      (PASS, PASS, PASS, PASS)
Opera 10.10: (PASS, PASS, PASS, PASS)

r53468:      (PASS, PASS, FAIL, PASS)
This patch:  (PASS, PASS, PASS, PASS)

Yuzo
Comment 21 WebKit Review Bot 2010-01-28 05:00:21 PST
Attachment 47602 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/216671
Comment 22 WebKit Review Bot 2010-01-28 05:00:34 PST
Attachment 47602 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/216673
Comment 23 Yuzo Fujishima 2010-01-28 14:08:16 PST
Comment on attachment 47602 [details]
Change flex rule such that unclosed string is properly closed.

Sorry, this patch contains a few issues. Make it obsolete.
Comment 24 Yuzo Fujishima 2010-01-28 23:58:34 PST
Created attachment 47684 [details]
Change flex rule such that unclosed string/url is properly handled.
Comment 25 Yuzo Fujishima 2010-01-29 00:02:16 PST
Hi, reviewers,

Can you review the latest patch?

Yuzo
Comment 26 WebKit Review Bot 2010-01-29 00:03:11 PST
Attachment 47684 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSParser.cpp:5344:  yy_more_flag is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/css/CSSParser.cpp:5345:  yy_more_len is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Eric Seidel (no email) 2010-02-01 15:35:37 PST
Comment on attachment 47684 [details]
Change flex rule such that unclosed string/url is properly handled.

Please explain the changes to fast/css/font_property_normal.html in the ChangeLog.
Comment 28 Yuzo Fujishima 2010-02-01 17:49:10 PST
Hi, Eric,

Thank you for the review.

LayoutTests/Changelog contains:
    Note that LayoutTests/fast/css/font_property_normal.html is changed because it has contained wrong quotes.

Do you need more detailed description?

Yuzo
Comment 29 Robert Blaut 2010-02-01 23:22:47 PST
(In reply to comment #27)
> (From update of attachment 47684 [details])
> Please explain the changes to fast/css/font_property_normal.html in the
> ChangeLog.

There are quotes inserted there by mistake. Probably from copy and paste of https://bugs.webkit.org/show_bug.cgi?id=5564#c1 These quotes are now incorrectly ignored by WebKit. 

This stylesheet:
 
<STYLE type="text/css">
.one {font: 24pt italic;"}
.two {font: 24pt italic Arial;}
.three {font: 24pt italic 'Arial';}
.four {font: italic 24pt;}
.five {font: italic 24pt Arial;"}
.six {font: italic 24pt 'Arial';}
.seven {font: Arial 24pt italic;}
.eight {font: 'Arial' 24pt italic;}
.nine {font: Arial italic 24pt;}
.ten {font: 'Arial' italic 24pt;}
</STYLE>

according CSS 2.1 - http://www.w3.org/TR/CSS21/syndata.html#block - "Single (') and double quotes (") must also occur in matching pairs, and characters between them are parsed as a string." is equivalent to this stylesheet:

<STYLE type="text/css">
.one {font: 24pt italic;}
.six {font: italic 24pt 'Arial';}
.seven {font: Arial 24pt italic;}
.eight {font: 'Arial' 24pt italic;}
.nine {font: Arial italic 24pt;}
.ten {font: 'Arial' italic 24pt;}
</STYLE>

After applying Yuzo patch the above mentioned test case should be fixed.
Comment 30 Yuzo Fujishima 2010-02-07 22:35:33 PST
Created attachment 48317 [details]
Change flex rule such that unclosed string/url is properly handled.
Comment 31 Yuzo Fujishima 2010-02-07 22:38:22 PST
Hi, Eric,

Can you take another look?

As to fast/css/font_property_normal.html, I've quoted Robert's comment above.
As to the style error, I believe we should ignore it because the names are defined by flex.

Yuzo
Comment 32 WebKit Review Bot 2010-02-07 22:41:14 PST
Attachment 48317 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSParser.cpp:5344:  yy_more_flag is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/css/CSSParser.cpp:5345:  yy_more_len is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 33 Yuzo Fujishima 2010-02-10 01:37:04 PST
As to the style check failure,
please see https://bugs.webkit.org/show_bug.cgi?id=34787
Comment 34 Yuzo Fujishima 2010-02-11 21:39:26 PST
Created attachment 48621 [details]
Change flex rule such that unclosed string/url is properly handled.
Comment 35 Yuzo Fujishima 2010-02-11 21:40:26 PST
Added NOLINT for yy_* identifiers.
Comment 36 Darin Adler 2010-02-12 11:17:23 PST
Comment on attachment 48621 [details]
Change flex rule such that unclosed string/url is properly handled.

> -typedef int yy_state_type;
> +typedef int yy_state_type; // NOLINT

What tool respects these comments? I have not seen them in WebKit before.

> +#define yytext_ptr yytext

Why is this added? The change log has no mention of it. Is it related to the rest of the changes somehow?

> +static int yy_more_flag = 0; // NOLINT
> +static int yy_more_len = 0; // NOLINT

Why are we now defining these here? The change log does not explain how this change relates to what the patch is doing.

> +#define yymore() ((yy_more_flag) = 1)

Why is this a macro instead of a function? Why does it have a bison/flex style name?

> +#define dquoted_string 3
> +#define squoted_string 4
> +#define uri 5
> +#define uri_pending 6

Why are you using underscores in these names? Can't these follow normal WebKit naming?

> +#define YY_START (((yy_start) - 1) / 2)

What is this for?

> +    int string_caller = INITIAL;
> +    int uri_caller = INITIAL;
> +    int content_length = 0;
> +    int content_offest = 0;

Can't these follow normal WebKit naming conventions? I don't see any reason to use a different naming style here.

These names seem a bit vague to me. What specific content is the "content" in "content offset"? I think we can do better naming this.

I also see this code twice. Once in maketokenizer and once in the flex file. Is that correct?

> +    BEGIN(string_caller == uri ? uri_pending : string_caller);

I'm not sure that "caller" is the clearest term we could use use here. Maybe "token" would be clearer (or perhaps "context" or "type"). I don't really understand what these represent, which I suppose is why I don't get what the names should be.

> +    default:
> +        yyterminate();

I think ASSERT_NOT_REACHED is more appropriate here than yyterminate. Unless this code can be reached.

Do the new test cases cover all the code paths here? If I put an ASSERT_NOT_REACHED in each rule, would I have to remove them all to pass the test?

I'm going to say review- for now because I think the new code is not well enough documented, does not follow WebKit style as much as it should, may not be well tested, and is hard for me to understand despite that fact that I am familiar with both CSS and flex. Maybe you can't solve all of those problems, but please try to solve at least some of them.
Comment 37 Yuzo Fujishima 2010-02-15 23:45:15 PST
Created attachment 48792 [details]
Change flex rule such that unclosed string/url is properly handled.
Comment 38 Yuzo Fujishima 2010-02-15 23:49:10 PST
Hi, Darin,

Thank you for reviewing this!

(In reply to comment #36)
> (From update of attachment 48621 [details])
> > -typedef int yy_state_type;
> > +typedef int yy_state_type; // NOLINT
>
> What tool respects these comments? I have not seen them in WebKit before.

WebKitTools/Scripts/check-webkit-style (more specifically,
WebKitTools/Scripts/webkitpy/style/processors/cpp.py) respects them.

Now that https://bugs.webkit.org/show_bug.cgi?id=34787 is closed,
I removed the comments.

>
> > +#define yytext_ptr yytext
>
> Why is this added? The change log has no mention of it. Is it related to the
> rest of the changes somehow?

This is needed for flex to support yymore().

I've moved the definition to WebCore/css/maketokenizer.

>
> > +static int yy_more_flag = 0; // NOLINT
> > +static int yy_more_len = 0; // NOLINT
>
> Why are we now defining these here? The change log does not explain how this
> change relates to what the patch is doing.

I've updated the Change log.
Also, I've moved yy_more_{flag,len} to WebCore/css/maketokenizer.

>
> > +#define yymore() ((yy_more_flag) = 1)
>
> Why is this a macro instead of a function? Why does it have a bison/flex style
> name?

yymore() is a flex function (or macro):
http://flex.sourceforge.net/manual/Actions.html#index-yymore_0028_0029-115

>
> > +#define dquoted_string 3
> > +#define squoted_string 4
> > +#define uri 5
> > +#define uri_pending 6
>
> Why are you using underscores in these names? Can't these follow normal WebKit
> naming?

They are flex start condition names.
I've chosen to use underscores because mediaquery and forkeyword
are not in CamelCase.

>
> > +#define YY_START (((yy_start) - 1) / 2)
>
> What is this for?

This is now needed because I use YY_START in WebCore/css/tokenizer.flex.
http://flex.sourceforge.net/manual/Start-Conditions.html#Start-Conditions

I've moved it to WebCore/css/maketokenizer.

>
> > +    int string_caller = INITIAL;
> > +    int uri_caller = INITIAL;
> > +    int content_length = 0;
> > +    int content_offest = 0;
>
> Can't these follow normal WebKit naming conventions? I don't see any reason to
> use a different naming style here.

They are used in tokenizer.flex where underscored names are used.

I can change them to stringCaller, etc., if you prefer.

>
> These names seem a bit vague to me. What specific content is the "content" in
> "content offset"? I think we can do better naming this.

OK, renamed content to string_or_uri_content.

>
> I also see this code twice. Once in maketokenizer and once in the flex file. Is
> that correct?

I found that I can remove the ones in tokenizer.flex. Removed.

>
> > +    BEGIN(string_caller == uri ? uri_pending : string_caller);
>
> I'm not sure that "caller" is the clearest term we could use use here. Maybe
> "token" would be clearer (or perhaps "context" or "type"). I don't really
> understand what these represent, which I suppose is why I don't get what the
> names should be.

http://flex.sourceforge.net/manual/Start-Conditions.html#Start-Conditions
uses *_caller naming. That's why I named the variables that way.
I'm open to changing the names if you prefer otherwise.

>
> > +    default:
> > +        yyterminate();
>
> I think ASSERT_NOT_REACHED is more appropriate here than yyterminate. Unless
> this code can be reached.
>
> Do the new test cases cover all the code paths here? If I put an
> ASSERT_NOT_REACHED in each rule, would I have to remove them all to pass the
> test?

The default case is reached if the start condition is either
INITIAL, mediaquery, or forkeyword.

>
> I'm going to say review- for now because I think the new code is not well
> enough documented, does not follow WebKit style as much as it should, may not
> be well tested, and is hard for me to understand despite that fact that I am
> familiar with both CSS and flex. Maybe you can't solve all of those problems,
> but please try to solve at least some of them.
Comment 39 Yuzo Fujishima 2010-02-17 00:49:59 PST
Created attachment 48868 [details]
Change flex rule such that unclosed string/url is properly handled.
Comment 40 Yuzo Fujishima 2010-02-17 00:50:55 PST
Added explanation to WebCore/ChangeLog.
Comment 41 Yuzo Fujishima 2010-03-08 00:29:31 PST
Ping? This is blocking 6891.
Comment 42 Yuzo Fujishima 2010-03-14 23:43:02 PDT
Ping again.
Comment 43 Eric Seidel (no email) 2010-03-15 13:16:54 PDT
Tip for the future:

Information is always more useful in the ChangeLog than in the bug.  Because ChangeLogs are what reviewers use during reviews, bugs (like this one) tend to get crowded and hard to read.

In this case, I had to go digging to find if this made us match FF/IE

A comment in the ChangeLog that this makes our behavior match various other browsers (listing which ones and how) would make this review easier.
Comment 44 Eric Seidel (no email) 2010-03-15 13:18:21 PDT
Basically in posting patches for review, you want to make it as easy as possible for a reviewer to say yes.  Passing style, passing EWS bots, having tests, having a nice long ChangeLog, saying nice things in the ChangeLog like "fixes crash" or "matches other browsers", all make hitting that r+ button very easy. :)
Comment 45 Eric Seidel (no email) 2010-03-15 13:21:22 PDT
Comment on attachment 48868 [details]
Change flex rule such that unclosed string/url is properly handled.

As much as I woudl like to r+ this, I do not trust my yacc skills enough to give this a final r+.  In general it looks good though.
Comment 46 Adam Barth 2010-03-22 09:08:03 PDT
I wanted to review this patch, but was unable to.  It's unclear to me whether you actually address Darin's comments.  You seemed to reject all of them, but I'm not sure whether he was satisfied with your responses.

Who's a good person to review changes to the CSS parser?
Comment 47 Yuzo Fujishima 2010-03-31 23:39:38 PDT
Hi, Darin,

Would you mind taking another look?

Yuzo
Comment 48 Darin Adler 2010-04-06 09:20:12 PDT
(In reply to comment #38)
> > > +#define yymore() ((yy_more_flag) = 1)
> >
> > Why is this a macro instead of a function? Why does it have a bison/flex style
> > name?
> 
> yymore() is a flex function (or macro):
> http://flex.sourceforge.net/manual/Actions.html#index-yymore_0028_0029-115

You answered my second question, but ignored the first. Why is this a macro instead of a function? Please make it a function unless it must be a macro.

> > > +#define dquoted_string 3
> > > +#define squoted_string 4
> > > +#define uri 5
> > > +#define uri_pending 6
> >
> > Why are you using underscores in these names? Can't these follow normal WebKit
> > naming?
> 
> They are flex start condition names.
> I've chosen to use underscores because mediaquery and forkeyword
> are not in CamelCase.

You should not do that. Instead please follow normal WebKit naming. You don't need to rename existing conditions, but you may if you like.

> > > +    int string_caller = INITIAL;
> > > +    int uri_caller = INITIAL;
> > > +    int content_length = 0;
> > > +    int content_offest = 0;
> >
> > Can't these follow normal WebKit naming conventions? I don't see any reason to
> > use a different naming style here.
> 
> They are used in tokenizer.flex where underscored names are used.
> 
> I can change them to stringCaller, etc., if you prefer.

Yes, I would like you to make that change.

> > > +    BEGIN(string_caller == uri ? uri_pending : string_caller);
> >
> > I'm not sure that "caller" is the clearest term we could use use here. Maybe
> > "token" would be clearer (or perhaps "context" or "type"). I don't really
> > understand what these represent, which I suppose is why I don't get what the
> > names should be.
> 
> http://flex.sourceforge.net/manual/Start-Conditions.html#Start-Conditions
> uses *_caller naming. That's why I named the variables that way.
> I'm open to changing the names if you prefer otherwise.

Yes, I think you should use some other name. I do see that identifier named comment_caller in the example in the Flex manual, but I think it's not really clear.

> > > +    default:
> > > +        yyterminate();
> >
> > I think ASSERT_NOT_REACHED is more appropriate here than yyterminate. Unless
> > this code can be reached.
> >
> > Do the new test cases cover all the code paths here? If I put an
> > ASSERT_NOT_REACHED in each rule, would I have to remove them all to pass the
> > test?
> 
> The default case is reached if the start condition is either
> INITIAL, mediaquery, or forkeyword.

I don't understand how this sentence answers my question.

Do the test cases cover all these code paths or not?
Comment 49 Yuzo Fujishima 2010-04-07 17:49:03 PDT
Created attachment 52812 [details]
Fix unclosed string/url handling.
Comment 50 Yuzo Fujishima 2010-04-07 17:53:04 PDT
Created attachment 52814 [details]
Fix unclosed string/url handling.
Comment 51 WebKit Review Bot 2010-04-07 17:54:26 PDT
Attachment 52814 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSParser.h:261:  yy_more_flag is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/css/CSSParser.h:262:  yy_more_len is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 52 Yuzo Fujishima 2010-04-07 18:08:55 PDT
Thank you very much for the review.

(In reply to comment #48)
> (In reply to comment #38)
> > > > +#define yymore() ((yy_more_flag) = 1)
> > >
> > > Why is this a macro instead of a function? Why does it have a bison/flex style
> > > name?
> > 
> > yymore() is a flex function (or macro):
> > http://flex.sourceforge.net/manual/Actions.html#index-yymore_0028_0029-115
> 
> You answered my second question, but ignored the first. Why is this a macro
> instead of a function? Please make it a function unless it must be a macro.

Changed yymore to be a method of CSSParser.

> 
> > > > +#define dquoted_string 3
> > > > +#define squoted_string 4
> > > > +#define uri 5
> > > > +#define uri_pending 6
> > >
> > > Why are you using underscores in these names? Can't these follow normal WebKit
> > > naming?
> > 
> > They are flex start condition names.
> > I've chosen to use underscores because mediaquery and forkeyword
> > are not in CamelCase.
> 
> You should not do that. Instead please follow normal WebKit naming. You don't
> need to rename existing conditions, but you may if you like.

Renamed them doubleQuotedStringState, etc.
Also renamed the existing ones.

I named them *State rather than *StartCondition, because
State should sound more familiar to more people
and, less importantly, StartCondition is too long as suffix.

> 
> > > > +    int string_caller = INITIAL;
> > > > +    int uri_caller = INITIAL;
> > > > +    int content_length = 0;
> > > > +    int content_offest = 0;
> > >
> > > Can't these follow normal WebKit naming conventions? I don't see any reason to
> > > use a different naming style here.
> > 
> > They are used in tokenizer.flex where underscored names are used.
> > 
> > I can change them to stringCaller, etc., if you prefer.
> 
> Yes, I would like you to make that change.
> 
> > > > +    BEGIN(string_caller == uri ? uri_pending : string_caller);
> > >
> > > I'm not sure that "caller" is the clearest term we could use use here. Maybe
> > > "token" would be clearer (or perhaps "context" or "type"). I don't really
> > > understand what these represent, which I suppose is why I don't get what the
> > > names should be.
> > 
> > http://flex.sourceforge.net/manual/Start-Conditions.html#Start-Conditions
> > uses *_caller naming. That's why I named the variables that way.
> > I'm open to changing the names if you prefer otherwise.
> 
> Yes, I think you should use some other name. I do see that identifier named
> comment_caller in the example in the Flex manual, but I think it's not really
> clear.

Renamed string_caller stringPreState, etc.
I hope the meaning is clearer this way.

Also, I've changed the variables to CSSParser instance variables.
I believe it must not be static variable as in the previous patch. 

> 
> > > > +    default:
> > > > +        yyterminate();
> > >
> > > I think ASSERT_NOT_REACHED is more appropriate here than yyterminate. Unless
> > > this code can be reached.
> > >
> > > Do the new test cases cover all the code paths here? If I put an
> > > ASSERT_NOT_REACHED in each rule, would I have to remove them all to pass the
> > > test?
> > 
> > The default case is reached if the start condition is either
> > INITIAL, mediaquery, or forkeyword.
> 
> I don't understand how this sentence answers my question.
> 
> Do the test cases cover all these code paths or not?

Yes. yyterminate is called when the tests run.
Comment 53 Yuzo Fujishima 2010-04-28 00:52:04 PDT
Created attachment 54534 [details]
Fix unclosed string/url handling.
Comment 54 WebKit Review Bot 2010-04-28 00:53:09 PDT
Attachment 54534 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/css/CSSParser.h:277:  yy_more_flag is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/css/CSSParser.h:278:  yy_more_len is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 55 Yuzo Fujishima 2010-04-28 00:58:10 PDT
Hi, Darin,

Can you take yet another look?
This time I've addressed all of your comments.
Comment 56 Eric Seidel (no email) 2010-05-17 01:48:28 PDT
This is a very scary change to review. :(
Comment 57 Adam Barth 2010-06-21 16:49:44 PDT
Comment on attachment 54534 [details]
Fix unclosed string/url handling.

Sigh.  Not sure how to make progress here.  I wish the CSS spec defined a parsing algorithm in the same level of detail as the HTML spec.  In any case, we need a CSS parsing expert to review this change.

Drive-by:

WebCore/css/CSSParser.h:266
 +          void yymore() {(yy_more_flag) = 1; /* Copied from flex ouput. */}
Surely the parenthesis aren't needed here now that this isn't a macro.
Comment 58 Nikolas Zimmermann 2010-07-30 22:46:56 PDT
Comment on attachment 54534 [details]
Fix unclosed string/url handling.

LayoutTests/fast/css/parsing-unclosed-string-expected.txt:6
 +  Test 1(ref) Test 1(test) 
I don't understand these messages. You should rephrase the messages to make clear what they're supposed to explain.

WebCore/css/maketokenizer:57
 +  #define YY_START (((yy_start) - 1) / 2)
This meaning is unclear.

WebCore/css/maketokenizer:58
 +  #define yytext_ptr yytext
This seems unused.


WebCore/css/tokenizer.flex:79
 +          yyleng--; // Remove the '\0' representing EOF.
// Remove the ...
--yyleng;

Comment should go on the line before.

WebCore/css/CSSParser.h:266
 +          void yymore() {(yy_more_flag) = 1; /* Copied from flex ouput. */}
I don't get this concept. yymore() gets called by the lexer, but all it does is setting CSSParsers yy_more_flag member variable -- the variable seems not used at all to me.

WebCore/css/CSSParser.h:278
 +          int yy_more_len;
This is completly unused.

Please clear up these issues.
Comment 59 Yuzo Fujishima 2010-08-09 01:43:49 PDT
Created attachment 63869 [details]
Patch
Comment 60 Yuzo Fujishima 2010-08-09 01:44:09 PDT
Hi, thank you for the reviews.

(In reply to comment #57)
> (From update of attachment 54534 [details])
> Sigh.  Not sure how to make progress here.  I wish the CSS spec defined a parsing algorithm in the same level of detail as the HTML spec.  In any case, we need a CSS parsing expert to review this change.
> 
> Drive-by:
> 
> WebCore/css/CSSParser.h:266
>  +          void yymore() {(yy_more_flag) = 1; /* Copied from flex ouput. */}
> Surely the parenthesis aren't needed here now that this isn't a macro.

Done.


(In reply to comment #58)
> (From update of attachment 54534 [details])
> LayoutTests/fast/css/parsing-unclosed-string-expected.txt:6
>  +  Test 1(ref) Test 1(test) 
> I don't understand these messages. You should rephrase the messages to make clear what they're supposed to explain.

Changed such that the tests are more self-explanatory.

> 
> WebCore/css/maketokenizer:57
>  +  #define YY_START (((yy_start) - 1) / 2)
> This meaning is unclear.

This and other seemingly unused variables/functions are actually used in
piece of code generated by flex. Added comments.

> 
> WebCore/css/maketokenizer:58
>  +  #define yytext_ptr yytext
> This seems unused.
> 
> 
> WebCore/css/tokenizer.flex:79
>  +          yyleng--; // Remove the '\0' representing EOF.
> // Remove the ...
> --yyleng;
> 
> Comment should go on the line before.

Done.

> 
> WebCore/css/CSSParser.h:266
>  +          void yymore() {(yy_more_flag) = 1; /* Copied from flex ouput. */}
> I don't get this concept. yymore() gets called by the lexer, but all it does is setting CSSParsers yy_more_flag member variable -- the variable seems not used at all to me.
> 
> WebCore/css/CSSParser.h:278
>  +          int yy_more_len;
> This is completly unused.
> 
> Please clear up these issues.
Comment 61 WebKit Review Bot 2010-08-09 01:46:43 PDT
Attachment 63869 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSParser.h:297:  yy_more_flag is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/css/CSSParser.h:298:  yy_more_len is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 62 Yuzo Fujishima 2010-10-29 00:15:16 PDT
Ping?

I think we should ignore style error warnings here, because the names are used by flex.
Comment 63 Yuzo Fujishima 2010-11-21 22:22:49 PST
Ping again? This is blocking 6891.
Comment 64 Tony Gentilcore 2011-01-05 11:03:24 PST
I didn't look at the patch, but just wanted to point out that Hixie has some good test cases here:
http://www.hixie.ch/tests/adhoc/css/parsing/core-syntax/strings/investigation/002.html
Comment 65 Tony Gentilcore 2011-01-05 11:04:58 PST
*** Bug 18612 has been marked as a duplicate of this bug. ***
Comment 66 Yuzo Fujishima 2011-01-11 00:24:51 PST
Created attachment 78499 [details]
Pre Source/WebCore move
Comment 67 WebKit Review Bot 2011-01-11 00:28:02 PST
Attachment 78499 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7337474
Comment 68 WebKit Review Bot 2011-01-11 00:31:03 PST
Attachment 78499 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7354141
Comment 69 Early Warning System Bot 2011-01-11 00:31:53 PST
Attachment 78499 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7438105
Comment 70 WebKit Review Bot 2011-01-11 01:14:28 PST
Attachment 78499 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7385115
Comment 71 WebKit Review Bot 2011-01-11 01:44:35 PST
Attachment 78499 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7413115
Comment 72 Yuzo Fujishima 2011-01-11 03:21:55 PST
Comment on attachment 78499 [details]
Pre Source/WebCore move

Sorry, wrong patch (webkit-patch has been confused by the change log).
Comment 73 Yuzo Fujishima 2011-01-11 03:22:56 PST
I'm releasing ownership of this bug.
Comment 74 Robert Hogan 2012-02-13 14:20:35 PST

*** This bug has been marked as a duplicate of bug 78538 ***