398006 - [FIX]Crash [@ ChildIterator::get] with table, form, button and bindings

Status: VERIFIED FIXED

(Core :: XBL, defect, P1)

Description

[Martijn Wargers (dead)]

Attachment: binding needed for testcase

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

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase2

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 6

[Daniel Veditz [:dveditz]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Martijn Wargers (dead)]

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

Comment 10

[Martijn Wargers (dead)]

Attachment: bug 372769 testcase that crashes current branch builds

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch version of fix

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 13

[Daniel Veditz [:dveditz]]

Comment on attachment 285966 [details] [diff] [review]
Branch version of fix

approved for 1.8.1.12, a=dveditz for release-drivers

Comment 14

[Boris Zbarsky [:bzbarsky]]

Fixed on branch.

Comment 15

[Daniel Veditz [:dveditz]]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Backed out on branch to fix regressions.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 285966 [details] [diff] [review]
Branch version of fix

unapproving patch, need a version without regressions.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[Alexander Sack]

blocking 1.8.0.15

Comment 20

[Alexander Sack]

Attachment: for 1.8.0

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

Comment 21

[Alexander Sack]

ok ... not for the branches