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)
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)
(deleted),
application/xhtml+xml
|
Details | |
(deleted),
patch
|
jst
:
review+
jst
:
superreview+
dveditz
:
approval1.9.1.4+
dveditz
:
approval1.9.0.15+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
Makes nsContentUtils::PositionIsBefore call random addresses, like 0x72655372.
Reporter | ||
Updated•16 years ago
|
Whiteboard: [sg:critical]
Reporter | ||
Comment 1•16 years ago
|
||
Doesn't work when loaded from Bugzilla, probably because this is a security-sensitive bug and Bugzilla doesn't allow token reuse.
Assignee | ||
Comment 2•16 years ago
|
||
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.
Comment 3•15 years ago
|
||
This regressed between 2008-03-18 and 2008-03-24:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-03-18+08%3A00%3A00&enddate=2008-03-24+08%3A00%3A00
Keywords: regression
Assignee: nobody → jonas
Comment 4•15 years ago
|
||
Sorry, I meant a regression between 2009-03-18 and 2009-03-24:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-03-18+08%3A00%3A00&enddate=2009-03-24+08%3A00%3A00
Assignee | ||
Comment 5•15 years ago
|
||
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?
Assignee | ||
Comment 6•15 years ago
|
||
Hmm. Except for extra fun my nightlies _are_ crashing while my self-builds are not...
Assignee | ||
Comment 7•15 years ago
|
||
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.
Assignee | ||
Comment 8•15 years ago
|
||
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 | ||
Comment 9•15 years ago
|
||
For extra fun, that patch causes some chrome mochitests to fail. Investigating now.
Assignee | ||
Comment 10•15 years ago
|
||
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?
Assignee | ||
Comment 11•15 years ago
|
||
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.
Assignee | ||
Comment 12•15 years ago
|
||
I filed bug 508819 on the sessionstore issue. It's actually assuming it can get anon nodes via getElementById....
Comment 13•15 years ago
|
||
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?
Updated•15 years ago
|
Updated•15 years ago
|
Attachment #392883 -
Flags: superreview+
Attachment #392883 -
Flags: review?(jst)
Attachment #392883 -
Flags: review+
Comment 14•15 years ago
|
||
Comment on attachment 392883 [details] [diff] [review]
Proposed fix
bz, any updates on this breaking chrome mochitests? If it isn't, r+sr=jst...
Updated•15 years ago
|
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2
Assignee | ||
Comment 15•15 years ago
|
||
> bz, any updates on this breaking chrome mochitests?
I fixed that in bug 508819.
Assignee | ||
Comment 16•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 17•15 years ago
|
||
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 18•15 years ago
|
||
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 19•15 years ago
|
||
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?
Updated•15 years ago
|
Target Milestone: mozilla1.9.2 → mozilla1.9.3a1
Assignee | ||
Comment 20•15 years ago
|
||
Assignee | ||
Comment 21•15 years ago
|
||
Assignee | ||
Comment 22•15 years ago
|
||
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
Comment 23•15 years ago
|
||
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.
Comment 24•15 years ago
|
||
This is also the case when using the testcase on my own webserver with Firefox 3.0.14. I cannot reproduce the crash.
Comment 25•15 years ago
|
||
Jesse, any suggestions on getting the testcase to crash with pre-fix builds?
Reporter | ||
Comment 26•15 years ago
|
||
I was testing loading from file:, using a Mac build from mozilla-central at the time. Try that?
Comment 27•15 years ago
|
||
Tried doing that with retail and debug 1.9.1.3 and no crash. Looks like it might be trunk only.
Comment 28•15 years ago
|
||
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
Updated•15 years ago
|
Group: core-security
Updated•13 years ago
|
Crash Signature: [@ nsContentUtils::PositionIsBefore]
You need to log in
before you can comment on or make changes to this bug.
Description
•