Last Comment Bug 489925 - Crash [@ nsContentUtils::PositionIsBefore] with XBL, two elements that have the same ID
: Crash [@ nsContentUtils::PositionIsBefore] with XBL, two elements that have t...
Status: VERIFIED FIXED
: [sg:critical]
: crash, fixed1.9.0.15, regression, testcase, verified1.9.1
Product: Core
Classification: Components
Component: XBL
: Trunk
: x86 Mac OS X
: P2 critical (vote)
: mozilla1.9.3a1
Assigned To: Boris Zbarsky (:bz)
: xbl
:
: 521969 508819 521639 522030
: 343943 348483
  Show dependency treegraph
 
Reported: 2009-04-23 18:16 PDT by Jesse Ruderman
Modified: 2009-11-09 18:59 PST (History)
15 users (show)
jst: blocking1.9.2+
dveditz: blocking1.9.0.15+
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
[@ nsContentUtils::PositionIsBefore]
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta1-fixed
  .4+
  .4-fixed


Attachments
testcase (crashes Firefox when loaded) (786 bytes, application/xhtml+xml)
2009-04-23 18:16 PDT, Jesse Ruderman
no flags Details
Proposed fix (6.32 KB, patch)
2009-08-05 22:33 PDT, Boris Zbarsky (:bz)
jst: review+
jst: superreview+
dveditz: approval1.9.1.4+
dveditz: approval1.9.0.15+
Details | Diff | Splinter Review
Patch merged to 1.9.0 (3.27 KB, patch)
2009-09-08 13:55 PDT, Boris Zbarsky (:bz)
no flags Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2009-04-23 18:16:25 PDT
Created attachment 374395 [details]
testcase (crashes Firefox when loaded)

Makes nsContentUtils::PositionIsBefore call random addresses, like 0x72655372.
Comment 1 Jesse Ruderman 2009-04-23 18:23:02 PDT
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 (:bz) 2009-04-23 19:38:49 PDT
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 [:mw22] (QA - IRC nick: mw22) 2009-06-10 14:30:55 PDT
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 [:mw22] (QA - IRC nick: mw22) 2009-06-10 14:53:50 PDT
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 (:bz) 2009-08-05 21:23:12 PDT
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 (:bz) 2009-08-05 21:28:21 PDT
Hmm.  Except for extra fun my nightlies _are_ crashing while my self-builds are not...
Comment 7 Boris Zbarsky (:bz) 2009-08-05 21:37:13 PDT
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 (:bz) 2009-08-05 22:33:52 PDT
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....
Comment 9 Boris Zbarsky (:bz) 2009-08-06 06:46:45 PDT
For extra fun, that patch causes some chrome mochitests to fail.  Investigating now.
Comment 10 Boris Zbarsky (:bz) 2009-08-06 08:14:22 PDT
We probably need to fix this on branches, though the compat issue sort of scares me....
Comment 11 Boris Zbarsky (:bz) 2009-08-06 08:15:16 PDT
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 (:bz) 2009-08-06 08:47:07 PDT
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) 2009-08-06 09:14:33 PDT
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, jst@mozilla.com) 2009-09-01 09:39:12 PDT
Comment on attachment 392883 [details] [review]
Proposed fix

bz, any updates on this breaking chrome mochitests? If it isn't, r+sr=jst...
Comment 15 Boris Zbarsky (:bz) 2009-09-01 10:13:13 PDT
> bz, any updates on this breaking chrome mochitests?

I fixed that in bug 508819.
Comment 16 Boris Zbarsky (:bz) 2009-09-02 08:35:18 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/c5fe17b1caa9
Comment 17 Boris Zbarsky (:bz) 2009-09-02 08:36:12 PDT
Comment on attachment 392883 [details] [review]
Proposed fix

Need this on all the branches too.
Comment 18 Daniel Veditz 2009-09-02 15:25:22 PDT
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 19 Samuel Sidler (old account; do not CC) 2009-09-02 15:25:51 PDT
Comment on attachment 392883 [details] [review]
Proposed fix

This doesn't need approval1.9.2 since it's already blocking.
Comment 21 Boris Zbarsky (:bz) 2009-09-08 13:55:46 PDT
Created attachment 399309 [details] [review]
Patch merged to 1.9.0
Comment 22 Boris Zbarsky (:bz) 2009-09-08 13:57:54 PDT
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] 2009-09-14 15:53:07 PDT
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] 2009-09-16 16:48:06 PDT
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] 2009-09-28 16:07:21 PDT
Jesse, any suggestions on getting the testcase to crash with pre-fix builds?
Comment 26 Jesse Ruderman 2009-09-28 19:30:29 PDT
I was testing loading from file:, using a Mac build from mozilla-central at the time.  Try that?
Comment 27 Al Billings [:abillings] 2009-09-29 09:50:11 PDT
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 2009-10-01 11:42:39 PDT
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

Note You need to log in before you can comment on or make changes to this bug.