1289129 - Sort out how AutoJSAPI should handle gray-colored objects

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This is basically bug 1283634 comment 12, but split out into a separate bug.  We're getting orange from this even without the JSAutoCompartment asserts of bug 1283634.  The particular orange I observed looks like this:

  js::CompartmentChecker::check [jscntxtinlines.h:f983da56886a : 71 + 0x18]
  js::AssertSameCompartment [jscntxtinlines.h:f983da56886a : 162 + 0x4]
  mozilla::dom::NativeInterface2JSObjectAndThrowIfFailed [BindingUtils.cpp:f983da56886a : 1000 + 0x4]
  mozilla::dom::XPCOMObjectToJsval [BindingUtils.cpp:f983da56886a : 1075 + 0x11]
  mozilla::dom::WrapNativeISupports<nsPIDOMWindowInner> [BindingUtils.h:f983da56886a : 1536 + 0xf]
  mozilla::dom::FindAssociatedGlobal<nsPIDOMWindowInner*> [BindingUtils.h:f983da56886a : 1613 + 0x4]
  mozilla::dom::IDBDatabaseBinding::Wrap [IDBDatabaseBinding.cpp:f983da56886a : 918 + 0x16]
  mozilla::dom::IDBDatabaseBinding::Wrap<mozilla::dom::IDBDatabase> [IDBDatabaseBinding.h:f983da56886a : 54 + 0x23]
  mozilla::dom::binding_detail::DoGetOrCreateDOMReflector<mozilla::dom::IDBDatabase, (mozilla::dom::binding_detail::GetOrCreateReflectorWrapBehavior)0u> [BindingUtils.h:f983da56886a : 955 + 0xa]
  ResultHelper::GetResult [BindingUtils.h:f983da56886a : 1026 + 0xe]
  mozilla::dom::IDBRequest::SetResultCallback [IDBRequest.cpp:f983da56886a : 386 + 0x8] 

This is failing under js::AssertSameCompartment(aCx, aScope) in NativeInterface2JSObjectAndThrowIfFailed which means aScope is gray.  

The caller is XPCOMObjectToJsval which just passes "scope" on through.

The caller of _that_ is WrapNativeISupports<nsPIDOMWindowInner> which does:

  JS::Rooted<JSObject*> scope(cx, JS::CurrentGlobalOrNull(cx)); 

OK, so where did the current global come from?  It came from IDBRequest::SetResultCallback which set up an AutoJSAPI and then started wrapping things using that JSContext.

So we need to figure out how this stuff should work with AutoJSAPI.  Another comment coming up (for clarity) which describes the setup there and why it's failing.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I will posit that fundamentally having a gray current global is buggy.  If nothing else, the current global is a GC root, so having a gray object there is bad.

There are two ways to use AutoJSAPI:

1)  Init without an nsIGlobalObject then enter some compartment on the resulting JSContext.
    Not much we can really do about this one.
2)  Init with an nsIGlobalObject.  This uses GetGlobalJSObject() and enters its compartment.
    Importantly, GetGlobalJSObject can return a gray object.

It seems to me that we should really ExposeObjectToActiveJS in case 2.  That will silence immediate asserts, at least.  But then there's the question of _keeping_ the object black (this is the sec-sensitive part of this bug).  Consider this sort of code:

  AutoJSAPI jsapi;
  jsapi.Init(GetGlobal1());
  {
    JSAutoCompartment ac(jsapi.cx(), GetGlobal2());
    // Run some script
  }

At this point, we're back in the compartment of global1.  The object is still alive, because the AutoJSAPI has a strong ref to the nsIGlobalObject.  But I don't see anything that would prevent it from being gray.

One option here is to have AutoJSAPI stick the global in a Rooted inside itself, to ensure it stays black while the AutoJSAPI is alive.  Another option, which would more generally solve this sort of problem, is for JSAutoCompartment to stick oldCompartment's global in a Rooted inside itself....

In either case we need to do some auditing for things like Maybe<AutoJSAPI> and Maybe<JSAutoCompartment> respectively to ensure that things don't interleave in bad ways.

Or am I just missing something that makes all of this work somehow and we just need that initial ExposeObjectToActiveJS() call?

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm told that we, in practice, root the globals of all compartments that have a nonzero enterCompartmentDepth.  So it sounds like we do in fact just need to do the simple thing.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: When setting up an AutoJSAPI with a global, make sure to expose that global to active JS

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dddc81f6047d
When setting up an AutoJSAPI with a global, make sure to expose that global to active JS.  r=mccr8

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dddc81f6047d