1225839 - Class constructors throw TypeError from wrong realm

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Test case:
---
var cls = newGlobal().eval(`(class {})`);
try {
  cls();
} catch (e) {
  assertEq(e instanceof TypeError, true);
}
---

Expected: The assertion passes
Actual: Throws `Assertion failed: got false, expected true`

Spec: ES2015, 9.2.1 [[Call]](thisArgument, argumentsList), step 2.
  The TypeError is thrown before replacing the caller context with a new execution context, that means the caller's Realm is still the current Realm (ES2015, 8.3).

Comment 1

[Boris Zbarsky [:bzbarsky]]

That means we need to do the check for class constructor in the cross-compartment proxy code, right?

Comment 2

[Jeff Walden [:Waldo]]

It means we actually have to have wrappers handle constructor-ness at all.  Right now:

bool
Wrapper::isConstructor(JSObject* obj) const
{
    // For now, all wrappers are constructable if they are callable. We will want to eventually
    // decouple this behavior, but none of the Wrapper infrastructure is currently prepared for
    // that.
    return isCallable(obj);
}

isCallable() currently *does* inspect through the compartment boundary.  So the claim in the testcase is likely that it's callable, but when we get to brass tacks and actually do a construction, we get the other realm's not-constructible error.

I'm not sure what "currently prepared for" means there.  It might be vestigial, from a time when isCallable() didn't actually inspect through wrappers/proxies to their targets, as we do now.  If so, this might be trivially fixed by changing that to obj->isConstructor().

Comment 3

[Eric Faust [:efaust]]

Attachment: Fix

We don't need any additional wrapper machinery. I think we have it anyway, but I didn't use any of it.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8766930 [details] [diff] [review]
Fix

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

Clearing r? for now because I'm not sure this is right.

::: js/src/proxy/CrossCompartmentWrapper.cpp
@@ +311,5 @@
>  
>  bool
>  CrossCompartmentWrapper::call(JSContext* cx, HandleObject wrapper, const CallArgs& args) const
>  {
> +    // Spec says we have to throw this in the callers compartment. Sigh.

"caller's"

@@ +318,5 @@
> +    // to drill (but not through security) and assert strongly that the
> +    // corresponding security checks never fail.
> +    JSObject* target = CheckedUnwrap(wrapper);
> +    MOZ_RELEASE_ASSERT(target);
> +    if (MOZ_UNLIKELY(target->is<JSFunction>() && target->as<JSFunction>().isClassConstructor())) {

Is this really sufficient? What if the target is a scripted Proxy whose target is a class?

I'm not sure we can do this correctly without stamping a proxy, when we create it, with a bit that tells if it's callable. (And another bit for constructibility, if I am right in thinking that JS::IsConstructor and JSObject::isConstructor are wrong for some proxies.)

Comment 5

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 7

[Jan de Mooij [:jandem]]

anba, is this correct now since your change here? https://hg.mozilla.org/mozilla-central/rev/78e57707b124

I came across this bug and was thinking this should at least be easy to fix for same-compartment realms, but then I saw your change.

Comment 8

[André Bargull [:anba]]

Yes, our behaviour is now correct according to the spec after the changes from https://github.com/tc39/ecma262/pull/2216 (bug 1681567).