Last Comment Bug 790997 - Rendering layered page is completely broken
: Rendering layered page is completely broken
Status: RESOLVED FIXED
invalid?
: regression
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: 15 Branch
: x86 All
: -- normal (vote)
: mozilla50
Assigned To: Boris Zbarsky [:bz] (TPAC)
:
Mentors:
Depends on:
Blocks: 1153981 748254
  Show dependency treegraph
 
Reported: 2012-09-13 09:02 PDT by publisher
Modified: 2016-07-14 02:48 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Minimalish testcase (182 bytes, text/html)
2012-09-14 02:00 PDT, Boris Zbarsky [:bz] (TPAC)
no flags Details
Align our tokenization of CSS bad-url-token with the CSS Syntax Level 3 CR (16.13 KB, patch)
2016-07-13 09:50 PDT, Boris Zbarsky [:bz] (TPAC)
cam: review+
ttromey: review+
Details | Diff | Splinter Review

Description publisher 2012-09-13 09:02:50 PDT
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.
Comment 1 Alice0775 White 2012-09-13 09:39:57 PDT
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
Comment 2 Alice0775 White 2012-09-13 09:47:34 PDT
The error message may be unrelated to this bug, because the same message is output in Firefox14.
Comment 3 Boris Zbarsky [:bz] (TPAC) 2012-09-13 16:07:06 PDT
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...
Comment 4 Boris Zbarsky [:bz] (TPAC) 2012-09-14 02:00:44 PDT
Created attachment 661159 [details]
Minimalish testcase

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.
Comment 5 Boris Zbarsky [:bz] (TPAC) 2012-09-14 03:18:48 PDT
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?
Comment 6 Kang-Hao (Kenny) Lu [:kennyluck] 2012-10-03 07:59:52 PDT
(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...
Comment 7 ovidiu boca[:Ovidiu][PTO from: 9/19, until: 9/30] for urgent issues please contact Brindusa Tot 2016-07-12 05:23:32 PDT
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.
Comment 8 Boris Zbarsky [:bz] (TPAC) 2016-07-12 10:01:08 PDT
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.
Comment 9 Boris Zbarsky [:bz] (TPAC) 2016-07-12 22:12:26 PDT
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.
Comment 10 Boris Zbarsky [:bz] (TPAC) 2016-07-12 22:50:13 PDT
Though I'm not sure how this will affect CSSParserImpl::SkipBalancedContentUntil.  How should something like url(foo') be handled there?
Comment 11 Cameron McCormack (:heycam) 2016-07-13 00:00:19 PDT
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.
Comment 12 Boris Zbarsky [:bz] (TPAC) 2016-07-13 06:18:31 PDT
> 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 ')'.
Comment 13 Boris Zbarsky [:bz] (TPAC) 2016-07-13 09:50:29 PDT
Created attachment 8770635 [details] [diff] [review]
Align our tokenization of CSS bad-url-token with the CSS Syntax Level 3 CR

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 14 Tom Tromey :tromey 2016-07-13 10:49:35 PDT
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.
Comment 15 Boris Zbarsky [:bz] (TPAC) 2016-07-13 11:41:53 PDT
> Maybe dom/webidl/CSSLexer.webidl needs a comment update to reflect the change to bad_url.

Good idea, done.
Comment 16 Cameron McCormack (:heycam) 2016-07-13 18:39:06 PDT
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.
Comment 17 Pulsebot 2016-07-13 19:54:40 PDT
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
Comment 18 Carsten Book [:Tomcat] 2016-07-14 02:48:04 PDT
https://hg.mozilla.org/mozilla-central/rev/bf190e326bfd

Note You need to log in before you can comment on or make changes to this bug.