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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nathan Froyd [:froydnj]]

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

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: release globals properly when GetFilesHelper is destroyed on the main thread

This should be all we need.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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

oh, this is bad.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d2a9160dcaf
release globals properly when GetFilesHelper is destroyed on the main thread; r=smaug

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8d2a9160dcaf

Comment 5

[Marcia Knous [:marcia]]

Tracking 52/53/54+ to address this memory leak.

Comment 6

[Nathan Froyd [:froydnj]]

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.

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d06f2bf30b22

Comment 9

[Julien Cristau [:jcristau]]

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.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cc219ce6b182