See upcoming testcase, which crashes current trunk build on load. It didn't crash with the 2007-09-28 build, so I guess this is a regression somehow of bug 372769. http://crash-stats.mozilla.com/report/index/bab95751-6e9f-11dc-bb69-001a4bd43e5c 0 ChildIterator::get() mozilla/layout/base/nsChildIterator.h:105 1 nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsIFrame*, int, nsFrameItems&, int) mozilla/layout/base/nsCSSFrameConstructor.cpp:11230 2 nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsStyleDisplay const*, nsIContent*, nsIFrame*, nsIFrame*, nsStyleContext*, nsIFrame**, nsFrameItems&, int) mozilla/layout/base/nsCSSFrameConstructor.cpp:12321 3 nsCSSFrameConstructor::ConstructFrameByDisplayType(nsFrameConstructorState&, nsStyleDisplay const*, nsIContent*, int, nsIAtom*, nsIFrame*, nsStyleContext*, nsFrameItems&, int) mozilla/layout/base/nsCSSFrameConstructor.cpp:6516 4 nsCSSFrameConstructor::ConstructFrameInternal(nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int, nsStyleContext*, nsFrameItems&, int) mozilla/layout/base/nsCSSFrameConstructor.cpp:7662 5 nsCSSFrameConstructor::ConstructFrame(nsFrameConstructorState&, nsIContent*, nsIFrame*, nsFrameItems&) mozilla/layout/base/nsCSSFrameConstructor.cpp:7484 6 nsCSSFrameConstructor::ContentInserted(nsIContent*, nsIContent*, int, nsILayoutHistoryState*) mozilla/layout/base/nsCSSFrameConstructor.cpp:9039 7 nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*) mozilla/layout/base/nsCSSFrameConstructor.cpp:11076 8 nsCSSFrameConstructor::ProcessRestyledFrames(nsStyleChangeList&) mozilla/layout/base/nsCSSFrameConstructor.cpp:9878 9 PresShell::RecreateFramesFor(nsIContent*) mozilla/layout/base/nsPresShell.cpp:3329 10 nsXBLBindingRequest::DocumentLoaded(nsIDocument*) mozilla/content/xbl/src/nsXBLService.cpp:202 etc..

Ok, that didn't work, apparently, it doesn't crash online. But I bet that stuffing the binding in a data uri will make the testcase crash online.

I don't know why this hasn't popped up before (that is, why the <field> thing is relevant), but it's a long-standing issue. We need to make sure to not run script while constructing frames under RecreateFramesFor. As things stand, if we recreate a frame for a XUL element whose parent has a binding which is not attached yet for some reason, we'll end up wrapping the parent in order to wrap the kid, and wrapping the parent will execute its binding constructor. At least that seems to be what's going on here.

Still need a test to make sure the PostCreate code actually functions as desired in the common case (access from script). Help on writing that much appreciated. I think we want something like this on branch too....

Comment on attachment 282955 [details] [diff] [review] Proposed fix Why do we end up wrapping the kid just because we create its frame? The nsDOMClassInfo seems a bit regression prone to me.

bug 372769 is not currently nominated or blocking any 1.8 release, if this is a regression from that fix do we really need this on the branch? Or are you saying we actually need that one too? Given the blocking list we already have I would love to wait until a later release on this one unless I'm missing something that's unapparent.

Daniel, I think you're missing the "I don't know why this hasn't popped up before (that is, why the <field> thing is relevant), but it's a long-standing issue" part of comment 3. I could probably try to create a testcase that would exploitably crash branch and doesn't depend on <field>s in any way whatsoever, but that seems like a waste of time: it's clear the the current code is unsound. > Why do we end up wrapping the kid just because we create its frame? Because it has an XBL binding, so we wrap it in the process of binding attachment. And yes, the DOMClassInfo bit is "regression prone" in that it could change constructor firing order. All cases where it does so are exploitable right now, however. And I really don't see a better solution.

Checked in on trunk. Need a branch patch.... or at least to see whether this one might work on branch.

Verified fixed, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007101905 Minefield/3.0a9pre

Yeah, indeed. We should make sure to get both testcases checked in when we check in tests. I've been working on a branch patch; I hope to have it done shortly.

This does NOT fix the "testcase that crashes current branch builds". That testcase is running into bug 372769. A testcase for this bug would do the removal in the constructor, and end up having the constructor run during frame construction via the classinfo codepath. One way to maybe do that on current branch is to have two bindings: one with a field that grabs another element from JS, and another attached to the element that the field grabs that removes the element the first binding is attached to from the DOM. At least that would be a simple and reliable way to produce this scenario, I think....

Comment on attachment 285966 [details] [diff] [review] Branch version of fix approved for 1.8.1.12, a=dveditz for release-drivers

Fixed on branch.

not blocking1.8.1.12 on this because of regressions and the fact that the fix can be bypassed without landing other fixes on the branch.

Backed out on branch to fix regressions.

Comment on attachment 285966 [details] [diff] [review] Branch version of fix unapproving patch, need a version without regressions.

I don't think this is even wanted on branch, in fact.

blocking 1.8.0.15

bz, please take a look if this is the right thing for 1.8.0

ok ... not for the branches