Phil Ringnalda (:philor) Reporter
	Description • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=3b9368919def (or https://tbpl.mozilla.org/?tree=Try&rev=66f4e2177c4f without the unrelated crasher that was recently backed out of aurora) - everything except cpp/jit/xpcshell, which don't open windows, will assert on debug "Failed to get script global: 'NS_SUCCEEDED(rv) && newInnerGlobal && newInnerWindow->GetWrapperPreserveColor() == newInnerGlobal'" and then crash [@ nsGlobalWindow::DefineArgumentsProperty(nsIArray*)].

Since both DefineArgumentsProperty and the only RELEASE_BUILD ifdef in dom/base/ are bholley's, I'm kind of looking in that direction, though the appearance of GetWrapperPreserveColor in both the assertion and in DefineArgumentsProperty is suspicious too.

Merge is July 21st, so you've got three weeks starting tomorrow.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 1 • 10 years ago

Looks like the stuff added in bug 1010577 fails, and once that happens, setting up the wrapper for the window doesn't work.
.

19:36:17     INFO -  [2011] ###!!! ASSERTION: This is not supposed to fail!: 'Error', file /builds/slave/try-lx-d-000000000000000000000/build/js/xpconnect/src/nsXPConnect.cpp, line 303
19:36:17     INFO -  UnexpectedFailure<tag_nsresult> [js/xpconnect/src/nsXPConnect.cpp:305]
19:36:17     INFO -  nsXPConnect::GetWrappedNativePrototype(JSContext*, JSObject*, nsIClassInfo*, nsIXPConnectJSObjectHolder**) [js/xpconnect/src/nsXPConnect.cpp:864]
19:36:17     INFO -  GetXPCProto [dom/base/nsDOMClassInfo.cpp:2390]
19:36:17     INFO -  nsWindowSH::GlobalResolve(nsGlobalWindow*, JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JSPropertyDescriptor>) [dom/base/nsDOMClassInfo.cpp:2832]
19:36:17     INFO -  nsGlobalWindow::DoNewResolve(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JSPropertyDescriptor>) [dom/base/nsGlobalWindow.cpp:4287]
19:36:17     INFO -  mozilla::dom::WindowBinding::_newResolve [obj-firefox/dom/bindings/WindowBinding.cpp:12846]
19:36:17     INFO -  CallResolveOp [js/src/jsobj.cpp:4022]
19:36:17     INFO -  LookupPropertyInline<(js::AllowGC)1u> [js/src/jsobj.cpp:4111]
19:36:17     INFO -  js::FindClassObject(js::ExclusiveContext*, JS::MutableHandle<JSObject*>, js::Class const*) [js/src/jsobj.cpp:4246]
19:36:17     INFO -  js::FindClassPrototype(js::ExclusiveContext*, JS::MutableHandle<JSObject*>, js::Class const*) [js/src/jsobj.cpp:5560]
19:36:17     INFO -  js::FindProto(js::ExclusiveContext*, js::Class const*, JS::MutableHandle<JSObject*>) [js/src/jsobjinlines.h:822]
19:36:17     INFO -  js::NewObjectWithClassProtoCommon(js::ExclusiveContext*, js::Class const*, JSObject*, JSObject*, js::gc::AllocKind, js::NewObjectKind) [js/src/jsobj.cpp:1568]
19:36:17     INFO -  JS_NewObject(JSContext*, JSClass const*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) [js/src/jsapi.cpp:2586]
19:36:17     INFO -  nsWindowSH::GlobalResolve(nsGlobalWindow*, JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JSPropertyDescriptor>) [dom/base/nsDOMClassInfo.cpp:2652]
19:36:17     INFO -  nsGlobalWindow::DoNewResolve(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JSPropertyDescriptor>) [dom/base/nsGlobalWindow.cpp:4287]
19:36:17     INFO -  mozilla::dom::WindowBinding::_newResolve [obj-firefox/dom/bindings/WindowBinding.cpp:12846]
19:36:17     INFO -  CallResolveOp [js/src/jsobj.cpp:4022]
19:36:17     INFO -  LookupOwnPropertyInline<(js::AllowGC)1u> [js/src/jsobj.cpp:4111]
19:36:17     INFO -  js::DefineNativeProperty(js::ExclusiveContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>), bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>), unsigned int) [js/src/jsobj.cpp:4146]
19:36:17     INFO -  JSObject::defineGeneric(js::ExclusiveContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>), bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>), unsigned int) [js/src/jsobj.cpp:3607]
19:36:17     INFO -  DefinePropertyById [js/src/jsapi.cpp:2976]
19:36:17     INFO -  DefineProperty [obj-firefox/dist/include/js/RootingAPI.h:811]
19:36:17     INFO -  JS_DefineProperties(JSContext*, JS::Handle<JSObject*>, JSPropertySpec const*) [js/src/jsapi.cpp:3391]
19:36:17     INFO -  bool mozilla::dom::DefinePrefable<JSPropertySpec const>(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Prefable<JSPropertySpec const> const*) [dom/bindings/BindingUtils.cpp:314]
19:36:17     INFO -  mozilla::dom::DefineProperties(JSContext*, JS::Handle<JSObject*>, mozilla::dom::NativeProperties const*, mozilla::dom::NativeProperties const*) [dom/bindings/BindingUtils.cpp:624]
19:36:17     INFO -  mozilla::dom::WindowBinding::Wrap(JSContext*, nsGlobalWindow*, nsWrapperCache*, JS::CompartmentOptions&, JSPrincipals*, bool) [obj-firefox/dom/bindings/WindowBinding.cpp:13047]
19:36:17     INFO -  nsGlobalWindow::SetNewDocument(nsIDocument*, nsISupports*, bool) [dom/base/nsGlobalWindow.cpp:2275]

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 10 years ago

I can't reproduce this in my beta build, nor do I see the assertion from comment 1 in the failing tinderbox logs... :(

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 10 years ago

> nor do I see the assertion

Er, it helps to look at a debug log.  Now I do see it, and I guess the issue is not beta per se but aurora with RELEASE_BUILD bits.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 10 years ago

OK, so what's happening here is that an nsGlobalChromeWindow.  We go to define webidl properties on it, since on 32 Window is on WebIDL bindings.  This includes defining the ChromeOnly ones (unlike 31, where quickstubs only defined the normal props).  And _that_ includes "controllers", so we land in nsWindowSH::GlobalResolve while still creating the window's JS object, with id == IDX_CONTROLLERS.

We check for !nsContentUtils::IsSystemPrincipal(aWin->GetPrincipal()), which tests true because aWin->GetPrincipal() is null.  It's null because we're still in nsGlobalWindow::SetNewDocument and we haven't gotten far enough along yet to have set mDoc on the new inner we're creating.

Bobby, can we just replace this check by a subject principal check or an object principal check on "obj" (presumably the same thing in practice, since we're in the compartment of "obj")?

	Phil Ringnalda (:philor) Reporter
	Comment 5 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #3)
> I guess the issue is not beta per se but aurora with RELEASE_BUILD bits.

I'll switch to filing them with explict STR, instead of "go look at my try push's queue and parent, just look at it!"

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 10 years ago

Nah, it was just me not reading very carefully.  Or rather forgetting the context by the time I came back to this bug and just reading comment 1.

	Peter Van der Beken [:peterv]
	Comment 7 • 10 years ago

An alternative is to move the code to CreateGlobalOptions<nsGlobalWindow>::PostCreateGlobal. Not a very elegant solution, but just throwing it out there.

	Bobby Holley (:bholley) Assignee
	Comment 8 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #4)
> Bobby, can we just replace this check by a subject principal check or an
> object principal check on "obj" (presumably the same thing in practice,
> since we're in the compartment of "obj")?

Yeah, let's use |obj|, with a comment.

	Bobby Holley (:bholley) Assignee
	Comment 10 • 10 years ago

Philor, can you test this patch on try with the appropriate magic?

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 10 years ago

Comment on attachment 8449030 [details] [diff] [review]
Grab the principal from the object rather than the window when resolving the controller shim. v1

r=me

	Phil Ringnalda (:philor) Reporter
	Comment 12 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=e5b52aa034c9

	Bobby Holley (:bholley) Assignee
	Comment 13 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/76ac9af11768

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 14 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/76ac9af11768

	Phil Ringnalda (:philor) Reporter
	Comment 15 • 10 years ago

Don't forget to request approval-aurora.

	Bobby Holley (:bholley) Assignee
	Comment 16 • 10 years ago

Comment on attachment 8449030 [details] [diff] [review]
Grab the principal from the object rather than the window when resolving the controller shim. v1

See comment 0. We need to take this to avoid breaking all of CI in two weeks.

Approval Request Comment
[Feature/regressing bug #]: bug 1010577
[User impact if declined]: Beta is all orange after the merge
[Describe test coverage new/current, TBPL]: No explicit test coverage
[Risks and why]: Very low risk
[String/UUID change made/needed]: None

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/58fd51655b27