1323324 - Compartment mismatch in Promise::CreateFromExisting

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a patch for testing

Promise::Resolve (nor Reject) doesn't seem to guarantee same compartment.
I got the mismatch when JS callback returned a Promise to C++.
(a bit unusual tentative localization API for chrome)

JS::CallOriginalPromiseResolve seems to do CheckedUnwrap.
Making Promise::Resolve to JS_WrapObject causes later then Is<PromiseObject> or some such to fail.


STR:
Use https://hg.mozilla.org/projects/larch and apply the patch and compile debug version and start the browser.

I'll try to debug this a bit more tomorrow, especially to ensure that we don't actually
use these code paths in shipping browser.
But if anyone knows the expected relationship of mPromiseObj and mGlobal->GetGlobalJSObject(), and why such relationship, feel free to tell.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Relevant code in bindings
already_AddRefed<Promise>
L10nCallback::Call(JSContext* cx, JS::Handle<JS::Value> aThisVal, const Sequence<L10nElement>& l10nElements, ErrorResult& aRv)
{
  JS::Rooted<JS::Value> rval(cx, JS::UndefinedValue());
  JS::AutoValueVector argv(cx);
  if (!argv.resize(1)) {
    aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
    return nullptr;
  }
  unsigned argc = 1;

  do {

    uint32_t length = l10nElements.Length();
    JS::Rooted<JSObject*> returnArray(cx, JS_NewArrayObject(cx, length));
    if (!returnArray) {
      aRv.Throw(NS_ERROR_UNEXPECTED);
      return nullptr;
    }
    // Scope for 'tmp'
    {
      JS::Rooted<JS::Value> tmp(cx);
      for (uint32_t sequenceIdx24 = 0; sequenceIdx24 < length; ++sequenceIdx24) {
        // Control block to let us common up the JS_DefineElement calls when there
        // are different ways to succeed at wrapping the object.
        do {
          if (!l10nElements[sequenceIdx24].ToObjectInternal(cx, &tmp)) {
            aRv.Throw(NS_ERROR_UNEXPECTED);
            return nullptr;
          }
          break;
        } while (0);
        if (!JS_DefineElement(cx, returnArray, sequenceIdx24, tmp,
                              JSPROP_ENUMERATE)) {
          aRv.Throw(NS_ERROR_UNEXPECTED);
          return nullptr;
        }
      }
    }
    argv[0].setObject(*returnArray);
    break;
  } while (0);

  JS::Rooted<JS::Value> callable(cx, JS::ObjectValue(*mCallback));
  if (!JS::Call(cx, aThisVal, callable,
                JS::HandleValueArray::subarray(argv, 0, argc), &rval)) {
    aRv.NoteJSContextException(cx);
    return nullptr;
  }
  RefPtr<mozilla::dom::Promise> rvalDecl;
  { // Scope for our GlobalObject, FastErrorResult, JSAutoCompartment,
    // etc.

    JS::Rooted<JSObject*> globalObj(cx, JS::CurrentGlobalOrNull(cx));
    JSAutoCompartment ac(cx, globalObj);
    GlobalObject promiseGlobal(cx, globalObj);
    if (promiseGlobal.Failed()) {
      aRv.Throw(NS_ERROR_UNEXPECTED);
      return nullptr;
    }

    JS::Rooted<JS::Value> valueToResolve(cx, rval);
    if (!JS_WrapValue(cx, &valueToResolve)) {
      aRv.Throw(NS_ERROR_UNEXPECTED);
      return nullptr;
    }
    binding_detail::FastErrorResult promiseRv;
#ifdef SPIDERMONKEY_PROMISE
    nsCOMPtr<nsIGlobalObject> global =
      do_QueryInterface(promiseGlobal.GetAsSupports());
    if (!global) {
      promiseRv.Throw(NS_ERROR_UNEXPECTED);
      promiseRv.MaybeSetPendingException(cx);
      aRv.Throw(NS_ERROR_UNEXPECTED);
      return nullptr;
    }
    rvalDecl = Promise::Resolve(global, cx, valueToResolve,
                                    promiseRv);
    if (promiseRv.MaybeSetPendingException(cx)) {
      aRv.Throw(NS_ERROR_UNEXPECTED);
      return nullptr;
    }
#else
    JS::Handle<JSObject*> promiseCtor =
      PromiseBinding::GetConstructorObjectHandle(cx);
    if (!promiseCtor) {
      aRv.Throw(NS_ERROR_UNEXPECTED);
      return nullptr;
    }
    JS::Rooted<JS::Value> resolveThisv(cx, JS::ObjectValue(*promiseCtor));
    JS::Rooted<JS::Value> resolveResult(cx);
    Promise::Resolve(promiseGlobal, resolveThisv, valueToResolve,
                     &resolveResult, promiseRv);
    if (promiseRv.MaybeSetPendingException(cx)) {
      aRv.Throw(NS_ERROR_UNEXPECTED);
      return nullptr;
    }
    nsresult unwrapRv = UNWRAP_OBJECT(Promise, &resolveResult.toObject(), rvalDecl);
    if (NS_FAILED(unwrapRv)) { // Quite odd
      promiseRv.Throw(unwrapRv);
      promiseRv.MaybeSetPendingException(cx);
      aRv.Throw(NS_ERROR_UNEXPECTED);
      return nullptr;
    }
#endif // SPIDERMONKEY_PROMISE
  }
  return rvalDecl.forget();
}

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible fix

Not sure if this approach is too ugly

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Hmm, that patch wouldn't work with Promise::State(), since that uses JS::GetPromiseState which can't deal with wrapped PromiseObj().
And similar issue with Promise::MaybeResolve and Promise::MaybeReject.

But those could be fixed, I guess.
Or other option is to make Promise::Resolve and Promise::Reject to not use aGlobal for
creation, but get the global from 'p'.


In principle this issue seems to come from CallOriginalPromiseResolve() doing CheckedUnwrap, but
us still using the global for the wrapped value in Promise.

Any comments?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> But if anyone knows the expected relationship of mPromiseObj and
> mGlobal->GetGlobalJSObject(), and why such relationship, feel free to tell

They are expected to be same-compartment.  mGlobal is by definition the global associated with our promise.  In the non-spidermonkey-promise world it represented the global to create the WebIDL reflector in.

In theory now that we've moved to spidermonkey-only promises we could drop mGlobal and just always recover it from mPromiseObj.  We'd need to figure out what to do with CreateWrapper as called from DetailedPromise.

> JS::CallOriginalPromiseResolve seems to do CheckedUnwrap.

Argh, but then doesn't wrap the result into the context compartment.  This looks like a bug in JS::CallOriginalPromiseResolve to me from first principles.  Same for JS::CallOriginalPromiseReject.

But ok, we have a more fundamental question: what should happen with Promise::Resolve when it's passed a cross-compartment wrapper for a Promise?  What DOM promises used to do was take the incoming nsIGlobalObject (which was _very_ carefully picked in bindings code; see the huge comment in the isPromise case in getJSToNativeConversionInfo) and then do all the NewPromiseCapability stuff, etc.  They made no guarantees about the resulting object being in any particular compartment, because that stuff can do whatever.  What they did guarantee is that the return JS value would (1) be in the caller's compartment and (2) would be a possibly-cross-compartment-wrapper for a reflector for a dom::Promise.  The dom::Promise would be guaranteed to be in the compartment we carefully picked, except when the "is already a Promise with the right .constructor" fast-path happened, in which case it would just be whatever was passed in.

Note that this is what the spec kinda requires, as far as I can tell: the thing returned from the initial Promise.resolve per spec will be guaranteed to be a Promise whose prototype is the canonical Promise.prototype for the same global as the Promise constructor passed as the "this" value to resolve(), unless people mess with .constructor values.  In that case it can end up being whatever, as long as it's an actual Promise.

OK, so what's the new code doing?  I'm ... actually not sure.  It's got comments about "Instead of getting the `constructor` property, do an unforgeable check" and corresponding code that matches neither the old code nor the spec, as far as I can tell.  That's the CheckedUnwrap codepath under discussion here.  I don't see why it's doing that!  It needs to check for being a PromiseObject after unwrapping _and_ having the right constructor.

OK, so say we did that.  You could still spoof the constructor and end up with the current situation, kinda.  How would we want to represent that?  I think what we would want to do is just go ahead and create a dom::Promise around whatever we got back from the canonical Promise.resolve call.

What that means in practice is that we should do the following:

1)  Fix PromiseObject::unforgeableResolve to do the right thing in terms
    of .constructor checks.
2)  Check whether PromiseObject::unforgeableResolve does the right thing in terms
    of acting as if the canonical resolve method were being called with the Promise
    ctor for the current compartment as "this".  If it's not, fix it.
3)  Fix PromiseObject::unforgeableResolve to return the result in the caller
    compartment in all cases.  Just returning the argument (not its CheckedUnwrap) in the
    fast-path case should do the trick.
4)  Change Promise::Resolve to not take an nsIGlobalObject argument at all.
    Instead, it should work in the current compartment of aCx, then CheckedUnwrap
    the return value of JS::CallOriginalPromiseResolve and use the global of that
    unwrapped thing, and the unwrapped thing itself, to construct its dom::Promise.
    That will mirror what the old code ended up doing in practice, I believe.
    I'd appreciate a double-check here!

And we need tests.  Unfortunately, this code is only reachable for Promise WebIDL arguments or callback return values.  We have a shortage of such things.  Especially if we exclude the serviceworker bits, because those don't give a way to test the multiple-compartments behavior.

Maybe we should just add an explicit testing function on a testing interface, sort of like we have right now for testing iterables.  It could take a Promise and return a dictionary containing the Promise it ended up with and the global of the Promise it ended up with (because determining this last from JS is not so simple, iirc) and then we could test all the fun edge cases, right?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Also, I'm really glad I added that assertion.  ;)

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

till, do you think you could take this, given that you're way more familiar with Promise code in js engine?

Comment 7

[Boris Zbarsky [:bzbarsky]]

So I'm not sure we need to keep this hidden if this is in fact not reachable right now.  We should just fix it before landing the l10ncallback stuff.

Comment 8

[Daniel Veditz [:dveditz]]

Although not reachable now, this would be a sec-high problem if not fixed by time this code is turned on. Fine to unhide it (with a rating) if it's really inactive.

Comment 9

[Till Schneidereit [:till]]

I'm looking into this.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Some tests

The passWrongPromiseWithMatchingConstructor and passPromiseToOtherGlobal tests trigger the assert from this bug.  The passCorrectPromiseWithMismatchedConstructor and passPromiseSubclass fail because we're not doing the .constructor check correctly

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Tests now include Xray tests

We still need tests for the callback return value and JSImpl return value cases...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Tests now include Xray tests

Comment 13

[Boris Zbarsky [:bzbarsky]]

I can try to put together callback/jsimpl tests too, if desired.

Comment 14

[Till Schneidereit [:till]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> I can try to put together callback/jsimpl tests too, if desired.

That would definitely be helpful.

I didn't manage to spend much time on this this evening, but have an entire long train ride for it tomorrow ...

Comment 15

[Boris Zbarsky [:bzbarsky]]

The servo folks should probably know about this too, since presumably their impl has similar problems for the same reasons.

Comment 16

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I tried running those tests without SPIDERMONKEY_PROMISE (and getting that tree to build was all sorts of fun).  The non-xray tests pass.  The xray tests hit a fatal assert like so:

Assertion failure: js::IsObjectInContextCompartment(givenProto, cx), at ../../../dist/include/mozilla/dom/BindingUtils.h:900

under the testConstructor1 test, which isn't even one of the new ones I added...  That said, that assertion is new on tip.  It's firing because the old dom::Promise::CreateWrapper doesn't wrap aGivenProto into the compartment it's creating in.  If I comment that assertion out, I get two test failures:

TEST-UNEXPECTED-FAIL | dom/promise/tests/test_promise_xrays.html | Should have left the Promise alone because its constructor matched - got [object Promise], expected [object Promise]

and

TEST-UNEXPECTED-FAIL | dom/promise/tests/test_promise_xrays.html | Should have wrapped promise in a promise from the other global - didn't expect [object Promise], but got it

Now that I think about it, those _would_ fail in the Xray case, because the current compartment (and hence the %Promise% we use for comparing to .constructor) is window, not frames[0], when we're doing the Xray calls there.  So we should fix the tests accordingly; I need to think about how to do that best while sharing the code as much as possible.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: With the Xray tests fixed

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Update to the tests to unclude callbacks, on top of the previous test patch

These tests fail.  They fail because in the callback case we're totally not following the spec: the compartment we end up using for the return value wrapping ends up being whatever matches our random cross-compartment wrapper, not the underlying object.  We should probably spin the callback thing off to a separate bug.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I filed bug 1323930 to fix the callback behavior.

Comment 20

[Till Schneidereit [:till]]

Attachment: Make Promise::unforgeable{Resolve,Reject} spec-compliant

I'm really not entirely sure what I was thinking here. I do remember that I thought the "unforgeable" aspect was supposed to go much further than just "use the original behavior of Promise.{resolve,reject} instead of whatever user code stores in these properties." Why I thought that or why I thought that the particular behavior I chose - even ignoring the failure to re-wrap the results - I don't know anymore.

In any case, this patch just changes the unforgeable versions of resolve,reject to match the behavior of the JS-exposed functions, and passes all the new tests.

bz, does that look good to you? If so, I'll land this as part 1 and the tests as part 2 with you as the author.

I guess we can un-hide this bug with the fix at hand?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8819277 [details] [diff] [review]
Make Promise::unforgeable{Resolve,Reject} spec-compliant

>+CommonStaticResolveRejectImpl(JSContext* cx, HandleValue cVal, HandleValue x, ResolutionMode mode)

What is cval?  I guess it's the "this" value to be used for the resolve/reject call (the "constructor value"), but it could use better naming and comments.  Same thing for "x".

Maybe just name them "thisVal" and "argVal" to make things clear?

r=me with that.  I think landing this plus the first test patch and unhiding makes sense; I'll land the callback tests in bug 1323930.

Please do review the tests, though, to make sure I didn't get something wrong there.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Tests including the callback tests, since I landed the callback patch

Comment 23

[Boris Zbarsky [:bzbarsky]]

There _is_ still a problem here, though.  I filed bug 1324140 on it.

Comment 24

[Till Schneidereit [:till]]

Comment on attachment 8819370 [details] [diff] [review]
Tests including the callback tests, since I landed the callback patch

Review of attachment 8819370 [details] [diff] [review]:
-----------------------------------------------------------------

These look great. Thank you again for writing them!

::: dom/promise/tests/file_promise_argument_tests.js
@@ +1,5 @@
> +/*
> + * This file is meant to provide common infrastructure for several consumers.
> + * The consumer is expected to define the following things:
> + *
> + * 1) An ensurePromiseGlobal function which does whatever test the consumer

I would probably call this something like "verifyPromiseGlobal" or "checkPromiseGlobal", but that's mainly because of how "ensure" is used for lazy initialization stuff inside SpiderMonkey, so might not be relevant here.

@@ +8,5 @@
> + *    differenly based on that boolean.
> + * 3) A function named getPromise.  This function is given a global object and a
> + *    single argument to use for getting the promise.  This will be used to get
> + *    a promise by triggering a Promise.resolve with the given argument in
> + *    particular ways.

Probably not too important, but it's not immediately clear that the expectation is that `getPromise` somehow triggers the `Promise.resolve` internally.

@@ +9,5 @@
> + * 3) A function named getPromise.  This function is given a global object and a
> + *    single argument to use for getting the promise.  This will be used to get
> + *    a promise by triggering a Promise.resolve with the given argument in
> + *    particular ways.
> + * 4) A subrame (frames[0]) which can be used as a second global for creating

nit: "subframe"

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4c935b466e1f

Comment 26

[Boris Zbarsky [:bzbarsky]]

Till, is there a reason you didn't check in the tests, or unhide this bug?

Comment 27

[Boris Zbarsky [:bzbarsky]]

OK, so per conversation with till we want to land the tests and unhide the bug, because we believe this is not reachable from web code at the moment.  He hadn't realized he should land the tests, and didn't have permissions to unhide the bug.

Unfortunately, I can't remove the security flag that was added by dveditz in https://bugzilla.mozilla.org/show_bug.cgi?id=1323324#a466804_1689 and I'm not sure now whether I should be checking in tests for this bug if it stays closed.  Unfortunately, those tests are blocking other tests from landing.

Dveditz, what's the story here?  Can you please unhide the bug and then I'll land the tests?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Nevermind, the checkbox was just hiding.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> I would probably call this something like "verifyPromiseGlobal" or "checkPromiseGlobal"

Yeah, good idea.  I've gone with verifyPromiseGlobal.

> Probably not too important, but it's not immediately clear that the expectation is 

Now this comment says:

 * 3) A function named getPromise.  This function is given a global object and a
 *    single argument to use for getting the promise.  The getPromise function
 *    is expected to trigger the canonical Promise.resolve for the given global
 *    with the given argument in some way that depends on the test, and return
 *    the result.

>nit: "subframe"

Fixed.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2de389794304
tests.  r=till

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2de389794304