Closed Bug 988881 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 - affected
firefox31 --- fixed
firefox-esr24 --- wontfix

People

(Reporter: jesup, Assigned: jesup)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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."
Component: Security → Security: PSM
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......
Attachment #8407845 - Flags: review?(benjamin)
Attachment #8407845 - Attachment is obsolete: true
Attachment #8407845 - Flags: review?(benjamin)
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.
Attachment #8407957 - Attachment is obsolete: true
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().
Attachment #8407989 - Flags: review?(benjamin)
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)
Attachment #8407989 - Attachment is obsolete: true
Attachment #8407989 - Flags: review?(benjamin)
Attachment #8408015 - Flags: review?(benjamin)
Comment on attachment 8408015 [details] [diff] [review]
clean up CryptoTask (SignedJar) tasks instead of leaking them

I think we should have PSM review here.
Attachment #8408015 - Flags: review?(honzab.moz)
Attachment #8408015 - Flags: review?(benjamin)
Attachment #8408015 - Flags: feedback+
Attachment #8408015 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/mozilla-central/rev/d947c69441bc
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Uplift nominations, please
Flags: needinfo?(rjesup)
I'm not sure a memory leak that has existed for quite a while deserves beta uplift.  This is already on Aurora.
Flags: needinfo?(rjesup)
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.
Blocks: 988464
No longer depends on: 988464
You need to log in before you can comment on or make changes to this bug.