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

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?)

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to restore old behavior

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.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Not just

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

?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 522589 [details] [diff] [review]
patch to restore old behavior

r=me with the XXX comment removed or replaced by an explanation.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/14fa948cc732