Closed Bug 913977 Opened 11 years ago Closed 11 years ago

OdinMonkey: Crash [@ js::CallJSNative]


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox25 --- unaffected
firefox26 --- fixed
firefox-esr17 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected


(Reporter: gkw, Assigned: luke)



(Keywords: crash, regression, testcase, Whiteboard: [jsbugmon:update])

Crash Data


(3 files)

Attached file stack
s = newGlobal()
try {
    y = new Array;\, this);\
    var x = x++;\
    __iterator__ = (function() {\, (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) {}
    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:
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?
Flags: needinfo?(dtc-moz)
This crashes opt builds too.
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
Flags: needinfo?(dtc-moz)
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.
Attachment #801354 - Flags: review?(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.
Attachment #801354 - Flags: review?(luke)
Attached patch fix and testSplinter Review
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.
Assignee: general → luke
Attachment #801657 - Flags: review?(dtc-moz)
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.
Attachment #801657 - Flags: review?(dtc-moz) → review+
Oh heh, I didn't know there was already bug 913758; I just fixed the assert b/c I was hitting it on qemu :)
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 680c89c76100).
This landed on m-c:
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update]
Target Milestone: --- → mozilla26
Oops, Gary beat me to it :)
Flags: in-testsuite?
JSBugMon: This bug has been automatically verified fixed.
Group: core-security
You need to log in before you can comment on or make changes to this bug.