1369774 - Assertion failure: types, at js/src/jit/SharedIC.cpp:2560 with TypedObject

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision aeb3d0ca558f (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --baseline-eager --ion-offthread-compile=off):

var T = TypedObject;
var ObjectStruct = new T.StructType({
    f: T.Object
});
var o = new ObjectStruct();
function testGC(o, p) {
    for (var i = 0; i < 5; i++) {
        o.f = p;
    }
}
testGC(o, null);


Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x000000000080a2f9 in js::jit::ICUpdatedStub::addUpdateStubForValue (this=this@entry=0x7ffff434c3c0, cx=cx@entry=0x7ffff6924000, outerScript=..., outerScript@entry=..., obj=..., obj@entry=..., id=id@entry=..., val=val@entry=...) at js/src/jit/SharedIC.cpp:2560
#0  0x000000000080a2f9 in js::jit::ICUpdatedStub::addUpdateStubForValue (this=this@entry=0x7ffff434c3c0, cx=cx@entry=0x7ffff6924000, outerScript=..., outerScript@entry=..., obj=..., obj@entry=..., id=id@entry=..., val=val@entry=...) at js/src/jit/SharedIC.cpp:2560
#1  0x00000000005edc3e in js::jit::DoTypeUpdateFallback (cx=0x7ffff6924000, frame=<optimized out>, stub=0x7ffff434c3c0, objval=..., value=...) at js/src/jit/BaselineIC.cpp:331
#2  0x000012bf8da155e5 in ?? ()
[...]
#43 0x00007fffffffc640 in ?? ()
#44 0x00000000005e5fd2 in EnterBaseline (cx=0x7fffffffc268, data=...) at js/src/jit/BaselineJIT.cpp:162
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
rax	0x0	0
rbx	0x0	0
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffc0c0	140737488339136
rsp	0x7fffffffbfc0	140737488338880
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7ffff6924000	140737330167808
r13	0x7ffff434c3c0	140737290486720
r14	0x7fffffffc220	140737488339488
r15	0x7fffffffc160	140737488339296
rip	0x80a2f9 <js::jit::ICUpdatedStub::addUpdateStubForValue(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>)+1961>
=> 0x80a2f9 <js::jit::ICUpdatedStub::addUpdateStubForValue(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>)+1961>:	movl   $0x0,0x0
   0x80a304 <js::jit::ICUpdatedStub::addUpdateStubForValue(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>)+1972>:	ud2    


Marking s-s because it's an IC assert and involves TypedObject.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/ba5cee986655
user:        Jan de Mooij
date:        Wed May 31 09:43:16 2017 +0200
summary:     Bug 1368509 part 2 - Generalize Baseline unknown/unknownObject type update stubs. r=tcampbell

This iteration took 269.181 seconds to run.

Comment 2

[Julien Cristau [:jcristau]]

tracking as regression in 55.

Comment 3

[Andrew McCreight [:mccr8]]

Sounds like a type confusion, so I will mark it sec-high.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

In addUpdateStubForValue we assumed we always had a HeapTypeSet if the group doesn't have unknownProperties(), but unfortunately that's not true for TypedObjects - see DoTypeUpdateFallback. So this patch just restructures the code a bit to deal with this.

TypedObjects are still Nightly only so this doesn't affect 55 (but we should probably backport it anyway to help fuzzing).

Comment 5

[Ted Campbell [:tcampbell]]

Comment on attachment 8878543 [details] [diff] [review]
Patch

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

Your solution makes sense to me.

::: js/src/jit/SharedIC.cpp
@@ +2565,5 @@
> +            // We don't record null/undefined types for certain TypedObject
> +            // properties. See DoTypeUpdateFallback.
> +            MOZ_ASSERT(obj->is<TypedObject>());
> +            MOZ_ASSERT(val.isNullOrUndefined());
> +        }

This took me a few reads to understand that the Null/Undef case is handled as normal primitive types.

Maybe add a sentence in the comment that in these |types| is allowed to be nullptr in this case without implying unknown types.

(Your comment in the bug explained well, but the patch was harder to follow)

Comment 6

[Jan de Mooij [:jandem]]

Pushed this as TypedObjects are Nightly-only:

https://hg.mozilla.org/integration/mozilla-inbound/rev/140285b81cd388178e539dc67bd3820689949073

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8878543 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1368509.
[User impact if declined]: This only affects TypedObjects which are Nightly-only. However I'd like to have this fixed on beta, as I have more patches for this code and it's better to use the same code everywhere.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Patch is pretty straight-forward, very local change.
[String changes made/needed]: None.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/140285b81cd3

Comment 9

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8878543 [details] [diff] [review]
Patch

sec-high fix, with test, beta55+

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx55