Closed Bug 781660 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: gkw, Assigned: nbp)

References

Details

(4 keywords, Whiteboard: [ion:p1:fx18] [jsbugmon:update,ignore])

Crash Data

Attachments

(3 files, 1 obsolete file)

Attached file 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),
Summary: IonMonkey: " Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth)," → IonMonkey: "Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth),"
(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.
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.
Blocks: 780451
Whiteboard: [ion:p1:fx18]
Related testcases seem also to produce this assertion: "Assertion failure: pcdepth >= nuses,"
Summary: IonMonkey: "Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth)," → IonMonkey: "Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth)," or "Assertion failure: pcdepth >= nuses,"
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.
Group: core-security
Attached file 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
Crash Signature: [@ ExpressionDecompiler::decompilePC]
Summary: IonMonkey: "Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth)," or "Assertion failure: pcdepth >= nuses," → IonMonkey: Crash [@ ExpressionDecompiler::decompilePC] or "Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth)," or "Assertion failure: pcdepth >= nuses,"
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Crash Signature: [@ ExpressionDecompiler::decompilePC]
Summary: IonMonkey: Crash [@ ExpressionDecompiler::decompilePC] or "Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth)," or "Assertion failure: pcdepth >= nuses," → IonMonkey: "Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth)," or "Assertion failure: pcdepth >= nuses,"
Fix first test case reported in the bug by avoiding continuing on a throw instruction.
Attachment #652981 - Flags: review?(luke)
Attachment #652981 - Flags: review?(luke) → review+
> 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.
(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.
Crash Signature: [@ ExpressionDecompiler::decompilePC]
Summary: IonMonkey: "Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth)," or "Assertion failure: pcdepth >= nuses," → IonMonkey: Crash [@ ExpressionDecompiler::decompilePC] or "Assertion failure: script->analysis()->getCode(pc).stackDepth == ((hpcdepth == unsigned(-1)) ? pcdepth : hpcdepth)," or "Assertion failure: pcdepth >= nuses,"
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
Attachment #652981 - Flags: checkin+
Attachment #654478 - Flags: review?(luke)
(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 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?
Attachment #654478 - Flags: review?(luke) → review+
(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 ?
https://hg.mozilla.org/projects/ionmonkey/rev/4c24e4fc3472
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #654478 - Flags: checkin+
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
The last patch has been reverted after some test failures on mochitest.
https://hg.mozilla.org/projects/ionmonkey/rev/6c7537647873
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Whiteboard: [ion:p1:fx18] → [ion:p1:fx18][jsbugmon:update]
Whiteboard: [ion:p1:fx18][jsbugmon:update] → [ion:p1:fx18] [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 92b9b2840a79).
Comment on attachment 654478 [details] [diff] [review]
Follow try notes when we cannot follow gotos.

This patch was backed out.
Attachment #654478 - Flags: checkin+
Whiteboard: [ion:p1:fx18] [jsbugmon:update,ignore] → [ion:p1:fx18] [jsbugmon:update,reconfirm]
Whiteboard: [ion:p1:fx18] [jsbugmon:update,reconfirm] → [ion:p1:fx18] [jsbugmon:update,reconfirm,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision a26469331bf9).
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.
Attachment #654478 - Attachment is obsolete: true
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)
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
No longer blocks: 787309
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [ion:p1:fx18] [jsbugmon:update,reconfirm,ignore] → [ion:p1:fx18] [jsbugmon:update,ignore]
Flags: in-testsuite?
Group: core-security
You need to log in before you can comment on or make changes to this bug.