User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20100101 Firefox/15.0.1 Build ID: 20120905151427 Steps to reproduce: loaded http://www.ipulpfiction.com/books/000DEMOS/reader/DEMO-PrincessOfMars.html Actual results: This page is composed of several stacked layers that are variously hidden or shown by the viewer. This includes a iframe containing a multi-column document that should scroll horizontally in response to left & right margin clicks. Expected results: view http://www.ipulpfiction.com/books/000DEMOS/reader/DEMO-PrincessOfMars.html in Firefox 14.0.1 where it was working perfectly.

Error: TypeError: document.getElementById(...).contentWindow.doV is not a function Source file: http://www.ipulpfiction.com/books/_bookReader/reader/cReader.js Line: 102 Error: TypeError: document.getElementById(...).contentWindow.doLARGE is not a function Source file: http://www.ipulpfiction.com/books/_bookReader/reader/cReader.js Line: 124 Error: TypeError: parent.resizeIframe is not a function Source file: http://www.ipulpfiction.com/books/ads/HC-GunGames.html Line: 1 Error: TypeError: parent.resizeIframe is not a function Source file: http://www.ipulpfiction.com/books/PrincessOfMars-Part1/book_PoMerinvd85/textPaged.html Line: 1 Error: TypeError: pcontent is null Source file: http://www.ipulpfiction.com/books/_bookReader/reader/cReader.js Line: 1797 Regression window(m-c) Good: http://hg.mozilla.org/mozilla-central/rev/cc5254f9825f Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20120425 Firefox/15.0a1 ID:20120425210440 Bad: http://hg.mozilla.org/mozilla-central/rev/811b1ce5f4b2 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20120426 Firefox/15.0a1 ID:20120426054640 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cc5254f9825f&tochange=811b1ce5f4b2 Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/7321b7a0c775 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20120425 Firefox/15.0a1 ID:20120425193942 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/ecfe99b2c9cc Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20120425 Firefox/15.0a1 ID:20120425214344 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7321b7a0c775&tochange=ecfe99b2c9cc

The error message may be unrelated to this bug, because the same message is output in Firefox14.

Hmm. I bet the problematic rule is this one, at line 698 in cReader.css: @font-face { font-family: 'AdobeGaramondProRegular'; src: url('/fonts/agaramondpro-regular-webfont.eot'); src: url('/fonts/agaramondpro-regular-webfont.eot?#iefix') format('embedded-opentype'), url('/fonts/agaramondpro-regular-webfont.woff') format('woff'), url('/fonts/agaramondpro-regular-webfont.ttf') format('truetype'), url(/fonts/agaramondpro-regular-webfont.svg#AdobeGaramondProRegular') format('svg'); font-weight: normal; font-style: normal; } which gives the following error console messages: [00:00:19.075] Error in parsing value for 'src'. Skipped to next declaration. @ http://www.ipulpfiction.com/books/_bookReader/reader/cReader.css:704 [00:00:19.075] Found unclosed string '');'. Unexpected end of file while searching for closing } of declaration block. Unexpected end of file while searching for '}'. @ http://www.ipulpfiction.com/books/_bookReader/reader/cReader.css:705 The src value is in fact invalid: note the missing single quote at the start of the url() there. That means there's an unclosed string involved, as reported. What I'm not sure of is why we don't find the '}' yet. More debugging to come tomorrow. But _man_ does that stylesheet have a bunch of broken CSS in it...

On this testcase, Firefox 14 and WebKit are green. Current nightly and Presto are red. Still working through what the spec says and why the behavior changed here.

OK, so first of all what our behavior is. We enter ParseFontSrc. The first token we get is a BadURL token with "foo" as mIdent. We don't know what to do with it, so we UngetToken and return. In fact, we return all the way up to ParseFontFaceRule. At that point we call SkipDeclaration(). SkipDeclaration() calls GetToken() and comes out with the BadURL token we just ungot. So we call SkipUntil(')'). In SkipUntil, the first token we get is a String token containing ") format(". Then an Ident token for "something". Then a BadString token for ");". At this point there are no more ')' to be had in the stylesheet, so we keep scanning forward looking for a ')' until we get to the end of a stylesheet. Before bug 748254, we get into SkipDeclaration, the first token we get is the ") format (" string. So we don't go into SkipUntil, and keep getting tokens, getting the same Ident and BadString. Then we get a Symbol for the '}' and SkipDeclaration knows that means the end of a declaration and returns. On the original page, we would have kept going until the Symbol for ';' at the end of "font-weight: normal" and then returned. In either case, the rest of the CSS in the sheet got processed. I believe the new behavior is actually the correct one in terms of the spec: we need to look for the closing ')' of the "url(", and we weren't doing that before. Note that Presto implements the same behavior. David, is my reading of the spec correct here?

(In reply to Boris Zbarsky (:bz) from comment #5) > David, is my reading of the spec correct here? Probably, although it was never clear in CSS 2.1 whether a BAD_URI contributes to an opening '('. I guess this would then gives another reason why Tab's new bad-url spec would be favorable because a missing quote in url() would have minimal impact? Though the fact that WebKit accepts this is becuase it never observes matching parenthesis...

Testing this issue on Mac OS X 10.10 with FF 50.0a1, FF 47 release, the minimal test case attached is red, on other browsers the test is green. Boris, what do you think we should do with this one? Based on your comment 5 this is the correct behavior.

What I think we should do with this one is verify that the CSS parsing spec changes that have happened in the last 4 years haven't changed what we're supposed to do here and if so resolve this invalid and file bugs on the other browsers.

So I talked to Tab and looked at the current CSS Syntax draft. The upshot is that per spec in my minimal testcase once we hit the single-quote we land in https://www.w3.org/TR/css-syntax-3/#consume-the-remnants-of-a-bad-url0 which ignores all characters except literal (not escaped) ')'. In terms of our parser, nsCSSScanner::NextURL when we set aToken.mType = eCSSToken_Bad_URL we need to go ahead and consume everything to EOF or next unescaped ')', I think.

Though I'm not sure how this will affect CSSParserImpl::SkipBalancedContentUntil. How should something like url(foo') be handled there?

Bad_URL tokens are invalid in CSS variable values, and we reject them in ParseValueWithVariables, so we shouldn't be getting in here with a Bad_URL token. But in case someone else calls it in the future, we can just continue the loop (and not call stack.AppendElement(')')). Same thing for SkipUntil. Similarly we'll need to remove the SkipUntil(')') call when encountering a Bad_URL in: * SkipAtRule * ParseSupportsConditionInParens * SkipUntilOneOf * SkipDeclaration * SkipRuleSet * ParseValueWithVariables And then in ParseSupportsCondition we should return false immediately. Unless in comment 9 you mean that NextURL consumes *up until* the ')', in which case the the above functions can keep their SkipUntil(')'), which they'll find immediately. But it's probably a better match to the spec if the ')' is considered part of the Bad_URL and we don't need to care about it up in CSSParserImpl.

> Unless in comment 9 you mean that NextURL consumes *up until* the ')' Yeah, I was wandering about that tool. There are lots of places that basically treat Bad_URL and Function in parallel and assume that neither one consumes the ')', so the minimal change here is to make NextURL consistently consume up until the ')' but leave the ')'.

Tom, can you review the devtools bits? Cameron, can you review the layout/ bits? They're basically the same, for both the xpcshell tests and the implementations...

Comment on attachment 8770635 [details] [diff] [review] Align our tokenization of CSS bad-url-token with the CSS Syntax Level 3 CR Review of attachment 8770635 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for doing this. I think the devtools parts look good. Maybe dom/webidl/CSSLexer.webidl needs a comment update to reflect the change to bad_url. This is the main documentation for (both of) the CSS Lexer interface. I leave it to your discretion.

> Maybe dom/webidl/CSSLexer.webidl needs a comment update to reflect the change to bad_url. Good idea, done.

Comment on attachment 8770635 [details] [diff] [review] Align our tokenization of CSS bad-url-token with the CSS Syntax Level 3 CR Review of attachment 8770635 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks.

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf190e326bfd Align our tokenization of CSS bad-url-token with the CSS Syntax Level 3 CR. r=heycam,tromey

Backed out on all branches; the above implemented the TR draft, but the ED is different and TR breaks other webpages...