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)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: benjamin.lerner, Assigned: roc)

References

Details

(4 keywords)

Crash Data

Attachments

(3 files)

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
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 ]
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
Severity: normal → critical
Keywords: crash
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?)
No.

I'm not sure we should block on this, but I'll see what I can do.
Assignee: nobody → roc
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
Attached patch fixSplinter Review
Assignee: nobody → roc
Attachment #380935 - Flags: review?(bzbarsky)
Attachment #380935 - Flags: superreview+
Attachment #380935 - Flags: review?(bzbarsky)
Attachment #380935 - Flags: review+
Flags: blocking1.9.1? → blocking1.9.1+
Whiteboard: [needs landing]
Actually I don't have the power to do that.
Flags: blocking1.9.1+ → blocking1.9.1?
this is approved to land on mozilla-central, and if it stays green there, please nominate for 191
http://hg.mozilla.org/mozilla-central/rev/fa268a628b36
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Flags: wanted1.9.1.x+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Is this ready for 1.9.1 yet?  If so, can we get it landed?
Yeah, it's ready.
Whiteboard: [needs 191 landing]
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
Target Milestone: --- → mozilla1.9.2a1
Crash Signature: [@ ntdll.dll@0x60f34 ]
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.

Attachment

General

Creator:
Created:
Updated:
Size: