Leak with WPT FileAPI/FileReader/workers.html

RESOLVED FIXED in Firefox 65

Status

()

defect
P3
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks 1 bug, {memory-leak})

unspecified
mozilla66
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox64 wontfix, firefox65 fixed, firefox66 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

5 months ago
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

5 months 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
Priority: -- → P3
Assignee

Updated

5 months ago
Blocks: 1517309
Assignee

Comment 2

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

Comment 3

4 months 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

4 months 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.

Comment 5

4 months ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b7589bf68d0
Clear AsyncWaitRunnable::mStream when cancelled. r=froydnj

Comment 7

4 months ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e4cbf560928e
Clear AsyncWaitRunnable::mStream when cancelled. r=froydnj
Assignee

Updated

4 months ago
Flags: needinfo?(continuation)
Assignee

Comment 8

4 months 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

4 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 4 months 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.

Assignee

Comment 11

4 months 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

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.