751939 - end-of-file handling for empty URI token (i.e. "url(") is wrong

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: test case for "url(" fixing at end-of-file

"url(" at the end of file should be fixed into "url()". See test case. It seems that only IE9 passes this at the moment.

Comment 1

[Kang-Hao (Kenny) Lu [:kennyluck]]

It's pretty simple, and [1] is pretty much where this should be fixed. It would be helpful to look at how the url-token tests in /layout/style/test/test_css_eof_handling.html are handled. (The patch should include a test in that file too.)

The URL field above points to the relevant CSS2.1 prose.  

[1] https://hg.mozilla.org/mozilla-central/annotate/032d43b1770f/layout/style/nsCSSScanner.cpp#l840

Comment 2

[Alex Chronopoulos [:achronop]]

Hi, I am new here, can I work on this?

Comment 3

[Kang-Hao (Kenny) Lu [:kennyluck]]

Hi Alexandros,

Sorry for my super late response.

(In reply to Alexandros from comment #2)
> Hi, I am new here, can I work on this?

Sorry about that but this has been picked up by Sanchit Garg in 2012-05-07. I would encourage you to find other mentored bugs (if you are looking for one), and I will ping you when Sanchit gave up (hopefully that won't happen).

Comment 4

[Sanchit Garg]

Attachment: This is the patch for Bug-751939

Comment 5

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to Sanchit Garg from comment #4)
> Created attachment 634752 [details] [diff] [review]
> This is the patch for Bug-751939

You missed your commit message. It should be something like

"Bug 751939 - (say something here). r=dbaron"

Comment 6

[Kang-Hao (Kenny) Lu [:kennyluck]]

Otherwise, it looks good (to me, it needs dbaron's review of course). I don't mean to put you down. :p

Comment 7

[Sanchit Garg]

Attachment: The url( if fixed to url().

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 636143 [details] [diff] [review]
The url( if fixed to url().

Really sorry for taking so long to get to this.

This patch doesn't seem quite right, since if you're going to set the token type to eCSSToken_URL, you also need to set the identifier to the empty string.

I also think the test you added doesn't actually test this case, because of the " ".  The test you added is a good test to have, but I think to test the code you modified you need to test the behavior of "url(" rather than "url( ".

And I think what this patch does is make "url(" be treated like "url(url)" rather than like "url()" as it probably should.

(I'd also note the spec isn't particularly clear here; however, I agree this is probably the right thing to do.)

Furthermore, to be truly correct (although I think it doesn't actually affect any real cases at present), you should change CSSParserImpl::GetURLInParens (which is used for functions in the argument to @-moz-document) to also ignore the return value from NextURL, and change nsCSSScanner::NextURL to return void rather than bool.

Comment 9

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to David Baron [:dbaron] from comment #8)
> This patch doesn't seem quite right, since if you're going to set the token
> type to eCSSToken_URL, you also need to set the identifier to the empty
> string.

Ah, right. Could just move the later SetLength(0) call to the beginning.
 
> I also think the test you added doesn't actually test this case, because of
> the " ".  The test you added is a good test to have, but I think to test the
> code you modified you need to test the behavior of "url(" rather than "url(
> ".

There's EatWhiteSpace() before this code so I think that's the same thing.

> And I think what this patch does is make "url(" be treated like "url(url)"
> rather than like "url()" as it probably should.

Yeah... Sanchit, did you actually run the mochitest and did you get a PASS?

> (I'd also note the spec isn't particularly clear here; however, I agree this
> is probably the right thing to do.)
> 
> Furthermore, to be truly correct (although I think it doesn't actually
> affect any real cases at present), you should change
> CSSParserImpl::GetURLInParens (which is used for functions in the argument
> to @-moz-document) to also ignore the return value from NextURL, and change
> nsCSSScanner::NextURL to return void rather than bool.

Sanchit, I'll let you decide if you want to do these.

Comment 10

[Sanchit Garg]

Attachment: Patch to fix url( to url()

Comment 11

[Kang-Hao (Kenny) Lu [:kennyluck]]

Sanchit, I don't see any difference from the previous patch (the SetLength(0) part isn't in). I suppose you attached the wrong thing and I am canceling the review.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's byte-for-byte the same as the previous patch.

Comment 13

[Sanchit Garg]

Attachment: Patch for bug 751939.

Sorry, I accidentally uploaded the previous patch file.

Comment 14

[Kang-Hao (Kenny) Lu [:kennyluck]]

Suppose the history of your change was

* Unchanged (rev 1)
* With "aToken.mType = eCSSToken_URL;" (rev 2)
* With "ident.SetLength(0);" (rev 3)

I'd note that your patch here is the diff between rev2 and rev3, which is unusually called interdiff. For samll patches like this, I think it would be better if you just provide the diff between rev1 and rev3 (I'm not canceling the review because David might be willing to reivew it...). I hope you play with hg more and make sure you know how to make such a diff.

Also, bugzilla allows you to mark previous patches as obsolete and you should do so.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yes, could you:
 * address the last paragraph of comment 8
 * add a test for "url(" without the space
 * make sure the tests pass, and
 * attach a combined patch rather than an interdiff?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 649980 [details] [diff] [review]
Patch for bug 751939.

see comment 15

Comment 17

[Mike Hoye [:mhoye]]

Can you confirm that you're still working on this bug?

Comment 18

[Mike Hoye [:mhoye]]

I'm throwing this back in the pool. Sanchit, if you'd like to pick it up again, let us know.

Comment 19

[Alex Chronopoulos [:achronop]]

I guess I can work on it (now). I will review the history and I will provide an update soon. Thanks!

Comment 20

[Alex Chronopoulos [:achronop]]

Attachment: bug-751939-fix.patch

This patch contains the following changes

1. Fix for unexpected eof on empty uri token
2. Test case for that case
3. Change nsCSSScanner::NextURL to return void

Test executed and pass. Thanks!

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8458928 [details] [diff] [review]
bug-751939-fix.patch

This looks good except for one minor comment and two even more minor comments (in the reverse order):

>Bug 751939 - EOF handling for empty uri token. r=dbaron

The commit message should really say more clearly what's changing.  Perhaps:

Bug 751939 - make url( followed by EOF valid.  r=dbaron

>       NS_ASSERTION(!mHavePushBack, "mustn't have pushback at this point");
>-      if (!mScanner->NextURL(mToken) || mToken.mType != eCSSToken_URL) {
>+
>+      mScanner->NextURL(mToken);
>+      if (mToken.mType != eCSSToken_URL) {

Probably best not to insert the blank line, since the assertion is associate with the direct access to the scanner (mScanner->NextURL()).

>   int32_t ch = Peek();
>   if (ch < 0) {
>-    return false;
>+    // EOF instead of closing assume valid url()
>+    aToken.mType = eCSSToken_URL;
>   }
> 

I don't think setting aToken.mType here should be needed.  You should be able to just omit this test of ch < 0 completely.  However, it's worth a comment noting that ch might be less than 0 (indicating EOF), but that the rest of the function's logic deals with that.

>diff --git a/layout/style/test/test_css_eof_handling.html b/layout/style/test/test_css_eof_handling.html
>--- a/layout/style/test/test_css_eof_handling.html
>+++ b/layout/style/test/test_css_eof_handling.html

Did you test that this new test fails without the patch and passes with it?

r=dbaron with those comments addressed, assuming that you've tested the test appropriately

If you attach a revised patch you an use checkin-needed as described in https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in .

Comment 22

[Alex Chronopoulos [:achronop]]

Attachment: Fix for bug-751939 re-submit

Thank you for the comments. I re-submit the patch for review because accidentally erased a return statement in previous one. It is when bad string is identified line 1170. The rest of the comments also applied.

New test fails without the fix. When review completed I will send to to try for additional verification. Thanks!

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8459770 [details] [diff] [review]
Fix for bug-751939 re-submit

Could you add the code comment I mentioned in my previous review to nsCSSScanner::NextURL, pointing out that ch might be less than 0, indicating end-of-file.

r=dbaron with that

Comment 24

[Alex Chronopoulos [:achronop]]

Added comment missing from last review. Try looks pretty green.

https://tbpl.mozilla.org/?tree=Try&rev=cdc92579ad54

Later today I will upload a check-in version on the patch. Additional comments are welcome.

Comment 25

[Alex Chronopoulos [:achronop]]

Attachment: Patch ready for check-in

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b3ccec6fd4a

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8b3ccec6fd4a