Implement the CSS 3 syntax rules for parsing url() (was: Rendering layered page is completely broken)

REOPENED
Unassigned

Status

()

Core
CSS Parsing and Computation
REOPENED
5 years ago
5 months ago

People

(Reporter: publisher, Unassigned)

Tracking

(Blocks: 1 bug, {regression})

15 Branch
x86
All
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: invalid?)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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

5 years ago
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
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Keywords: regression
OS: Mac OS X → All
Product: Firefox → Core

Comment 2

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

Updated

5 years ago
Blocks: 748254
Component: DOM → Style System (CSS)
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...
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.
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?
Whiteboard: invalid?
(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.
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
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?
Flags: needinfo?(cam)
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.
Flags: needinfo?(cam)
> 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 ')'.
Blocks: 1153981
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...
Attachment #8770635 - Flags: review?(ttromey)
Attachment #8770635 - Flags: review?(cam)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED

Comment 14

11 months ago
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.
Attachment #8770635 - Flags: review?(ttromey) → review+
> 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.
Attachment #8770635 - Flags: review?(cam) → review+

Comment 17

11 months ago
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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf190e326bfd
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1315368
Backed out on all branches; the above implemented the TR draft, but the ED is different and TR breaks other webpages...
Assignee: bzbarsky → nobody
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla50 → ---
Summary: Rendering layered page is completely broken → Implement the CSS 3 syntax rules for parsing url() (was: Rendering layered page is completely broken)
status-firefox50: fixed → ---
You need to log in before you can comment on or make changes to this bug.