Closed
Bug 992571
Opened 11 years ago
Closed 11 years ago
Null deref in nsXBLPrototypeResources::FlushSkinSheets called during unlinking
Categories
(Core :: XBL, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: mccr8, Assigned: wchen)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•11 years ago
|
||
Another solution would be to just return after we've cleared mRuleProcessor and mStyleSheetList if doc is null...
Comment 2•11 years ago
|
||
In
nsCOMPtr<nsIDocument> doc =
mLoader->mBinding->XBLDocumentInfo()->GetDocument();
what is null exactly?
Reporter | ||
Comment 3•11 years ago
|
||
doc. The crash is on the next line, that uses doc.
Reporter | ||
Comment 4•11 years ago
|
||
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().
Reporter | ||
Comment 5•11 years ago
|
||
Something in my stack of about 30 XBL cleanup patches seems to fix this...
Reporter | ||
Comment 6•11 years ago
|
||
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)
Reporter | ||
Comment 7•11 years ago
|
||
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
Assignee | ||
Comment 8•11 years ago
|
||
Yeah, I'll investigate this.
Assignee: nobody → wchen
Flags: needinfo?(wchen)
Assignee | ||
Comment 9•11 years ago
|
||
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?
Reporter | ||
Comment 10•11 years ago
|
||
It looked like that works, thanks!
XP debug M4 looks very green now: https://tbpl.mozilla.org/?tree=Try&rev=a68cdbaea22e
Reporter | ||
Updated•11 years ago
|
Attachment #8402227 -
Attachment is obsolete: true
Reporter | ||
Comment 11•11 years ago
|
||
William, is this ready for review and landing?
Flags: needinfo?(wchen)
Assignee | ||
Updated•11 years ago
|
Attachment #8404357 -
Flags: review?(mrbkap)
Flags: needinfo?(wchen)
Updated•11 years ago
|
Attachment #8404357 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 12•11 years ago
|
||
Flags: in-testsuite-
Comment 13•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•