Closed Bug 495269 Opened 11 years ago Closed 11 years ago

Firebug CSS adjustments to a page with @font-face crashes

Categories

(Core :: CSS Parsing and Computation, defect)

1.9.1 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: sgarrity, Assigned: bzbarsky)

Details

(Keywords: crash, regression, verified1.9.1)

Attachments

(2 files)

Working on a page using @font-face and text-shadow CSS stuff, I'm getting a crash when adjusting CSS properties in Firebug. I'll see if I can narrow it down to a simpler test case, but filing for now ("shaver: file first, narrow second").

Using the 3.5pre nightly (20090528) on Linux and Mac.

Crash report on Linux here:
http://crash-stats.mozilla.com/report/index/ec56a336-6cdf-4d5c-9507-3548c2090528

Crash report on Mac here:
http://crash-stats.mozilla.com/report/index/9e08370e-0823-4634-a907-fd0e22090528

Firebug version 1.4.0a30
Update, removed the text-shadow stuff and still getting the crash.
Summary: Firebug CSS adjustments to a page with @font-face & text-shadow crashes → Firebug CSS adjustments to a page with @font-face crashes
Component: General → Style System (CSS)
Product: Firefox → Core
QA Contact: general → style-system
Version: 3.5 Branch → 1.9.1 Branch
OK, I see what's up here.
This is a guaranteed null-deref crash, but a virtual function call on the null.  I do think we should block on this; it's not that hard to trigger without firebug.

Patch coming up soon; need to write some tests.
Flags: blocking1.9.1?
Block or just take a safe patch? Not a topcrash, just wondering why we think it's a hard release blocker. Might be for firebug-compat, I guess ...
Take a safe patch, for sure.

For the rest, good question.  Like I said, websites can trigger it without firebug; they just need to be using an @font-face, @page, or @-moz-document rule...
Attached file Test case
Here's the simplest test case I could get down to. There's a reference to an external JS file that's empty - but removing that seems to eliminate the crash, which is weird.
The JS file ref is needed to trigger CSS prefetch, which is what gives you a sheet clone in this case.  It's possible to trigger in other ways too; test will be coming up that shows that.
Attached patch FixSplinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #380224 - Flags: superreview?(dbaron)
Attachment #380224 - Flags: review?(dbaron)
Attachment #380224 - Flags: superreview?(dbaron)
Attachment #380224 - Flags: superreview+
Attachment #380224 - Flags: review?(dbaron)
Attachment #380224 - Flags: review+
Comment on attachment 380224 [details] [diff] [review]
Fix

>+  // Note that nsCSSStyleSheet code assumes that no other kinds of rules can
>+  // come between rules of type IMPORT_RULE.

Maybe say nsCSSStyleSheet::RebuildChildList specifically?

Also, maybe say "between two rules of type IMPORT_RULE" to make it clearer that you didn't accidentally omit a second type?

r+sr=dbaron
Flags: blocking1.9.1? → blocking1.9.1+
Pushed http://hg.mozilla.org/mozilla-central/rev/bb7d5f416b60
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/af8176bd8b21

Both pushes had the comment fix, btw.
Keywords: fixed1.9.1
Isn't this technically an interface change? (i.e. the enum values are changing)
Yes, but this is an internal interface.  For 1.9.0, I'll probably work on a fix without the enum change.
Or was the question whether we need to rev the iid?
I can no longer crash the new nightly. Thanks for the quick response.
Status: RESOLVED → VERIFIED
(In reply to comment #14)
> Or was the question whether we need to rev the iid?

Yes. And if so, shouldn't a new interface be made for 1.9.1 as well... (I assumed from your previous response that the answer to both questions is no since consumers aren't supposed to use this interface)
I do think we should rev it.  I don't think we need a new interface; this is very much internal.  I'll see if I can get a= for revving on 1.9.1.  If not, we shouldn't rev on trunk either, probably.
Comment 13 last sentence is wrong; the code in question doesn't exist on 1.9.0, so no need for changes there.
Verified fixed on 1.9.1 with builds on OS X and Windows like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090609 Shiretoko/3.5pre ID:20090609030956
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.