Closed Bug 645951 Opened 11 years ago Closed 11 years ago
crashes [@ mozilla::css::Import
Rule::Set Sheet(ns CSSStyle Sheet*)]
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+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
OS: Linux → All
Priority: -- → P1
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
Crash Signature: [@ mozilla::css::ImportRule::SetSheet(nsCSSStyleSheet*)]
You need to log in before you can comment on or make changes to this bug.