Closed Bug 617851 Opened 9 years ago Closed 9 years ago

Restore session history on crash or OOM

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set

Tracking

(fennec2.0b4+)

VERIFIED FIXED
fennec2.0b3
Tracking Status
fennec 2.0b4+ ---

People

(Reporter: dougt, Assigned: mbrubeck)

References

Details

Attachments

(2 files, 1 obsolete file)

Recovery from a crash, or a low memory situation can be handled pretty nicely by having a quick recovery using session history.
Attached patch first cut. (obsolete) — Splinter Review
Attachment #496402 - Flags: review?(mark.finkle)
+ *   Mark 'evil' Finkle <mfinkle@mozilla.com>

It worth a r+ just for it.
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 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!
Attachment #496402 - Flags: review?(mark.finkle) → review-
tracking-fennec: --- → 2.0b4+
Attached patch patchSplinter Review
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.
Assignee: nobody → mark.finkle
Attachment #496402 - Attachment is obsolete: true
Attachment #497045 - Flags: review?(mbrubeck)
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
Attachment #497045 - Flags: review?(21) → review+
(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)
Attachment #497045 - Flags: review?(mbrubeck) → review+
Filed bug 619591 for adding MozAsyncCanvasRender event back
pushed:
http://hg.mozilla.org/mobile-browser/rev/574465359a37
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attached patch followupSplinter Review
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.
Assignee: mark.finkle → mbrubeck
Status: RESOLVED → REOPENED
Attachment #498039 - Flags: review?(mark.finkle)
Resolution: FIXED → ---
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.
Attachment #498039 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mobile-browser/rev/1c8519353a3c
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
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.
Target Milestone: --- → fennec2.0b3
Blocks: 615287
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.