1605641 - Crash [@ js::ToBooleanSlow(JS::Handle<JS::Value>)] or Assertion failure: v.isObject(), at builtin/Boolean.cpp:172

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20191222-ca5ff9e3c66e (build with --disable-debug, run with --fuzzing-safe --ion-offthread-compile=off --ion-full-warmup-threshold=0 --baseline-eager):

var obj = typeof prototype;
var thrower = function() {
    throw new x();
};
[...{} [thrower(...[typeof unescape])]] = obj;

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x00005555557bac4b in js::ToBooleanSlow(JS::Handle<JS::Value>) ()
#0  0x00005555557bac4b in js::ToBooleanSlow(JS::Handle<JS::Value>) ()
#1  0x0000555555f75ca8 in js::jit::HandleException(js::jit::ResumeFromException*) ()
#2  0x000007684d53982b in ?? ()
#3  0xfffe14e206200668 in ?? ()
#4  0x00007fffffffbce8 in ?? ()
#5  0x0000000000000000 in ?? ()
rax	0x480000000000a	1266637395197962
rbx	0x7fffffffbe10	140737488338448
rcx	0xfffe000000000000	-562949953421312
rdx	0xfffb000000000000	-1407374883553280
rsi	0xfff8000100000000	-2251795518717952
rdi	0x7fffffffb810	140737488336912
rbp	0x7fffffffb710	140737488336656
rsp	0x7fffffffb710	140737488336656
r8	0x8	8
r9	0x3b539eb90660	65230331250272
r10	0x7ffff4bca800	140737299392512
r11	0x9b5487da	2606008282
r12	0x7ffff4c3f5ac	140737299871148
r13	0x7fffffffbb01	140737488337665
r14	0x20	32
r15	0x7ffff5e22000	140737318625280
rip	0x5555557bac4b <js::ToBooleanSlow(JS::Handle<JS::Value>)+75>
=> 0x5555557bac4b <_ZN2js13ToBooleanSlowEN2JS6HandleINS0_5ValueEEE+75>:	mov    (%rax),%rcx
   0x5555557bac4e <_ZN2js13ToBooleanSlowEN2JS6HandleINS0_5ValueEEE+78>:	mov    (%rcx),%rcx

This looks like a dangerous type confusion, both from the assert and the non-zero crash, marking s-s.

Comment 1

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

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/d9c1e44eb0ad
user: Jan de Mooij
date: Sun Dec 15 11:34:34 2019 +0000
summary: Bug 1601599 part 4 - Support Ion OSR at all loops. r=tcampbell

Jan, is bug 1601599 a likely regressor?

Comment 2

[Jan de Mooij [:jandem]]

Destructuring bytecode has a "done" bool on the stack that has no bytecode uses, it's only used by exception unwinding. Because it has no explicit uses, phi elimination replaces it with the "optimized out" magic value and this then confuses exception handling.

Calling graph().setHasTryBlock() for JSOP_TRY_DESTRUCTURING disables aggressive phi elimination and fixes this. Something like that might work but I want to see if there's a more targeted fix...

Comment 3

[Jan de Mooij [:jandem]]

Another option: after adding loop phis in IonBuilder, look for matching try notes and mark relevant phis as having implicit uses.

Comment 4

[Ted Campbell [:tcampbell]]

(In reply to Jan de Mooij [:jandem] from comment #3)

Another option: after adding loop phis in IonBuilder, look for matching try notes and mark relevant phis as having implicit uses.

I'm in favour of marking implicit uses. It documents the cause and effect much better than the proposal in Comment 2.

Comment 5

[Jan de Mooij [:jandem]]

Guaranteed crash: the magic value payload interpreted as object pointer is a near-null value.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1605641 - Mark 'done' stack value for destructuring assignment as having implicit uses. r?tcampbell!

We need to make sure this value isn't optimized out because the exception
handler may need it.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ccaefced3df
Mark 'done' stack value for destructuring assignment as having implicit uses. r=tcampbell

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/6ccaefced3df

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact here which justifies uplift to Fx73? Please nominate the patch for Beta approval if so.

Comment 10

[Jan de Mooij [:jandem]]

Yeah, I wanted to give it a few days before requesting uplift.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 9118793 [details]
Bug 1605641 - Mark 'done' stack value for destructuring assignment as having implicit uses. r?tcampbell!

Beta/Release Uplift Approval Request

• User impact if declined: Potential crashes in some cases.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Adds some code to handle a particular edge case better. Should be low risk and has been on Nightly for a few days, but hard to say for sure.
• String changes made/needed: N/A

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9118793 [details]
Bug 1605641 - Mark 'done' stack value for destructuring assignment as having implicit uses. r?tcampbell!

JS crash fix, approved for 73.0b6.

Comment 13

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b6cc01dd41a2