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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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");

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Andrew McCreight [:mccr8]]

This assertion was added in bug 932180.  I don't know if the problem is new, or there is stricter checking.

Comment 3

[Jan de Mooij [:jandem]]

Needinfo from wingo since bug 932180 may have introduced this :)

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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?

Comment 5

[Kannan Vijayan [:djvj]]

Yep.  Taking.

Comment 6

[Kannan Vijayan [:djvj]]

Attachment: bug-945223.patch

Gotta push the temp values back on the stack before doing NoSuchMethodLookup, because decompiler might be invoked during the lookup.

More register surgery, ick.

Comment 7

[Eric Faust [:efaust]]

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

Comment 8

[Kannan Vijayan [:djvj]]

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

Comment 9

[Kannan Vijayan [:djvj]]

Testcase added, pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f4d11a99e3

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d4f4d11a99e3

This didn't land for 28, so you'd need to request Aurora uplift.

Comment 11

[Kannan Vijayan [:djvj]]

Attachment: bug-945223.patch

Updating with review-comments-fixed patch before aurora uplift request.

Comment 12

[Kannan Vijayan [:djvj]]

Comment on attachment 8345881 [details] [diff] [review]
bug-945223.patch

Forwarding r+ from previous patch to this one.

Comment 13

[Kannan Vijayan [:djvj]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ce854959b0a0

Comment 15

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.