ReleaseRunnable::MaybeReleaseOnMainThread leaks nsIGlobalObject references if run on the main thread

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

Trunk
mozilla54
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52+ fixed, firefox53+ fixed, firefox54+ fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
[Tracking Requested - why for this release]: Significant potential memory leak.

Looking at https://dxr.mozilla.org/mozilla-central/source/dom/filesystem/GetFilesHelper.cpp#23:

  static void
  MaybeReleaseOnMainThread(nsTArray<RefPtr<Promise>>& aPromises,
                           nsTArray<RefPtr<GetFilesCallback>>& aCallbacks,
                           Sequence<RefPtr<File>>& aFiles,
                           already_AddRefed<nsIGlobalObject> aGlobal)
  {
    if (NS_IsMainThread()) {
      return;
    }

    RefPtr<ReleaseRunnable> runnable =
      new ReleaseRunnable(aPromises, aCallbacks, aFiles, Move(aGlobal));
    NS_DispatchToMainThread(runnable);
  }

The early return for NS_IsMainThread will destroy the already_AddRefed<nsIGlobalObject>, but that won't actually release the reference to the nsIGlobalObject within.  Assuming that we have a non-null nsIGlobalObject there, this code should be asserting in tests if it's ever run on the main thread.  But either we don't have tests for it, or the tests never release these things on the main thread.

Regardless, if we were to release these things on the main thread in the wild, that would be a significant memory leak.
(Assignee)

Comment 1

9 months ago
Created attachment 8832103 [details] [diff] [review]
release globals properly when GetFilesHelper is destroyed on the main thread

This should be all we need.
Attachment #8832103 - Flags: review?(bugs)
Comment on attachment 8832103 [details] [diff] [review]
release globals properly when GetFilesHelper is destroyed on the main thread

oh, this is bad.
Attachment #8832103 - Flags: review?(bugs) → review+
(Assignee)

Updated

9 months ago
Assignee: nobody → nfroyd

Comment 3

9 months ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d2a9160dcaf
release globals properly when GetFilesHelper is destroyed on the main thread; r=smaug

Comment 4

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8d2a9160dcaf
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Tracking 52/53/54+ to address this memory leak.
tracking-firefox52: ? → +
tracking-firefox53: ? → +
tracking-firefox54: ? → +
(Assignee)

Comment 6

9 months ago
Comment on attachment 8832103 [details] [diff] [review]
release globals properly when GetFilesHelper is destroyed on the main thread

Approval Request Comment
[Feature/Bug causing the regression]: bug 1287747
[User impact if declined]: bad memory leaks
[Is this code covered by automated tests?]: No.  We certainly exercise this code during tests, but we don't catch any leaks, or if we do, they only happen intermittently and get rolled in with regular oranges.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: No idea how to manually reproduce this.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]:
[String changes made/needed]: None.
Attachment #8832103 - Flags: approval-mozilla-beta?
Attachment #8832103 - Flags: approval-mozilla-aurora?

Comment 7

9 months ago
Comment on attachment 8832103 [details] [diff] [review]
release globals properly when GetFilesHelper is destroyed on the main thread

Potentially fixes a mem leak, let's bake it on Aurora53 before deciding whether to uplift to Beta52 or not.
Attachment #8832103 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 8

9 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/d06f2bf30b22
status-firefox53: affected → fixed
Comment on attachment 8832103 [details] [diff] [review]
release globals properly when GetFilesHelper is destroyed on the main thread

fix memory leak in beta52

Doesn't look like this exploded in aurora, time to go one more step.
Attachment #8832103 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 10

9 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/cc219ce6b182
status-firefox52: affected → fixed
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.