944278 - OdinMonkey: Crash [@ js::Invoke]

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

Attachment: debug and opt stacks

x = function() {}
m = []
x.toString = (function(a, foreign) {
    "use asm";
    var ff = foreign.ff
    function f() {
        +ff()
    }
    return f
})(this, {
    ff: function() {
        return RegExp("()")
    }
})
Object.defineProperty(m, 6, {
    value: x
})
m.join()
m.join()
m.join()

crashes 64-bit js debug and opt shell on m-c changeset 5eb1c89fc2bc with --ion-eager at js::Invoke.

s-s because this is one of the rare Windows crashes (it didn't seem to reproduce on Mac), setting needinfo from Luke due to it seeming to involve asm.js.

My configure flags are:

AR=ar sh ./configure --host=x86_64-pc-mingw32 --target=x86_64-pc-mingw32 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-threadsafe <other NSPR options>

Comment 1

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

The prevalence of this is blocking asm.js fuzzing on Windows, at least for 64-bit.

Comment 2

[Luke Wagner [:luke]]

Could you autobisect?

Comment 3

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/2810e80e1393
user:        Hannes Verschore
date:        Wed Jun 12 21:58:22 2013 +0200
summary:     Bug 860838: OdinMonkey: Optimize FFI calls to ionmonkey, r=luke

Comment 4

[Hannes Verschore [:h4writer]]

Taking for now since this is caused by a bug by my patch. Nothing pops out of the testcase of backtrace. I'll probably have to debug deeper. Annoying that it only reproduces on windows, since I'm not used to do that.

Comment 5

[Hannes Verschore [:h4writer]]

I think I finally found the actual issue here. After Luke pointing me to ShadowStackSpace, I looked somewhat deeper into the calling convention on x64. Now I think the actual issue is that the stack wasn't aligned properly on 16bytes. I have no idea why using --disable-optimize doesn't mind being badly aligned. And I don't know yet why the same code in generateFFIInterpreterExit generates code that is works. (I'll check if it is properly aligned!)

But now I can get some good night sleep, finally. I'll create the patch tomorrow.

Comment 6

[Luke Wagner [:luke]]

Nice detectiving!  Too bad we didn't AssertStackAlignment; that certainly would have helped.

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: patch

Comment 8

[Hannes Verschore [:h4writer]]

In windows 64bit pushRegsInMask pushes 136bytes, so framePushed was 136. And we assumed in generateOOL it was aligned on 16bytes and set it to 0 framesPushed. Notice the 136%16 = 8. So here we got the misalignment.

Regarding that, we are actually lucky that on other platforms pushRegsInMask pushes a 16fold of memory to the stack. E.g. in windows 32bit we push 16bytes. (I assume other platforms too. I'm gonna check linux quickly too. But now we have AssertStackAlignment in place that should catch these.)

Comment 9

[Hannes Verschore [:h4writer]]

So linux has 16bytes for 32bit and 64bit for pushRegsInMask. So also aligned by accident.

(Such a big difference with windows 64bit! Since windows needs to push floats and some extra general purpose registers)

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 8343679 [details] [diff] [review]
patch

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

Thanks a lot for tracking this down!

::: js/src/jit/AsmJS.cpp
@@ +6136,5 @@
>  #else
>      masm.branchTestMagic(Assembler::Equal, JSReturnOperand, throwLabel);
>  #endif
>  
> +    uint32_t framePushed = masm.framePushed();

How about oolConvertFramePushed?

Comment 11

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30bdfabba1e0

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/30bdfabba1e0

Comment 13

[Hannes Verschore [:h4writer]]

This is windows x64 only. So do we care to uplift to FF27/FF28? I know this is not supported. But have we encouraged to use 64bit for asm.js developing (e.g. to lift the 3Gb memory limitation?)

Comment 14

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

We should take this at least on Aurora if we could, shouldn't we?

Comment 15

[Hannes Verschore [:h4writer]]

Comment on attachment 8343679 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 860838

User impact if declined: Crashes on Windows 64bit only

Testing completed (on m-c, etc.): Week on m-c

Risk to taking this patch (and alternatives if risky): No risk.

String or IDL/UUID changes made by this patch: /

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a5f796e3a9de

This landed on trunk during the Fx28 cycle, so this is already on Aurora.

Comment 17

[Paul Theriault [:pauljt] (no longer reading bugmail)]

If wontfix for 1.2, then I assume its wontfix for 1.1 as well.