1571289 - Assertion failure: !fun->containsPure(cx->names().name), at js/src/vm/JSFunction.cpp:2483

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 30a8df41ff6d (build with --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

class C66 {
    0 = class {
        static set name(x) {}
    };
}
new C66();

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  js::SetFunctionName (cx=0x7ffff5f23000, fun=..., name=..., prefixKind=FunctionPrefixKind::None) at js/src/vm/JSFunction.cpp:2483
#1  0x00005555558d1ab3 in Interpret (cx=0x7ffff5f23000, state=...) at js/src/vm/Interpreter.cpp:3682
#2  0x00005555558dd7c6 in js::RunScript (cx=0x7ffff5f23000, state=...) at js/src/vm/Interpreter.cpp:425
#3  0x00005555558e0f6c in js::ExecuteKernel (cx=<optimized out>, script=..., script@entry=..., envChainArg=..., newTargetValue=..., evalInFrame=..., evalInFrame@entry=..., result=result@entry=0x0) at js/src/vm/Interpreter.cpp:787
[...]
#11 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:11375
rax	0x555557d0b200	93825033875968
rbx	0x0	0
rcx	0x555556c76eb8	93825016491704
rdx	0x0	0
rsi	0x7ffff6eeb770	140737336227696
rdi	0x7ffff6eea540	140737336223040
rbp	0x7fffffffc820	140737488341024
rsp	0x7fffffffc7e0	140737488340960
r8	0x7ffff6eeb770	140737336227696
r9	0x7ffff7fe6cc0	140737354034368
r10	0x58	88
r11	0x7ffff6b927a0	140737332717472
r12	0x7fffffffcbb0	140737488341936
r13	0x7ffff5f23000	140737319677952
r14	0x7fffffffcab0	140737488341680
r15	0xe0ec8eba0b0	15456663216304
rip	0x555555acf4d9 <js::SetFunctionName(JSContext*, JS::Handle<JSFunction*>, JS::Handle<JS::Value>, FunctionPrefixKind)+505>
=> 0x555555acf4d9 <js::SetFunctionName(JSContext*, JS::Handle<JSFunction*>, JS::Handle<JS::Value>, FunctionPrefixKind)+505>:	movl   $0x0,0x0
   0x555555acf4e4 <js::SetFunctionName(JSContext*, JS::Handle<JSFunction*>, JS::Handle<JS::Value>, FunctionPrefixKind)+516>:	ud2

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This has seemed to exist ever since m-c rev 7a1ad6647c22.

Setting needinfo? from Ashley as a start since it's classes/fields-related.

Comment 2

[Ashley Hauck [:khyperia]]

I think I found the issue (heck that was a wild debugging session). We should call emitAnonymousFunctionWithComputedName here, instead of deferring emitting JSOP_SETFUNNAME until later https://searchfox.org/mozilla-central/rev/e62c920f7f6463239c6634113f8a8351e263b936/js/src/frontend/BytecodeEmitter.cpp#4097

(Note that emitAnonymousFunctionWithComputedName has a case for emitting classes, which is the relevant bit we want to trigger here).

I have yet to research what the spec says to do here, and if emitting JSOP_SETFUNNAME in what might be a different location is an observable change. (The fields spec seems to be completely busted around this particular issue - referring to nonexistant variable names, etc. - but I think this general thing exists in the main spec as well)

Comment 3

[Ashley Hauck [:khyperia]]

Attachment: Bug 1571289 - Emit JSOP_SETFUNNAME immediately, instead of deferring until later.

Comment 4

[Ashley Hauck [:khyperia]]

Attachment: Bug 1571289 - Remove dead/duplicated code.

Comment 5

[André Bargull [:anba]]

The main spec was changed in https://github.com/tc39/ecma262/pull/1372 (bug 1523791) to emit the class "name" property directly as part of ClassDefinitionEvaluation.

Before https://github.com/tc39/ecma262/pull/1372, the spec was something along the lines:

1. Let C be the result of evaluating the class node.
2. If the class is a direct-rhs-anon-function, then
  a. If HasOwnProperty(C, "name") is false, then
    i. Invoke Set(C, "name", <lhs-name>).

The extra HasOwnProperty call was present to cover the case when the class had a "static name" function or accessor, because in that case we don't want to override that function with the inferred name, i.e. let C = class {static get name() { return "ok"; }}; assertEq(C.name, "ok") should pass.

With https://github.com/tc39/ecma262/pull/1372 the spec was changed to perform the Invoke Set(C, "name", <lhs-name>) before invoking the PropertyDefinitionEvaluations in ClassDefinitionEvaluation. That ensures the let C = ... test case from above still passes, but it is no longer necessary to emit a dynamic check for an already present "name" property through HasOwnProperty.

The spec change itself was motivated because of this issue https://github.com/tc39/proposal-class-fields/issues/85.

And none of the three class-related proposals (https://github.com/tc39/proposal-class-fields, https://github.com/tc39/proposal-static-class-features/, and https://github.com/tc39/proposal-private-methods) have been updated to match the changes from https://github.com/tc39/ecma262/pull/1372.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c56d7a275fae
Emit JSOP_SETFUNNAME immediately, instead of deferring until later. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/af2eaabe5e41
Remove dead/duplicated code. r=jorendorff

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c56d7a275fae
https://hg.mozilla.org/mozilla-central/rev/af2eaabe5e41