1484001 - GeckoRuntime is not restored from savedInstanceState after an OOM kill

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P3)

Description

[Jan Henning [:JanH]]

+++ This bug was initially created as a clone of Bug #1483893 +++

This bug is for finding a proper solution as to why we end up with a null mRuntime in GeckoView.onAttachedToWindow() after Firefox is brought back into the foreground after having previously been OOM-killed.

Comment 1

[Jan Henning [:JanH]]

So it seems that the Runtime survives a save/restore cycle through the savedInstanceState if e.g. the containing activity is merely being destroyed/recreated for a configuration change (use the GeckoView example app to see this in action), however if the app has been OOM-killed while in background [1], the runtime is somehow lost and not restored when recreating the activity from the savedInstanceState.


[1] Try either "adb shell am kill <package name>", or if that doesn't work, go into your phone's developer settings, "Running services", switch to "Cached processes" and kill Firefox/the GeckoView example app from there.

Comment 2

[Jan Henning [:JanH]]

So, what would the correct solution here be?

1) Don't restore the session for the savedInstanceState if it is closed. This means that after a background OOM kill, the owning activity is expected to always call setSession during onCreate even if a savedInstanceState is available. If it doesn't, it ends up using the fallback session with a default runtime that gets created during onAttachedToWindow if mSession is still null at that point.

2) Always restore the session we retrieve from the savedInstanceState, but don't overwrite mRuntime if the restored session is closed. If the owning activity already called setSession beforehand, we can use that runtime to open the restored session. If it didn't, we would currently crash when attempting to set the restored session without a corresponding runtime at [1].

3) Change the architecture such that the runtime can be restored for the savedInstanceState even if the whole application had been destroyed in the meantime. As I'm still rather hazy on the GeckoView architecture, I don't know whether that would make sense, or whether the lifetime of the runtime is supposed to be tied to the lifetime of Gecko.

On the balance of things, I'd say option 1) sound the most sensible, but then I might be completely wrong.


[1] https://dxr.mozilla.org/mozilla-central/rev/4e56a2f51ad739ca52046723448f3129a58f1666/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java#393

Comment 3

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

1) sounds right. The app should always call `setSession` during `onCreate` if it's using a custom runtime. Any saved session in `savedInstanceState` will be transferred to the new session during `onRestoreInstanceState`.

Comment 4

[Jan Henning [:JanH]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #3)
> Any saved session in `savedInstanceState` will be transferred to the new session during `onRestoreInstanceState`.

Hmm, with option 1 I had in mind to only set or transfer the session from the savedInstanceState if it was still open. If you really want "any" saved session as far as possible, that sounds more like option 1b: Allow transferring the state of a closed savedInstanceState session as well if we've already got a valid mRuntime available.

Or formulated the other way round: If mRuntime != null, restore any session, else only restore a still open session.

Comment 5

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

I think it's a combination of both. There are three possible `GeckoSession` states on either side when restoring: no session (none), closed session, and open session. I think we want the following operations in each situation:

>                                        saved session
>                           none       |      closed      |      open
>                   +------------------+------------------+------------------
>   GV       none   |  transfer saved  |  transfer saved  |  transfer saved
> session   closed  |      keep GV     |  transfer saved  |  transfer saved
>            open   |      keep GV     |      keep GV     |  transfer saved
>

The none/none case is effectively no-op.
The open/open case involves closing the GV session first before transferring.
The GV-none/saved-closed case involves using the default runtime for restoring.

Comment 6

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

Also the GV-open/saved-closed case is "keep GV", but we might still want to transfer settings from the saved session to the GV session.

Comment 7

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

(In reply to Jan Henning [:JanH] from comment #4)
> 
> Or formulated the other way round: If mRuntime != null, restore any session,
> else only restore a still open session.

IMHO, the table in comment #5 seems like a lot of complexity, and I'm not sure how an app author would predict the behavior without having that table pasted into the docs. I guess it's not worse than what we're doing now, though.

One thing that surprised me though is that we use GeckoSession.transferFrom() instead of just assigning the session directly.

Comment 8

[Jan Henning [:JanH]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
> Also the GV-open/saved-closed case is "keep GV", but we might still want to
> transfer settings from the saved session to the GV session.

Although right we only have the transferFrom() method, which transfers more than just the settings and requires the receiving session to be closed.

(In reply to James Willcox (:snorp) ([e-mail]) from comment #7)
> I guess it's not worse than what we're doing now, though.

I agree that this feels somewhat confusing, but again, I'm not familiar enough with this to propose something better. Also as you say, it's not radically different from the current state of things, were the activity might call setSession on the GeckoView after being created, and then the GeckoView goes ahead and does its own session thing anyway when/if it receives the savedInstanceState.

(In reply to James Willcox (:snorp) ([e-mail]) from comment #7)
> One thing that surprised me though is that we use
> GeckoSession.transferFrom() instead of just assigning the session directly.

Maybe because the containing activity might have already set various delegates on the session etc. (the GeckoViewActivity in the example app does that, for example), and we don't want to clobber that part of the session state?

To be honest, though, my main interest currently is only to make sure we don't end up with a null mRuntime in onAttachedToWindow(). We can always clean this up more later, once we have a better idea how this should work.

Comment 9

[Jan Henning [:JanH]]

Attachment: Bug 1484001 - Avoid ending up with a null mRuntime after restoring state in GeckoView. r?snorp

If we don't have a saved session, we do nothing. If somebody called setSession
beforehand, we continue using that session, otherwise we create a fallback
session in onAttachedToWindow().

If we have a saved session and nobody called setSession, we use the saved
session. If the saved session was closed and doesn't have a runtime, we use the
default runtime as a fallback.

If we have both a saved session and somebody already called setSession, we
transfer what can be transferred from the saved session, unless the saved
session is closed and the session from setSession is open.
If the saved session was open, we use its runtime as well going forward (since
transferring the state from an open session transfers the window and the runtime
as well), otherwise we keep the old mRuntime.

Comment 10

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

Comment on attachment 9005435 [details]
Bug 1484001 - Avoid ending up with a null mRuntime after restoring state in GeckoView. r?snorp

James Willcox (:snorp) ([e-mail]) has approved the revision.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34d7cf7fa7eb
Avoid ending up with a null mRuntime after restoring state in GeckoView. r=snorp

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/34d7cf7fa7eb