"ASSERTION: |First()| called on an empty string" with @font-face #

VERIFIED FIXED in mozilla1.9.1b3

Status

()

P3
normal
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: jruderman, Assigned: zwol)

Tracking

(Blocks: 1 bug, {assertion, testcase, verified1.9.1})

Trunk
mozilla1.9.1b3
assertion, testcase, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Posted file testcase
###!!! 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

10 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

10 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

10 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

10 years ago
Flags: wanted1.9.1?
Keywords: checkin-needed
Whiteboard: [needs landing]
(Assignee)

Updated

10 years ago
Blocks: 473815
Fixed: http://hg.mozilla.org/mozilla-central/rev/90cded872e97
Status: ASSIGNED → RESOLVED
Last Resolved: 10 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+
Keywords: checkin-needed
(Assignee)

Updated

10 years ago
Attachment #357218 - Flags: approval1.9.1?
(Assignee)

Comment 9

10 years ago
Comment on attachment 357218 [details] [diff] [review]
rev 1: dbaron's comments addressed

clearing unnecessary approval1.9.1?
(Reporter)

Updated

10 years ago
Blocks: 476744
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.