Closed Bug 1318039 Opened 3 years ago Closed 3 years ago

Crash in vcruntime140.dll@0xc5aa | js::CompileAsmJS , when running Nightly 64bit with profile which had been used 32bit

Categories

(Core :: JavaScript Engine: JIT, defect, P1, critical)

52 Branch
x86_64
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 + fixed
firefox53 --- fixed

People

(Reporter: alice0775, Assigned: luke)

References

()

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files)

This bug was filed from the Socorro interface and is 
report bp-821ddbf6-f0c6-45ba-8df8-eeb082161116.
=============================================================


Reproducible: 100%

Steps To Reproduce:
1. Create New profile with Nightly53.0a1 32bit.
2. Start Nightly53.0a1 32bit with the profile
3. Open http://beta.unity3d.com/jonas/AngryBots/ and wait to start the game
4. Quit browser

5. Start Nightly53.0a1 x64 with the same profile of step2
6. Open http://beta.unity3d.com/jonas/AngryBots/

Actual Results:
Tab crashes

Expected Results:
Not crash
I am not able to reproduce the crash. I tested Nightly53.0a1 x64 on Windows 10 (Insider Preview "Slow" channel build 14931.rs_prelease.160916-1700).

Alice, you set the tracking flag status-firefox51:unaffected. Is this a regression in Firefox 52?
Blocks: support-win64
No longer blocks: win64-migration
Flags: needinfo?(alice0775)
(In reply to Chris Peterson [:cpeterson] from comment #1)
> I am not able to reproduce the crash. I tested Nightly53.0a1 x64 on Windows
> 10 (Insider Preview "Slow" channel build 14931.rs_prelease.160916-1700).
> 
> Alice, you set the tracking flag status-firefox51:unaffected. Is this a
> regression in Firefox 52?

Yes.

Regression window(m-i):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9d1b72a21a395e36cbfd9982243054e64a955bb3&tochange=eafea39ebf3977beefda0a7cdcef8c8266060ca0

Regressed by: eafea39ebf39	Luke Wagner — Bug 1313180 - Baldr: fix accidental disabling of asm.js when wasm is disabled (r=sunfish)
Flags: needinfo?(alice0775)
[Tracking Requested - why for this release]:

Luke, Alice reports this crash is a regression in Firefox 52 from your wasm changes in bug 1313180. I am unable to reproduce the crash on my machine.
Blocks: 1313180
Flags: needinfo?(luke)
Keywords: regression
Any chance the regression range could be extended too by setting about:config javascript.options.wasm to true (like in the other bug)? It might even be the same cause.
bisecting further with force set javascript.options.wasm=true.

Regression window(with javascript.options.wasm=true):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=099453a244223d8f85c4f19a25da5d7de0058dff&tochange=f325d73d76d14f2b4ee2e05d6c23a8f8f521907f

Triggered by: Bug 1276029
Blocks: 1276029
No longer blocks: 1313180
Right, I think I see the bug: there is a size_t being used in LookupAsmJSModuleInCache.  Normally this is fine, b/c the build-id mismatches, but this is *before* the build-id mismatch.  Should be simple to fix.
Flags: needinfo?(luke)
Attached patch fix-asmjs-cacheSplinter Review
This should fix it; I'll try to confirm when the try build is finished.
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #8811507 - Flags: review?(bbouvier)
(I'm able to reproduce the crash before and no crash with the patch.)
Priority: -- → P1
Comment on attachment 8811507 [details] [diff] [review]
fix-asmjs-cache

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

Wow, amazing STR, thanks Alice0775 White! And great find, luke!
Attachment #8811507 - Flags: review?(bbouvier) → review+
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48e65314c869
Baldr: Make preamble of asm.js cache entries word-size-invariant (r=bbouvier)
Comment on attachment 8811507 [details] [diff] [review]
fix-asmjs-cache

Approval Request Comment
[Feature/regressing bug #]: 1276029
[User impact if declined]: crash if they switch between 32/64-bit browsers with the same profile
[Describe test coverage new/current, TreeHerder]: m-c
[Risks and why]: low, small fix to recently-introduced regression
[String/UUID change made/needed]: none
Attachment #8811507 - Flags: approval-mozilla-aurora?
I am suffering from this today. I run 64-bit Nightly. I also have 64-bit release and 32-bit release installed. I'm really glad this has been tracked down.

Million dollar question: When this lands will it correct my situation (64 bit versions don't start-up)?
I assume we're talking a damaged profile that 64-bit can't read. Perhaps I can get a 32-bit nightly version and load the profile after the fix lands.
(In reply to Matt from comment #12)
Sorry for the trouble.  This patch fixes switching between 32- and 64-bit profiles *with the patch*, but your question actually makes me realize that I should make the entire path up to the build-id checks fallible, and then new builds will always robustly handle any previous generated cache files.  Thanks for asking!
This improved patch should ensure we can't crash before build-id checking, after which point we're good.  (In theory you could have an accidental byte match, but given that we're not talking about arbitrary binaries, but only binaries generated by previous asm.js caching which have a fixed prologue, there should be zero chance.)
Attachment #8811830 - Flags: review?(bbouvier)
Comment on attachment 8811507 [details] [diff] [review]
fix-asmjs-cache

Cancelling; I'll put up a folded patch.
Attachment #8811507 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/48e65314c869
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8811830 [details] [diff] [review]
better-asmjscache-fix

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

Thanks!
Attachment #8811830 - Flags: review?(bbouvier) → review+
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/386d729a4992
Better fix for asm.js cache build-id mismatches (r=bbouvier)
Approval Request Comment
[Feature/regressing bug #]: 1276029
[User impact if declined]: crash if they switch between 32/64-bit browsers with the same profile
[Describe test coverage new/current, TreeHerder]: m-c
[Risks and why]: low, small fix to recently-introduced regression
[String/UUID change made/needed]: none

(This patch is just a folded version of the two that landed on m-c.)
Attachment #8812307 - Flags: review+
Attachment #8812307 - Flags: approval-mozilla-aurora?
Comment on attachment 8812307 [details] [diff] [review]
folded-for-aurora

take this change in aurora52 to guard against word-size mismatch in asmjs cache
Attachment #8812307 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.