Closed Bug 992571 Opened 10 years ago Closed 10 years ago

Null deref in nsXBLPrototypeResources::FlushSkinSheets called during unlinking

Categories

(Core :: XBL, defect)

24 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: mccr8, Assigned: wchen)

References

Details

Attachments

(1 file, 1 obsolete file)

To reproduce, run ./mach mochitest-plain dom/tests/mochitest/webcomponents/

You get a null-deref crash with this stack on shutdown:

  * frame #0: 0x0000000100096691 libmozalloc.dylib`mozalloc_abort(msg=<unavailable>) + 81 at mozalloc_abort.cpp:30
    frame #1: 0x0000000101524df9 XUL`Abort(aMsg=<unavailable>) + 9 at nsDebugImpl.cpp:421
    frame #2: 0x0000000101524b96 XUL`NS_DebugBreak(aSeverity=0, aStr=<unavailable>, aExpr=<unavailable>, aFile=<unavailable>, aLine=<unavailable>) + 1350 at nsDebugImpl.cpp:378
    frame #3: 0x0000000102aa205a XUL`nsXBLPrototypeResources::FlushSkinSheets() [inlined] nsCOMPtr<nsIDocument>::operator->(this=0x0000000000000000, this=<unavailable>) const + 46 at nsCOMPtr.h:870
    frame #4: 0x0000000102aa202c XUL`nsXBLPrototypeResources::FlushSkinSheets(this=0x00000001168231c0) + 92 at nsXBLPrototypeResources.cpp:71
    frame #5: 0x0000000102aa1fc2 XUL`nsXBLPrototypeBinding::FlushSkinSheets(this=<unavailable>) + 18 at nsXBLPrototypeBinding.cpp:249
    frame #6: 0x0000000102bd25d6 XUL`mozilla::dom::ShadowRoot::Restyle(this=0x000000010e028790) + 22 at ShadowRoot.cpp:120
    frame #7: 0x0000000102c8d5bd XUL`nsStyleLinkElement::DoUpdateStyleSheet(this=0x000000010ee02ed8, aOldDocument=<unavailable>, aOldShadowRoot=<unavailable>, aObserver=0x0000000000000000, aWillNotify=0x00007fff5fbf6eff, aIsAlternate=0x00007fff5fbf6efe, aForceUpdate=true) + 413 at nsStyleLinkElement.cpp:308
    frame #8: 0x0000000102c8e1bd XUL`nsStyleLinkElement::UpdateStyleSheetInternal(this=<unavailable>, aOldDocument=<unavailable>, aOldShadowRoot=<unavailable>, aForceUpdate=<unavailable>) + 29 at nsStyleLinkElement.cpp:190
    frame #9: 0x0000000102dac76c XUL`mozilla::dom::HTMLStyleElement::UnbindFromTree(this=<unavailable>, aDeep=true, aNullParent=true) + 188 at HTMLStyleElement.cpp:162
    frame #10: 0x0000000102bc337e XUL`mozilla::dom::FragmentOrElement::cycleCollection::Unlink(this=<unavailable>, p=0x000000010e028790) + 446 at FragmentOrElement.cpp:1270
    frame #11: 0x0000000102bd1b9c XUL`mozilla::dom::ShadowRoot::cycleCollection::Unlink(this=0x00000001062cec70, p=0x000000010e028790) + 60 at ShadowRoot.cpp:44
    frame #12: 0x000000010152b6bb XUL`nsCycleCollector::CollectWhite(this=0x000000010066f000) + 523 at nsCycleCollector.cpp:2999
    frame #13: 0x000000010152c755 XUL`nsCycleCollector::Collect(this=0x000000010066f000, aCCType=ShutdownCC, aBudget=0x00007fff5fbfed98, aManualListener=0x0000000000000000) + 197 at nsCycleCollector.cpp:3302
    frame #14: 0x000000010152d1e2 XUL`nsCycleCollector::Shutdown() [inlined] nsCycleCollector::ShutdownCollect(this=0x000000010066f000) + 39 at nsCycleCollector.cpp:3245
    frame #15: 0x000000010152d1bb XUL`nsCycleCollector::Shutdown(this=0x000000010066f000) + 43 at nsCycleCollector.cpp:3478

The problem is on this line in FlushSkinSheets:

  nsCOMPtr<nsIDocument> doc =
    mLoader->mBinding->XBLDocumentInfo()->GetDocument();

This ends up being null, so we get the crash when we try to dereference doc.  I think the problem is that we're running FlushSkinSheets() after the XBLDocumentInfo has been Unlinked, so mDocument is null.  nsXBLPrototypeResources::FlushSkinSheets() returns early if mStyleSheetList is empty, and nsXBLPrototypeBinding::FlushSkinSheets() returns early if mResources is empty, so maybe there's some way to change those classes unlink?  But then I don't think we can ensure those are unlinked earlier, so I'm not sure.
Another solution would be to just return after we've cleared mRuleProcessor and mStyleSheetList if doc is null...
In
  nsCOMPtr<nsIDocument> doc =
    mLoader->mBinding->XBLDocumentInfo()->GetDocument();
what is null exactly?
doc.  The crash is on the next line, that uses doc.
Here's what I was talking about, in terms of a null check.  You end up hitting this assertion instead:

Assertion failure: found (Trying to remove a sheet from a ShadowRoot that does not exist.), at /Users/amccreight/mc/content/base/src/ShadowRoot.cpp:172

again, inside HTMLStyleElement::UnbindFromTree().
Something in my stack of about 30 XBL cleanup patches seems to fix this...
Blocks: 957109
Well, something else in that stack of patches seems to cause problems, so that's not much of a solution.

With ICC and the patch in bug 957109, a crash with what looks like the same stack is near permaorange on WinXP debug, and has shown up at least once on L64 opt: https://tbpl.mozilla.org/?tree=Try&rev=3dca6f6d19db

William, could you please look into why we get this crash on trunk when you run the single directory, as in comment 0?  Hopefully if that is fixed, then my problem with ICC enabled will also go away.
Flags: needinfo?(wchen)
here's the try run with my stack of patches that fixed this crash, but had other problems: https://tbpl.mozilla.org/?tree=Try&rev=282ebdae9bba

here's the try run with the patch for bug 957109 and ICC:
https://tbpl.mozilla.org/?tree=Try&rev=3dca6f6d19db
Yeah, I'll investigate this.
Assignee: nobody → wchen
Flags: needinfo?(wchen)
It looks like there is a race between unlinking the nsXBLDocumentInfo and unlinking the ShadowRoot. Unlinking the ShadowRoot unbinds all the children, and unbinding the style element requires us to remove the associated style sheet from the nsXBLPrototypeResources and then flush. Unlinking nsXBLDocumentInfo nulls out the document pointer that is used during ::FlushSkinSheets.

This doesn't occur in XBL because we can't dynamically add/remove styles with the style element.

mccr8: I've modified your patch slightly so that it doesn't clear out some state when we don't have a document (this is what caused the subsequent assertion). Does this patch make the ICC problem go away?
It looked like that works, thanks!

XP debug M4 looks very green now: https://tbpl.mozilla.org/?tree=Try&rev=a68cdbaea22e
Attachment #8402227 - Attachment is obsolete: true
William, is this ready for review and landing?
Flags: needinfo?(wchen)
Attachment #8404357 - Flags: review?(mrbkap)
Flags: needinfo?(wchen)
Attachment #8404357 - Flags: review?(mrbkap) → review+
Blocks: 1000974
https://hg.mozilla.org/mozilla-central/rev/5e6769176ce1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.