Closed
Bug 473720
Opened 15 years ago
Closed 15 years ago
"ASSERTION: |First()| called on an empty string" with @font-face #
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla1.9.1b3
People
(Reporter: jruderman, Assigned: zwol)
References
Details
(Keywords: assertion, testcase, verified1.9.1)
Attachments
(2 files, 1 obsolete file)
97 bytes,
text/html
|
Details | |
6.04 KB,
patch
|
zwol
:
review+
zwol
:
superreview+
|
Details | Diff | Splinter Review |
###!!! ASSERTION: |First()| called on an empty string: 'mLength > 0', file nsTSubstring.h, line 246 ... #5 0x12abe2b9 in nsAString_internal::First #6 0x12a6c243 in NonMozillaVendorIdentifier at nsCSSParser.cpp:887 #7 0x12a8056d in CSSParserImpl::ParseAtRule at nsCSSParser.cpp:1406 #8 0x12a80bf6 in CSSParserImpl::Parse at nsCSSParser.cpp:862 ...
Assignee | ||
Comment 1•15 years ago
|
||
ParseAtRule was running the logic to recover from an unrecognized @-rule whenever any of the parser functions for a specific @-rule reported an error; in particular, it was trying to determine whether the @-keyword was a non-Moz vendor identifier, using mToken, which in the test case was no longer the @-keyword. You could get the same effect with any @-rule. The patch separates these cases and also slightly improves error reporting for @font-face, which was being *silently* discarded when the first token after the @-keyword wasn't '{'. We oughta have new strings for that but IIRC we're in a string freeze so I reused ones that talk about declaration blocks; hope that's ok. This passes reftests, layout mochitests, and layout/style crashtests including the new test case. The full crashtest set goes into an infinite loop somewhere in SVG for me, which I am assuming is unrelated bustage. Also I manually verified that the assertion does not appear anymore.
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
Attachment #357186 -
Flags: superreview?(dbaron)
Attachment #357186 -
Flags: review?(dbaron)
(In reply to comment #1) > the new test case. The full crashtest set goes into an infinite loop somewhere > in SVG for me, which I am assuming is unrelated bustage. Also I manually Yeah, it does that for me too. Are you on 64-bit Linux?
Comment on attachment 357186 [details] [diff] [review] rev 0: fix recovery from parse errors mid-@-rule >+ if ((this->*parseFunc)(aAppendFunc, aData)) { >+ mSection = newSection; >+ return PR_TRUE; >+ } else { >+ // Skip over invalid at rule, don't advance section > OUTPUT_ERROR(); >+ return SkipAtRule(); >+ } Could you restructure this bit so you still have a return at the end of the function, and it's the normal case, i.e.,: if (!(this->*parseFunc)(...)) { OUTPUT_ERROR(); return SkipAtRule(); } mSection = newSection; return PR_TRUE; With that, r+sr=dbaron.
Attachment #357186 -
Flags: superreview?(dbaron)
Attachment #357186 -
Flags: superreview+
Attachment #357186 -
Flags: review?(dbaron)
Attachment #357186 -
Flags: review+
And if you think the strings should be fixed, could you do a separate patch for that, and then land it on m-c only?
Assignee | ||
Comment 5•15 years ago
|
||
(In reply to comment #2) > (In reply to comment #1) > > The full crashtest set goes into an infinite loop somewhere > > in SVG for me, which I am assuming is unrelated bustage. > > Yeah, it does that for me too. Are you on 64-bit Linux? Yup.
Assignee | ||
Comment 6•15 years ago
|
||
> Could you restructure this bit so you still have a return at the end of the > function, and it's the normal case Done here. > And if you think the strings should be fixed, could you do a separate patch for > that, and then land it on m-c only? Will do and file as separate bug.
Attachment #357186 -
Attachment is obsolete: true
Attachment #357218 -
Flags: superreview+
Attachment #357218 -
Flags: review+
Attachment #357218 -
Flags: approval1.9.1?
Assignee | ||
Updated•15 years ago
|
Fixed: http://hg.mozilla.org/mozilla-central/rev/90cded872e97
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Flags: wanted1.9.1? → blocking1.9.1+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c63572c73733
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
Updated•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•15 years ago
|
Attachment #357218 -
Flags: approval1.9.1?
Assignee | ||
Comment 9•15 years ago
|
||
Comment on attachment 357218 [details] [diff] [review] rev 1: dbaron's comments addressed clearing unnecessary approval1.9.1?
Reporter | ||
Updated•15 years ago
|
Blocks: jesse-css-grammar-fuzzer
Comment 10•15 years ago
|
||
verified FIXED on debug builds (no assertions): Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090608 Shiretoko/3.5pre ID:20090608122057 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090608 Minefield/3.6a1pre ID:20090608122028
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•