Closed
Bug 1397071
Opened 7 years ago
Closed 7 years ago
Assertion failure: this->is<T>(), at js/src/jsobj.h:575
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | --- | fixed |
People
(Reporter: gkw, Assigned: jandem)
References
Details
(4 keywords, Whiteboard: [jsbugmon:][post-critsmash-triage])
Attachments
(3 files)
The following testcase crashes on mozilla-central revision 973e8b890a62 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --ion-eager --no-unboxed-objects): See attachment. Backtrace: #0 0x000000000074a00a in JSObject::as<js::UnboxedPlainObject> (this=<optimized out>) at js/src/jsobj.h:575 #1 js::InitPropertyOperation (rhs=..., id=..., obj=..., op=<optimized out>, cx=<optimized out>) at js/src/vm/Interpreter-inl.h:413 #2 js::jit::IonSetPropertyIC::update (cx=0x7f5835576000, outerScript=..., ic=0x7f5832340658, obj=..., idVal=..., rhs=...) at js/src/jit/IonIC.cpp:247 #3 0x000012cc7bc450ed in ?? () #4 0x00007fff2867fba0 in ?? () #5 0x00007fff2867fa50 in ?? () /snip For detailed crash information, see attachment. This testcase gets increasingly unreliable as one reduces it, moreover IC is on the stack, so setting s-s as a start.
Reporter | ||
Comment 1•7 years ago
|
||
Reporter | ||
Comment 2•7 years ago
|
||
Reporter | ||
Comment 3•7 years ago
|
||
autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/a14fc8f5babc user: André Bargull date: Mon Aug 28 14:19:34 2017 +0200 summary: Bug 1303335: Move parts of Object.getOwnProperty and Object.defineProperty to self-hosted code. r=till Andre, is bug 1303335 a likely regressor?
Blocks: 1303335
Flags: needinfo?(andrebargull)
Updated•7 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:]
Comment 4•7 years ago
|
||
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Comment 5•7 years ago
|
||
I can reproduce the failure: --- Thread 1 "mozjs-debug" received signal SIGSEGV, Segmentation fault. 0x00000000005ab57b in JSObject::as<js::UnboxedPlainObject> (this=(JSObject * const) 0x7ffff44e1f00 [object ArrayBuffer]) at /home/andre/git/mozilla-central/js/src/jsobj.h:575 575 MOZ_ASSERT(this->is<T>()); (gdb) bt #0 0x00000000005ab57b in JSObject::as<js::UnboxedPlainObject>() (this=(JSObject * const) 0x7ffff44e1f00 [object ArrayBuffer]) at /home/andre/git/mozilla-central/js/src/jsobj.h:575 #1 0x00000000005a7cb1 in js::InitPropertyOperation(JSContext*, JSOp, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>) (cx=0x7ffff6976000, op=JSOP_INITELEM, obj=(JSObject * const) 0x7ffff44e1f00 [object ArrayBuffer], id=$jsid("substr"), rhs=$jsval((JSObject *) 0x7ffff44a50a0 [object Set])) at /home/andre/git/mozilla-central/js/src/vm/Interpreter-inl.h:413 #2 0x00000000008abbce in js::jit::IonSetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, js::jit::IonSetPropertyIC*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) (cx=0x7ffff6976000, outerScript=0x7ffff3d122f0, ic=0x7ffff38f3658, obj=(JSObject * const) 0x7ffff44e1f00 [object ArrayBuffer], idVal=$jsval("substr"), rhs=$jsval((JSObject *) 0x7ffff44a50a0 [object Set])) at /home/andre/git/mozilla-central/js/src/jit/IonIC.cpp:247 #3 0x00002012a3eca0ed in () #4 0x00007fffffff9a80 in () #5 0x00007fffffff9930 in () #6 0x000000000268ea80 in js::jit::IonSetPropertyICInfo () ... --- Reduced test case which instant fails for me (configure options: --enable-debug --disable-optimize --disable-tests; run with: <no extra options>; mozilla-central revision 973e8b890a62): --- var b1 = new ArrayBuffer(64); // Or any other object except PlainObject or FunctionObject for (var i = 0; i < 100000; ++i) { Object.defineProperty(b1, "substr", { configurable: true, enumerable: true, writable: true, value: null }); } --- Jan, it looks like it's actually not allowed to use _DefineDataProperty for all property keys. Is that correct?
Flags: needinfo?(andrebargull) → needinfo?(jdemooij)
Comment 6•7 years ago
|
||
(In reply to André Bargull from comment #5) > Jan, it looks like it's actually not allowed to use _DefineDataProperty for > all property keys. Is that correct? I guess this would also explain the results in bug 1397026.
Assignee | ||
Comment 7•7 years ago
|
||
Yeah we assume InitProp is only used for literals etc but that's no longer true with _DefineDataProperty. INITELEM seems to do the right thing. I'll take a look; we should probably just allow all objects there.
Assignee | ||
Comment 9•7 years ago
|
||
This avoids calling InitPropertyOperation for JSOP_INITELEM optimized like INITPROP in Ion. We now just call InitElemOperation and I think that's fine. We could also make InitPropertyOperation more generic, but that would make the interpreter and Baseline a bit slower. This is probably the simplest fix.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8905479 -
Flags: review?(andrebargull)
Comment 10•7 years ago
|
||
Comment on attachment 8905479 [details] [diff] [review] Patch Review of attachment 8905479 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! ::: js/src/jit/IonIC.cpp @@ +246,5 @@ > + // This might be a JSOP_INITELEM op with a constant string id. We > + // can't call InitPropertyOperation here as that function is > + // specialized for JSOP_INIT*PROP (it does not support arbitrary > + // objects that might show up here). > + if (!InitElemOperation(cx, pc, obj, idVal, rhs)) Do you think calling InitElemOperation instead of InitPropOperation will have any negative performance effects, IOW do you think it's worthwhile to have different code paths for |JSOp(*pc) == JSOP_INITELEM| and |JSOp(*pc) == JSOP_INITPROP| ? The only major difference between both InitXOperations seems to be the additional |DefinePropertyOp op = obj->getOpsDefineProperty()| lookup in js::DefineDataProperty(...), right?
Attachment #8905479 -
Flags: review?(andrebargull) → review+
Assignee | ||
Comment 11•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf6e51ea07a7ed46ee55f53bb67cf3ed7e7b1282
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to André Bargull from comment #10) > Do you think calling InitElemOperation instead of InitPropOperation will > have any negative performance effects, IOW do you think it's worthwhile to > have different code paths for |JSOp(*pc) == JSOP_INITELEM| and |JSOp(*pc) == > JSOP_INITPROP| ? The only major difference between both InitXOperations > seems to be the additional |DefinePropertyOp op = > obj->getOpsDefineProperty()| lookup in js::DefineDataProperty(...), right? I think it's fine - JSOP_INITPROP is optimized pretty well in IonBuilder and with IC stubs.
Comment 13•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bf6e51ea07a7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Group: javascript-core-security → core-security-release
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [jsbugmon:] → [jsbugmon:][post-critsmash-triage]
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•