1607670 - Figure out what to do with CreateThis

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P1)

Description

[Jan de Mooij [:jandem]]

Fixing 0-day bug 1607443 made us discover some weirdness with MCreateThis. It made sense before, but since ES6 classes it's now an effectful operation (see the GetPrototypeFromConstructor call in CreateThisForFunction) and we don't handle that well.

We have a number of options:

• We add JSOP_CREATETHIS and use that right before JSOP_NEW and JSOP_SUPERCALL. A bit annoying is that we have to emit this after evaluating the arguments due to side-effects.

• We keep JSOP_NEW as-is, but disallow using it with an arbitrary new.target. The complicated cases where new.target != callee (Reflect.construct, super calls?) would then use JSOP_CREATETHIS with an op that's not JSOP_NEW. This results in two different paths which isn't great.

• More radical: move this-allocation into the callee with a JSOP_CREATETHIS in the prologue. This is nice because it matches the spec better and is consistent with native functions (where callee constructs |this|) so it means we can remove special cases for bound functions, derived class constructors, native functions. It could also improve performance of polymorphic constructor calls.
  Downside is that it would add an extra guard to non-constructing calls (although Ion could probably avoid that), and would require rethinking |this| TI type checks...

Comment 1

[André Bargull [:anba]]

The current approach isn't actually spec-compliant: bug 1380953, which shows we retrieve callee.prototype too often. (IonBuilder::createThisScripted() emits code which isn't idempotent, contrary to the comment in that method.)

Comment 2

[Jan de Mooij [:jandem]]

You're right, that's one of the issues we need to fix.

For what it's worth, JSC does callee-side |this| allocation, but they have specialized bytecode for constructing vs non-constructing so that's nicer for them than it is for us...

Comment 3

[Jan de Mooij [:jandem]]

Current plan is to have calls with non-function new.target always go through the slow invoke path, for now. So basically:

1. In IonBuilder always use MCreateThis if callee != new.target.
2. In jit::CreateThis, if new.target is not a JSFunction, return a magic value similar to some other cases.
3. In scripted call codegen check for new.target not being a function and take the slow path.

Comment 4

[Andrew McCreight [:mccr8]]

Bug 1607443 comment 10 says "We've looked at the MCreateThis case and while it is weird, it is not a sec concern" so I'm going to mark this sec-other.

Comment 5

[Jan de Mooij [:jandem]]

I'm working on this. It's a bit complicated because some (scripted) constructor functions don't have a builtin .prototype property (bound functions..), but this is work that should have happened when classes/newTarget were added...

Comment 6

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: !cx->runtime()->jitRuntime()->disallowArbitraryCode, at js/src/vm/Interpreter.cpp:394
Build version: mozilla-central revision 9f55d547e196+
Build flags: --enable-gczeal --enable-optimize --enable-debug
Runtime options: --fuzzing-safe --no-threads --ion-warmup-threshold=1 --baseline-warmup-threshold=1 --ion-full-warmup-threshold=1 --blinterp-warmup-threshold=1 --ion-extra-checks --disable-oom-functions

Testcase:

    See attachment.

Backtrace:

    received signal SIGSEGV, Segmentation fault.
    #0  js::RunScript (cx=0x7ffff5f27000, state=...) at js/src/vm/Interpreter.cpp:393
    #1  0x0000555556bc6e91 in js::InternalCallOrConstruct (cx=0x7ffff5f27000, args=..., construct=js::NO_CONSTRUCT, reason=(unknown: -168772608)) at js/src/vm/Interpreter.cpp:583
    #2  0x0000555556bc7b50 in js::Call (cx=0x7ffff6eea540 <_IO_2_1_stderr_>, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:628
    #3  0x0000555556d728bb in js::ScriptedProxyHandler::get (this=<optimized out>, cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) atjs/src/proxy/ScriptedProxyHandler.cpp:1152
    #4  0x0000555556d7c5da in js::Proxy::getInternal (cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:332
    #5  0x0000555556d684c5 in js::Proxy::get (cx=0x7ffff5f27000, proxy=..., receiver_=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:340
    #6  0x0000555556b34b3e in js::GetProperty (cx=<optimized out>, obj=..., receiver=..., id=..., vp=...) at js/src/vm/ObjectOperations-inl.h:114
    #7  js::GetProperty (cx=0x7ffff5f27000, obj=..., receiver=..., name=<optimized out>, vp=...) at js/src/vm/ObjectOperations-inl.h:124
    #8  0x0000555556b349d5 in js::GetProperty (cx=0x7ffff5f27000, obj=..., receiver=..., name=0x387e69d25e20, vp=...) at js/src/vm/ObjectOperations-inl.h:138
    #9  0x0000555556d6c003 in GetProxyTrap (cx=0x7ffff5f27000, handler=..., name=..., func=...) at js/src/proxy/ScriptedProxyHandler.cpp:184
    #10 0x0000555556d725db in js::ScriptedProxyHandler::get (this=<optimized out>, cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) at js/src/proxy/ScriptedProxyHandler.cpp:1128
    #11 0x0000555556d7c5da in js::Proxy::getInternal (cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:332
    #12 0x0000555556d684c5 in js::Proxy::get (cx=0x7ffff5f27000, proxy=..., receiver_=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:340
    #13 0x0000555556b34b3e in js::GetProperty (cx=<optimized out>, obj=..., receiver=..., id=..., vp=...) at js/src/vm/ObjectOperations-inl.h:114
    #14 js::GetProperty (cx=0x7ffff5f27000, obj=..., receiver=..., name=<optimized out>, vp=...) at js/src/vm/ObjectOperations-inl.h:124
    #15 0x0000555556bcbd72 in js::GetProperty (cx=0x7ffff5f27000, v=..., name=..., vp=...) at js/src/vm/Interpreter.cpp:4453
    #16 0x000000c1ad70f00b in ?? ()
    [...]
    #31 0x0000000000000000 in ?? ()
    rax	0x555555a019b7	93824997136823
    rbx	0x4	4
    rcx	0x55555838c380	93825040696192
    rdx	0x0	0
    rsi	0x7ffff6eeb770	140737336227696
    rdi	0x7ffff6eea540	140737336223040
    rbp	0x7fffffff8440	140737488323648
    rsp	0x7fffffff83f0	140737488323568
    r8	0x7ffff6eeb770	140737336227696
    r9	0x7ffff7fe6c80	140737354034304
    r10	0x58	88
    r11	0x7ffff6b927a0	140737332717472
    r12	0x7ffff5f27000	140737319694336
    r13	0x7fffffff85f0	140737488324080
    r14	0x7fffffff8490	140737488323728
    r15	0xfffdffffffffffff	-562949953421313
    rip	0x555556bb1c07 <js::RunScript(JSContext*, js::RunState&)+919>
    => 0x555556bb1c07 <js::RunScript(JSContext*, js::RunState&)+919>:	movl   $0x18a,0x0
       0x555556bb1c12 <js::RunScript(JSContext*, js::RunState&)+930>:	callq  0x555556b48be0 <abort()>


Note: This testcase is only partly reduced and still contains fuzzing logic (including code rewriting). I am working on a reduced version, but filing this for analysis in the meantime.

Comment 7

[Christian Holler (:decoder)]

Attachment: Testcase for comment 6

Comment 8

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

[Tracking Requested - why for this release]:

Comment 9

[Ted Campbell [:tcampbell]]

Attachment: Simple testcase

I crafted a new test-case that also trips the assert.

Comment 10

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: !cx->runtime()->jitRuntime()->disallowArbitraryCode, at js/src/vm/Interpreter.cpp:394
Build version: mozilla-central revision 9f55d547e196+
Build flags: --enable-gczeal --enable-optimize --enable-debug
Runtime options: --fuzzing-safe --no-threads --ion-warmup-threshold=1 --baseline-warmup-threshold=1 --ion-full-warmup-threshold=1 --blinterp-warmup-threshold=1 --ion-extra-checks --disable-oom-functions

Testcase:

    function await () {}
    function LoggingProxyHandlerWrapper(name, handler={}) {
        return new Proxy(handler, {
            get(t6, id) {
                return function (...args) {
                    return Reflect[id].apply(null, args);
                };
            }
        });
    }
    function LoggingProxy(name, target) {
        return new Proxy(await, new LoggingProxyHandlerWrapper(name));
    }
    let proto_proxy = new LoggingProxy("proto", new Date);
    niche = proto_proxy;
    new niche;
    new niche;
    new niche;
    new niche;
    new niche;
    new niche;

Backtrace:

    received signal SIGSEGV, Segmentation fault.
    js::RunScript (cx=0x7ffff5f27000, state=...) at js/src/vm/Interpreter.cpp:393
    #0  js::RunScript (cx=0x7ffff5f27000, state=...) at js/src/vm/Interpreter.cpp:393
    #1  0x0000555556bc6e91 in js::InternalCallOrConstruct (cx=0x7ffff5f27000, args=..., construct=js::NO_CONSTRUCT, reason=(unknown: -168772608)) at js/src/vm/Interpreter.cpp:583
    #2  0x0000555556bc7b50 in js::Call (cx=0x7ffff6eea540 <_IO_2_1_stderr_>, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:628
    #3  0x0000555556d728bb in js::ScriptedProxyHandler::get (this=<optimized out>, cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) at js/src/proxy/ScriptedProxyHandler.cpp:1152
    #4  0x0000555556d7c5da in js::Proxy::getInternal (cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:332
    #5  0x0000555556d684c5 in js::Proxy::get (cx=0x7ffff5f27000, proxy=..., receiver_=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:340
    #6  0x0000555556b34b3e in js::GetProperty (cx=<optimized out>, obj=..., receiver=..., id=..., vp=...) at js/src/vm/ObjectOperations-inl.h:114
    #7  js::GetProperty (cx=0x7ffff5f27000, obj=..., receiver=..., name=<optimized out>, vp=...) at js/src/vm/ObjectOperations-inl.h:124
    #8  0x0000555556b349d5 in js::GetProperty (cx=0x7ffff5f27000, obj=..., receiver=..., name=0x57a83425e20, vp=...) at js/src/vm/ObjectOperations-inl.h:138
    #9  0x0000555556d6c003 in GetProxyTrap (cx=0x7ffff5f27000, handler=..., name=..., func=...) at js/src/proxy/ScriptedProxyHandler.cpp:184
    #10 0x0000555556d725db in js::ScriptedProxyHandler::get (this=<optimized out>, cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) at js/src/proxy/ScriptedProxyHandler.cpp:1128
    #11 0x0000555556d7c5da in js::Proxy::getInternal (cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:332
    #12 0x0000555556d684c5 in js::Proxy::get (cx=0x7ffff5f27000, proxy=..., receiver_=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:340
    #13 0x0000555556b34b3e in js::GetProperty (cx=<optimized out>, obj=..., receiver=..., id=..., vp=...) at js/src/vm/ObjectOperations-inl.h:114
    #14 js::GetProperty (cx=0x7ffff5f27000, obj=..., receiver=..., name=<optimized out>, vp=...) at js/src/vm/ObjectOperations-inl.h:124
    #15 0x0000555556bcbd72 in js::GetProperty (cx=0x7ffff5f27000, v=..., name=..., vp=...) at js/src/vm/Interpreter.cpp:4453
    #16 0x00003ac8eb19e00b in ?? ()
    #17 0x0000000000000000 in ?? ()
    rax	0x555555a019b7	93824997136823
    rbx	0x4	4
    rcx	0x55555838c380	93825040696192
    rdx	0x0	0
    rsi	0x7ffff6eeb770	140737336227696
    rdi	0x7ffff6eea540	140737336223040
    rbp	0x7fffffffa750	140737488332624
    rsp	0x7fffffffa700	140737488332544
    r8	0x7ffff6eeb770	140737336227696
    r9	0x7ffff7fe6c80	140737354034304
    r10	0x58	88
    r11	0x7ffff6b927a0	140737332717472
    r12	0x7ffff5f27000	140737319694336
    r13	0x7fffffffa900	140737488333056
    r14	0x7fffffffa7a0	140737488332704
    r15	0xfffdffffffffffff	-562949953421313
    rip	0x555556bb1c07 <js::RunScript(JSContext*, js::RunState&)+919>
    => 0x555556bb1c07 <js::RunScript(JSContext*, js::RunState&)+919>:	movl   $0x18a,0x0
       0x555556bb1c12 <js::RunScript(JSContext*, js::RunState&)+930>:	callq  0x555556b48be0 <abort()>

Comment 11

[Christian Holler (:decoder)]

(In reply to Ted Campbell [:tcampbell] from comment #9)

Created attachment 9119929 [details]
Simple testcase

I crafted a new test-case that also trips the assert.

Hah, your test is nicer, you win ;)

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1607670 part 1 - Remove unnecessary isConstructor check from IonBuilder::createThis. r?tcampbell!

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Bug 1607670 part 2 - Remove calleeScript argument from js::CreateThis. r?tcampbell!

We can call isDerivedClassConstructor() on the function instead.

Depends on D59502

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Bug 1607670 part 3 - Split jit::CreateThis in CreateThisFromIC and CreateThisFromIon. r?tcampbell!

The callers are very different and have different constraints, especially in
later patches in this stack.

Depends on D59503

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Bug 1607670 part 4 - Add JSFunction::constructorNeedsUninitializedThis helper function and some asserts. r?tcampbell!

Depends on D59505

Comment 16

[Jan de Mooij [:jandem]]

Attachment: Bug 1607670 part 5 - Fix some issues with this-creation in Ion. r?tcampbell!,anba!

In particular, we can't do effectful .prototype lookups in CreateThis and
elsewhere before the call instruction. This patch returns NullValue from
CreateThisFromIon in that case and changes LCallGeneric to take the slow
invoke path when it sees this value.

Depends on D59506

Comment 17

[Jan de Mooij [:jandem]]

Attachment: Bug 1607670 part 6 - Add tests. r?tcampbell!

Depends on D59507

Comment 18

[Julien Cristau [:jcristau]]

Per tcampbell "[this bug] is internally identified, probably-not-exploitable, and complex to fix so it is not a good uplift target."

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Given comment 18, I don't think we want to rush this into Fx73 either. I'm going to leave ESR68 set to affected for now in case we want to look at uplifting it there next cycle (i.e. for 68.6esr shipping alongside Fx74), but I'd be OK with calling it wontfix too if others agree.

Comment 20

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/autoland/rev/596958878df0b51e5e672cd6c274509d5d249dd2
https://hg.mozilla.org/integration/autoland/rev/878930cde05bd79f5dcbd03b3b80e3cac3442206
https://hg.mozilla.org/integration/autoland/rev/e062ea07194dbd8ef065d150ff5ecf01f8d84636
https://hg.mozilla.org/integration/autoland/rev/b2162495abd782b2fa3637935dbcccd7e0eb1561
https://hg.mozilla.org/integration/autoland/rev/c20655f4a4c12382c1fb71f332fc52ff78ea935b
https://hg.mozilla.org/integration/autoland/rev/a1d01d3c0bafc51dc745cc7f66f2b595aa4b4934

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/596958878df0
https://hg.mozilla.org/mozilla-central/rev/878930cde05b
https://hg.mozilla.org/mozilla-central/rev/e062ea07194d
https://hg.mozilla.org/mozilla-central/rev/b2162495abd7
https://hg.mozilla.org/mozilla-central/rev/c20655f4a4c1
https://hg.mozilla.org/mozilla-central/rev/a1d01d3c0baf