Open Bug 1225839 Opened 6 years ago Updated 3 years ago

Class constructors throw TypeError from wrong realm

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

REOPENED
Tracking Status
firefox45 --- affected

People

(Reporter: anba, Assigned: efaust)

References

(Blocks 2 open bugs)

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: 3 years ago
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
You need to log in before you can comment on or make changes to this bug.