Closed Bug 645951 Opened 11 years ago Closed 11 years ago

crashes [@ mozilla::css::ImportRule::SetSheet(nsCSSStyleSheet*)]


(Core :: CSS Parsing and Computation, defect, P1)






(Reporter: dbaron, Assigned: dbaron)



(Keywords: crash, regression, topcrash)

Crash Data


(1 file)

I noticed a bunch of crashes at mozilla::css::ImportRule::SetSheet(nsCSSStyleSheet*)
in crash-stats.

The crash address is consistent with trying to modify nsCSSStyleSheet::mOwnerRule of a null nsCSSStyleSheet.  (Based on my math, that is 0x28 in 32-bit and 0x48 in 64-bit -- since nsString is a pointer and two 32-bit ints, and everything else in nsCSSStyleSheet is pointers, or a reference count plus an alignment hole.)

Such a crash would have been introduced by:
which changed:

>-  nsresult rv;
>+  NS_PRECONDITION(aSheet, "null arg");

I still want to look into why aSheet might be null.

Crash reports at:*%29
Maybe this happens when we clone an @import rule whose child sheet hasn't loaded yet, or for some reason can't load?  (If it can't load, do we get a sheet object at all?)

(Did the old code behave correctly in such a case, if the child sheet was still loading?)
I think this gets us back to the old behavior.  The other SetSheet caller, in the loader, doesn't pass null, and this caller already has a null-check, so I think it's better to leave the precondition and check at the caller.
Attachment #522589 - Flags: review?(bzbarsky)
Not just

  nsRefPtr<nsCSSStyleSheet> sheet = aCopy.mChildSheet->Clone(nsnull, this, nsnull, nsnull);

Assignee: nobody → dbaron
> Maybe this happens when we clone an @import rule whose child sheet hasn't
> loaded yet

That wouldn't matter; we create the sheet object no matter what .... _if_ the load security checks are passed.

If they are not, then the import rule will have a null child sheet, though, looks like.
Oh, and we create the sheet object when we _start_ the load.  So once Loader::LoadChildSheet runs, either the rule has a sheet or it will never get one.
Comment on attachment 522589 [details] [diff] [review]
patch to restore old behavior

r=me with the XXX comment removed or replaced by an explanation.
Attachment #522589 - Flags: review?(bzbarsky) → review+
Closed: 11 years ago
OS: Linux → All
Priority: -- → P1
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
Flags: in-testsuite+
Crash Signature: [@ mozilla::css::ImportRule::SetSheet(nsCSSStyleSheet*)]
You need to log in before you can comment on or make changes to this bug.