Closed
Bug 495635
Opened 15 years ago
Closed 15 years ago
Using removeelement="true" with nested overlays leads to crash [@ ntdll.dll@0x60f34 ]
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: benjamin.lerner, Assigned: roc)
References
Details
(4 keywords)
Crash Data
Attachments
(3 files)
1.01 KB,
application/x-compressed-tar
|
Details | |
3.25 KB,
application/zip
|
Details | |
3.87 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.10) Gecko/2009042523 Ubuntu/9.04 (jaunty) Firefox/3.0.10 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b5pre) Gecko/20090529 Shiretoko/3.5b5pre In examining exactly how XUL overlays get loaded and overlay points get merged (nsXULDocument::AddChromeOverlays and ::Merge), and looking at the loading order of overlays, I found the following combination leads to a reliable crash: In extension 1: Two overlays are registered, such that extA1.xul overlays browser.xul, and extA2.xul overlays extA1.xul. extA1 creates an extension point (a statusbar) that extA2 then uses (creating a statusbar panel). (This pattern is simplified from actual code in the DownThemAll extension, for instance.) There is an alert in extA2 to determine when exactly the statusbarpanel is loaded. In extension 2: There is one overlay, extB1.xul, that extends browser.xul. It overlays the statusbar with removeelement="true". Perhaps it really didn't like the extra status bar :-p Reproducible: Always Steps to Reproduce: 1. Install both extensions in the attached zip 2. Run Firefox 3. Crash. The loading order goes as follows: 1) Both extension manifests are read. 2) browser.xul is loaded. The mUnloadedOverlays queue is [ extB1, extA1, other stuff ] <- head of queue 3) extA1 is eventually loaded, enqueuing extA2. The extAbar statusbar is created in the merge queue. The mUnloadedOverlays queue is now [ extA2, extB1 ] <- head of queue 4) extB1 is loaded, and the removal of extAbar is created in the merge queue. 5) extA2 is loaded, and the extApanel statusbarpanel is created in the merge queue. The inline script runs at this point, btw. 6) As the calls to nsXULDocument::Merge work their way down the queue, the extAbar is created in the document, then removed in the document, and then finally when the extApanel tries to get loaded, we get a crash. The relevant snippet of the stack trace is #7 0xb5b4599c in nsXULDocument::GetElementById (this=0xa5d3e38, aId=@0xbfb0080c, aReturn=0xbfb008ac) at /home/blerner/projects/safebrowser/fx-3.5/src/content/xul/document/src/nsXULDocument.cpp:1670 #8 0xb5b3c329 in nsXULDocument::OverlayForwardReference::Resolve (this=0xa29b4d8) at /home/blerner/projects/safebrowser/fx-3.5/src/content/xul/document/src/nsXULDocument.cpp:4414 #9 0xb5b3dbce in nsXULDocument::ResolveForwardReferences (this=0xa5d3e38) at /home/blerner/projects/safebrowser/fx-3.5/src/content/xul/document/src/nsXULDocument.cpp:1209 #10 0xb5b4b431 in nsXULDocument::ResumeWalk (this=0xa5d3e38) at /home/blerner/projects/safebrowser/fx-3.5/src/content/xul/document/src/nsXULDocument.cpp:3606
Reporter | ||
Comment 1•15 years ago
|
||
Comment 2•15 years ago
|
||
Crash report: http://crash-stats.mozilla.com/report/index/f0aedee1-36f7-4114-a6c4-96a8b2090530?p=1 Regression window is: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-06-22+16%3A00%3A00&enddate=2008-06-22+21%3A00%3A00
Blocks: 344258
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9.1?
Keywords: regression,
testcase
Summary: Using removeelement="true" with nested overlays leads to crash → Using removeelement="true" with nested overlays leads to crash [@ ntdll.dll@0x60f34 ]
Comment 3•15 years ago
|
||
Updated•15 years ago
|
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
Comment 4•15 years ago
|
||
roc/jst: this looks like it was caused by: Bug 344258 - Make XUL use the shared ID-element map or Bug 344258 - Move ID-content map up from nsHTMLDocument to nsDocument to prepare for using it across all document types. of the two of those, I'd suspect the first patch, which is: http://hg.mozilla.org/mozilla-central/rev/b1c0e2562c68 (should this be --> Core::SVG?)
Assignee | ||
Comment 5•15 years ago
|
||
No. I'm not sure we should block on this, but I'll see what I can do.
Assignee: nobody → roc
Comment 6•15 years ago
|
||
OK, so I see us process the remove: nsXULDocument::RemoveElement nsXULDocument::OverlayForwardReference::Merge nsXULDocument::OverlayForwardReference::Resolve Then we unwind to Resolve, where the element we just removed is on the stack in the |target| nsCOMPtr. Then we run this code: if (!notify) { // Add child and any descendants to the element map rv = mDocument->AddSubtreeToDocument(target); if (NS_FAILED(rv)) return eResolve_Error; } But the node is not in the document anymore, and in fact it has an mRefCnt of 1 (well, its purple buffer entry does). So once this nsCOMPtr goes out of scope, the node is dead. And we have a pointer to this dead node in the id hashtable. The code above is just completely bogus if the node can get removed from the document by Merge.
Assignee: roc → nobody
Assignee | ||
Comment 7•15 years ago
|
||
Assignee: nobody → roc
Attachment #380935 -
Flags: review?(bzbarsky)
Updated•15 years ago
|
Attachment #380935 -
Flags: superreview+
Attachment #380935 -
Flags: review?(bzbarsky)
Attachment #380935 -
Flags: review+
Assignee | ||
Updated•15 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Whiteboard: [needs landing]
Assignee | ||
Comment 8•15 years ago
|
||
Actually I don't have the power to do that.
Flags: blocking1.9.1+ → blocking1.9.1?
Comment 9•15 years ago
|
||
this is approved to land on mozilla-central, and if it stays green there, please nominate for 191
Updated•15 years ago
|
Attachment #380935 -
Flags: approval1.9.1+
Assignee | ||
Comment 10•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/fa268a628b36
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Updated•15 years ago
|
Flags: wanted1.9.1.x+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Comment 11•15 years ago
|
||
Is this ready for 1.9.1 yet? If so, can we get it landed?
Assignee | ||
Comment 13•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4ea1589d3662
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Comment 14•15 years ago
|
||
Verified fixed on trunk and 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090604 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090604042555 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090604 Shiretoko/3.5pre ID:20090604045218
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Target Milestone: --- → mozilla1.9.2a1
Updated•13 years ago
|
Crash Signature: [@ ntdll.dll@0x60f34 ]
Comment 15•6 years ago
|
||
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
You need to log in
before you can comment on or make changes to this bug.
Description
•