I hate XUL. Fix + test coming up, though I still need to try-server this.

Er, found one issue I'm not sure how to deal with. There's an AddElementForID caller in the template code that seems to add an element to the id table. Can this ever be an anonymous element? Does that element ever actually get added to the DOM? If so, where? As far as I can tell, it's added to the id table before it's added to the DOM, which means we can't tell whether it's anonymous...

We're going to re-spin both Firefox 3.0.15 and 3.5.4 for this issue.

Ugh. Here's the deal: according to bz there are two fixes for this: a branch-safe one and a more-correct one. The branch-safe fix will come with some small perf costs (~1% on various metrics, though not Ts) and potential for memory leaks (this worries bz the most, and consequently, me) but has the benefit of not absolutely shattering add-on compatibility. The more-correct fix comes without perf or memory leak costs, but absolutely shatters add-on compatibility and parts of the Firefox UI (which bz said he can fix). We've decided to start with the more pressing branch-safe fix, which will be checked into mozilla-central and mozilla-1.9.2 to get test coverage and measure the performance impact. This means holding the beta a little longer, so marking this as a P1 blocker. Based on the performance impact, we'll make a decision about what to do for mozilla-1.9.2; in order to preserve add-on compatibility, we might have to suck it up.

This applies on top of a backout of the patch for bug 489925.

Comment on attachment 406072 [details] [diff] [review] Proposed fix Looks good! r=jst

Pushed: http://hg.mozilla.org/mozilla-central/rev/b40bf2ef14a7 http://hg.mozilla.org/mozilla-central/rev/b5f738553e38 (backout of the original fix for bug 489925 plus this fix) and http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9ef0bceaf6b2 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/96207da1b7c1 Will wait for those to cycle and give us perf and leak data, then request branch approvals. Will also file followup bug for the real trunk (and maybe 1.9.2) fix.

This is most emphatically NOT in-testsuite+. It can't be until we open up the bug (at which point a modified version of the testcase in bug 489925 should be checked in as a crashtest.

Pushed http://hg.mozilla.org/mozilla-central/rev/36e79c905e64 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/970631f8f91a to fix orange.

Please pay particular attention to the ID_NOT_IN_DOCUMENT stuff. I made changes to Traverse(), ~nsIdentifierMapEntry, and AddIdContent to handle it; any other obvious changes that need to happen? Again, this applies on top of a backout of bug 489925 on 1.9.1 branch.

Lots of merging here... Please look over carefully?

No discerned perf impact or leaking on mozilla-central or mozilla-1.9.2. Boris, how should we be looking for leaks? I'm copying in Leakcat who might be able to help there ... In the meantime, I think we should consider this as fixed for mozilla-1.9.2 until we get evidence that it's caused bigger problems.

As far as leaks, I had two specific worries: 1) Document leaks through shutdown. Just browsing around a bunch with leak logging enabled will catch these. I suspect there won't be many, if any, given the cycle collection hookups this patch ended up taking advantage of. 2) Element leaks for document lifetime. I don't know that we have a sane way to measure this; it would only bite situations where the elements are anonymous, so not likely to hit web pages. Extensions that leak due to this category of leaks would have crashed before, so probably a net win...

(In reply to comment #13) > As far as leaks, I had two specific worries: > So for 1) i will run a test over the Top 10000 Topsites to check for leaks and for 2) a run with the Top Extensions installed

That sounds great. Thanks! We probably want to do that with all of 1.9.2, 1.9.1, and 1.9.0, since the code is significantly different and the cycle collector behavior is significantly different between them. :(

(In reply to comment #15) > That sounds great. Thanks! > > We probably want to do that with all of 1.9.2, 1.9.1, and 1.9.0, since the code > is significantly different and the cycle collector behavior is significantly > different between them. :( yeah that's no problem, can cover all this branches with the vm's

Johnny's not actually CCed to this bug...

Comment on attachment 406177 [details] [diff] [review] 1.9.1 fix Looks good.

Comment on attachment 406179 [details] [diff] [review] 1.9.0 fix Approved for 1.9.0.15. a=ss for release-drivers

Comment on attachment 406177 [details] [diff] [review] 1.9.1 fix Approved for 1.9.1.4. a=ss for release-drivers

Pushed on 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b475463d243a http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e90895696bde for 1.9.1.5 and: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/873b4ef2a3e4 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e178dbd2a389 for 1.9.1.4 (on GECKO1914_20091006_RELBRANCH).

Checked into CVS: Checking in content/html/document/src/nsHTMLDocument.cpp; /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v <-- nsHTMLDocument.cpp new revision: 3.794; previous revision: 3.793 done Checking in content/html/document/src/nsHTMLDocument.h; /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.h,v <-- nsHTMLDocument.h new revision: 3.232; previous revision: 3.231 done Checking in content/html/document/src/nsHTMLDocument.cpp; /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v <-- nsHTMLDocument.cpp new revision: 3.795; previous revision: 3.794 for 1.9.0.16 and: Checking in content/html/document/src/nsHTMLDocument.cpp; /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v <-- nsHTMLDocument.cpp new revision: 3.793.2.1; previous revision: 3.793 done Checking in content/html/document/src/nsHTMLDocument.cpp; /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v <-- nsHTMLDocument.cpp new revision: 3.793.2.1; previous revision: 3.793 done for 1.9.0.15

Er, the last part of the 1.9.0.15 cvs thing should have been: Checking in content/html/document/src/nsHTMLDocument.h; /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.h,v <-- nsHTMLDocument.h new revision: 3.231.12.1; previous revision: 3.231 done Checking in content/html/document/src/nsHTMLDocument.cpp; /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v <-- nsHTMLDocument.cpp new revision: 3.793.2.2; previous revision: 3.793.2.1 done

Boris, I wonder how I can verify this fix. I tried your attached crash test with builds from 10/13 but I cannot get those to crash.

The attached test doesn't crash in those builds due to the fix for bug 489925 being present in them. Let me see if I can write a testcase which will crash them.

OK, I haven't succeeded at it yet, because XUL does weird things on subtree removals... I _think_ it's still possible to trigger such crashes, but I'm not sure its worth spending a few days to figure out the exact right codepath to hit.

This doesn't crash with build 3 of 1.9.0.15 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.15) Gecko/2009101601 Firefox/3.0.15 (.NET CLR 3.5.30729)) or build 1 of it or 1.9.0.14. Is there a means to verify this bug for 1.9.0 and 1.9.1?

Al, I don't have a crash testcase for you, sorry. One could try to verify buy fuzzing the testcase posted in this bug and especially a XUL (not XHTML) version of that testcase?