1517071 - Leak with WPT FileAPI/FileReader/workers.html

Status: RESOLVED FIXED

Description

[Andrew McCreight [:mccr8]]

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

Comment 1

[Andrew McCreight [:mccr8]]

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

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

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?

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Bug 1517071 - Clear AsyncWaitRunnable::mStream when cancelled.

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b7589bf68d0
Clear AsyncWaitRunnable::mStream when cancelled. r=froydnj

Comment 6

[Andreea Pavel [:apavel]]

Backed out for build bustages

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=3b7589bf68d0033774d061fbd7b03c50984046d9

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4cbf560928e
Clear AsyncWaitRunnable::mStream when cancelled. r=froydnj

Comment 8

[Andrew McCreight [:mccr8]]

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

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/e4cbf560928e

Comment 10

[Nathan Froyd [:froydnj]]

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

[Andrew McCreight [:mccr8]]

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

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5b52f9f83004