If app is opened with new URL and is restoring state, back stack is lost

mcomella commented 5 years ago

Depends on #186

Steps to reproduce

(to simulate a user's app running out of resources, it getting killed, and a new navigation request to a specific URL, then have no history)

Enable don't keep activities
Open app
Navigate to several pages to create back stack
Press home
Open app with new url (adb shell am start -n org.mozilla.connect.firefox/org.mozilla.focus.MainActivity -a android.intent.action.VIEW "https://google.com"

Expected behavior

New URL is opened as latest site in back stack

Actual behavior

Back stack is cleared and only google.com is present.

We can't systematically reproduce this on-device (no don't keep activities) but in theory, it's possible to hit this state. However, since timeout is in 5 minutes, it's unlikely we'll see this often in practice.

Device information

Echo Show device (1st gen? 2nd?): emulator (for don't keep activites)
Latest affected Firefox version: master (post v1.3)
Earliest affected Firefoxx version: ?

This happens because ScreenController.showBrowserScreenForUrl actually creates an additional Session instead of restoring the first one and loading the new url into that: https://github.com/mozilla-mobile/firefox-echo-show/blob/8fe56e46499dcb9f229457f95f1c93f96333311f/app/src/main/java/org/mozilla/focus/ScreenController.kt#L83

We shouldn't be using, isBrowserScreenVisible as our branching logic, apparently.

mcomella commented 5 years ago

In #186, this issue also prevents us from both restoring the back stack and loading a new url from an intent (e.g. if the user is not familiar with "Open Firefox" but is familiar with "Open google").

mcomella commented 5 years ago

In order to fix this cleanly, it's a big refactor:

Simplify startup code so it isn't a bunch of hard-to-maintain callbacks (this may just fix the issue)
Create a permanent BrowserFragment (unless data is cleared? Though we can just clear the WebView instead of recreating the fragment). BrowserFragments are tied to a single session but if we are sure we only have one session, we can keep the BrowserFragment around forever.

For these types of changes, we may want to wait to use components and copy the work that Fire TV has already done in this area (can we share code with android-components?).

I was able to fix this with a small change but it's unclear if this will break anything and I'm not sure why it even works:

@@ -80,13 +92,14 @@ object ScreenController {
         // However, from a user perspective, the behavior is correct (e.g. back stack functions
         // correctly with multiple sessions).
         val browserFragment = fragmentManager?.getBrowserFragment()
-        if (browserFragment?.isVisible == true) {
+        if (SessionManager.getInstance().hasSession()) {
             // We can't call loadUrl on the Fragment until the view hierarchy is inflated so we check
             // for visibility in addition to existence.
-            browserFragment.loadUrl(url)
+            browserFragment?.loadUrl(url) // todo: would drop events.

mozilla-mobile / firefox-echo-show