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)

x86_64
Linux
defect
Not set
critical

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.
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)
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
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)
(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.
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.
Attached patch PatchSplinter Review
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)
Keywords: sec-high
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/bf6e51ea07a7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: javascript-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [jsbugmon:] → [jsbugmon:][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.