Reconsider a synchronous saveState API for GeckoView
Categories
(GeckoView :: General, defect, P1)
Tracking
(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.
Assignee | ||
Comment 1•6 years ago
|
||
A simple implementation using CountDownLatch for synchronization.
Comment 2•6 years ago
|
||
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).
Assignee | ||
Comment 3•6 years ago
|
||
(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 4•6 years ago
|
||
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.
Comment 6•6 years ago
|
||
(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.
Assignee | ||
Comment 7•6 years ago
|
||
(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.
Assignee | ||
Comment 8•6 years ago
|
||
Comment on attachment 8980092 [details] [diff] [review] Synchronous saveState API Canceling review because we probably won't use this approach.
Updated•5 years ago
|
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?).
Comment 10•5 years ago
•
|
||
P1 for Fenix MVP
Comment 11•5 years ago
|
||
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).
Assignee | ||
Comment 12•5 years ago
|
||
(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.
Comment 13•5 years ago
|
||
Scheduling for Fenix M3 milestone because this bug is related to Dylan's history stack API bug 1509110 in Fenix M2.
Assignee | ||
Comment 14•5 years ago
|
||
Assignee | ||
Comment 15•5 years ago
|
||
Depends on D23696
Updated•5 years ago
|
Comment 16•5 years ago
|
||
Backed out 2 changesets (bug 1463878) for ESlint and checkstyle failure
Backout: https://hg.mozilla.org/integration/autoland/rev/0da1eacffbea5669c6acf2531650da4fd89bdcdd
Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=235271833&revision=2cc833baea24171ad0d196894eda77cacb8c8981
Failure log ESlint: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=235284923&repo=autoland&lineNumber=297
Failure log checkstyle: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=235284913&repo=autoland&lineNumber=2840
Comment 17•5 years ago
|
||
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
Comment 18•5 years ago
|
||
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
Comment 19•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/51b9ea359763
https://hg.mozilla.org/mozilla-central/rev/f857b15c8d3c
Assignee | ||
Updated•5 years ago
|
Comment 20•5 years ago
|
||
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.
Description
•