Closed Bug 1289129 Opened 4 years ago Closed 4 years ago

Sort out how AutoJSAPI should handle gray-colored objects

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

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.
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?
Group: dom-core-security
Flags: needinfo?(terrence)
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.
Group: dom-core-security
Flags: needinfo?(terrence)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8774368 - Flags: review?(continuation) → review+
Pushed by bzbarsky@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/dddc81f6047d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.