Assertion failure: exprStack == stackDepth, at js/src/jit/Recover.cpp:110

VERIFIED FIXED in Firefox 53

Status

()

defect
P1
critical
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: gkw, Assigned: h4writer)

Tracking

(Blocks 2 bugs, 4 keywords)

Trunk
mozilla53
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 unaffected, firefox51 unaffected, firefox52 unaffected, firefox53 verified)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

3 years ago
The following testcase crashes on mozilla-central revision 489f981e8c2b (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager --ion-gvn=off):

// Adapted from randomly chosen test: js/src/jit-test/tests/debug/Script-isInCatchScope.js
var g = newGlobal();
var dbg = new Debugger(g);
dbg.onExceptionUnwind = function(m) {
    do {
        m = m.older;
    } while (m != null);
};
g.eval("try { throw (function() {});} finally {}");


Backtrace:

0   js-dbg-64-dm-clang-darwin-489f981e8c2b	0x0000000106446a99 js::jit::MResumePoint::writeRecoverData(js::jit::CompactBufferWriter&) const + 969 (Recover.cpp:110)
1   js-dbg-64-dm-clang-darwin-489f981e8c2b	0x00000001064847e5 js::jit::RecoverWriter::writeInstruction(js::jit::MNode const*) + 21 (Snapshots.cpp:722)
2   js-dbg-64-dm-clang-darwin-489f981e8c2b	0x00000001064c49bb js::jit::CodeGeneratorShared::encode(js::jit::LRecoverInfo*) + 203 (CodeGenerator-shared.cpp:569)
3   js-dbg-64-dm-clang-darwin-489f981e8c2b	0x00000001064c4b01 js::jit::CodeGeneratorShared::encode(js::jit::LSnapshot*) + 49 (CodeGenerator-shared.cpp:584)
4   js-dbg-64-dm-clang-darwin-489f981e8c2b	0x000000010654f79f void js::jit::CodeGeneratorX86Shared::bailout<js::jit::BailoutLabel>(js::jit::BailoutLabel const&, js::jit::LSnapshot*) + 31 (JitFrames.h:257)
/snip

For detailed crash information, see attachment.
Reporter

Comment 2

3 years ago
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/3daa33144b64
user:        Hannes Verschore
date:        Thu Dec 08 13:53:05 2016 -1000
summary:     Bug 1310155 - IonMonkey, part 1.0: Split graph creation from IonBuilder, r=jandem

Hannes, is bug 1310155 a likely regressor?
Blocks: 1310155
Flags: needinfo?(hv1989)
Reporter

Comment 3

3 years ago
Note that this seems to be different from other assertion failures with similar messages as this seems to require --ion-gvn=off.
This error message usually means that the resume point does not exactly capture the stack used by Baseline / Interpreter.  Thus, using the snapshot which uses this resume point will probably cause mismatch when resuming the execution in Baseline.
Group: javascript-core-security
Assignee

Comment 5

3 years ago
The issue is that the backedge block (which is usually empty) points to a the last pc of the loop:

> Dumping cfg:
> 
>  Block 0, 0:0
>   LoopEntry [1]
> 
>  Block 1, 16:22
>   Test [3, 4]
> 
>  Block 3, 22:22
>   BackEdge [1]
> 
>  Block 4, 27:28
>   27: jumptarget
>   RetRVal []

But we manage to add a MFilterTypeSet and MUnbox in the backedge block. As a result we use pc:22 to encode the bailout, which doesn't work. Since that is the start of the block outside loop. As a result that is faulty.
I think the easiest fix would be to adjust the pc to the loopEntry pc.
Assignee

Comment 6

3 years ago
Posted patch Patch (obsolete) — Splinter Review
Update the "pc" to be the start of the loop if the block start/stop pc points to the same pc, which is actually after the loop. By pointing the the start of the loop, we can use that pc correctly for bailing.
Assignee: nobody → hv1989
Flags: needinfo?(hv1989)
Attachment #8820307 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8820307 [details] [diff] [review]
Patch

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

::: js/src/jit/MIR.h
@@ +13027,5 @@
>      jsbytecode* pc() const {
>          return pc_;
>      }
> +    void updatePc(jsbytecode* pc) {
> +        pc_ = pc;

The pc of the resume point is supposed to match the stack depth of the emulated interpreter stack (MBasicBlock::push & MBasicBlock::pop within IonBuilder)

Thus mutating the pc after the allocation of the FixedList operands sounds like an error. The pc should remain constant once the ResumePoint is created.

Using the Loop Entry PC is the right approach to this problem, as long as the backedge block is empty.

However, I would recommend to do that as part of the creation of the backedges block, in IonControlFlow.cpp.  Also, I wonder why this is a problem now, and why it was not there before?
Attachment #8820307 - Flags: review?(nicolas.b.pierron)
Reporter

Comment 8

3 years ago
Another testcase:

g = (function () {
    "use asm";
    function f(i0, d1) {
        d1 = 4258326539 >>> 0;
        switch (-8 && i0) {
            case -1:
                d1 = 0;
            case 0:
        }
    }
    return f;
})();
g();
g();

This one involves "use asm".
Flags: needinfo?(hv1989)
Assignee

Updated

3 years ago
Flags: needinfo?(hv1989)
Priority: -- → P1
Assignee

Comment 9

3 years ago
This the issue Gary reported.

In IonControlFlow we were having a block after the "case" (which pops 2) which in IonControlFlow we only pop 1 goto a empty block and pop again. For that empty block we needed an "pc", but there is no corresponding PC. That worked all right, but one of our passes is adding fallible functions in that empty block. Hey presto we got this issue.
I fixed it by making sure CFGCompare directly pops 2 values. I still need the empty blocks, since "visitCompare" in IonBuilder cannot handle an already existing block. That block gets cleaned up later anyhow. As a result I don't see the need to fix that. If you think it would be better it should be a follow-up bug, since I wouldn't be surprised that introduced another bug ;)
Attachment #8824448 - Flags: review?(nicolas.b.pierron)
Assignee

Comment 10

3 years ago
Attachment #8820307 - Attachment is obsolete: true
Attachment #8824943 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8824943 [details] [diff] [review]
Part 1: Use a more correct pc for backedges in empty blocks

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

::: js/src/jit-test/tests/bug1323854-2.js
@@ +1,1 @@
> +// |jit-test| --ion-gvn=off; Exception

note, do not land the test case with the patch if this is on aurora, also do we throw an Exception while the test case has a try-catch ?
Attachment #8824943 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8824448 [details] [diff] [review]
Part 2: Keep the same symantics as bytecode on CFGCompare

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

Would it make more sense to have a CFGCase (-2, -1) and a CFGDefault (-2) similar to how they are implemented in the interpreter as JSOP_CASE and JSOP_DEFAULT?

::: js/src/jit/IonBuilder.cpp
@@ +2807,5 @@
>  AbortReasonOr<Ok>
>  IonBuilder::visitCompare(CFGCompare* compare)
>  {
> +    MDefinition* lhs = current->peek(-1);
> +    MDefinition* rhs = current->peek(-2);

existing issue: -2 is lhs, and -1 is rhs.
Assignee

Updated

3 years ago
Keywords: leave-open
Assignee

Comment 13

3 years ago
Attachment #8824448 - Attachment is obsolete: true
Attachment #8824448 - Flags: review?(nicolas.b.pierron)
Attachment #8826145 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8826145 [details] [diff] [review]
Part 2: Keep the same symantics as bytecode on CFGCompare

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

nit: s/symantic/semantic/ in the patch subject.

::: js/src/jit/IonControlFlow.cpp
@@ +131,5 @@
>              CFGCompare* old = ins->toCompare();
>              CFGBlock* trueBranch = &blocks_[old->trueBranch()->id()];
>              CFGBlock* falseBranch = &blocks_[old->falseBranch()->id()];
> +            copy = CFGCompare::New(alloc, trueBranch, old->truePopAmount(),
> +                                          falseBranch, old->falsePopAmount());

nit: I would much prefer if we could let the CFGCompare constructor copy the pop amount instead of giving them as argument here.

::: js/src/jit/IonControlFlow.h
@@ +323,5 @@
>   */
>  class CFGCompare : public CFGAryControlInstruction<2>
>  {
> +    size_t truePopAmount_;
> +    size_t falsePopAmount_;

maybe-nit: "const size_t …;" ?

@@ +329,1 @@
>      CFGCompare(CFGBlock* succ1, CFGBlock* succ2)

This constructor is unused, remove it.

@@ +343,5 @@
>      }
>  
>    public:
>      CFG_CONTROL_HEADER(Compare);
>      TRIVIAL_CFG_NEW_WRAPPERS

Replace the TRIVIAL_CFG_NEW_WRAPPER, as we do not want to be able to set the pop amount manually.
Instead add the following function:
    static CFGCompare* CopyWithNewTargets(TempAllocator&, CFGCompare*, CFGBlock*, CFGBlock*);
Attachment #8826145 - Flags: review?(nicolas.b.pierron) → review+
Assignee

Updated

3 years ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/a2819ba15312
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Hannes: Can I assume that Gary was right about the regressor and that this affects only 53? Otherwise please retroactive fill out the sec-approval questions so we can evaluate whether we need to back-port this. Thanks!
Flags: needinfo?(hv1989)
Keywords: regression
Assignee

Comment 20

2 years ago
Yes it only affects 53. Regression range was correct. (bug 1310155 which landed in FF53).
Group: javascript-core-security → core-security-release

Updated

2 years ago
Status: RESOLVED → VERIFIED

Comment 21

2 years ago
JSBugMon: This bug has been automatically verified fixed.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.