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

Status: VERIFIED FIXED

(Core :: XBL, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Makes nsContentUtils::PositionIsBefore call random addresses, like 0x72655372.

Comment 1

[Jesse Ruderman]

Doesn't work when loaded from Bugzilla, probably because this is a security-sensitive bug and Bugzilla doesn't allow token reuse.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

[Martijn Wargers (dead)]

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

Comment 4

[Martijn Wargers (dead)]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.  Except for extra fun my nightlies _are_ crashing while my self-builds are not...

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: 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....

Comment 9

[Boris Zbarsky [:bzbarsky]]

For extra fun, that patch causes some chrome mochitests to fail.  Investigating now.

Comment 10

[Boris Zbarsky [:bzbarsky]]

We probably need to fix this on branches, though the compat issue sort of scares me....

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I filed bug 508819 on the sessionstore issue.  It's actually assuming it can get anon nodes via getElementById....

Comment 13

[Samuel Sidler (old account; do not CC)]

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 14

[Johnny Stenback (:jst)]

Comment on attachment 392883 [details] [diff] [review]
Proposed fix

bz, any updates on this breaking chrome mochitests? If it isn't, r+sr=jst...

Comment 15

[Boris Zbarsky [:bzbarsky]]

> bz, any updates on this breaking chrome mochitests?

I fixed that in bug 508819.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/c5fe17b1caa9

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 392883 [details] [diff] [review]
Proposed fix

Need this on all the branches too.

Comment 18

[Daniel Veditz [:dveditz]]

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

Comment 19

[Samuel Sidler (old account; do not CC)]

Comment on attachment 392883 [details] [diff] [review]
Proposed fix

This doesn't need approval1.9.2 since it's already blocking.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Pushed:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/936e1b6732a0
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dd9b10ed7eeb

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch merged to 1.9.0

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Comment 23

[Al Billings [:abillings - ex-MoCo]]

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

[Al Billings [:abillings - ex-MoCo]]

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

[Al Billings [:abillings - ex-MoCo]]

Jesse, any suggestions on getting the testcase to crash with pre-fix builds?

Comment 26

[Jesse Ruderman]

I was testing loading from file:, using a Mac build from mozilla-central at the time.  Try that?

Comment 27

[Al Billings [:abillings - ex-MoCo]]

Tried doing that with retail and debug 1.9.1.3 and no crash. Looks like it might be trunk only.

Comment 28

[Tracy Walker [:tracy]]

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