Closed Bug 645951 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: crash, regression, topcrash)

Crash Data

Attachments

(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:
http://hg.mozilla.org/mozilla-central/rev/5de818f2b992
which changed:

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

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

Crash reports at:
https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=exact&query=&date=04%2F06%2F2011%2000%3A00%3A00&range_value=30&range_unit=days&hang_type=crash&process_type=browser&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&admin=1&signature=mozilla%3A%3Acss%3A%3AImportRule%3A%3ASetSheet%28nsCSSStyleSheet*%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
Status: NEW → ASSIGNED
> 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+
http://hg.mozilla.org/mozilla-central/rev/14fa948cc732
Status: ASSIGNED → RESOLVED
Closed: 9 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.