Bugzilla@Mozilla – Bug 489925
Crash [@ nsContentUtils::PositionIsBefore] with XBL, two elements that have the same ID
Last modified: 2009-11-09 18:59:00 PST
Summon comment box
Created attachment 374395 [details] testcase (crashes Firefox when loaded) Makes nsContentUtils::PositionIsBefore call random addresses, like 0x72655372.
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.
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
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
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.
Created attachment 392883 [details] [review] Proposed fix 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....
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....
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.
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.
Comment on attachment 392883 [details] [review] Proposed fix bz, any updates on this breaking chrome mochitests? If it isn't, r+sr=jst...
> bz, any updates on this breaking chrome mochitests? I fixed that in bug 508819.
Pushed http://hg.mozilla.org/mozilla-central/rev/c5fe17b1caa9
Comment on attachment 392883 [details] [review] Proposed fix Need this on all the branches too.
Comment on attachment 392883 [details] [review] Proposed fix Approved for 1.9.1.4 and 1.9.0.15, a=dveditz for release-drivers
Comment on attachment 392883 [details] [review] Proposed fix This doesn't need approval1.9.2 since it's already blocking.
Pushed: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/936e1b6732a0 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dd9b10ed7eeb
Created attachment 399309 [details] [review] Patch merged to 1.9.0
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
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