1448605 - Assertion failure: segments1_.empty(), at js/src/wasm/WasmProcess.cpp:129

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Bob Clary [:bc]]

Attachment: asan debug log

1. https://mega.nz/#fm/transfers
   using bughunter's marionette based runner
   and an debug asan build.

   Most of the urls (though not all) are from mega.nz.

   python runner.py --binary nightly-asan/mozilla/firefox-debug/dist/bin/firefox --url "https://mega.nz/#fm/transfers"

2. Assertion failure: segments1_.empty(), at /builds/worker/workspace/build/src/js/src/wasm/WasmProcess.cpp:129
    #0 0x7f3816770c09 in ProcessCodeSegmentMap::freeAll() /builds/worker/workspace/build/src/js/src/wasm/WasmProcess.cpp:130:9
    #1 0x7f38161aa830 in JS_ShutDown() /builds/worker/workspace/build/src/js/src/vm/Initialization.cpp:177:5
    #2 0x7f380c55aa8d in mozilla::ShutdownXPCOM(nsIServiceManager*) /builds/worker/workspace/build/src/xpcom/build/XPCOMInit.cpp:1003:5

during shutdown. This has been going on since at least December 2017 with Firefox 59.

You can find the script at <https://hg.mozilla.org/automation/sisyphus/file/tip/python/sisyphus/automation/runner.py>. You may need to requests, mozprofile,
marionette_driver.

Comment 1

[Luke Wagner [:luke]]

Do you have an automated way to bisect this failure?

Comment 2

[Bob Clary [:bc]]

No, sorry. If it is important I can try to get asan builds working locally and do it via hg bisect. Let me know if you think it is worth it.

Comment 3

[Luke Wagner [:luke]]

Oh wait, this is probably just a case where JSRuntime::hasLiveRuntimes().  In a failing log, do you see "WARNING: YOU ARE LEAKING THE WORLD" soon before the assert?  Alternatively, does changing the two asserts to
  MOZ_ASSERT_IF(!JSRuntime::hasLiveRuntimes(), segments(1|2)_.empty());
fix the assert?

Comment 4

[Bob Clary [:bc]]

re: WARNING: YOU ARE LEAKING THE WORLD

Yes.

Comment 5

[Luke Wagner [:luke]]

Excellent, simple fix then.

Comment 6

[Luke Wagner [:luke]]

Attachment: tweak-init-shutdown

Comment 7

[Benjamin Bouvier [:bbouvier]]

Comment on attachment 8962493 [details] [diff] [review]
tweak-init-shutdown

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

LGTM, thanks.

::: js/src/wasm/WasmProcess.cpp
@@ +247,5 @@
>  
> +bool
> +wasm::Init()
> +{
> +    return InitSignatureSet();

I was curious about the lack of builtin thunks initialization here, but I guess it is still better to do it lazily if there's at least one wasm module (as we do now), because the thunk generation can take some time. So that makes me wonder whether the signature set could also be lazily initialized later?

Comment 8

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
The nice thing about eagerly creating the sigIdSet during process-wide init is you can assume it is done once in a race-free manner.  Otherwise, you have to have a lock for initialization like WasmBuiltins.cpp does.  But since sigIdSet points to an ExclusiveData that also has a lock, that's two locks which feels a bit silly.

I was wondering why sigIdSet was a pointer-to-ExclusiveData instead of just an ExclusiveData (needing no init/release) and found bug 1309909 comment 6.  Why is it ok for WasmBuiltins.cpp to have a global Mutex but not ok for WasmInstance.cpp to have a global ExclusiveData (which is just a Mutex+T)?  Because the dtor of ExclusiveData conservatively takes a lock.  I'm not sure that's necessary though, so I filed bug 1449189.  Assuming we remove the lock, sigIdSet can use lazy init.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77288857a25b
Baldr: quiet shutdown assertions when hasLiveRuntimes (r=bbouvier)

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/77288857a25b

Comment 11

[Julien Cristau [:jcristau]]

Is this something we should uplift to beta?

Comment 12

[Luke Wagner [:luke]]

No, it's just a spurious DEBUG-only assert.