913977 - OdinMonkey: Crash [@ js::CallJSNative]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

s = newGlobal()
try {
    evalcx("\
    y = new Array;\
    Array.prototype.push.call(y, this);\
    var x = x++;\
    __iterator__ = (function() {\
        Array.prototype.some.call(y, (function(stdlib, foreign, heap) {\
            \"use asm\";\
            var Infinity = stdlib.Infinity;\
            var ff = foreign.ff;\
            var Float32ArrayView = new stdlib.Float32Array(heap);\
            function f(i0) {\
                i0 = i0 | 0;\
                Float32ArrayView[2] = Infinity;\
            }\
            return f\
        })(this, {\
            ff: x\
        }, ArrayBuffer(4096)))\
    });\
    for (p in this) {}\
", s)
} catch (e) {}
evalcx("\
    function x() ( {\
        t: c\
    });\
    for (v in this) {}\
", s)

crashes js debug shell on m-c changeset c7cc85e13f7a with --baseline-eager at js::CallJSNative with an unknown memory address at the top of the stack (0x015d7018).

s-s because I don't know how bad this is.

My configure flags are:

LD=ld CROSS_COMPILE=1 CXX="clang++ -Qunused-arguments -arch i386" RANLIB=ranlib CC="clang -Qunused-arguments -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments" HOST_CXX="clang++ -Qunused-arguments" sh ./configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --with-ccache --enable-threadsafe <other NSPR options>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/9f988f6ee6df
user:        Douglas Crosher
date:        Fri Sep 06 07:44:06 2013 +1000
summary:     Bug 865516 - Optimize access to the heap with a constant index. r=luke

Douglas, is bug 865516 possibly related?

Comment 1

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

This crashes opt builds too.

Comment 2

[Douglas Crosher [:dougc]]

Good find, thanks.  It is related to changes in bug 865516, but it's not yet
clear if it is the direct cause.  The problem is that the asm.js code is
being linked twice.  Bug 865516 stores heap offsets within the instructions
and these are destructively modified when linking, which is ok if only
linked once.  When linked twice, the resulting heap offset is bad.  It's not
clear if it should be possible to link the code twice?  Let me explore it
further.

Comment 3

[Douglas Crosher [:dougc]]

Attachment: Don't bake in the heap offsets until it is clear that linking will succeed.

Here's one possible fix. This just moves the setting of the instruction heap
pointers to the end of asm.js linking, after the success has been validated.

Some other alternatives would be: reverse the offsets upon failure or when
relinking, store the offsets elsewhere so they are not lost when linking.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 801354 [details] [diff] [review]
Don't bake in the heap offsets until it is clear that linking will succeed.

I think the root cause of this bug is that we check "if (module.isLinked()) FAIL" at the top of DynamicallyLinkModule but mark the module as linked at the bottom (which causes a problem if we reenter the VM from a getter called during linking).

Right now, we also set the heap from setIsLinked, these could be decoupled so we could setIsLinked() immediately and then initHeap from within the module.hasArrayView() branch.

Comment 5

[Luke Wagner [:luke]]

Attachment: fix and test

The included testcase hits the assert w/o requiring any ARM details.  One annoying side-effect of this patch is that, with this fix, the testcases which linked multiple times (failing each time) needed to be updated to generate a new module each time.  However, this change shouldn't perturb any real code.

Comment 6

[Douglas Crosher [:dougc]]

Comment on attachment 801657 [details] [diff] [review]
fix and test

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

Looks good, thank you.

The patch includes a fix for bug 913758 which might not have been intended.

Comment 7

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3387d9c1005c

Comment 8

[Luke Wagner [:luke]]

Oh heh, I didn't know there was already bug 913758; I just fixed the assert b/c I was hitting it on qemu :)

Comment 9

[Christian Holler (:decoder)]

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

Comment 10

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

This landed on m-c:

http://hg.mozilla.org/mozilla-central/rev/3387d9c1005c

Comment 11

[Luke Wagner [:luke]]

https://hg.mozilla.org/mozilla-central/rev/3387d9c1005c

Comment 12

[Luke Wagner [:luke]]

Oops, Gary beat me to it :)

Comment 13

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.