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.

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

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