Closed Bug 1517071 Opened 1 year ago Closed 1 year ago

Leak with WPT FileAPI/FileReader/workers.html

Categories

(Core :: XPCOM, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- wontfix
firefox64 --- wontfix
firefox65 --- fixed
firefox66 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak)

Attachments

(1 file)

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
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
Priority: -- → P3
Blocks: 1517309

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: nobody → continuation
Component: DOM: Workers → XPCOM

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?

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.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b7589bf68d0
Clear AsyncWaitRunnable::mStream when cancelled. r=froydnj
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e4cbf560928e
Clear AsyncWaitRunnable::mStream when cancelled. r=froydnj
Flags: needinfo?(continuation)

I don't understand runnable stuff enough to be confident that we should backport this so close to the end of beta.

Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

(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.

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

Attachment #9035755 - Flags: approval-mozilla-beta?

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.

Attachment #9035755 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.