617851 - Restore session history on crash or OOM

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Doug Turner (:dougt)]

Recovery from a crash, or a low memory situation can be handled pretty nicely by having a quick recovery using session history.

Comment 1

[Doug Turner (:dougt)]

Attachment: first cut.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

+ *   Mark 'evil' Finkle <[e-mail]>

It worth a r+ just for it.

Comment 3

[Madhava Enros [:madhava]]

Agreed - especially if we're just loading the front tab and loading the others when a user switches to them, this is a great crash recovery experience.  We'll have to change the text in the crash reporter UI.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 496402 [details] [diff] [review]
first cut.

Overall, this is a good patch. We need to add support for:
* selecting the right tab when restoring
* some ways to disable crash recovery (altogether and after a timeout)
* fix the thumbnail canvases which go black after restoring them. this happens because the canvas is loaded from the main process, via an image. then it is loaded from a async draw happening in the child process - the canvas can't handle that and goes black. we need to recreate the canvas in those cases.

r-, but good stuff. I can take over this patch and get some front-end reviews. Thank you Mr. Turner!

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

This patch builds on Doug's patch and adds the missing parts. I tired to comment the code so we know what's happening in this "tricky" process.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 497045 [details] [diff] [review]
patch

>diff --git a/chrome/content/tabs.xml b/chrome/content/tabs.xml
>+
>+              // We don't have an event for the async drawContent anymore, so hack it
>+              setTimeout(function() {
>+                // Save the thumbnail to the session in case we need to use it in a restore
>+                let data = thumbnail.toDataURL("image/png");
>+                let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
>+                ss.setTabValue(self, "thumbnail", data);
>+              }, 800);

Is the solution to avoid a setTimeout is to fired a MozAsyncCanvasRender event in http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/DocumentRendererParent.cpp#56 ?
If not I guess you can remove the MozAsyncCanvasRender event fired by drawContent for local tabs (which will make dougt happy per bug 617850#c1)

>+  restoreLastSession: function ss_restoreLastSession() {
>+    // The previous session data has already been renamed to the backup file
>+    let dirService = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties);
>+    let session = dirService.get("ProfD", Ci.nsILocalFile);
>+    session.append("sessionstore.bak");
>+
>+    let data = JSON.parse(this._readFile(session));
>+    if (!data || data.windows.length == 0)
>+      return;
>+
>+    let window = Services.wm.getMostRecentWindow("navigator:browser");
>+
>+    let selected = data.windows[0].selected;

Nit: maybe the "selected" property should be rename selectedTabIndex to be clearer?

>+    let tabs = data.windows[0].tabs;
>+    for (let i=0; i<tabs.length; i++) {
>+      let tabData = tabs[i];
>+
>+      // Add a tab, but don't load the URL until we need to
>+      let params = { getAttention: false, delayLoad: true };
>+      if (i + 1 == selected)
>+        params.delayLoad = false;
>+
>+      // 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
>+      let bringToFront = (i + 1 <= selected);
>+      let tab = window.Browser.addTab(tabData.entries[0].url, bringToFront, null, params);

I'm curious does the "errors" is the tab order, because the background tabs are opened after the selected one? Or is it something else?

>+      // Recreate the thumbnail if we are delay loading the tab
>+      if (tabData.extData && params.delayLoad) {
>+          let canvas = tab.chromeTab.thumbnail;
>+          canvas.setAttribute("restored", "true");
>+
>+          let image = new window.Image();
>+          image.onload = function() {
>+            if (canvas)
>+              canvas.getContext("2d").drawImage(image, 0, 0);
>+          };
>+          image.src = tabData.extData.thumbnail;
>+      }

How is extData.thumbnail stored? Why can't we directly draw it on the canvas?


r+ but I want to understand why we're not drawing directly to the canvas

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #6)
> Comment on attachment 497045 [details] [diff] [review]
> patch
> 
> >diff --git a/chrome/content/tabs.xml b/chrome/content/tabs.xml
> >+
> >+              // We don't have an event for the async drawContent anymore, so hack it
> >+              setTimeout(function() {
> >+                // Save the thumbnail to the session in case we need to use it in a restore
> >+                let data = thumbnail.toDataURL("image/png");
> >+                let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
> >+                ss.setTabValue(self, "thumbnail", data);
> >+              }, 800);
> 
> Is the solution to avoid a setTimeout is to fired a MozAsyncCanvasRender event
> in
> http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/DocumentRendererParent.cpp#56
> ?
> If not I guess you can remove the MozAsyncCanvasRender event fired by
> drawContent for local tabs (which will make dougt happy per bug 617850#c1)

Yes, we want to add the MozAsyncCanvasRender event back into the platform. I can file a followup for that.

> 
> >+  restoreLastSession: function ss_restoreLastSession() {
> >+    // The previous session data has already been renamed to the backup file
> >+    let dirService = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties);
> >+    let session = dirService.get("ProfD", Ci.nsILocalFile);
> >+    session.append("sessionstore.bak");
> >+
> >+    let data = JSON.parse(this._readFile(session));
> >+    if (!data || data.windows.length == 0)
> >+      return;
> >+
> >+    let window = Services.wm.getMostRecentWindow("navigator:browser");
> >+
> >+    let selected = data.windows[0].selected;
> 
> Nit: maybe the "selected" property should be rename selectedTabIndex to be
> clearer?

| data.windows[0].selected | is what desktop Firefox uses for it's session store state, so I used "selected" too.

> >+      // 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
> >+      let bringToFront = (i + 1 <= selected);
> >+      let tab = window.Browser.addTab(tabData.entries[0].url, bringToFront, null, params);
> 
> I'm curious does the "errors" is the tab order, because the background tabs are
> opened after the selected one? Or is it something else?

Too much of our code tries to getBrowser() or Browser.selectedTab or Browser.selectedBrowser, which will fail badly if we don't have a selected browser. We should probably enforce having a selected browser in Browser.addTab

The fix I use works around this issue by selecting all browsers we add, until we add the browser we actually want to select. Then we can safely stop selecting browsers.

> >+      // Recreate the thumbnail if we are delay loading the tab
> >+      if (tabData.extData && params.delayLoad) {
> >+          let canvas = tab.chromeTab.thumbnail;
> >+          canvas.setAttribute("restored", "true");
> >+
> >+          let image = new window.Image();
> >+          image.onload = function() {
> >+            if (canvas)
> >+              canvas.getContext("2d").drawImage(image, 0, 0);
> >+          };
> >+          image.src = tabData.extData.thumbnail;
> >+      }
> 
> How is extData.thumbnail stored? Why can't we directly draw it on the canvas?

Session is saved to a JSON file, so the image is saved as a data: URI (base 64 encoded)

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Filed bug 619591 for adding MozAsyncCanvasRender event back

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/574465359a37

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: followup

I missed a problem in my review: If Fennec crashes/OOMs, and then is reopened with a command-line argument (e.g., a link from another app), then the command-line argument will now be ignored.

This patch fixes this regression.  It get rid of the getHomePage function in BrowserCLH, which is unnecessary because it's duplicated in Browser.getHomePage.  If there is a previous session *and* a command-line argument, then Fennec will open both.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 498039 [details] [diff] [review]
followup

Moving session restore to the CLH might be best in the long run. This works well for now.

Comment 12

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mobile-browser/rev/1c8519353a3c

Comment 13

[cris]

I understand the session restore when it crashes.

But how about when I kill it from session manager? Thats user initiated kill, and fennec won't goto homepage anymore.

I suggest:

1. add an option in start page option as "last visited"

or
2. add a quit function somewhere.

Comment 14

[Cristian Nicolae (:xti)]

Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:8.0a2)Gecko/20110908
Firefox/8.0a2 Fennec/8.0a2
Device: Samsung Galaxy S
OS: Android 2.2