Closed Bug 1448605 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1)

61 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: bc, Assigned: luke)

References

()

Details

(Keywords: assertion)

Attachments

(2 files)

Attached file 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.
Flags: needinfo?(luke)
Do you have an automated way to bisect this failure?
Flags: needinfo?(bob)
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.
Flags: needinfo?(bob)
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?
Flags: needinfo?(luke) → needinfo?(bob)
re: WARNING: YOU ARE LEAKING THE WORLD

Yes.
Flags: needinfo?(bob)
Excellent, simple fix then.
Assignee: nobody → luke
Attachment #8962493 - Flags: review?(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?
Attachment #8962493 - Flags: review?(bbouvier) → review+
Priority: -- → P1
(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.
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77288857a25b
Baldr: quiet shutdown assertions when hasLiveRuntimes (r=bbouvier)
https://hg.mozilla.org/mozilla-central/rev/77288857a25b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Is this something we should uplift to beta?
Flags: needinfo?(luke)
No, it's just a spurious DEBUG-only assert.
Flags: needinfo?(luke)
You need to log in before you can comment on or make changes to this bug.