[[Call]] on a class constructor should throw

RESOLVED FIXED in Firefox 41

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
ES6 5/3/2015 9.2.1 step 2 reads

If F’s [[FunctionKind]] internal slot is "classConstructor", throw a TypeError exception.
(Assignee)

Comment 1

3 years ago
Created attachment 8615034 [details] [diff] [review]
Patch
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.
(Assignee)

Comment 4

3 years ago
Created attachment 8615759 [details] [diff] [review]
Patch v2

With issues addressed.
Attachment #8615034 - Attachment is obsolete: true
Attachment #8615759 - Flags: review?(jdemooij)
(Assignee)

Comment 5

3 years ago
Created attachment 8615760 [details] [diff] [review]
Patch v2

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
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1181336
You need to log in before you can comment on or make changes to this bug.