Closed Bug 1463878 Opened 2 years ago Closed 1 year ago

Reconsider a synchronous saveState API for GeckoView

Categories

(GeckoView :: General, defect, P1)

All
Android
defect

Tracking

(firefox64 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 wontfix, firefox68 fixed)

RESOLVED FIXED
mozilla68
Tracking Status
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: droeh, Assigned: droeh)

References

(Depends on 1 open bug, Blocks 2 open bugs, Regressed 1 open bug)

Details

(Whiteboard: [geckoview:fenix:m3])

Attachments

(3 files)

It seems like a very common use-case for the GV saveState API is in the life-cycle functions of an Activity or Fragment, where the async saveState call may not respond before onDestroy() is called. It might be better here to provide a synchronous saveState call.
A simple implementation using CountDownLatch for synchronization.
Attachment #8980092 - Flags: review?(snorp)
Attachment #8980092 - Flags: review?(nchen)
Which of course brings us back to the situation that this can make Gecko potentially block the Android UI. I suppose we have to see how bad of a problem that turns out to be in practice and then consider our options (it would be bad if each app would have to create its own workaround here).
(In reply to Jan Henning [:JanH] from comment #2)
> Which of course brings us back to the situation that this can make Gecko
> potentially block the Android UI. I suppose we have to see how bad of a
> problem that turns out to be in practice and then consider our options (it
> would be bad if each app would have to create its own workaround here).

Yeah, right now we have pretty limited information on how people are going to want to use saveState. The one project using it at the moment (Focus/Klar) wants to use it in Fragment lifecycle calls though, and I'm worried that as things progress it'll be pretty much what you predict: each app writing its own workaround to block on saveState responses.

Other possibilities that I can see:
- If we're going to have a WebView shim (ugh), we can possibly make the sync saveState call part of that and leave the API purely async in GeckoSession
- We can resort to proactively saving the state and sending it to Java, which would let us return a reasonably up-to-date state without blocking the UI thread. But in early discussions, we specifically wanted to avoid this in favor of saving the state reactively/on-demand.
Comment on attachment 8980092 [details] [diff] [review]
Synchronous saveState API

Review of attachment 8980092 [details] [diff] [review]:
-----------------------------------------------------------------

TBH I'm not convinced we need something synchronous. IMO a synchronous `saveState` will not give us much more advantage here. Android can still kill the process even if we block during `onPause`, for example if the user swipe-closes the app. Actually we are more likely to get killed if we block, because Android P will crash apps that block the UI thread for too long.

Eventually I think we want a "push" approach rather than a "pull" approach for anything synchronous. That means GV will regularly update and cache the current state during normal operation. Regular caching is also required for session restore to work reliably because, again, Android can kill the process at any time without calling `onPause`.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +1161,5 @@
>      }
>  
> +    private SessionState mSessionState;
> +
> +    private class SaveStateRunnable implements Runnable {

Use an anonymous class

@@ +1162,5 @@
>  
> +    private SessionState mSessionState;
> +
> +    private class SaveStateRunnable implements Runnable {
> +        private CountDownLatch mLatch;

SynchronousQueue is probably what you want.

@@ +1189,5 @@
> +     *
> +     * @return A snapshot of the current state of the session. Can be null if we fail to 
> +     *         save the state for any reason.
> +     */
> +    public SessionState saveState() {

I think we want a different name for the synchronous version.

@@ +1191,5 @@
> +     *         save the state for any reason.
> +     */
> +    public SessionState saveState() {
> +        final CountDownLatch stateSaved = new CountDownLatch(1);
> +        new Thread(new SaveStateRunnable(stateSaved)).start();

Use the background thread. We don't want to spin up a thread for this.
Attachment #8980092 - Flags: review?(nchen) → review-
P2 while we have a workaround.
Priority: -- → P2
(In reply to Dylan Roeh (:droeh) from comment #3)
> and I'm worried that as things progress it'll be pretty much what you predict: each
> app writing its own workaround to block on saveState responses.

You're right, that wouldn't be desirable, too, but what I predicted was that apps would find the API proposed here problematic because it makes Gecko block the Android UI thread, so they would *then* start each doing their individual workarounds.

(In reply to Jim Chen [:jchen] [:darchons] from comment #4)
> Eventually I think we want a "push" approach rather than a "pull" approach
> for anything synchronous. That means GV will regularly update and cache the
> current state during normal operation.

My point of view, too.
(In reply to Jan Henning [:JanH] from comment #6)
> (In reply to Dylan Roeh (:droeh) from comment #3)
> > and I'm worried that as things progress it'll be pretty much what you predict: each
> > app writing its own workaround to block on saveState responses.
> 
> You're right, that wouldn't be desirable, too, but what I predicted was that
> apps would find the API proposed here problematic because it makes Gecko
> block the Android UI thread, so they would *then* start each doing their
> individual workarounds.

Ah, good point!

(In reply to Jim Chen [:jchen] [:darchons] from comment #4)
> Comment on attachment 8980092 [details] [diff] [review]
> Synchronous saveState API
> 
> Review of attachment 8980092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> TBH I'm not convinced we need something synchronous. IMO a synchronous
> `saveState` will not give us much more advantage here. Android can still
> kill the process even if we block during `onPause`, for example if the user
> swipe-closes the app. Actually we are more likely to get killed if we block,
> because Android P will crash apps that block the UI thread for too long.
> 
> Eventually I think we want a "push" approach rather than a "pull" approach
> for anything synchronous. That means GV will regularly update and cache the
> current state during normal operation. Regular caching is also required for
> session restore to work reliably because, again, Android can kill the
> process at any time without calling `onPause`.

Yeah, the more I think about this the more I'm inclined to agree that having a proactive/push approach is probably the best way to go. I'll look into that -- for now, I think Focus/Klar is fine using a workaround, as they aren't concerned with saving anything in the event the process gets killed, just using this for tabs.
Comment on attachment 8980092 [details] [diff] [review]
Synchronous saveState API

Canceling review because we probably won't use this approach.
Attachment #8980092 - Flags: review?(snorp)
Product: Firefox for Android → GeckoView

I'm gonna poke this bug again because Reference Browser is effectively unusable with the current saveState() implementation. They are making a blocking call (using runBlocking in kotlin) during stop(), and it oftens ends up deadlocking.

In addition to a synchronous saveState() that returns a cached state, we should consider adding onStateChange() to one of the delegates (content delegate?).

See Also: → 1519351

P1 for Fenix MVP

Priority: P2 → P1
Whiteboard: [geckoview:fenix:p1]
See Also: 1519351
Blocks: 1519351

Can I assume that this would end up with something similar to parts of the current session store design, i.e. we'd keep a cache of session store data somewhere in the parent process, and pre-emptively keep that up-to-date it in response to various browsing events?
A synchronous call to saveState from Android would then only have to retrieve a copy of this cached data and wouldn't potentially block on a child process doing who knows what.

If so, before we end up reimplementing half (or more) of ContentSessionStore.jsm, it might be better to investigate the new proposed parent process SessionStore API from bug 1474130, and make sure that that will turn out suitable for GeckoView, too.

The only thing I know for sure that is missing from the desktop session store is the ability to save and restore the current pinch zoom. If we could add that [1], we'd hopefully be able to implement this without any additional custom frame scripts on the GeckoView-side.

[1] And sooner or later we'd have to anyway, because pinch zoom is planned for desktop, too (bug 1525259).

(In reply to Jan Henning [:JanH] from comment #11)

Can I assume that this would end up with something similar to parts of the current session store design, i.e. we'd keep a cache of session store data somewhere in the parent process, and pre-emptively keep that up-to-date it in response to various browsing events?
A synchronous call to saveState from Android would then only have to retrieve a copy of this cached data and wouldn't potentially block on a child process doing who knows what.

Yeah, that's the idea; also we would have a callback to notify the app when the cached session store data is updated.

If so, before we end up reimplementing half (or more) of ContentSessionStore.jsm, it might be better to investigate the new proposed parent process SessionStore API from bug 1474130, and make sure that that will turn out suitable for GeckoView, too.

The only thing I know for sure that is missing from the desktop session store is the ability to save and restore the current pinch zoom. If we could add that [1], we'd hopefully be able to implement this without any additional custom frame scripts on the GeckoView-side.

[1] And sooner or later we'd have to anyway, because pinch zoom is planned for desktop, too (bug 1525259).

I'm not sure we can rely on the new SessionStore API being ready in time for us to use it in Fenix 1.0, but you're right that we need to make sure it is usable by GV when it is ready. Performance is a pretty common concern here and one of the reasons we tried to do an on-demand saveState in the first place.

Depends on: 1528509

Scheduling for Fenix M3 milestone because this bug is related to Dylan's history stack API bug 1509110 in Fenix M2.

Whiteboard: [geckoview:fenix:p1] → [geckoview:fenix:m3]
See Also: → 1531575
Attachment #9051325 - Attachment description: Bug 1463878 - Add ContentSessionStore.js (mostly lifted from desktop code with modifications to reflect GV's needs) and code to send incremental session storage updates to Java. r=snorp! → Bug 1463878 - Add SessionStateAggregator.js (mostly lifted from desktop code with modifications to reflect GV's needs) and code to send incremental session storage updates to Java. r=snorp!
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11415a3e72f6
Add SessionStateAggregator.js (mostly lifted from desktop code with modifications to reflect GV's needs) and code to send incremental session storage updates to Java. r=snorp,JanH
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51b9ea359763
Add SessionStateAggregator.js (mostly lifted from desktop code with modifications to reflect GV's needs) and code to send incremental session storage updates to Java. r=snorp,JanH
https://hg.mozilla.org/integration/autoland/rev/f857b15c8d3c
Update GeckoView API to reflect new session storage and remove old API and associated dead code. r=snorp
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Flags: needinfo?(droeh)
Blocks: 1538291
Blocks: 1538294
Depends on: 1538702

67=wontfix. Neither Fenix MVP nor Firefox for Fire TV will use GeckoView 67, so we don't need to uplift this fix to 67 Beta.

Regressions: 1578812
Blocks: 1578814
You need to log in before you can comment on or make changes to this bug.