Stub restored tabs before Gecko starts

RESOLVED FIXED in Firefox 19

Status

()

defect
P3
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: lucasr, Assigned: bnicholson)

Tracking

Trunk
Firefox 19
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(fennec+)

Details

(Whiteboard: startup-ux)

Attachments

(6 attachments, 3 obsolete attachments)

Right now there's a delay before the session is stored (i.e. tabs from last session are restored). The delay comes from the fact that we wait for Gecko to load to then restore tabs. Ideally, the Java side would immediately stub all restored tabs by directly reading the sessionstore file. Gecko would then "revive" the tabs once they're selected.
Not a blocker for Fx11, but a nice to have at some point
Assignee: nobody → bnicholson
tracking-fennec: --- → +
Priority: -- → P3
I think this would be similar to the solution to bug 718465. To fix that bug, I think we want to be able to stub a Tab in java before we get information about it from gecko.
Depends on: 797075
This patch restores tabs in Java and sends the session data to the SessionStore component. The SessionStore component reads the stubbed IDs from the data and restores the session history for the tabs.

Stubbing is only done after OOM kills. Since we need to read prefs from Gecko to determine whether to restore after a crash (e.g, reading the resume_from_crash pref, or reading the number of recent crashes), we can't create the tabs immediately in Java. Java does still initiate the restore, however, by sending a Session:Restore message and sending the session data string if it finds sessionstore.js at startup.

The only case where the SessionStore component might initiate a restore without being signalled from Java would be cases where the "browser.sessionstore.resume_session_once" pref is set to true. Should we continue supporting this pref? The problem is that this pref would be used after clean quits (like restarting the browser); this means that sessionstore.js won't exist at startup, which in turn means that we'd be showing about:home for a few seconds before the restore kicked in.
Attachment #672151 - Flags: review?(mark.finkle)
We previously used the Session:RestoreBegin and Session:RestoreEnd messages to signal whether new tabs should be animated. Since we now create the tabs in Java, we can create flags for loadUrl() to indicate whether the tabs should be animated. 
Unfortunately, since creating tabs from OOM kills uses a different path than crash restores, this means that we need to keep the RestoreBegin/RestoreEnd code if we still want to prevent animations for a crash restore.

Personally, I think we should just not do restores after crashes so we can kill off this extra code (like these animations, the 30 second timeout, the restoringOOM block in SessionStore.js, etc). We've come a long way in reducing our crashes, and we still show tabs from last time for people who want them.
Attachment #672164 - Flags: review?(mark.finkle)
This allows private and non-private browsing tabs to be restored all in one go.
Attachment #672165 - Flags: review?(mark.finkle)
We now send a Session:Restore message to trigger the restore, and we no longer have any of this code in BrowserApp's startup().
Attachment #672166 - Flags: review?(mark.finkle)
While I try to digest these patches, can you make a Try server run? I am looking for any performance regressions as well as unit test failures.
Posted patch Part 6: Miscellaneous fixes (obsolete) — Splinter Review
A number of other small fixes:
* We can remove SAVED_STATE_TITLE since Java can now read the restored tab data directly at startup
* Calls hideAboutHome() if we aren't doing a restore since this will reveal the white LayerView and make the page look like it's loading faster
* Adds some missing null checks for tab.getURL() since the tab's URL can be null
Attachment #672171 - Flags: review?(mark.finkle)
(In reply to Mark Finkle (:mfinkle) from comment #8)
> While I try to digest these patches, can you make a Try server run? I am
> looking for any performance regressions as well as unit test failures.

Sure: https://tbpl.mozilla.org/?tree=Try&rev=e38589a9d620
Removed the fix to loadUrlInTab() since that's now covered in bug 799977. Also added an animation fix so that the tab counter doesn't animate at startup.
Attachment #672171 - Attachment is obsolete: true
Attachment #672171 - Flags: review?(mark.finkle)
Attachment #672983 - Flags: review?(mark.finkle)
Blocks: 789867
Comment on attachment 672135 [details] [diff] [review]
Part 1: Factor out session JSON parsing into SessionParser

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

>+        new SessionParser() {

>+        }.parse(jsonString);

I assume "parse" is  blocking call? Maybe you should add an "onFinishRead" call and put the "more" code in that? Then in the future, we could possibly move the code entirely to the background? If you think it's something to consider, file a new bug.
Attachment #672135 - Flags: review?(mark.finkle) → review+
Comment on attachment 672151 [details] [diff] [review]
Part 2: Stub tabs in Java when doing an OOM restore


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

>+        if (mRestoreMode != GeckoAppShell.RESTORE_NONE) {

A comment right here about what we are going to start would be nice

>+                if (mRestoreMode == GeckoAppShell.RESTORE_OOM) {

>+                    SessionParser parser = new SessionParser() {

>+                            int flags = Tabs.LOADURL_NEW_TAB
>+                                      | ((isExternalURL || !sessionTab.isSelected()) ? Tabs.LOADURL_DELAY_LOAD : 0)
>+                                      | (tabObject.optBoolean("desktopMode") ? Tabs.LOADURL_DESKTOP : 0);
>+

int flags = Tabs.LOADURL_NEW_TAB;
flags |= ((isExternalURL || !sessionTab.isSelected()) ? Tabs.LOADURL_DELAY_LOAD : 0);
flags |= (tabObject.optBoolean("desktopMode") ? Tabs.LOADURL_DESKTOP : 0);

would be a little easier to read

>+                    if (tabs.length() > 0) {
>+                        sessionString = new JSONObject().put("windows", new JSONArray().put(new JSONObject().put("tabs", tabs))).toString();
>+                    } else {
>+                        throw new Exception("no tabs could be read from session file");
>+                    }

What are you doing this for? What is different about the new sessionString?

also, nit: "No tabs could be read from the session file"

>+                // If restore failed, do a normal startup
>+                Log.e(LOGTAG, "restore error", e);

nit: "An error occurred during restore"

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

>     public boolean shouldRestoreSession() {
>         Log.w(LOGTAG, "zerdatime " + SystemClock.uptimeMillis() + " - start check sessionstore.js exists");

Remove this log

>         Log.w(LOGTAG, "zerdatime " + SystemClock.uptimeMillis() + " - finish check sessionstore.js exists");

Remove this log

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>+        isPrivate: (data.isPrivate == true),
>+        pinned: data.pinned,
>+        delayLoad: delayLoad,
>+        desktopMode: (data.desktopMode == true)

why not 
          isPrivate: data.isPrivate,
...
          desktopMode: data.desktopMode

>diff --git a/mobile/android/components/SessionStore.js b/mobile/android/components/SessionStore.js
>       if (!this._sessionCache.exists() || !this._sessionCache.isDirectory())
>         this._sessionCache.create(Ci.nsIFile.DIRECTORY_TYPE, 0700);
>     } catch (ex) {
>       Cu.reportError(ex); // file was write-locked?
>     }

I just realized that we no longer use the sessionCache to store thumbnails. That was XUL only. Can you file a bug to remove all sessionCache code?

>-      // Nothing to restore, notify observers things are complete
>-      if (!this._shouldRestore) {
>-        this._clearCache();
>-        Services.obs.notifyObservers(null, "sessionstore-windows-restored", "");
>-      }

Are you sure we don't need this to always make sure the notification is sent?

>+        if (!Services.prefs.getBoolPref("browser.sessionstore.resume_from_crash")) {
>+          throw "session restore is disabled via prefs";

"SessionStore: Restore is disabled via prefs"

>+        if (recentCrashes > maxCrashes) {
>+          throw "exceeded maximum number of allowed restores";

"SessionStore: Exceeded maximum number of allowed restores"

>+        if (!this._sessionFileBackup.exists()) {
>+          throw "session file doesn't exist";

"SessionStore: Session file doesn't exist"


Throwing works the same as the Cu.reportError + return ?

r+, but please answer my question before landing
Attachment #672151 - Flags: review?(mark.finkle) → review+
Comment on attachment 672164 [details] [diff] [review]
Part 3: Prevent animation of restored OOM tabs

I am not liking this patch. I don't like tying presentation state (animation) into the tab state.

Need more time to consider it.
Attachment #672165 - Flags: review?(mark.finkle) → review+
Attachment #672166 - Flags: review?(mark.finkle) → review+
Comment on attachment 672983 [details] [diff] [review]
Part 6: Miscellaneous fixes, v2


>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java
>             if (mRestoreMode == RESTORE_NONE) {
>-                Tab tab = Tabs.getInstance().loadUrl("about:home", Tabs.LOADURL_NEW_TAB);
>+                Tab tab = Tabs.getInstance().loadUrl("about:home", Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_NO_ANIMATION);

Not yet

>-            Tabs.getInstance().loadUrl(uri, Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_USER_ENTERED);
>+            int flags = Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_USER_ENTERED;
>+            if (Tabs.getInstance().getCount() == 0) {
>+                // If there are already open tabs, we've done an OOM restore,
>+                // so we want the external URL tab to be animated (because
>+                // that's what normally happens when Gecko is running). But if
>+                // this is the first tab, do not use animation.
>+                flags |= Tabs.LOADURL_NO_ANIMATION;

Not yet

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

>                     } catch (JSONException e) {
>-                        Log.e(LOGTAG, "error building json arguments");
>+                        Log.e(LOGTAG, "error building json arguments", e);

"Error building JSON arguments"

r+, but don't do the Tabs.LOADURL_NO_ANIMATION parts yet. Add a new patch for those, once we get the initial patch landed.
Attachment #672983 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #13)
> Comment on attachment 672151 [details] [diff] [review]
> Part 2: Stub tabs in Java when doing an OOM restore
> 
> >+                    if (tabs.length() > 0) {
> >+                        sessionString = new JSONObject().put("windows", new JSONArray().put(new JSONObject().put("tabs", tabs))).toString();
> >+                    } else {
> >+                        throw new Exception("no tabs could be read from session file");
> >+                    }
> 
> What are you doing this for? What is different about the new sessionString?

This:
+                                tabObject.put("tabId", tab.getId());

The purpose of this SessionParser code is to stub the tabs in Java, send the session data to Gecko, and have Gecko match the restored tabs with the tab stubs. Injecting the tabId into the restore JSON allows Gecko to to which tab corresponds to what data. My first implementation kept them separate and built an array of tabIds, but I think this is a bit more reliable (since it handles the case if the tab happens to be closed before the restore).

> 
> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> >+        isPrivate: (data.isPrivate == true),
> >+        pinned: data.pinned,
> >+        delayLoad: delayLoad,
> >+        desktopMode: (data.desktopMode == true)
> 
> why not 
>           isPrivate: data.isPrivate,
> ...
>           desktopMode: data.desktopMode

To ensure that these params are booleans. If these the isPrivate or desktopMode properties weren't in the data object, we'd be passing undefined.

> I just realized that we no longer use the sessionCache to store thumbnails.
> That was XUL only. Can you file a bug to remove all sessionCache code?

Filed bug 806454.

> 
> >-      // Nothing to restore, notify observers things are complete
> >-      if (!this._shouldRestore) {
> >-        this._clearCache();
> >-        Services.obs.notifyObservers(null, "sessionstore-windows-restored", "");
> >-      }
> 
> Are you sure we don't need this to always make sure the notification is sent?

We need to make sure every code path in restoreLastSession() send out this notification, but this is being sent out in onWindowOpen(), so it seems unnecessary to have it here. Also, shouldRestore will rarely be true anymore. The only thing that makes it true now is "browser.sessionstore.resume_session_once" being set, so this code wouldn't be working as intended.

> 
> >+        if (!Services.prefs.getBoolPref("browser.sessionstore.resume_from_crash")) {
> >+          throw "session restore is disabled via prefs";
> 
> "SessionStore: Restore is disabled via prefs"
> 
> >+        if (recentCrashes > maxCrashes) {
> >+          throw "exceeded maximum number of allowed restores";
> 
> "SessionStore: Exceeded maximum number of allowed restores"
> 
> >+        if (!this._sessionFileBackup.exists()) {
> >+          throw "session file doesn't exist";
> 
> "SessionStore: Session file doesn't exist"
> 
> 
> Throwing works the same as the Cu.reportError + return ?
> 

These exceptions are all caught here:

     } catch (e) {
       Cu.reportError("SessionStore: Could not restore session: " + e);
       notifyObservers("fail");
     }

So throwing will result in a Cu.reportError() (whose message will be prefixed with "SessionStore: Could not restore session: "), and the "sessionstore-windows-restored" message is dispatched.
(In reply to Brian Nicholson (:bnicholson) from comment #16)
> (In reply to Mark Finkle (:mfinkle) from comment #13)
> > Comment on attachment 672151 [details] [diff] [review]
> > Part 2: Stub tabs in Java when doing an OOM restore
> > 
> > >+                    if (tabs.length() > 0) {
> > >+                        sessionString = new JSONObject().put("windows", new JSONArray().put(new JSONObject().put("tabs", tabs))).toString();
> > >+                    } else {
> > >+                        throw new Exception("no tabs could be read from session file");
> > >+                    }
> > 
> > What are you doing this for? What is different about the new sessionString?
> 
> This:
> +                                tabObject.put("tabId", tab.getId());
> 
> The purpose of this SessionParser code is to stub the tabs in Java, send the
> session data to Gecko, and have Gecko match the restored tabs with the tab
> stubs. Injecting the tabId into the restore JSON allows Gecko to to which
> tab corresponds to what data. My first implementation kept them separate and
> built an array of tabIds, but I think this is a bit more reliable (since it
> handles the case if the tab happens to be closed before the restore).

Acceptable :)

> > >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> > >+        isPrivate: (data.isPrivate == true),
> > >+        pinned: data.pinned,
> > >+        delayLoad: delayLoad,
> > >+        desktopMode: (data.desktopMode == true)
> > 
> > why not 
> >           isPrivate: data.isPrivate,
> > ...
> >           desktopMode: data.desktopMode
> 
> To ensure that these params are booleans. If these the isPrivate or
> desktopMode properties weren't in the data object, we'd be passing undefined.

What about:

pinned: data.pinned  ?

> > >-      // Nothing to restore, notify observers things are complete
> > >-      if (!this._shouldRestore) {
> > >-        this._clearCache();
> > >-        Services.obs.notifyObservers(null, "sessionstore-windows-restored", "");
> > >-      }
> > 
> > Are you sure we don't need this to always make sure the notification is sent?
> 
> We need to make sure every code path in restoreLastSession() send out this
> notification, but this is being sent out in onWindowOpen(), so it seems
> unnecessary to have it here. Also, shouldRestore will rarely be true
> anymore. The only thing that makes it true now is
> "browser.sessionstore.resume_session_once" being set, so this code wouldn't
> be working as intended.

OK

> These exceptions are all caught here:
> 
>      } catch (e) {
>        Cu.reportError("SessionStore: Could not restore session: " + e);
>        notifyObservers("fail");
>      }
> 
> So throwing will result in a Cu.reportError() (whose message will be
> prefixed with "SessionStore: Could not restore session: "), and the
> "sessionstore-windows-restored" message is dispatched.

OK. Just make sure the text has a leading CAP.

Good to go
(In reply to Mark Finkle (:mfinkle) from comment #12)
> Comment on attachment 672135 [details] [diff] [review]
> Part 1: Factor out session JSON parsing into SessionParser
> 
> >diff --git a/mobile/android/base/AboutHomeContent.java.in b/mobile/android/base/AboutHomeContent.java.in
> 
> >+        new SessionParser() {
> 
> >+        }.parse(jsonString);
> 
> I assume "parse" is  blocking call? Maybe you should add an "onFinishRead"
> call and put the "more" code in that? Then in the future, we could possibly
> move the code entirely to the background? If you think it's something to
> consider, file a new bug.

Filed bug 806619.
Leaving open for animations patch.
Whiteboard: startup-ux → startup-ux, [leave open]
This uses a different approach, which notifies the tabs manager that all of the initial tabs have been created. "Initial tabs" are those that the user expects to see immediately when Fennec is loaded (session restored tabs or the initial about:home tab after a clean start).
Attachment #672164 - Attachment is obsolete: true
Attachment #672164 - Flags: review?(mark.finkle)
Attachment #677232 - Flags: review?(mark.finkle)
Comment on attachment 677232 [details] [diff] [review]
Prevent animation of restored OOM tabs, v2

I hate being this way, but here are some other thoughts:
* Can we use mRestoringSession to help as a getter?
* Can we use the existing "Session:RestoreBegin" / "Session:RestoreEnd" handlers to notify RESTORED, if we really need to notify.
This uses the TabEvents.RESTORED message to signal that new tabs should be animated. RESTORED is a bit of a misnomer, though, since we're also using it after normal startups to prevent the initial about:home tab from being animated.
Attachment #677232 - Attachment is obsolete: true
Attachment #677232 - Flags: review?(mark.finkle)
Attachment #677478 - Flags: review?(mark.finkle)
Comment on attachment 677478 [details] [diff] [review]
Prevent animation of restored OOM tabs, v3

I like this better because it is more explicit. I think desktop uses the same idea. We always fire a "done with session restore" message, regardless of whether we really restore session or not. The consistency is good for other code that depends on the notification, like add-ons.
Attachment #677478 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/6cafbe512866
Whiteboard: startup-ux, [leave open] → startup-ux
https://hg.mozilla.org/mozilla-central/rev/6cafbe512866
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Depends on: 810732
Depends on: 819070
You need to log in before you can comment on or make changes to this bug.