883657 - Don't force a GC in mozJSComponentLoader::UnloadModules()

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

Bug 881266 revealed that mozJSComponentLoader::UnloadModules() runs JS_DestroyContext(), which forces a GC, right after we shutdown the CC.  This feels archaic.  Just changing this to JS_DestroyContextNoGC() causes tiny shutdown leaks, so probably we need to move UnloadModules() to before we shutdown the CC, then see if we can get away with not forcing a GC.  Note that we still may end up running a GC here unless bug 881309, but it is a start.

Also, once this stuff is cleaned up, it might be worth asserting if the GC ever runs after the CC shuts down.

Comment 1

[Andrew McCreight [:mccr8]]

Let's see how this works.

move unload before CC shutdown: https://tbpl.mozilla.org/?tree=Try&rev=36fda2b321b3
don't force a GC in mozJSComponentLoader::UnloadModules: https://tbpl.mozilla.org/?tree=Try&rev=104b83032368

Comment 2

[Andrew McCreight [:mccr8]]

Both are green in L64 debug.  I suppose I should see if this actually reduces the number of shutdown GCs in opt builds.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 1: call UnloadModules before we shutdown the CC

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 2: don't force a GC in UnloadModules

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 766753 [details] [diff] [review]
part 1: call UnloadModules before we shutdown the CC

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

I'm not sure who really knows this code, but it involves shutdown, so maybe you can review this, bsmedberg.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 766754 [details] [diff] [review]
part 2: don't force a GC in UnloadModules

With UnloadModules moved before the shutdown GC+CCs, we don't need to force a GC in shutdown to avoid tiny leaks.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 766754 [details] [diff] [review]
part 2: don't force a GC in UnloadModules

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

Nice. r=bholley

Comment 8

[Andrew McCreight [:mccr8]]

Thanks for the quick reviews!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b05165a7a277
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/47862004ed14

try run looked good https://tbpl.mozilla.org/?tree=Try&rev=c21852be5d44

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b05165a7a277
https://hg.mozilla.org/mozilla-central/rev/47862004ed14

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Congrats, mccr8! According to a lot of Try bisection and the below Try push (these patches on top of m-b tip), this is what also what made WinXP xpcshell stop timing out all the time.
https://tbpl.mozilla.org/?tree=Try&rev=748c38bed260

Can you please request approval to uplift this to Fx24 so we get this in for the next ESR? :)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 766753 [details] [diff] [review]
part 1: call UnloadModules before we shutdown the CC

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: sheriff pain on esr24
Testing completed (on m-c, etc.): missed fx24 by a day, no known regressions
Risk to taking this patch (and alternatives if risky): minimal per mccr8
String or IDL/UUID changes made by this patch: none

Comment 12

[bhavana bajaj [:bajaj]]

Comment on attachment 766753 [details] [diff] [review]
part 1: call UnloadModules before we shutdown the CC

low risk well baked, makes sheriff's life easier :) Lets get it on !

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a4ae95d37938
https://hg.mozilla.org/releases/mozilla-beta/rev/6325125d05b3

Comment 14

[u279076]

Assuming no QA needed here. Please remove [qa-] from the whiteboard and add the verifyme keyword if this needs QA.