Closed Bug 944278 Opened 10 years ago Closed 9 years ago

OdinMonkey: Crash [@ js::Invoke]


(Core :: JavaScript Engine: JIT, defect)

Windows 7
Not set



Tracking Status
firefox26 --- wontfix
firefox27 --- fixed
firefox28 --- fixed
firefox-esr24 --- wontfix
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- wontfix
b2g-v1.3 --- fixed


(Reporter: gkw, Assigned: h4writer)



(Keywords: crash, regression, testcase, Whiteboard: [fuzzblocker][qa-][adv-main27+])


(2 files)

Attached file debug and opt stacks
x = function() {}
m = []
x.toString = (function(a, foreign) {
    "use asm";
    var ff = foreign.ff
    function f() {
    return f
})(this, {
    ff: function() {
        return RegExp("()")
Object.defineProperty(m, 6, {
    value: x

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>
Flags: needinfo?(luke)
The prevalence of this is blocking asm.js fuzzing on Windows, at least for 64-bit.
Whiteboard: [fuzzblocker]
Could you autobisect?
Flags: needinfo?(luke)
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
user:        Hannes Verschore
date:        Wed Jun 12 21:58:22 2013 +0200
summary:     Bug 860838: OdinMonkey: Optimize FFI calls to ionmonkey, r=luke
Blocks: 860838
Keywords: regression
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.
Assignee: general → hv1989
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.
Nice detectiving!  Too bad we didn't AssertStackAlignment; that certainly would have helped.
Attached patch patchSplinter Review
Attachment #8343679 - Flags: review?(luke)
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.)
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 on attachment 8343679 [details] [diff] [review]

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?
Attachment #8343679 - Flags: review?(luke) → review+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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?)
Flags: needinfo?(luke)
We should take this at least on Aurora if we could, shouldn't we?
Flags: needinfo?(luke)
Comment on attachment 8343679 [details] [diff] [review]

[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: /
Attachment #8343679 - Flags: approval-mozilla-beta?
Attachment #8343679 - Flags: approval-mozilla-aurora?
Attachment #8343679 - Flags: approval-mozilla-beta?
Attachment #8343679 - Flags: approval-mozilla-beta+
Attachment #8343679 - Flags: approval-mozilla-aurora?
Attachment #8343679 - Flags: approval-mozilla-aurora+
Whiteboard: [fuzzblocker] → [fuzzblocker][qa-]
If wontfix for 1.2, then I assume its wontfix for 1.1 as well.
Whiteboard: [fuzzblocker][qa-] → [fuzzblocker][qa-][adv-main27+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.