988881 - SignedJAR threads are not ended by the end of xpcom-shutdown-threads

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

With the patch from bug 988464, we see many SignedJAR threads in M1 still active after xpcom-shutdown-threads (which is after xpcom-shutdown).  One run with an earlier version of the patch:

https://tbpl.mozilla.org/?tree=Try&rev=8ac1c136a95f

See https://wiki.mozilla.org/XPCOM_Shutdown 

xpcom-shutdown-threads: "Any non-main-thread event queues should be finished by the module which owns them. All threads running XPCOM code should be joined. Any special services that might be required for component loader shutdown should be obtained and cached by the end of this notification. Initial garbage collection should be performed at this stage."

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: clean up CryptoTask (SignedJar) tasks instead of leaking them

https://tbpl.mozilla.org/?tree=Try&rev=f309f44c0a95

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8407845 [details] [diff] [review]
clean up CryptoTask (SignedJar) tasks instead of leaking them

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

This is leaking a thread for every SignedJar examined......

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: clean up CryptoTask (SignedJar) tasks instead of leaking them

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: clean up CryptoTask (SignedJar) tasks instead of leaking them

Switched from Dispatch-in-NewNamedThread(), because the returned value isn't valid until the call returns, and the event may run before then.  In the AudioInitTask, we could have passed it in and used the CurrentThread to set mThread in ::Run(), but there's no real advantage to that over this, and this is clear.  A more direct AsyncShutdown(currentThread) would make this simpler, of course.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8407989 [details] [diff] [review]
clean up CryptoTask (SignedJar) tasks instead of leaking them

https://tbpl.mozilla.org/?tree=Try&rev=36c43142e548

https://tbpl.mozilla.org/php/getParsedLog.php?id=37970671&tree=Mozilla-Inbound is an example of why we need to make sure mThread is set before Dispatch()/Run().

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Note the Try has the patch from bug 988464 in it.  See https://tbpl.mozilla.org/?tree=Try&rev=65248c8ac654 for a run without this patch (leaks of 13 SignedJar threads in M1)

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Attachment: clean up CryptoTask (SignedJar) tasks instead of leaking them

split AudioInitTask code out to bug 997286

Comment 8

[Benjamin Smedberg]

Comment on attachment 8408015 [details] [diff] [review]
clean up CryptoTask (SignedJar) tasks instead of leaking them

I think we should have PSM review here.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d947c69441bc

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d947c69441bc

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Uplift nominations, please

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

I'm not sure a memory leak that has existed for quite a while deserves beta uplift.  This is already on Aurora.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Not going to track this then, if it's not a concern to you - but it was nominated for tracking 3 weeks ago by you and I had the impression this was worth getting uplifted to help with any shutdown slowness since we're experiencing a higher volume of that in FF29.  If it's not low risk to land, we can let it ride but if not and there's any positive user impact, please consider an uplift.