Closed
Bug 1516967
Opened 6 years ago
Closed 6 years ago
Frequent Win7 Opt dt8 shutdown hangs with same-compartment realms
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
With the patches for bug 1514210 I get frequent dt8 shutdown hangs on Win7 Opt. I think what's happening is that in ParentImpl::ShutdownBackgroundThread we check sLiveActorCount and if that's > 0 we spin the event loop and schedule a timer to kill remaining actors [0]. However in this case we have a same-process actor and those are not included in sLiveActorsForBackgroundThread (that array is empty) so we never kill it. It seems to me that sLiveActorCount should be sLiveBackgroundActorCount or something, and we should only increment it for actors that actually use the background thread. Jan, WDYT? [0] https://searchfox.org/mozilla-central/rev/fc229ed2c78648e402a9bbd50d99b69d0e227844/ipc/glue/BackgroundImpl.cpp#952-961
Flags: needinfo?(jvarga)
Assignee | ||
Comment 1•6 years ago
|
||
Or maybe we need to call actor->SetLiveActorArray(..) for actors created by CreateActorForSameProcess? For the other-process case we do that in ConnectActorRunnable...
Comment 2•6 years ago
|
||
Do you know why the same-process actor isn't being closed after your changes, when it seems like it was being closed before them? Also, do you happen to know which thread the actor belongs to?
Assignee | ||
Comment 3•6 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #2) > Do you know why the same-process actor isn't being closed after your > changes, when it seems like it was being closed before them? It might be GC/CC timing related - it only repros intermittently on Win32 Opt and when I try to narrow it down by disabling some tests in the directory where it triggers the problem usually disappears. Pretty frustrating. > Also, do you happen to know which thread the actor belongs to? I'll try to add some more instrumentation to answer this...
Comment 4•6 years ago
|
||
Last time I checked this, it was not so easy to add the same process actors to the array. I have to take a closer look again.
Assignee | ||
Comment 5•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #3) > > Also, do you happen to know which thread the actor belongs to? > > I'll try to add some more instrumentation to answer this... The actor is created on a "DOM Worker" thread and on shutdown I do see a DOM Worker thread that's still waiting for events. What's supposed to terminate these threads?
Assignee | ||
Comment 6•6 years ago
|
||
Oh, I think we will terminate these threads with the nsThreadManager::get().Shutdown() call at [0], but that comes after the observerService->NotifyObservers call where we end up hanging.. [0] https://searchfox.org/mozilla-central/rev/fc229ed2c78648e402a9bbd50d99b69d0e227844/xpcom/build/XPCOMInit.cpp#840
Assignee | ||
Comment 7•6 years ago
|
||
OK I think I figured it out. We're OOM'ing in the JS engine when setting up a worker runtime because of JIT code limits; here we can be a bit more aggressive with CC/GC before we fail. Then we fail to properly terminate the worker thread, I'll file a separate bug for that.
Flags: needinfo?(jvarga)
Assignee | ||
Comment 9•6 years ago
|
||
Comment 10•6 years ago
|
||
We should shut down Worker threads long before we shut down XPCOM threads. Workers that belong to windows should shut down when the window closes. System scope workers... I'm not sure off the top of my head. Either way, if fixing the OOM fixes the problem, that seems fine.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Comment 11•6 years ago
|
||
It looks like this turned out to not be an IPC bug. (But if there's a way PBackground could be have been more helpful, feature-request bugs are welcome.)
Component: IPC → JavaScript Engine
Comment 12•6 years ago
|
||
Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/85054765af34 Call the large allocation callback before reporting OOM in JSRuntime::createJitRuntime. r=lhansen
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/85054765af34
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in
before you can comment on or make changes to this bug.
Description
•