400705 - [FIX]Crash [@ js_StopResolving] with groupbox, removing binding and field that removes window

Status: VERIFIED FIXED

(Core :: XBL, defect, P1)

Description

[Martijn Wargers (dead)]

Attachment: iframe, needs to be embedded in iframe

See testcase, which crashes current trunk and branch builds after 100ms, so I'm marking it security sensitive for now.

http://crash-stats.mozilla.com/report/index/f0ebc0f0-80c0-11dc-9225-001a4bd46e84
0  	js_StopResolving  	 mozilla/js/src/jscntxt.c:585
1 	js_LookupPropertyWithFlags 	mozilla/js/src/jsobj.c:3422
2 	js_LookupProperty 	mozilla/js/src/jsobj.c:3268
3 	js_DeleteProperty 	mozilla/js/src/jsobj.c:3941
4 	JS_DeleteUCProperty2 	mozilla/js/src/jsapi.c:3561
5 	nsXBLProtoImpl::UndefineFields(JSContext*, JSObject*) 	mozilla/content/xbl/src/nsXBLProtoImpl.cpp:266
6 	nsXBLBinding::ChangeDocument(nsIDocument*, nsIDocument*) 	mozilla/content/xbl/src/nsXBLBinding.cpp:1065
7 	nsXBLService::FlushStyleBindings(nsIContent*) 	mozilla/content/xbl/src/nsXBLService.cpp:618
8 	nsXBLService::LoadBindings(nsIContent*, nsIURI*, nsIPrincipal*, int, nsXBLBinding**, int*) 	mozilla/content/xbl/src/nsXBLService.cpp:522
9 	nsCSSFrameConstructor::ConstructFrameInternal(nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int, nsStyleContext*, nsFrameItems&, int) 	mozilla/layout/base/nsCSSFrameConstructor.cpp:7639
10 	nsCSSFrameConstructor::ConstructFrame(nsFrameConstructorState&, nsIContent*, nsIFrame*, nsFrameItems&) 	mozilla/layout/base/nsCSSFrameConstructor.cpp:7594
etc...

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Apparently, it doesn't crash online. It crashes however, when you close the tab, and then do "undo close tab".
Probably, using data urls, would make it crash online, directly.

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase that uses data urls

Comment 4

[Boris Zbarsky [:bzbarsky]]

For what it's worth, branch and trunk have to be crashing for somewhat different reasons: the code in the stack in comment 0 doesn't exist on branch.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, so on branch, we're crashing because the initial binding attachment blows away the content.  That's bug 372769.

On trunk, binding _attachment_ is ok, but binding _detachment_ can run script because deleting a property resolves it.  I'll put up a fix for that.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 7

[Boris Zbarsky [:bzbarsky]]

That fixes bug 400780 too.  Or rather the testcase in that bug.

Comment 8

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

Comment on attachment 285837 [details] [diff] [review]
Like so

Alternatively, could you instead check if the property is set before undefining it? Or would that be a perf problem?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Checking whether the property is set means trying to resolve it, which means executing the field in the case of fields.

There is no public JS API for checking whether a property is set in just the scope of the object in question (which is ideally exactly what we would do).

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> Checking whether the property is set means trying to resolve it, which means
> executing the field in the case of fields.
> 
> There is no public JS API for checking whether a property is set in just the
> scope of the object in question (which is ideally exactly what we would do).

So you want JS_HasOwnProperty, etc. Please file a JS engine bug and a followup XBL bug blocked by it.

/be

Comment 11

[Boris Zbarsky [:bzbarsky]]

Filed bug 400793 and bug 400794.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 285837 [details] [diff] [review]
Like so

Fixes crash regression from the "actually detach bindings" change.  Also makes us not execute fields when we tear down the binding, which can only be a good thing.

Risk is very low, I think.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 285837 [details] [diff] [review]
Like so

a=endgame drivers for M9 landing

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 15

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/macppc/winxp on attachment 285745 [details] and attachment 285746 [details].

Comment 16

[Jeff Walden [:Waldo]]

http://mxr.mozilla.org/mozilla/source/content/xbl/test/test_bug400705.xhtml

Comment 17

[Daniel Veditz [:dveditz]]

Given comment 5, do we only need bug 372769 on the 1.8 branch or will we need this fix as well?

Comment 18

[Boris Zbarsky [:bzbarsky]]

It depends on how we fix bug 372769...  If we take sicking's fix (which does all the implementation-attachment stuff in EndUpdate) we shouldn't need this change.

Comment 19

[Daniel Veditz [:dveditz]]

(In reply to comment #18)
> It depends on how we fix bug 372769...

Given still-open regressions we're unlikely to take the fix in bug 372769 on the branch any time soon. Can we take this one instead?

Comment 20

[Boris Zbarsky [:bzbarsky]]

I think I failed to communicate there...

The patch in this bug fixes a regression from bug 372769.  That was the trunk crash.

The branch crash is just a duplicate of bug 372769.

So we only need this patch on branch if we take bug 372769 on branch (which I agree we shouldn't do).