Assertion failure: index >= size_t(parser.stackDepthAtPC(current)), at ../jsopcode.cpp:1796

VERIFIED FIXED in Firefox 28

Status

()

defect
--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: decoder, Assigned: djvj)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla29
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(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)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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");
This assertion was added in bug 932180.  I don't know if the problem is new, or there is stricter checking.
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?
Yep.  Taking.
Assignee: general → kvijayan
Flags: needinfo?(kvijayan)
Posted patch bug-945223.patch (obsolete) — Splinter Review
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 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+
(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.
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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updating with review-comments-fixed patch before aurora uplift request.
Attachment #8343179 - Attachment is obsolete: true
Comment on attachment 8345881 [details] [diff] [review]
bug-945223.patch

Forwarding r+ from previous patch to this one.
Attachment #8345881 - Flags: review+
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?
Attachment #8345881 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: core-security
You need to log in before you can comment on or make changes to this bug.