Closed Bug 1323854 Opened 6 years ago Closed 6 years ago

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


(Core :: JavaScript Engine, defect, P1)




Tracking Status
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- verified


(Reporter: gkw, Assigned: h4writer)



(4 keywords, Whiteboard: [jsbugmon:update])


(3 files, 2 obsolete files)

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


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)

For detailed crash information, see attachment.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
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)
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
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.
Attached 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]

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)
Another testcase:

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

This one involves "use asm".
Flags: needinfo?(hv1989)
Flags: needinfo?(hv1989)
Priority: -- → P1
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)
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.
Keywords: leave-open
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);

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+
Keywords: leave-open
Closed: 6 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
Yes it only affects 53. Regression range was correct. (bug 1310155 which landed in FF53).
Group: javascript-core-security → core-security-release
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.