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

RESOLVED FIXED in Firefox 61

Status

()

defect
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bc, Assigned: luke)

Tracking

(Blocks 1 bug, {assertion})

61 Branch
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

Details

()

Attachments

(2 attachments)

Reporter

Description

a year ago
Posted 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)
Assignee

Comment 1

a year ago
Do you have an automated way to bisect this failure?
Flags: needinfo?(bob)
Reporter

Comment 2

a year ago
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)
Assignee

Comment 3

a year ago
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)
Reporter

Comment 4

a year ago
re: WARNING: YOU ARE LEAKING THE WORLD

Yes.
Flags: needinfo?(bob)
Assignee

Comment 5

a year ago
Excellent, simple fix then.
Assignee

Comment 6

a year ago
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
Assignee

Comment 8

a year ago
(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

a year ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77288857a25b
Baldr: quiet shutdown assertions when hasLiveRuntimes (r=bbouvier)

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/77288857a25b
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Is this something we should uplift to beta?
Flags: needinfo?(luke)
Assignee

Comment 12

a year ago
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.