781660 - IonMonkey: Crash [@ ExpressionDecompiler::decompilePC] or "Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth)," or "Assertion failure: pcdepth >= nuses,"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack from Windows 7

options('strict')
x = 2
try {} catch (u if ({})) {}
x.e

asserts js debug shell on IonMonkey changeset c0195737650c without any CLI arguments at 
Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth),

Comment 1

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

(not sure if this is correct)

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   103173:0bc212d0183b
parent:      103059:b457b592f609
parent:      103172:a7fadfbad932
user:        David Anderson
date:        Fri Aug 03 18:58:30 2012 -0700
summary:     Merge from mozilla-central.

Quite sure this is IonMonkey, it does not affect mozilla-central changeset ed5c848f50e0 w/ w/o -m or -n.

Comment 2

[Nicolas B. Pierron [:nbp]]

I added this assertion to make sure that the ReconstructPCStack behave correctly by comparing it to the script analysis.  This assertion was added in Bug 780451 and pushed in IonMonkey.

Comment 3

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

Related testcases seem also to produce this assertion: "Assertion failure: pcdepth >= nuses,"

Comment 4

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

try {
    h
} catch (x
if gc()) {} finally {
    this.z.z
}

Run this on 64-bit debug shell on IonMonkey changeset 32b7b76d111c with --no-ti:

Assertion failure: pcdepth >= nuses,

It switches back to the other assertion in this bug without --no-ti, or if you replace gc with print.

s-s because gc seems essential here.

Comment 5

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

Attachment: stack from opt crash

(function () {
    try {} catch (x if true) { } finally {
       3(__defineSetter__("x", encodeURIComponent))
    }
})()

crashes 64-bit js opt shell on IonMonkey changeset d794f23798f4 without any CLI arguments at ExpressionDecompiler::decompilePC

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: Follow goto's target if identical to the reconstructed one.

Fix first test case reported in the bug by avoiding continuing on a throw instruction.

Comment 7

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

> crashes 64-bit js opt shell on IonMonkey changeset d794f23798f4 without any
> CLI arguments at ExpressionDecompiler::decompilePC

Was the clearing of the opt crash intentional? I forgot to mention that the testcase causes a similar assertion in debug builds.

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Gary Kwong [:gkw, :nth10sd] from comment #7)
> > crashes 64-bit js opt shell on IonMonkey changeset d794f23798f4 without any
> > CLI arguments at ExpressionDecompiler::decompilePC
> 
> Was the clearing of the opt crash intentional? I forgot to mention that the
> testcase causes a similar assertion in debug builds.

No, it wasn't.

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 652981 [details] [diff] [review]
Follow goto's target if identical to the reconstructed one.

https://hg.mozilla.org/projects/ionmonkey/rev/13b1019e04af

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Follow try notes when we cannot follow gotos.

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #10)
> Created attachment 654478 [details] [diff] [review]
> Follow try notes when we cannot follow gotos.

This patch should fix the last 2 test cases.

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 654478 [details] [diff] [review]
Follow try notes when we cannot follow gotos.

Review of attachment 654478 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsopcode.cpp
@@ +6465,5 @@
> +            JSTryNote *tnEnd = tn + script->trynotes()->length;
> +            for (; tn != tnEnd; tn++) {
> +                jsbytecode *start = script->main() + tn->start;
> +                jsbytecode *end = start + tn->length;
> +                if (start < pc && pc <= end && end <= target)

Looking at TryNoteIter, it seems like we'd want the inclusive range [start, end].  If not, could you comment as to why not?

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Luke Wagner [:luke] from comment #12)
> Comment on attachment 654478 [details] [diff] [review]
> Follow try notes when we cannot follow gotos.
> 
> Review of attachment 654478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsopcode.cpp
> @@ +6465,5 @@
> > +            JSTryNote *tnEnd = tn + script->trynotes()->length;
> > +            for (; tn != tnEnd; tn++) {
> > +                jsbytecode *start = script->main() + tn->start;
> > +                jsbytecode *end = start + tn->length;
> > +                if (start < pc && pc <= end && end <= target)
> 
> Looking at TryNoteIter, it seems like we'd want the inclusive range [start,
> end].  If not, could you comment as to why not?

Hum … Maybe I should just add another constructor to TryNoteIter and reuse it instead of adding another for loop to iterate over the try notes.  What do you think ?

Comment 14

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/projects/ionmonkey/rev/4c24e4fc3472

Comment 15

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 16

[Nicolas B. Pierron [:nbp]]

The last patch has been reverted after some test failures on mochitest.
https://hg.mozilla.org/projects/ionmonkey/rev/6c7537647873

Comment 17

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 92b9b2840a79).

Comment 18

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

Comment on attachment 654478 [details] [diff] [review]
Follow try notes when we cannot follow gotos.

This patch was backed out.

Comment 19

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision a26469331bf9).

Comment 20

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

Comment on attachment 654478 [details] [diff] [review]
Follow try notes when we cannot follow gotos.

An updated form of this patch to fix the testcases in comment 4 and comment 5 should be posted in bug 787309 now that Nicolas and I have agreed to spin them off into that bug after a f2f discussion.

Comment 21

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

Still resolving FIXED since the original testcase in comment 0 has been fixed by the patch in comment 6.

At Nicolas' request, please do not yet open up this bug until mentioned otherwise. (bug may affect m-c, but we are not sure yet)

Comment 22

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.