851340 - Use tracing to keep JS scripts in nsXULPrototypeCache alive

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

Right now we are tracking the exact same set of scripts in nsXULPrototypeCache::mScriptsTable and JSRuntime::GCLocksHash. We do this solely so that the GC can mark the scripts itself. A better approach is to let the cache mark the scripts directly. This also lets us get rid of the cumbersome {Hold,Drop}ScriptObject that we currently do manually on insertion and removed. It is also the last user of {Hold,Drop}ScriptObject and will allow us to GC a bunch of code.

This patch moves the marking of the scripts in the nsXULPrototypeCache from directly in MarkRuntime to the trace callback called out of MarkRuntime. This /should/ have no impact whatsoever on the liveness of these scripts. A try run is up at:

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

I'll remove {Hold,Drop}ScriptObject in a followup patch.

Comment 1

[Andrew McCreight [:mccr8]]

The patch is incomplete.  It doesn't include the stuff you deleted that's in your try push.

Comment 2

[Terrence Cole [:terrence]]

Attachment: v1

(In reply to Andrew McCreight [:mccr8] from comment #1)
> The patch is incomplete.  It doesn't include the stuff you deleted that's in
> your try push.

Thanks for the heads-up! I appear to not have re-exported before uploading.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 725171 [details] [diff] [review]
v1


>+static PLDHashOperator
>+MarkScriptsInGC(nsIURI* aKey, CacheScriptEntry &aScriptEntry, void *aClosure)
Be consistent with * and & usage. (nsIURI* aKey, CacheScriptEntry& aScriptEntry, void* aClosure)


>+{
>+    JSTracer *trc = static_cast<JSTracer *>(aClosure);
JSTracer*

>+nsXULPrototypeCache::MarkInGC(JSTracer *aTrc)
ditto

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

\o/

Comment 5

[Andrew McCreight [:mccr8]]

Huh, weird, you are leaking nsXULPrototypeCaches, and nothing else.  1 for some Mochitests, 50 for one of them.  Maybe removing the unused gXULCache broke something. ;)

Comment 6

[Andrew McCreight [:mccr8]]

nsXULPrototypeCache does addref sInstance if sInstance is null.  Maybe the XUL cache gets cleared in shutdown, then the GC is called, and we end up creating a new cache that remains empty?  Doesn't explain why we have 50 leaked ones.  You could try and see if you get the leak with just running a few tests locally, and then maybe do ref count logging or something for the XUL cache.

Comment 7

[Andrew McCreight [:mccr8]]

If we GC after nsLayoutStatics::Shutdown() it looks like we'll leak one prototype cache.

Comment 8

[Terrence Cole [:terrence]]

Your theory is 100% correct. Thank you for all the help!

Green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=cb37c29d2504

Pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b80bf15146c

Comment 9

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/9b80bf15146c