Closed
Bug 945223
Opened 9 years ago
Closed 9 years ago
Assertion failure: index >= size_t(parser.stackDepthAtPC(current)), at ../jsopcode.cpp:1796
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla29
Tracking | Status | |
---|---|---|
firefox25 | --- | unaffected |
firefox26 | --- | unaffected |
firefox27 | --- | unaffected |
firefox28 | --- | fixed |
firefox29 | --- | fixed |
firefox-esr17 | --- | unaffected |
firefox-esr24 | --- | unaffected |
b2g18 | --- | unaffected |
b2g-v1.1hd | --- | unaffected |
b2g-v1.2 | --- | unaffected |
b2g-v1.3 | --- | fixed |
b2g-v1.3T | --- | fixed |
b2g-v1.4 | --- | fixed |
People
(Reporter: decoder, Assigned: djvj)
References
Details
(4 keywords)
Attachments
(2 files, 1 obsolete file)
565 bytes,
text/plain
|
Details | |
13.13 KB,
patch
|
djvj
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
The following testcase asserts on mozilla-central revision 84a5a5800bd3 (threadsafe build, run with --fuzzing-safe --ion-eager --ion-parallel-compile=on): Array.prototype.__proto__ = Proxy.create({ getPropertyDescriptor: function(name) { return (560566); }, }, null); function f() {} function g() { } var x = [f,f,f,undefined,g]; for (var i = 0; i < 5; ++i) y = x[i]("x");
Reporter | ||
Comment 1•9 years ago
|
||
Updated•9 years ago
|
Keywords: regressionwindow-wanted,
sec-high
Comment 2•9 years ago
|
||
This assertion was added in bug 932180. I don't know if the problem is new, or there is stricter checking.
Comment 3•9 years ago
|
||
Needinfo from wingo since bug 932180 may have introduced this :)
Flags: needinfo?(wingo)
autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: http://hg.mozilla.org/mozilla-central/rev/58605e9a6ea1 user: Kannan Vijayan date: Tue Nov 12 14:20:34 2013 -0500 summary: Bug 912303 - Added noSuchMethod support to baseline CALLPROP/CALLELEM stubs. r=efaust Kannan, is bug 912303 a likely regressor?
Blocks: 912303
status-b2g18:
--- → unaffected
status-b2g-v1.1hd:
--- → unaffected
status-b2g-v1.2:
--- → unaffected
status-firefox25:
--- → unaffected
status-firefox26:
--- → unaffected
status-firefox27:
--- → unaffected
status-firefox28:
--- → affected
status-firefox29:
--- → unaffected
status-firefox-esr17:
--- → unaffected
status-firefox-esr24:
--- → unaffected
Flags: needinfo?(wingo) → needinfo?(kvijayan)
Keywords: regressionwindow-wanted → regression
Assignee | ||
Comment 6•9 years ago
|
||
Gotta push the temp values back on the stack before doing NoSuchMethodLookup, because decompiler might be invoked during the lookup. More register surgery, ick.
Attachment #8343179 -
Flags: review?(efaustbmo)
Comment 7•9 years ago
|
||
Comment on attachment 8343179 [details] [diff] [review] bug-945223.patch Review of attachment 8343179 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good. r=me with comments addressed. ::: js/src/jit/BaselineIC.cpp @@ +4289,5 @@ > + EmitRestoreTailCallReg(masm); > + masm.tagValue(JSVAL_TYPE_OBJECT, objReg, R0); > + masm.pushValue(R0); > + masm.pushValue(R1); > + EmitRepushTailCallReg(masm); isn't the operation normally written: masm.tagValue(JSVAL_TYPE_OBJECT, objReg, R0); EmitStowICValues(masm, 2); or is there some trouble with register aliasing that I'm not seeing? @@ +4305,5 @@ > leaveStubFrame(masm); > + > + // Pop pushed obj and key from baseline stack. > + EmitRestoreTailCallReg(masm); > + masm.add32(Imm32(sizeof(Value) * 2), BaselineStackReg); We previously had a patch that's EmitUnstowICValues(masm, 2, /* discard = */true); I think this model is cleaner, if we can move to EmitStowICValues above. @@ +4484,5 @@ > + masm.pushValue(val); > + masm.tagValue(JSVAL_TYPE_INT32, key, val); > + masm.pushValue(val); > + EmitRepushTailCallReg(masm); > + nit: whitespace on blank line
Attachment #8343179 -
Flags: review?(efaustbmo) → review+
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Eric Faust [:efaust] from comment #7) > isn't the operation normally written: > > masm.tagValue(JSVAL_TYPE_OBJECT, objReg, R0); > EmitStowICValues(masm, 2); > > or is there some trouble with register aliasing that I'm not seeing? Nope, your suggestion will work fine. Changing. > We previously had a patch that's EmitUnstowICValues(masm, 2, /* discard = > */true); > > I think this model is cleaner, if we can move to EmitStowICValues above. Good idea. EmitUnstowICValues doesn't take a discard parameter right now, so I added that. Running one last try before pushing.
Assignee | ||
Comment 9•9 years ago
|
||
Testcase added, pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f4d11a99e3
https://hg.mozilla.org/mozilla-central/rev/d4f4d11a99e3 This didn't land for 28, so you'd need to request Aurora uplift.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Target Milestone: --- → mozilla29
Assignee | ||
Comment 11•9 years ago
|
||
Updating with review-comments-fixed patch before aurora uplift request.
Attachment #8343179 -
Attachment is obsolete: true
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8345881 [details] [diff] [review] bug-945223.patch Forwarding r+ from previous patch to this one.
Attachment #8345881 -
Flags: review+
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8345881 [details] [diff] [review] bug-945223.patch [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 912303 User impact if declined: Crashes in code using __noSuchmethod__ Testing completed (on m-c, etc.): Green for a week on m-c Risk to taking this patch (and alternatives if risky): Low. String or IDL/UUID changes made by this patch: N/A
Attachment #8345881 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Attachment #8345881 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/ce854959b0a0
status-b2g-v1.3:
--- → fixed
Reporter | ||
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 15•9 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•9 years ago
|
status-b2g-v1.3T:
--- → fixed
status-b2g-v1.4:
--- → fixed
Updated•8 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•