1486596 - GeckoSession can leak a Gecko window

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P1)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

If you do `new GeckoSession().open(GeckoRuntime.getDefault(...))`, this will cause us to leak a Gecko window. I think consumers will definitely expect that to be cleaned up by the finalizer.

In conversations with :jchen, this is because the Parcelable implementation of GeckoSession allows the GeckoSession.Window to have shared ownership, and we don't want to close the window underneath someone else. Further, native code always holds a reference to the window, so it will never become garbage collected even if no other managed object holds a reference.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

One idea to solve this would be to add a method that would return a wrapped `GeckoSession.Window` and close the session. The gist of it is:

class SessionData implements Parcelable {
    private GeckoSession.Window window;

    public close() { ... }
}


class GeckoSession {
    ...
    SessionData forget() {
        SessionData data = new SessionData(mWindow);
        mWindow = null;

        return data;
    }
}

We'd then add a `GeckoSession` constructor that accepts a `SessionData` and basically does the same as `GeckoSession.transferFrom()` today. Naming is hard, so any suggestions there are welcome.

This would ensure GeckoSession.Window is owned in only one place at a time, so a finalizer (either in GeckoSession or SessionData) can always close it.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #1)
> One idea to solve this would be to add a method that would return a wrapped
> `GeckoSession.Window` and close the session.

Whoops -- it does not close the session, simply transfers it

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Another way to solve this would be to have a map of String -> WeakReference<GeckoSession>, where the strings are the result of GeckoSession.getId(). The parcelable impl in GeckoSession would then simply write the id into the bundle instead of the GeckoSession.Window instance as it does today. When (if) it's unparceled on the other side it can use the id to find the originating GeckoSession and use transferFrom() as we do today to move the GeckoSession.Window over. With this system the GeckoSession.Window is always owned by a single GeckoSession, so the owning GeckoSession can always close() in the finalizer.

I like this much better than the solution in comment #1, but concede that having the map is bit of a kludge.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Hmm, except we're racing the GC now if the session goes out of scope. No good.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1486596 - Transfer ownership of GeckoSession.Window to Parcel r=jchen

This change eliminates shared ownership of the window. It is always
owned by a GeckoSession or the newly-introduced WindowHolder.
This allows GeckoSession to close the window it holds in the
the finalizer, if any, preventing leaks.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1486596 - Make GeckoSession.open() a noop if we're already open r=jchen

This also changes GeckoSession.transferFrom() to close any
current window if one exists instead of throwing. This is
possible now since the GeckoSession has exclusive ownership
of the window (if it has one at all).

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

The attached patches are kind of a hybrid between comment #1 and #3. When GeckoSession is parceled, ownership of the GeckoSession.Window is transferred to a wrapper, WindowHolder, which is itself put into the parcel. When (if) inflated on the other side, the new GeckoSession takes ownership from WindowHolder. Both GeckoSession and WindowHolder implement finalize() to ensure the GeckoSession.Window cannot leak.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1486596 - Add WeakRef support to mozilla::jni r=jchen

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1486596 - Make nsWindow hold a weak reference to GeckoSession.Window r=jchen

This is necessary so that GeckoSession.Window can be garbage
collected, at which point the finalizer will close the
underlying nsWindow.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1486596 - Implement finalizer for GeckoSession.Window r=jchen

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1486596 - Ensure GeckoSession.Window is owned by a single GeckoSession r=jchen

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9005063 [details]
Bug 1486596 - Implement finalizer for GeckoSession.Window r=jchen

Jim Chen [:jchen] [:darchons] has approved the revision.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9005064 [details]
Bug 1486596 - Ensure GeckoSession.Window is owned by a single GeckoSession r=jchen

Jim Chen [:jchen] [:darchons] has approved the revision.

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9004980 [details]
Bug 1486596 - Add WeakRef support to mozilla::jni r=jchen

Jim Chen [:jchen] [:darchons] has approved the revision.

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9004981 [details]
Bug 1486596 - Make nsWindow hold a weak reference to GeckoSession.Window r=jchen

Jim Chen [:jchen] [:darchons] has approved the revision.

Comment 16

[Chris Peterson [:cpeterson]]

Jeff, does Focus explicitly close its GeckoSession? If it does, then this window leak won't affect Focus.

Does Focus use a separate GeckoSession for each tab?

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1486596 - Add some debugging helpers to Refs.h r=jchen

This will print a message each time a native JNI ref is
added or removed from an object.

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1486596 - Hold a WeakRef to PanZoomController from nsWindow r=jchen

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1486596 - Hold a WeakRef to LayerSession::Compositor from nsWindow r=jchen

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1486596 - Hold a WeakRef to EventDispatcher from native code r=jchen

Comment 21

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1486596 - Hold a WeakReference to GeckoSession from GeckoEditable r=jchen

This is necessary because the GC root for GeckoEditable is a native
reference, preventing the GC from recognizing that the only reference
to the GeckoEditable is the owning GeckoSession and doing normal
cycle collector stuff. We break the cycle ourselves by using a
WeakReference to hold the GeckoSession within GeckoEditable.

Comment 22

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1486596 - Add GeckoSessionTestRule.waitForCollection() r=jchen

This iterates the main loop until the specified WeakReference
has been enqueued.

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1486596 - Add tests to ensure GeckoSession can be collected by the GC r=jchen

Comment 24

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9006374 [details]
Bug 1486596 - Hold a WeakReference to GeckoSession from GeckoEditable r=jchen

Jim Chen [:jchen] [:darchons] has approved the revision.

Comment 25

[Chris Peterson [:cpeterson]]

Emily says Focus closes GeckoSessions individually in Focus' cleanup method and then as a batch if the user closes all tabs. Focus releases the GeckoSession for each tab switching tabs and creates a new one for a new tab.

Comment 26

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9006374 [details]
Bug 1486596 - Hold a WeakReference to GeckoSession from GeckoEditable r=jchen

Jim Chen [:jchen] [:darchons] has requested changes to the revision.

Comment 27

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9006370 [details]
Bug 1486596 - Hold a WeakRef to PanZoomController from nsWindow r=jchen

Jim Chen [:jchen] [:darchons] has approved the revision.

Comment 28

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9006372 [details]
Bug 1486596 - Hold a WeakRef to LayerSession::Compositor from nsWindow r=jchen

Jim Chen [:jchen] [:darchons] has approved the revision.

Comment 29

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9006374 [details]
Bug 1486596 - Hold a WeakReference to GeckoSession from GeckoEditable r=jchen

Jim Chen [:jchen] [:darchons] has approved the revision.

Comment 30

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9006376 [details]
Bug 1486596 - Add tests to ensure GeckoSession can be collected by the GC r=jchen

Jim Chen [:jchen] [:darchons] has approved the revision.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fabcbe61060e
Add WeakRef support to mozilla::jni r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ddac13c48f
Hold a WeakRef to GeckoSession.Window from nsWindow r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a8062e9732e
Implement finalizer for GeckoSession.Window r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0cef26afa24
Ensure GeckoSession.Window is owned by a single GeckoSession r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/a82faa24a0a6
Hold a WeakRef to PanZoomController from nsWindow r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/355f1c2a3f83
Hold a WeakRef to LayerSession::Compositor from nsWindow r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac1fbefa4f4
Hold a WeakReference to GeckoSession from GeckoEditable r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e5bfc676ff7
Hold a WeakRef to EventDispatcher from native code r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b5dfcbd692
Add tests to ensure GeckoSession can be collected by the GC r=jchen

Comment 32

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/fabcbe61060e
https://hg.mozilla.org/mozilla-central/rev/e7ddac13c48f
https://hg.mozilla.org/mozilla-central/rev/4a8062e9732e
https://hg.mozilla.org/mozilla-central/rev/c0cef26afa24
https://hg.mozilla.org/mozilla-central/rev/a82faa24a0a6
https://hg.mozilla.org/mozilla-central/rev/355f1c2a3f83
https://hg.mozilla.org/mozilla-central/rev/0ac1fbefa4f4
https://hg.mozilla.org/mozilla-central/rev/7e5bfc676ff7
https://hg.mozilla.org/mozilla-central/rev/c2b5dfcbd692