Closed Bug 1225839 Opened 9 years ago Closed 5 months ago

Class constructors throw TypeError from wrong realm

Categories

(Core :: JavaScript Engine, defect)

defect

Tracking

()

RESOLVED INVALID
Tracking Status
firefox45 --- affected

People

(Reporter: anba, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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).
That means we need to do the check for class constructor in the cross-compartment proxy code, right?
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().
Attached patch FixSplinter Review
We don't need any additional wrapper machinery. I think we have it anyway, but I didn't use any of it.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8766930 - Flags: review?(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.)
Attachment #8766930 - Flags: review?(jorendorff)
Blocks: test262
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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---

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

Assignee: efaustbmo → nobody
Severity: normal → S3

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.

Flags: needinfo?(andrebargull)

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

Status: REOPENED → RESOLVED
Closed: 6 years ago5 months ago
Flags: needinfo?(andrebargull)
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: