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)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: nbp, Assigned: nbp)
References
Details
Attachments
(2 files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
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.
Assignee | ||
Comment 2•7 years ago
|
||
(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 (?).
Assignee | ||
Comment 3•7 years ago
|
||
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.
Assignee | ||
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
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
Comment 7•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•