Closed Bug 489925 Opened 16 years ago Closed 15 years ago

Crash [@ nsContentUtils::PositionIsBefore] with XBL, two elements that have the same ID

Categories

(Core :: XBL, defect, P2)

x86
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed
blocking1.9.1 --- .4+
status1.9.1 --- .4-fixed

People

(Reporter: jruderman, Assigned: bzbarsky)

References

Details

(5 keywords, Whiteboard: [sg:critical])

Crash Data

Attachments

(3 files)

Makes nsContentUtils::PositionIsBefore call random addresses, like 0x72655372.
Whiteboard: [sg:critical]
Doesn't work when loaded from Bugzilla, probably because this is a security-sensitive bug and Bugzilla doesn't allow token reuse.
So fundamentally, I think the issue is that we let document.getElementById (and various other methods that use that table) see XBL anonymous content. The problem is, we might get no notifications when such content is unbound from the document, so we end up with a stale pointer in the id list. I think the right thing to do is to change the notification handling to ignore anon content, just like content lists do.
Yes, that would correspond to the point when the id table started keeping track of notifications in all cases. This could be triggered before that change by simply forcing a live table (do 65 getElementById lookups for nonexistent IDs) before setting up the XBL. That said, this is not crashing anymore for me... Any idea when that happened?
Hmm. Except for extra fun my nightlies _are_ crashing while my self-builds are not...
Ah, I see. This stopped crashing between 2009-08-02-03 (rev: 8366e5cc9f57) and 2009-08-03-03 (rev: 8795a7a9817c). Pushlog range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8366e5cc9f57&tochange=8795a7a9817c Backing out bug 494546 locally makes this crash reappear.
Attached patch Proposed fix (deleted) — Splinter Review
Pretty straightforward patch; the test makes it clear why we want to make this change. Note that I could have called the nsContentUtils method that nsContentList calls, but since we know we're a document, we can avoid the extra IsNodeOfType check that call would entail....
Assignee: jonas → bzbarsky
Status: NEW → ASSIGNED
Attachment #392883 - Flags: review?(jst)
For extra fun, that patch causes some chrome mochitests to fail. Investigating now.
We probably need to fix this on branches, though the compat issue sort of scares me....
blocking1.9.1: --- → ?
Flags: blocking1.9.2?
Flags: blocking1.9.0.14?
I suppose for branches we could try not changing the hashtable behavior but somehow notifying on binding teardown... That sounds pretty risky in terms of getting the code right, though, even if it might have better extension compat.
Depends on: 508819
I filed bug 508819 on the sessionstore issue. It's actually assuming it can get anon nodes via getElementById....
This hasn't landed on trunk yet. I think we should fix it, but it should probably wait 'til the next release since we're supposed to freeze this coming Tuesday.
blocking1.9.1: ? → needed
Flags: blocking1.9.0.14? → blocking1.9.0.15?
blocking1.9.1: needed → .4+
Flags: blocking1.9.0.15? → blocking1.9.0.15+
Attachment #392883 - Flags: superreview+
Attachment #392883 - Flags: review?(jst)
Attachment #392883 - Flags: review+
Comment on attachment 392883 [details] [diff] [review] Proposed fix bz, any updates on this breaking chrome mochitests? If it isn't, r+sr=jst...
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2
> bz, any updates on this breaking chrome mochitests? I fixed that in bug 508819.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 392883 [details] [diff] [review] Proposed fix Need this on all the branches too.
Attachment #392883 - Flags: approval1.9.2?
Attachment #392883 - Flags: approval1.9.1.4?
Attachment #392883 - Flags: approval1.9.0.15?
Comment on attachment 392883 [details] [diff] [review] Proposed fix Approved for 1.9.1.4 and 1.9.0.15, a=dveditz for release-drivers
Attachment #392883 - Flags: approval1.9.1.4?
Attachment #392883 - Flags: approval1.9.1.4+
Attachment #392883 - Flags: approval1.9.0.15?
Attachment #392883 - Flags: approval1.9.0.15+
Comment on attachment 392883 [details] [diff] [review] Proposed fix This doesn't need approval1.9.2 since it's already blocking.
Attachment #392883 - Flags: approval1.9.2?
Target Milestone: mozilla1.9.2 → mozilla1.9.3a1
Attached patch Patch merged to 1.9.0 (deleted) — Splinter Review
Checking in content/html/document/src/nsHTMLDocument.cpp; /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v <-- nsHTMLDocument.cpp new revision: 3.793; previous revision: 3.792
Keywords: fixed1.9.0.15
Running the attached test case on OS X on Firefox 3.5.3 does not cause a crash on load. This was loading the case over http from my own webserver (not bugzilla) as well.
This is also the case when using the testcase on my own webserver with Firefox 3.0.14. I cannot reproduce the crash.
Jesse, any suggestions on getting the testcase to crash with pre-fix builds?
I was testing loading from file:, using a Mac build from mozilla-central at the time. Try that?
Tried doing that with retail and debug 1.9.1.3 and no crash. Looks like it might be trunk only.
verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a1pre) Gecko/20090930 Minefield/3.7a1pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.4pre) Gecko/20091001 Shiretoko/3.5.4pre
Status: RESOLVED → VERIFIED
Keywords: verified1.9.1
Depends on: 521969
Depends on: 522030
Group: core-security
Crash Signature: [@ nsContentUtils::PositionIsBefore]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: