Closed Bug 1169731 Opened 6 years ago Closed 6 years ago

[[Call]] on a class constructor should throw

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: efaust, Assigned: efaust)

References

Details

Attachments

(1 file, 2 obsolete files)

ES6 5/3/2015 9.2.1 step 2 reads

If F’s [[FunctionKind]] internal slot is "classConstructor", throw a TypeError exception.
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8615034 - Flags: review?(jdemooij)
Comment on attachment 8615034 [details] [diff] [review]
Patch

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

::: js/src/jit/BaselineIC.cpp
@@ +10125,5 @@
>              return true;
>  
> +        // Likewise, if the callee is a class constructor, we have to throw.
> +        if (!constructing && fun->isClassConstructor())
> +            return true;

Baseline has a Call_AnyScripted stub that's used when we attach too many call stubs. When callee_ == nullptr in ICCallScriptedCompiler::generateStubCode, we have to do the check in JIT code. Can you add a test for this?

::: js/src/jit/CodeGenerator.cpp
@@ +3041,2 @@
>          masm.branchIfFunctionHasNoScript(calleereg, &invoke);
> +        masm.branchFunctionKind(Assembler::Equal, JSFunction::ClassConstructor, calleereg, objreg, &invoke);

Hm the overhead is probably (hopefully) negligible but if it's not, we should try to come up with a better way to test this, like have a single bit that's set when we have a script and are callable (and same for the construct case, too bad we're running out of JSFunction bits..)

::: js/src/jit/Lowering.cpp
@@ +471,5 @@
>  
>              lir = new(alloc()) LCallNative(tempFixed(cxReg), tempFixed(numReg),
>                                             tempFixed(vpReg), tempFixed(tmpReg));
> +        } else if (target->isClassConstructor() && !call->isConstructing()){
> +            lir = new(alloc()) LThrowClassConstructor();

I think it'd be simpler to just ignore the |target| in this case and fall through to the CallGeneric code, which has to deal with this case anyway.

Then you can also move the ThrowClassConstructor code in Interpreter.cpp into its single caller.

::: js/src/jit/MacroAssembler.h
@@ +946,5 @@
> +        Address flags(fun, JSFunction::offsetOfFlags());
> +        load32(flags, scratch);
> +        and32(Imm32(JSFunction::FUNCTION_KIND_MASK), scratch);
> +        rshiftPtr(Imm32(JSFunction::FUNCTION_KIND_SHIFT), scratch);
> +        branch32(cond, scratch, Imm32(kind), label);

Can we eliminate the rshiftPtr by doing something like this:

branch32(cond, scratch, Imm32(kind << JSFunction::FUNCTION_KIND_SHIFT), label);
Attachment #8615034 - Flags: review?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #2)
> like have a single bit
> that's set when we have a script and are callable (and same for the
> construct case, too bad we're running out of JSFunction bits..)

Wow, evilpie did this already for the constructor case in bug 1059908.

Anyway, your code is fine, hopefully we don't need a separate CALLABLE bit right now.
Attached patch Patch v2 (obsolete) — Splinter Review
With issues addressed.
Attachment #8615034 - Attachment is obsolete: true
Attachment #8615759 - Flags: review?(jdemooij)
Attached patch Patch v2Splinter Review
Wrong patch, sorry.
Attachment #8615759 - Attachment is obsolete: true
Attachment #8615759 - Flags: review?(jdemooij)
Attachment #8615760 - Flags: review?(jdemooij)
Comment on attachment 8615760 [details] [diff] [review]
Patch v2

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

::: js/src/jit/CodeGenerator.cpp
@@ +3133,5 @@
>      Label end, uncompiled;
>  
>      // Native single targets are handled by LCallNative.
>      MOZ_ASSERT(!target->isNative());
> +    MOZ_ASSERT(!target->isClassConstructor() || call->isConstructing());

Nit: personally I think this is a little clearer:

MOZ_ASSERT_IF(target->isClassConstructor(), call->isConstructing());
Attachment #8615760 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/f11b7896950f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.