Open Bug 1225839 Opened 8 years ago Updated 1 year ago

Class constructors throw TypeError from wrong realm


(Core :: JavaScript Engine, defect)




Tracking Status
firefox45 --- affected


(Reporter: anba, Unassigned)


(Blocks 2 open bugs)



(1 file)

Test case:
var cls = newGlobal().eval(`(class {})`);
try {
} 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:

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
Attachment #8766930 - Flags: review?(jorendorff)
Comment on attachment 8766930 [details] [diff] [review]

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.


@@ +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 If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Closed: 5 years ago
Resolution: --- → INACTIVE
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
You need to log in before you can comment on or make changes to this bug.