900711 - ~nsIThreadPool can run the event loop if shutdown has not been called

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Karl Tomlinson (:karlt)]

nsIThreadPool is scriptable.
Better to leak the thread than run an event loop during GC

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I think a better solution would be to just make the destructor delay-shutdown any remaining threads.

Comment 2

[Karl Tomlinson (:karlt)]

Hmm.  Perhaps that is possible.
During XPCOM shutdown, there is no thread to delay shutdown, but maybe that's OK because the thread manager will have already shut down the threads.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: changes.patch

Something like this perhaps.

Comment 4

[(no longer active)]

Comment on attachment 785112 [details] [diff] [review]
changes.patch

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

::: xpcom/threads/nsThreadPool.cpp
@@ +41,5 @@
> +{
> +public:
> +  NS_DECL_NSIRUNNABLE
> +
> +  ShutdownHelper(nsCOMArray<nsIThread>& aThreads,

Nit: const.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> Nit: const.

We spoke about this on irc, const can't work here due to the SwapElements calls.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7236065ca5bf

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Also https://hg.mozilla.org/integration/mozilla-inbound/rev/08b4c4071214 to remove the silly ampersand.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7236065ca5bf
https://hg.mozilla.org/mozilla-central/rev/08b4c4071214

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 785112 [details] [diff] [review]
changes.patch

Requesting approval for 914030 comment 4.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): old bug
User impact if declined: will need to make similar existing logic in webaudio more complex.
Testing completed (on m-c, etc.): on m-c for almost 3 weeks

Risk to taking this patch (and alternatives if risky): 
What gives me confidence about the low risk of taking this patch is that any scenarios affected by this patch would most likely have been a security risk and so should not exist.  If any do still exist, they are better off with this patch.

String or IDL/UUID changes made by this patch: none

Comment 9

[Alex Keybl [:akeybl]]

Is bug 914030 being targeted for FF25? Trying to understand the rationale here.

Comment 10

[Karl Tomlinson (:karlt)]

(In reply to Alex Keybl [:akeybl] from comment #9)
> Is bug 914030 being targeted for FF25? Trying to understand the rationale
> here.

Yes, webaudio is targeted for FF25, and bug 914030 is part of that.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 785112 [details] [diff] [review]
changes.patch

[Triage Comment]
FF25 is now on Beta, we are targeting Web Audio for 25 so let's get this in early in order to get testing & verification time.

Comment 12

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2335c2fde2a8
https://hg.mozilla.org/releases/mozilla-beta/rev/adb9fbeec38d

Comment 13

[u279076]

Assuming no verification needed here. Please add the verifyme keyword and remove the [qa-] whiteboard tag to request verification.

Comment 14

[Karl Tomlinson (:karlt)]

Sorry, this was mostly a non-bug.
Threads keep a reference to the nsThreadPool until they return from Run() after removing themselves from mThreads, so mThreads is always empty in the destructor, and Shutdown() was a no-op.

I think someone put the Shutdown() call in the destructor to confuse us ;)
I'll tidy this up in bug 926522.