Closed Bug 398006 Opened 17 years ago Closed 17 years ago

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

Categories

(Core :: XBL, defect, P1)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta1

People

(Reporter: martijn.martijn, Assigned: bzbarsky)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [sg:critical?] 1.8-branch not so crashy)

Crash Data

Attachments

(6 files)

Attached file binding needed for testcase (deleted) —
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..
Attached file testcase (deleted) —
Attached file testcase2 (deleted) —
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.
Attached patch Proposed fix (deleted) — Splinter Review
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #282955 - Flags: superreview?(dbaron)
Attachment #282955 - Flags: review?(jonas)
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....
Group: security
Flags: in-testsuite?
Flags: blocking1.8.1.8?
Priority: -- → P1
Summary: Crash [@ ChildIterator::get] with table, form, button and bindings → [FIX]Crash [@ ChildIterator::get] with table, form, button and bindings
Target Milestone: --- → mozilla1.9 M9
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.
Flags: blocking1.8.1.8? → blocking1.8.1.9?
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.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Attachment #282955 - Flags: superreview?(dbaron) → superreview?(roc)
Attachment #282955 - Flags: superreview?(roc) → superreview+
Attachment #282955 - Flags: approval1.9?
Attachment #282955 - Flags: approval1.9?
Checked in on trunk. Need a branch patch.... or at least to see whether this one might work on branch.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Verified fixed, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007101905 Minefield/3.0a9pre
Status: RESOLVED → VERIFIED
Attached file (deleted) —
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.
Attached patch Branch version of fix (deleted) — Splinter Review
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....
Attachment #285966 - Flags: superreview?(jonas)
Attachment #285966 - Flags: review?(jonas)
Attachment #285966 - Flags: superreview?(jonas)
Attachment #285966 - Flags: superreview+
Attachment #285966 - Flags: review?(jonas)
Attachment #285966 - Flags: review+
Attachment #285966 - Flags: approval1.8.1.12?
Attachment #285751 - Attachment description: testcase that crashes current branch builds → bug 372769 testcase that crashes current branch builds
Attachment #285751 - Attachment is private: true
Flags: blocking1.8.1.12? → blocking1.8.1.12+
Comment on attachment 285966 [details] [diff] [review] Branch version of fix approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #285966 - Flags: approval1.8.1.12? → approval1.8.1.12+
Flags: wanted1.8.1.x+
Whiteboard: [sg:critical?] 1.8-branch not so crashy
Fixed on branch.
Keywords: fixed1.8.1.12
Depends on: 410456
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.
Flags: blocking1.8.1.12+ → blocking1.8.1.12-
Backed out on branch to fix regressions.
Keywords: fixed1.8.1.12
Comment on attachment 285966 [details] [diff] [review] Branch version of fix unapproving patch, need a version without regressions.
Attachment #285966 - Flags: approval1.8.1.12+ → approval1.8.1.12-
I don't think this is even wanted on branch, in fact.
blocking 1.8.0.15
Flags: blocking1.8.0.15+
Attached patch for 1.8.0 (deleted) — Splinter Review
bz, please take a look if this is the right thing for 1.8.0
Attachment #308879 - Flags: review?
ok ... not for the branches
Flags: blocking1.8.0.15+ → blocking1.8.0.15-
Attachment #308879 - Flags: review? → review-
Crash Signature: [@ ChildIterator::get]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: