Closed Bug 992571 Opened 6 years ago Closed 5 years ago
Null deref in ns
XBLPrototype Resources::Flush Skin Sheets called during unlinking
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...
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.
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
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?
Attachment #8404357 - Flags: review?(mrbkap)
Attachment #8404357 - Flags: review?(mrbkap) → review+
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.