Leak with WPT FileAPI/FileReader/workers.html
Categories
(Core :: XPCOM, defect, P3)
Tracking
()
People
(Reporter: mccr8, Assigned: mccr8)
References
(Blocks 1 open bug)
Details
(Keywords: memory-leak)
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
If you run: ./mach wpt FileAPI/FileReader/workers.html Then there are leaks: 0:19.88 INFO leakcheck: tab leaked 2 AsyncWaitRunnable 0:19.88 INFO leakcheck: tab leaked 1 Blob 0:19.88 INFO leakcheck: tab leaked 2 BlobImpl 0:19.88 INFO leakcheck: tab leaked 2 CondVar 0:19.88 INFO leakcheck: tab leaked 2 DOMEventTargetHelper 0:19.88 INFO leakcheck: tab leaked 1 FileReader 0:19.88 INFO leakcheck: tab leaked 1 MultipartBlobImpl 0:19.88 INFO leakcheck: tab leaked 5 Mutex 0:19.88 INFO leakcheck: tab leaked 2 NonBlockingAsyncInputStream 0:19.88 INFO leakcheck: tab leaked 1 ProtoAndIfaceCache 0:19.88 INFO leakcheck: tab leaked 1 StringBlobImpl 0:19.88 INFO leakcheck: tab leaked 1 ThreadEventTarget 0:19.88 INFO leakcheck: tab leaked 1 ThreadTargetSink 0:19.88 INFO leakcheck: tab leaked 1 WorkerEventTarget 0:19.88 INFO leakcheck: tab leaked 1 WorkerGlobalScope 0:19.88 INFO leakcheck: tab leaked 1 WorkerThread 0:19.88 INFO leakcheck: tab leaked 1 nsStringBuffer 0:19.88 INFO leakcheck: tab leaked 2 nsStringInputStream 0:19.88 INFO leakcheck: tab leaked 9 nsTArray_base 0:19.88 INFO leakcheck: tab leaked 1 nsThread
Assignee | ||
Comment 1•2 years ago
|
||
Maybe these warnings from the log are related: WARNING: A runnable was posted to a worker that is already shutting down!: file /media/ssd/mc/dom/workers/WorkerPrivate.cpp, line 1356 WARNING: Failed to dispatch offline status change event!: file /media/ssd/mc/dom/workers/WorkerPrivate.cpp, line 1808
Updated•2 years ago
|
Assignee | ||
Comment 2•2 years ago
|
||
I started out analyzing this leak by looking at FileReader. In the CC log, the FileReader had two unknown references. From there, I used DMD heap scan mode to figure out what was holding alive the FileReader. I had to use an unoptimized build to get good stacks.
The FileReader is being held alive by two NonBlockingAsyncInputStream::AsyncWaitRunnable. Each runnable is part of a cycle of strong references with a NonBlockingAsyncInputStream. The cycle is broken in the RunAsyncWaitCallback() method of the stream, but if the runnable is cancelled the cycle remains and we get a leak. I wrote a patch that overrides the Cancel() method of the runnable to clear out the reference to the stream and that fixes the leak.
The bug is in xpcom/io/NonBlockingAsyncInputStream.cpp so I'm going to move this to XPCOM.
Assignee | ||
Comment 3•2 years ago
|
||
I don't know anything about streams so maybe this is not the right fix. Should Cancel() on the runnable call Close() on the stream or something?
Assignee | ||
Comment 4•2 years ago
|
||
AsyncWaitRunnable holds a strong reference to its stream, and NonBlockingAsyncInputStream holds a strong reference to the runnable. The cycle gets broken in the RunAsyncWaitCallback() method of the stream, but if the runnable is cancelled then we leak them both. This patch fixes that by clearing the pointer to the stream when the runnable is cancelled, breaking the cycle.
Updated•2 years ago
|
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3b7589bf68d0 Clear AsyncWaitRunnable::mStream when cancelled. r=froydnj
Comment 6•2 years ago
|
||
Backed out for build bustages
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=221336490&repo=autoland&lineNumber=11840
Backout: https://hg.mozilla.org/integration/autoland/rev/5c5b31c72808fc1ef6a0d7b1ac8c4481222d56ba
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e4cbf560928e Clear AsyncWaitRunnable::mStream when cancelled. r=froydnj
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 8•2 years ago
|
||
I don't understand runnable stuff enough to be confident that we should backport this so close to the end of beta.
Comment 9•2 years ago
|
||
bugherder |
![]() |
||
Comment 10•2 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #8)
I don't understand runnable stuff enough to be confident that we should backport this so close to the end of beta.
I think this would be a reasonable patch to uplift, and it would help with possible memory leaks in the wild.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 11•2 years ago
|
||
Comment on attachment 9035755 [details]
Bug 1517071 - Clear AsyncWaitRunnable::mStream when cancelled.
[Beta/Release Uplift Approval Request]
Feature/Bug causing the regression: bug 1371699
User impact if declined: Bad leaks. This isn't a new regression, and there's no evidence that this is actually happening for users.
Is this code covered by automated tests?: Yes
Has the fix been verified in Nightly?: Yes
Needs manual test from QE?: No
If yes, steps to reproduce:
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): It just clears out a pointer after we're done with something.
String changes made/needed: none
Comment 12•2 years ago
|
||
Comment on attachment 9035755 [details]
Bug 1517071 - Clear AsyncWaitRunnable::mStream when cancelled.
[Triage Comment]
Low-risk fix for leaks possibly happening in the wild. Approved for 65.0b11.
Comment 13•2 years ago
|
||
bugherderuplift |
Description
•