Last Comment Bug 790997 - Rendering layered page is completely broken
: Rendering layered page is completely broken
Status: NEW
invalid?
: regression
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: 15 Branch
: x86 All
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 748254
  Show dependency treegraph
 
Reported: 2012-09-13 09:02 PDT by publisher
Modified: 2012-10-03 07:59 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Minimalish testcase (182 bytes, text/html)
2012-09-14 02:00 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details

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] (Out June 25-July 6) 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] (Out June 25-July 6) 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] (Out June 25-July 6) 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...

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