Last Comment Bug 697858 - Restore tabs from previous session, including history
: Restore tabs from previous session, including history
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P2 normal (vote)
: Firefox 12
Assigned To: Brian Nicholson (:bnicholson)
:
Mentors:
: 701929 708929 711572 (view as bug list)
Depends on: 695176 696203 718240
Blocks: 710735 711572 711574 711578 810981
  Show dependency treegraph
 
Reported: 2011-10-27 15:25 PDT by Matt Brubeck (:mbrubeck)
Modified: 2014-04-29 11:35 PDT (History)
17 users (show)
andreea.pod: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified
11+


Attachments
WIP patch v1 (29.26 KB, patch)
2011-12-08 16:58 PST, Brian Nicholson (:bnicholson)
no flags Details | Diff | Review
WIP patch v2 (35.17 KB, patch)
2011-12-12 14:46 PST, Brian Nicholson (:bnicholson)
mark.finkle: feedback+
Details | Diff | Review
(1/3) tab/session history restore (37.07 KB, patch)
2011-12-16 20:06 PST, Brian Nicholson (:bnicholson)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review
(2/3) undo close tabs (16.41 KB, patch)
2011-12-16 20:08 PST, Brian Nicholson (:bnicholson)
mark.finkle: review-
Details | Diff | Review
(3/3) save restore state in bundle (11.66 KB, patch)
2011-12-16 20:09 PST, Brian Nicholson (:bnicholson)
mark.finkle: review-
Details | Diff | Review
follow-up patch: handle session restore failures (3.32 KB, patch)
2011-12-21 14:23 PST, Brian Nicholson (:bnicholson)
mark.finkle: review-
Details | Diff | Review
save restore state in bundle (11.21 KB, patch)
2011-12-21 14:24 PST, Brian Nicholson (:bnicholson)
mark.finkle: review-
blassey.bugs: feedback+
Details | Diff | Review
save restore state in bundle (11.93 KB, patch)
2012-01-04 17:09 PST, Brian Nicholson (:bnicholson)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Matt Brubeck (:mbrubeck) 2011-10-27 15:25:53 PDT
We do not restore the back/forward history of tabs from your previous session.

Note that the old SessionStore.js component from XUL Fennec is currently in the tree but is not built or packaged because it does not work in Native Fennec (bug 696203).
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-11 16:39:14 PST
*** Bug 701929 has been marked as a duplicate of this bug. ***
Comment 2 CoJaBo 2011-11-23 20:38:43 PST
Please make sure the use case described in Bug 656062, particularly Bug 656062 comment 2, works properly when this is done.
Comment 3 Brian Nicholson (:bnicholson) 2011-12-08 16:58:20 PST
Created attachment 580249 [details] [diff] [review]
WIP patch v1

This patch restores the tabs after a crash.  We still need to:
- restore favicons for each tab
- restore back/forward history
- reopen closed tabs
- change/remove 60 minute session timeout
Comment 4 Brian Nicholson (:bnicholson) 2011-12-12 14:46:18 PST
Created attachment 581062 [details] [diff] [review]
WIP patch v2

back/forward history added
Comment 5 Brian Nicholson (:bnicholson) 2011-12-12 15:35:10 PST
Some questions regarding the existing implementation:

Why do we delete the session information after an hour?
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/components/SessionStore.js#97

If we still want to delete session information after a crash, how can we differentiate between a crash and being killed by Android? As discussed in CoJaBo's bugs, we probably don't want to delete the session unless we have an actual crash, if at all.

Here,
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/bindings/browser.js#455
We send the session history on DOMContentLoaded, not on pageshow, so we won't necessarily have the correct history index after restoration.

Also, in onTabSelect,
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/components/SessionStore.js#436
we don't save the state after switching tabs, meaning the selected tab after restoration will not necessarily match the tab we were looking at.

Were these implemented like this intentionally, for performance reasons?
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-13 19:04:34 PST
Comment on attachment 581062 [details] [diff] [review]
WIP patch v2

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>-    // Broadcast a UIReady message so add-ons know we are finished with startup
>-    let event = document.createEvent("Events");
>-    event.initEvent("UIReady", true, false);
>-    window.dispatchEvent(event);

Keep

>diff --git a/mobile/android/components/SessionStore.js b/mobile/android/components/SessionStore.js
>+      case "pageshow": {
>+        let browser = aEvent.currentTarget;
>+        this.onTabLoad(window, browser);
>+        break;
>+      }
>+      case "DOMContentLoaded": {
>+        let browser = aEvent.currentTarget;
>+        this.onContentLoaded(window, browser);
>+        break;
>+      }

We talked about moving to only use pageshow and check aEvent.persisted to see if we are just moving through bf-cache

>           let tabData = tabs[i];
>+          let isSelected = i + 1 == selected;
>+          let currentEntry = tabData.entries[tabData.index - 1];

currentEntry -> entry

Note: the reason we used the funky "select all tabs until we it the one that is really selected" was due to the XUL deck not liking having nothing selected. So we tricked it and always had a tab selected, even if it was the wrong one. Eventually we looped to the real selected tab and could stop the trick.

>-          let params = { getAttention: false, delayLoad: true };
>+          let params = { selected: isSelected, delayLoad: !isSelected, title: currentEntry.title };
>+          let tab = window.BrowserApp.addTab(currentEntry.url, params);
> 
>-          // We must have selected tabs as soon as possible, so we let all tabs be selected
>-          // until we get the real selected tab. Then we stop selecting tabs. The end result
>-          // is that the right tab is selected, but we also don't get a bunch of errors

Yeah, this comment sums it up. As loong as life is better now, we can avoid it, but make sure we can.

>+          // Make sure the browser has its session data for the delay reload
>+          if (i + 1 != selected) {

if (!isSelected)  ?

>-            // Recreate the thumbnail if we are delay loading the tab
>-            let canvas = tab.chromeTab.thumbnail;
>-            canvas.setAttribute("restored", "true");
>-            canvas.removeAttribute("empty");

How do we handle the thumbnail now? Oh, we won't need to until Sriram lands the "use thumbnails in tab menu" patches
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-15 10:59:07 PST
*** Bug 708929 has been marked as a duplicate of this bug. ***
Comment 8 Brian Nicholson (:bnicholson) 2011-12-16 20:06:38 PST
Created attachment 582472 [details] [diff] [review]
(1/3) tab/session history restore
Comment 9 Brian Nicholson (:bnicholson) 2011-12-16 20:08:11 PST
Created attachment 582473 [details] [diff] [review]
(2/3) undo close tabs
Comment 10 Brian Nicholson (:bnicholson) 2011-12-16 20:09:38 PST
Created attachment 582474 [details] [diff] [review]
(3/3) save restore state in bundle
Comment 11 Brad Lassey [:blassey] (use needinfo?) 2011-12-19 15:17:03 PST
*** Bug 711572 has been marked as a duplicate of this bug. ***
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-20 09:12:54 PST
Comment on attachment 582472 [details] [diff] [review]
(1/3) tab/session history restore

This looks good. The Java code is likely bittrotted alittle.
Comment 13 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-20 09:17:12 PST
Comment on attachment 582473 [details] [diff] [review]
(2/3) undo close tabs

I want to try and avoid sending a new "Tab:ClosedChange" message and see if we can add this data to an existing message, like Tab:Added and/or TabClosed
Comment 14 Brian Nicholson (:bnicholson) 2011-12-20 17:46:19 PST
Landed patch #1 on inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9d3e6a6901f7
Comment 15 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-20 19:33:21 PST
Comment on attachment 582474 [details] [diff] [review]
(3/3) save restore state in bundle

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>-    if ("arguments" in window && window.arguments[0])
>-      uri = window.arguments[0];
>+    let shouldRestoreSession = false;

shouldRestoreSession -> shouldRestore

>+    if ("arguments" in window) {
>+      if (window.arguments[0])
>+        uri = window.arguments[0];
>+      shouldRestoreSession = window.arguments[1];
>+    }

We need to check for window.arguments[1] before attempting to use it.

        if (window.arguments.length == 2)
          shouldRestore = window.arguments[1];

>diff --git a/mobile/android/components/BrowserCLH.js b/mobile/android/components/BrowserCLH.js
>-function openWindow(aParent, aURL, aTarget, aFeatures, aArgs) {
>-  let argString = null;
>-  if (aArgs && !(aArgs instanceof Ci.nsISupportsArray)) {
>-    argString = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
>-    argString.data = aArgs;
>+function openWindow(aParent, aURL, aTarget, aFeatures, aUrl, aSessionRestore) {
>+  let argsArray = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
>+  let urlString = null;
>+  let sessionRestoreBool = Cc["@mozilla.org/supports-PRBool;1"].createInstance(Ci.nsISupportsPRBool);
>+
>+  if (aUrl) {
>+    urlString = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
>+    urlString.data = aUrl;
>   }
>+  sessionRestoreBool.data = aSessionRestore;
> 
>-  return Services.ww.openWindow(aParent, aURL, aTarget, aFeatures, argString || aArgs);
>+  argsArray.appendElement(urlString, false);
>+  argsArray.appendElement(sessionRestoreBool, false);
>+  return Services.ww.openWindow(aParent, aURL, aTarget, aFeatures, argsArray);
> }

I'd be happier if you sent aArgs as a JS array, and converted it to an nsISupportsArray. Kinda like this:
http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#260

>   handle: function fs_handle(aCmdLine) {
>     let urlParam = "about:home";
>+    let sessionRestore = false;
>     try {
>         urlParam = aCmdLine.handleFlagWithParam("remote", false);
>+        sessionRestore = aCmdLine.handleFlag("sessionrestore", false);
>     } catch (e) {
>       // Optional so not a real error
>     }

Use a second try/catch so wemake sure we actually attempt to handle the "sessionrestore" flag

>       } else {
>-        browserWin = openWindow(null, "chrome://browser/content/browser.xul", "_blank", "chrome,dialog=no,all", urlParam);
>+        browserWin = openWindow(null, "chrome://browser/content/browser.xul", "_blank", "chrome,dialog=no,all", urlParam, sessionRestore);

Given the changes to openWindow to handle a string or an array, I'd rather see this as:

          browserWin = openWindow(null, "chrome://browser/content/browser.xul", "_blank", "chrome,dialog=no,all", [urlParam, sessionRestore]);
Comment 16 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-20 20:05:33 PST
Comment on attachment 582473 [details] [diff] [review]
(2/3) undo close tabs


General nit: We should be referring to this feature as "Undo Closed Tab" not "Undo Close Tab"

I only mention that cause it affects your variable names and string entities, as well as actual strings. I only really care about the strings themselves.

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+            } else if (event.equals("Tab:ClosedChange")) {
>+                sUndoCloseTabEnabled = message.getBoolean("buttonEnabled");

I'd rather see the messaged called "Tab:CanUndo" and the data be an boolean called "allow".

>+    public boolean undoCloseTab() {
>+        Log.i(LOGTAG, "Undo close tab requested");

Let's drop this for checkin

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY undo_close_tab "Undo Close Tab">

"Undo Closed Tab"

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

> var BrowserApp = {

>+  _closedTabButtonEnabled: false,

I really would like to kill this

>+  updateClosedTabCount: function updateClosedTabCount(closedTabCount) {

And move this to SessionStore.js as a private method

>+  undoCloseTab: function undoCloseTab(aTab) {

No data is sent. No need for aTab

>+    } else if (aTopic == "Tab:UndoClose") {
>+      this.undoCloseTab(this.getTabForId(parseInt(aData)));

No data is sent, right?

>diff --git a/mobile/android/components/SessionStore.js b/mobile/android/components/SessionStore.js

>     if (!aNoNotification)
>       this.saveStateDelayed();
>+
>+    // update the browser with the number of closed tabs
>+    aWindow.BrowserApp.updateClosedTabCount(this.getClosedTabCount(aWindow));

This is in onTabRemove. We only need to do this if count == 1 right?

>     // Put back the extra data
>     tab.browser.__SS_extdata = closedTab.extData;
> 
>+    // update the browser with the number of closed tabs
>+    aWindow.BrowserApp.updateClosedTabCount(this.getClosedTabCount(aWindow));

This is in undoCloseTab. We only need to send now if count == 0 right?

>           tab.browser.__SS_extdata = tabData.extData;
>           self._restoreHistory(tabData, tab.browser.sessionHistory);
>         }
> 
>+        // recover closed tabs data
>+        self._windows[window.__SSID].closedTabs = data.windows[0].closedTabs;
>+        window.BrowserApp.updateClosedTabCount(self.getClosedTabCount(window));

Yeah, we need to do this all the time

How is this patch different than bug 701725? I think this would be a good first patch for that bug. We do not need "undo closed tab" for the initial sessionrestore landing. It might be better to keep them separate. Feel free to move this patch to bug 701725.
Comment 17 Ed Morley [:emorley] 2011-12-21 04:29:33 PST
https://hg.mozilla.org/mozilla-central/rev/9d3e6a6901f7
Comment 18 Brian Nicholson (:bnicholson) 2011-12-21 14:23:52 PST
Created attachment 583629 [details] [diff] [review]
follow-up patch: handle session restore failures

This patch waits for the "sessionstore-windows-restored" before adding tabs or restoring the session in case of a failure. I was getting a NPE in GeckoApp.java with this patch at getGeckoViewportMetrics (likely a race condition that assumes a tab exists), so that's what that null check is for.
Comment 19 Brian Nicholson (:bnicholson) 2011-12-21 14:24:47 PST
Created attachment 583630 [details] [diff] [review]
save restore state in bundle
Comment 20 Brian Nicholson (:bnicholson) 2011-12-21 14:25:17 PST
reopened until the rest of the patches are landed
Comment 21 Brian Nicholson (:bnicholson) 2011-12-21 14:34:54 PST
Comment on attachment 583629 [details] [diff] [review]
follow-up patch: handle session restore failures

Just noticed some minor typos. I changed:

+  restore: function init(uri) {
to
+  restore: function restore(uri) {

and

+  observe: function(aSubject, aTopic, aData) {
to
+  observe: function observe(aSubject, aTopic, aData) {
Comment 22 Henrik Skupin (:whimboo) 2011-12-22 00:33:00 PST
So what's the story for a yesterdays nightly build? When I open multiple tabs and explicitly close Firefox via Quit, those tabs are not getting restored when I start Firefox again. The start page gets loaded but there is no link which could be used to restore the last session.
Comment 23 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-22 06:30:02 PST
Comment on attachment 583630 [details] [diff] [review]
save restore state in bundle

> function openWindow(aParent, aURL, aTarget, aFeatures, aArgs) {

I'm not exactly happy with this function impl yet. I know it's hard to test the case where aArgs could be null, but I think we need to impl the function as if it could be null.

If aArgs is null, we want to pass null to Services.ww.openWindow and then browser.js would get a null window.arguments. browser.js is already setup to handle that condition.

>-  let argString = null;
>-  if (aArgs && !(aArgs instanceof Ci.nsISupportsArray)) {
>-    argString = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
>-    argString.data = aArgs;
>+  let argsArray = Cc["@mozilla.org/supports-array;1"].createInstance(Ci.nsISupportsArray);

Set argsArray to null here and init it to a nsISupportsArray inside the | if (aArgs) | block below

>+  let urlString = null;
>+  let sessionRestoreBool = Cc["@mozilla.org/supports-PRBool;1"].createInstance(Ci.nsISupportsPRBool);

Move these to inside the | if (aArgs) | block

>+
>+  if (aArgs[0]) {

    if (aArgs) {

>+    urlString = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
>+    urlString.data = aArgs[0];
>   }
>+  sessionRestoreBool.data = aArgs[1];

Move this into the aArgs block

>+  argsArray.AppendElement(urlString, false);
>+  argsArray.AppendElement(sessionRestoreBool, false);

Move these to inside the | if (aArgs) |  block

>+  return Services.ww.openWindow(aParent, aURL, aTarget, aFeatures, argsArray);

At this point we should have argsArray as null or as a filled array.

Does this make sense to you?

r- only to fix up this method
Comment 24 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-22 06:37:46 PST
Comment on attachment 583629 [details] [diff] [review]
follow-up patch: handle session restore failures

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js


>+    // restore the previous session
>+    if (shouldRestore || ss.shouldRestore())
>+      SessionStore.restore(uri);

I know this is not my normal opinion on code, but I'd rather have this helper code integrated into BrowserApp.startup

http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/browser.js#403

Instead of "dummyCleaner" you can call it "sessionRestorer"


>+    else {
>+      this.addTab(uri);
>+      this.startupFinished();

>+  startupFinished: function startupFinished() {

Yes, we keep this

>+var SessionStore = {

Remove this


r- just to see a new patch
Comment 25 Brian Nicholson (:bnicholson) 2012-01-04 17:09:45 PST
Created attachment 585946 [details] [diff] [review]
save restore state in bundle
Comment 26 Alex Keybl [:akeybl] 2012-01-05 13:57:06 PST
Comment on attachment 585946 [details] [diff] [review]
save restore state in bundle

[Triage Comment]
Please re-nominate once r+'d, landed on m-c, and baked for a day.
Comment 27 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-06 11:31:46 PST
Comment on attachment 585946 [details] [diff] [review]
save restore state in bundle

> BrowserCLH.prototype = {

>+    try {
>+        restoreSession = aCmdLine.handleFlag("restoresession", false);
>+    } catch (e) {
>+      // Optional so not a real error
>+    }

      } catch (e) { /* Optional */ }

LGTM. I bitrot the BrowserCLH part though with a recent landing on inbound. Sorry.
Comment 28 Brian Nicholson (:bnicholson) 2012-01-09 11:22:48 PST
Restore state landed on mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/af32a0da45c9
Comment 29 Ed Morley [:emorley] 2012-01-09 14:48:11 PST
(In reply to Brian Nicholson (:bnicholson) from comment #28)
> Restore state landed on mozilla-inbound:
> http://hg.mozilla.org/integration/mozilla-inbound/rev/af32a0da45c9

One of three changesets backed out of inbound due to test coalescing making it hard to identify which caused the native android test failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1927c7905f5e
Comment 30 Brian Nicholson (:bnicholson) 2012-01-10 21:24:49 PST
There was a typo in browser.js (s/uri/url). Fix passes try tests:
https://tbpl.mozilla.org/?tree=Try&rev=324f883c5e89

Pushed fix to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/03d77ca70f27
Comment 31 Ed Morley [:emorley] 2012-01-11 18:07:36 PST
https://hg.mozilla.org/mozilla-central/rev/03d77ca70f27
Comment 32 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-13 09:19:48 PST
Comment on attachment 582472 [details] [diff] [review]
(1/3) tab/session history restore

[Approval Request Comment]
Restoring from session is a core feature we want for release. Parity with other Mozilla browsers too.

This has been baking on m-c for a while and has had the fallout issues fixed.
Comment 33 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-13 09:20:23 PST
Comment on attachment 585946 [details] [diff] [review]
save restore state in bundle

This is now ready for moving to aurora.
Comment 34 Gabriela [:gaby2300] 2012-01-13 09:24:15 PST
Mark - Should I be able to see the fix in today's or tomorrow's build?
Comment 35 Aaron Train [:aaronmt] 2012-01-13 09:29:46 PST
(In reply to Gabriela from comment #34)
> Mark - Should I be able to see the fix in today's or tomorrow's build?

You can see it in today's Nightly.

Verified on Nightly (12.0a1)
Samsung Nexus S (Android 4.0.3)
Mozilla/5.0 (Android; Linux armv7l; rv:12.a01) Gecko/20120113 Firefox/12.0a1 Fennec/12.0a1
Comment 36 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-01-15 02:41:20 PST
As I understand, session is only restored after a crash (or app is killed)?

I would very much like a way to be able to open the tabs from a previous visit, after I quit Fennec. Is there a way or bug filed about this?
Comment 37 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-15 06:39:19 PST
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #36)
> As I understand, session is only restored after a crash (or app is killed)?
> 
> I would very much like a way to be able to open the tabs from a previous
> visit, after I quit Fennec. Is there a way or bug filed about this?

See bug 712970
Comment 38 Alex Keybl [:akeybl] 2012-01-16 12:59:09 PST
Comment on attachment 582472 [details] [diff] [review]
(1/3) tab/session history restore

[Triage Comment]
Mobile only - approved for Aurora.
Comment 40 Andreea Pod 2012-01-17 07:24:18 PST
Testcase added: https://litmus.mozilla.org/show_test.cgi?id=44977

Note You need to log in before you can comment on or make changes to this bug.