Closed Bug 1390863 Opened 7 years ago Closed 7 years ago

JSBC eager mode causes a memory leak in toolkit/components/url-classifier/tests/mochitest/test_cachemiss.html

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(2 files)

I can reproduce this issue with the following command. $ mach mochitest \ --setpref dom.script_loader.bytecode_cache.strategy=-1 \ --setpref dom.script_loader.bytecode_cache.enabled=true \ toolkit/components/url-classifier/tests/mochitest/test_cachemiss.html Changing enable to false remove the leak. I have no idea how to figure out this issue. Valgrind fails to connect to marionette, which I presume is caused by the overhead added by valgrind. So I recorded the issue under rr, and I am now trying to follow the ref-count changes of one of the leaked ScriptLoader instances, as this likely to be related to the JSBC state.
While investigating the leak of the ScriptLoader instance, I noticed: - 6 out of 14 ScriptLoader have calls to ScriptLoader::EvaluateScript. - One of the leaked ScriptLoader has no calls to ScriptLoader::EvaluateScript. I am currently logging all the ref-counter read/write of the leaked ScriptLoader which has no calls to EvaluateScript as I suspect that this might be the easiest one to investigate.
(In reply to Nicolas B. Pierron [:nbp] from comment #1) > I am currently logging all the ref-counter read/write of the leaked > ScriptLoader which has no calls to EvaluateScript as I suspect that this > might be the easiest one to investigate. Ok, I tracked the mutation of the refcount, and this script was only kept alive by the document, which owns it. So I went tracking down the document which owns it, and tracking its refcount as well. After a day of matching what up-ref goes with which down-ref (even across swap & forget calls) I have 3 signatures, which correspond to the ref-count of 3 remaining at the end of the program. (see attachment) My guesses would be that either: - 1 edge is not traced properly, keeping the 2 other edges alive as they might be involved in a cycle. - There is a forget, which is stored in some place which does not handle the ref-counting / tracing properly. - All edges are involved in cycles, and I am going to waste 3 more days traking this leak (?).
Looking at the log, there is apparently leaked 2 ScriptLoadRequest instances, which are created but never destroyed. These 2 instances are registered for encoding, but it does not seems to happen. I presume that the page is somehow discarded, and none of the events are triggered. What surprises me, is that they are not supposed to be able to keep a 4 documents alive, and they are supposed to participate in the cycle collector too.
This patch adds a ScriptLoader::Destroy function which is being called from the nsDocument::Destroy function. This ScriptLoader::Destory function calls the GiveUp function, to discard all JSScript and nsHttpChannelChild references of the nsScriptLoadRequest. Doing so prevents the nsScriptLoadRequest to keep pointers while waiting for a the load-end event to be fired, which never happens after nsDocument::Destroy is called.
Attachment #8899397 - Flags: review?(mrbkap)
Comment on attachment 8899397 [details] [diff] [review] Do not hold ScriptLoadRequest, when the load-end event is not fired. Review of attachment 8899397 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. ::: dom/script/ScriptLoader.cpp @@ +2167,5 @@ > void > ScriptLoader::MaybeTriggerBytecodeEncoding() > { > + // If we already gave up, ensure that we are not going to enqueue any script, > + // and that we finalize them proprely. Nit: properly ::: dom/script/ScriptLoader.h @@ +330,5 @@ > + * Destroy and prevent the ScriptLoader or the ScriptLoadRequests from owning > + * any references to the JSScript or to the Request which might be used for > + * caching the encoded bytecode. > + */ > + void Destroy() { Nit: Other inline functions in this file are written with the opening brace on its own line.
Attachment #8899397 - Flags: review?(mrbkap) → review+
Pushed by npierron@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/036b37b29cfa Do not hold ScriptLoadRequest, when the load-end event is not fired. r=mrbkap
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: