798823 - Crash [@ js::ion::IonJSFrameLayout::calleeToken] or [@ js::ion::SnapshotIterator::SnapshotIterator]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

function g() {
  switch (0) {
  default:
    w = newGlobal('');
  }
  return function(f, code) {
    try {
      evalcx(code, w)
    } catch (e) {}
  }
}

function f(code) {
  h(Function(code), code);
}
h = g()
f("\
    x = [];\
    y = new Set;\
    z = [];\
    Object.defineProperty(x, 5, {\
        get: (function(j) {}),\
    });\
    Object.defineProperty(z, 3, {});\
    z[9] = 1;\
    x.shift();\
");
f("\
    z.every(function() {\
        x.filter(function(j) {\
            if (j) {} else {\
                y.add()\
            }\
        });\
        return 2\
    })\
");

crashes js debug and opt shell on IonMonkey changeset 9f677c2bb33d with --ion-eager at js::ion::IonJSFrameLayout::calleeToken with js::ion::SnapshotIterator::SnapshotIterator on the stack.

s-s and assuming sec-critical because memory address 0x8fff4bb7db28 is being accessed.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   109524:20fe6e539f7f
user:        Jan de Mooij
date:        Sat Oct 06 11:38:18 2012 +0200
summary:     Bug 797131 part 3 - Add fast path for calling into Ion. r=dvander

Comment 1

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

I verify that this is not fixed by the patch in bug 798819.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

Storing the IonActivation in FastInvokeGuard is dangerous because it may leave rt->ionTop in an invalid state. Since it doesn't actually help performance, this patch just moves the IonActivation to FastInvoke, much nicer and safer and what I should have done in the first place.

This patch also enables bug 797131 part 3 again.

gkw or decoder, could one of you apply this patch to m-c/m-i tip and see if new crashes/asserts come up? We'd like to enable it before the merge tomorrow, but only if it's stable. Thanks!

Comment 3

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

Comment on attachment 668893 [details] [diff] [review]
Patch

Approving based on nothing showing up after 10 minutes of fuzzing, will help to land this on mozilla-inbound.

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d407f51ca61e

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 668893 [details] [diff] [review]
Patch

Thanks a lot Gary!

Comment 6

[Matthew Gregan [:kinetik]]

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

Comment 7

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.