[Tracking Requested - why for this release]: unbounded thread growth in the parent process is a bad thing.

My parent process currently has ~200 DOMCacheThreads running around inside it. I don't think this is expected behavior.

It looks like bug 1592488, specifically https://phabricator.services.mozilla.com/D74637 moved ownership of DOMCacheThread(s) to XPCOMThreadWrapper and deleted the code that actually shutdown the thread, assuming that XPCOMThreadWrapper would shut down the thread. That assumption does not appear to be correct.

jya, can you take a look?

ni? jya because filing the bug wouldn't let me.

https://phabricator.services.mozilla.com/D74637 didn't really remove the thread destruction.

the previous code used to dispatch a task to shutdown the thread on the main thread. While it could have just been left to be deleted instead in the destructor.

Calling AsyncShutdown doesn't force the thread to be delete AFAIK; nor is explicitly requiring to shutdown the thread for delete to occur. If that was the case we would have this issue everywhere as there are many nsThread in the tree that are never explicitly shutdown.

Nevertheless, this will be resolved with bug 1637518 where we can go back to using nsThread instead .

Nathan, can you confirm the behaviour or API I described above?

(In reply to Jean-Yves Avenard [:jya] from comment #2)

https://phabricator.services.mozilla.com/D74637 didn't really remove the thread destruction.

the previous code used to dispatch a task to shutdown the thread on the main thread. While it could have just been left to be deleted instead in the destructor.

Calling AsyncShutdown doesn't force the thread to be delete AFAIK; nor is explicitly requiring to shutdown the thread for delete to occur. If that was the case we would have this issue everywhere as there are many nsThread in the tree that are never explicitly shutdown.

I'm not exactly sure what you mean by "deleted" in this context. nsThread requires you to call Shutdown or AsyncShutdown on it for the thread to actually go away; just dropping the reference you own is not enough because a separate reference is held by the thread manager. If the previous code hadn't had the code to call AsyncShutdown in the Manager destructor, it would have been buggy as well.

Nevertheless, this will be resolved with bug 1637518 where we can go back to using nsThread instead .

Nathan, can you confirm the behaviour or API I described above?

Just going back to nsThread in the cache manager will not be enough; somebody has to explicitly shut the thread down. It's possible that just re-adding the AsyncShutdown call in Manager would be good enough if all the AbstractThread removal machinery is too complicated to uplift...but I don't know how that plays with the thread wrapper the Manager also holds...

Will be fixed once bug 1644009 merge to central

Do we need to fix this for 78 somehow though?

(In reply to Jean-Yves Avenard [:jya] from comment #4)

Will be fixed once bug 1644009 merge to central

*** This bug has been marked as a duplicate of bug 1644009 ***

We could uplift bug 1644009; but it's pretty big (though most of it is a reversal of the changes made in 78).

I could craft a fix for 78 that doesn't involve bug 1644009. But 1644009 will fix a lot of problem and shutdown crashes.

78 is ESR, isn't it? It seems to me like we have to do something.

It is. If we can do something less invasive than uplift bug 1644009 that would be nice :)

sorry, patch was pushed here : https://phabricator.services.mozilla.com/D80457

beta approval requested there.

This is required to break the cycle between the nsThread and the nsThreadManager and prevent leaks.

Comment on attachment 9158198 [details]
Bug 1643690 - Ensure thread gets shutdown when DOMCache manager gets destroyed. r=baku

Beta/Release Uplift Approval Request

• User impact if declined: lots of leaking unused threads.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: run the code for a while, there will be leakd of DOMCacheThread that keep growing
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): we revert to the previous code; just creating a single associated AbstractThread to allow for direct task dispatching (used with MozPromise IPDL)
• String changes made/needed: none

Comment on attachment 9158198 [details]
Bug 1643690 - Ensure thread gets shutdown when DOMCache manager gets destroyed. r=baku

Thanks, let's give that a try for 78 rc1.

https://hg.mozilla.org/releases/mozilla-release/rev/f6da69ba985b

I'm not sure on how to test this, can you please give some additional steps to better understand what QA can do here in order to verify that this issue is fixed?

Redirecting to Nathan.

Oooo, sorry about the delay on this one. My testcase has just be "do some browsing around the web and then open htop (on Linux) and examine how many threads there are". I suspect you could do roughly the same thing on other operating systems too.

(In reply to Nathan Froyd [:froydnj] from comment #16)

Oooo, sorry about the delay on this one. My testcase has just be "do some browsing around the web and then open htop (on Linux) and examine how many threads there are". I suspect you could do roughly the same thing on other operating systems too.

about:processes shows the threads, and is cross platform.