Closed Bug 851340 Opened 9 years ago Closed 9 years ago

Use tracing to keep JS scripts in nsXULPrototypeCache alive

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch v0 (obsolete) — Splinter Review
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.
Attachment #725153 - Flags: review?(bugs)
The patch is incomplete.  It doesn't include the stuff you deleted that's in your try push.
Attached patch v1Splinter Review
(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.
Attachment #725153 - Attachment is obsolete: true
Attachment #725153 - Flags: review?(bugs)
Attachment #725171 - Flags: review?(bugs)
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
Attachment #725171 - Flags: review?(bugs) → review+
Blocks: 851568
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. ;)
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.
If we GC after nsLayoutStatics::Shutdown() it looks like we'll leak one prototype cache.
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
https://hg.mozilla.org/mozilla-central/rev/9b80bf15146c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.