Closed Bug 1345090 Opened 7 years ago Closed 7 years ago

Modify SessionStore to restore tabs with lazy-browsers

Categories

(Firefox :: Tabbed Browser, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 55
Performance Impact none
Tracking Status
relnote-firefox --- 55+
firefox55 --- fixed

People

(Reporter: u462496, Assigned: u462496)

References

(Depends on 1 open bug)

Details

(Keywords: memory-footprint, perf)

Attachments

(1 file, 17 obsolete files)

37.24 KB, patch
dao
: review+
Details | Diff | Splinter Review
Support bug for bug 906076.

This bug will explore the strategy for implementing lazy-browser tab restoring.
Blocks: lazytabs
Related: bug 1345098

One thing we will be keeping in mind is having data associated with the tab/browser which will serve as proxy for the lazy-browser's _browserBindingProperties, so that lazy-browsers do not unnecessarily get inserted into the document before their time.  __SS_extdata and TabStateCache data may already cover this need, but we will need to make sure all bases are covered.
Attached patch Level 1, draft 1 (obsolete) — Splinter Review
This is a first draft of the first level of restoring tabs lazily.

Dao, can you look this over and give your insights?

This patch will actually do a lazy restore (although the lazy browsers will be vulnerable to a host of consumers which may quickly insert the browsers after the restore.)
Assignee: nobody → kevinhowjones
Status: NEW → ASSIGNED
Attachment #8851834 - Flags: feedback?(dao+bmo)
Comment on attachment 8851834 [details] [diff] [review]
Level 1, draft 1

Note that I maintained the order of events in ssi_setTabState for first-level simplicity.  I suspect however that some of the lazy-browser conditional stuff can be combined.  Feel free to advise on that also.
Comment on attachment 8851834 [details] [diff] [review]
Level 1, draft 1

Review of attachment 8851834 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.xml
@@ +2075,5 @@
>                let setter;
>                switch (name) {
> +                // ADVISE: Need to better evaluate how to deal with browser
> +                // remoteness.
> +                case "isRemoteBrowser":

Hey - I think I might be able to help advise for isRemoteBrowser.

If my understanding is correct on how all of this lazy tab stuff works (the <xul:browser> exists, but hasn't yet been attached to the DOM), then you should be able to check for the "remote" attribute on the tab.linkedBrowser node. If that value is "true", then this browser _will_ become remote once it's inserted into the DOM.
Comment on attachment 8851834 [details] [diff] [review]
Level 1, draft 1

>             for (let i = 0; i < names.length; i++) {
>               let name = names[i];
>               let getter;
>               let setter;
>               switch (name) {
>+                // ADVISE: Need to better evaluate how to deal with browser
>+                // remoteness.
>+                case "isRemoteBrowser":
>+                  getter = () => {
>+                    return false;
>+                  };
>+                  setter = (value) => {
>+                    return value;
>+                  };
>+                  break;
>+                case "audioMuted":
>+                  getter = () => {
>+                    return false;
>+                  };
>+                  setter = (value) => {
>+                    return value;
>+                  };
>+                  break;

These are read-only so setters don't really make sense. You can just leave the setter as undefined.

>-    // keep the data around to prevent dataloss in case
>-    // a tab gets closed before it's been properly restored
>-    browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE;
>+    if (tab.linkedPanel) {
...
>+      // keep the data around to prevent dataloss in case
>+      // a tab gets closed before it's been properly restored
>+      browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE;
>+    }

Why this change?

>   increasePriority: function NP_BH_increasePriority(aBrowser) {
>+    let tabbrowser = aBrowser.ownerGlobal.gBrowser;
>+    if (!tabbrowser.getTabForBrowser(aBrowser).linkedPanel) {
>+      return;
>+    }

Does this get called again at a later time? We need to make sure lazy tabs don't get the wrong network priority when they get inserted.
(In reply to Dão Gottwald [::dao] from comment #5)
> Comment on attachment 8851834 [details] [diff] [review]
> 
> > ...
> 
> These are read-only so setters don't really make sense. You can just leave
> the setter as undefined.

I'm wondering about simply setting properties such as these with eg `browser.isRemoteBrowser =` construct and removing those properties from the _browserBindingProperties list.

> >-    // keep the data around to prevent dataloss in case
> >-    // a tab gets closed before it's been properly restored
> >-    browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE;
> >+    if (tab.linkedPanel) {
> ...
> >+      // keep the data around to prevent dataloss in case
> >+      // a tab gets closed before it's been properly restored
> >+      browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE;
> >+    }
> 
> Why this change?

restoreTab will get called again on the tab when the browser gets inserted.  If __SS_restoreState has been set, restoreTab will throw:

    NS_ASSERT(!tab.linkedBrowser.__SS_restoreState,
              "must reset tab before calling restoreTab()");

 
> >   increasePriority: function NP_BH_increasePriority(aBrowser) {
> >+    let tabbrowser = aBrowser.ownerGlobal.gBrowser;
> >+    if (!tabbrowser.getTabForBrowser(aBrowser).linkedPanel) {
> >+      return;
> >+    }
> 
> Does this get called again at a later time? We need to make sure lazy tabs
> don't get the wrong network priority when they get inserted.

I believe `TabBrowserInserted` and `TabSelect` handlers in NetworkPrioritizer will take care of that.
(In reply to Kevin Jones from comment #6)
> (In reply to Dão Gottwald [::dao] from comment #5)
> > Comment on attachment 8851834 [details] [diff] [review]
> > 
> > > ...
> > 
> > These are read-only so setters don't really make sense. You can just leave
> > the setter as undefined.
> 
> I'm wondering about simply setting properties such as these with eg
> `browser.isRemoteBrowser =` construct and removing those properties from the
> _browserBindingProperties list.

This would however make these properties writable rather than read-only.

> > >-    // keep the data around to prevent dataloss in case
> > >-    // a tab gets closed before it's been properly restored
> > >-    browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE;
> > >+    if (tab.linkedPanel) {
> > ...
> > >+      // keep the data around to prevent dataloss in case
> > >+      // a tab gets closed before it's been properly restored
> > >+      browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE;
> > >+    }
> > 
> > Why this change?
> 
> restoreTab will get called again on the tab when the browser gets inserted. 
> If __SS_restoreState has been set, restoreTab will throw:
> 
>     NS_ASSERT(!tab.linkedBrowser.__SS_restoreState,
>               "must reset tab before calling restoreTab()");

So is "keep the data around to prevent dataloss in case a tab gets closed before it's been properly restored" not a concern here? If so, we'll need to clarify that comment.

> > >   increasePriority: function NP_BH_increasePriority(aBrowser) {
> > >+    let tabbrowser = aBrowser.ownerGlobal.gBrowser;
> > >+    if (!tabbrowser.getTabForBrowser(aBrowser).linkedPanel) {
> > >+      return;
> > >+    }
> > 
> > Does this get called again at a later time? We need to make sure lazy tabs
> > don't get the wrong network priority when they get inserted.
> 
> I believe `TabBrowserInserted` and `TabSelect` handlers in
> NetworkPrioritizer will take care of that.

Should probably add a comment for this too.
(In reply to Dão Gottwald [::dao] from comment #7)
> So is "keep the data around to prevent dataloss in case a tab gets closed
> before it's been properly restored" not a concern here? If so, we'll need to
> clarify that comment.

To be honest, I don't really understand this comment anyway in terms of preventing data loss.  If __SS_restoreState is set to TAB_STATE_NEEDS_RESTORE, the TabRemove handler simply resets the restoring state.

IAE, I believe there would not be a concern for lazy tabs, as there shouldn't be any data change until the browser is inserted and the tab gets used anyway.
(In reply to Kevin Jones from comment #8)
> (In reply to Dão Gottwald [::dao] from comment #7)
> > So is "keep the data around to prevent dataloss in case a tab gets closed
> > before it's been properly restored" not a concern here? If so, we'll need to
> > clarify that comment.
> 
> To be honest, I don't really understand this comment anyway in terms of
> preventing data loss.  If __SS_restoreState is set to
> TAB_STATE_NEEDS_RESTORE, the TabRemove handler simply resets the restoring
> state.
> 
> IAE, I believe there would not be a concern for lazy tabs, as there
> shouldn't be any data change until the browser is inserted and the tab gets
> used anyway.

Mike, can you chime in? Should we remove this code or does it serve a purpose, and should we worry about that purpose for "lazy" tabs that don't have an active browser attached (as opposed to "pending" tabs that have a browser but just don't use it)?
Flags: needinfo?(mdeboer)
Attached patch 1345090_patch_V3.exp.02.diff (obsolete) — Splinter Review
Rebased for recent change in ssi_restoreTab.

Also added additional code to further prevent premature insertion of lazy browsers.  This patch should be able to restore tabs lazily, and remain lazy through shutdown.  Though caveats of first patch regarding unexpected browser insertion still apply.

Fixed a few bugs, added diagnostic messages, added pref sniffing to determine whether to restore lazily or not.

Create bool pref:

browser.sessionstore.restore_tabs_lazily

Set to true for lazy restore, false for normal restore.
Attachment #8851834 - Attachment is obsolete: true
Attachment #8851834 - Flags: feedback?(dao+bmo)
Attachment #8852675 - Flags: feedback?(dao+bmo)
Comment on attachment 8852675 [details] [diff] [review]
1345090_patch_V3.exp.02.diff

I did some quick tests comparing lazy restore with normal restore.  Single window restore, time from when window first appears to when UI become fully responsive:

330 tabs:

normal: 11s
lazy: 1.5s

502 tabs:

normal: 24s
lazy: 2.4s

1008 tabs:

normal: 85s (with fans whirring)
lazy: 5.3s

I observed with the lazy restores, once the tabs appeared in the tabs bar, the UI was fully responsive.

Tests performed on Mac OS, Nightly build 20170317213149 (2017-03-28)
(In reply to Kevin Jones from comment #11)
> 330 tabs:
> lazy: 1.5s
> 
> 502 tabs:
> lazy: 2.4s
> 
> 1008 tabs:
> lazy: 5.3s

Now you've done it - you made my eyes all teary! Please finish this work! It's awesome!!
(In reply to Kevin Jones from comment #8)
> To be honest, I don't really understand this comment anyway in terms of
> preventing data loss.  If __SS_restoreState is set to
> TAB_STATE_NEEDS_RESTORE, the TabRemove handler simply resets the restoring
> state.
> 
> IAE, I believe there would not be a concern for lazy tabs, as there
> shouldn't be any data change until the browser is inserted and the tab gets
> used anyway.

I thought it was a concern, but then I looked further: __SS_restoreState is an internal flag used to keep track of the state whilst we're passing things around between the chrome and content processes. The data loss comment is about when the content process crashes inadvertently whilst restoring - might happen more often than you think, because session restore is also our safe-guard when things crash and need to be restored.
(Feel free to update the comment to make things a bit clearer. You might even get away with moving it closer to the `sendAsyncMessage("SessionStore:restoreHistory")` call.)

In other words, this should be the right way to go - certainly if restoreTab() is called again when the tab is selected and the browser linked/ instantiated.
Flags: needinfo?(mdeboer)
Comment on attachment 8852675 [details] [diff] [review]
1345090_patch_V3.exp.02.diff

Mike, I don't know what all areas of the browser you are proficient in, but if you wouldn't mind giving this patch a look and sharing your thoughts it would be much appreciated.

Note there are some comments that you and Dao suggested that I haven't updated yet.
Attachment #8852675 - Flags: feedback?(mdeboer)
Comment on attachment 8852675 [details] [diff] [review]
1345090_patch_V3.exp.02.diff

>               switch (name) {
>+                // ADVISE: Need to better evaluate how to deal with browser
>+                // remoteness.
>+                case "isRemoteBrowser":
>+                  getter = () => {
>+                    return false;

As mconley said, reusing the 'remote' attribute is probably the sanest thing we can do here.

>+                  };
>+                  setter = undefined;

It's already 'undefined' here, so you can drop this line.

>+                  break;
>+                case "audioMuted":
>+                  getter = () => {
>+                    return false;
>+                  };
>+                  setter = undefined;

ditto

>+                case "currentURI":
>+                  getter = () => {
>+                    return aTab.currentURI;
>+                  };
>+                  setter = (value) => {
>+                    aTab.currentURI = value;
>+                    return value;
>+                  };
>+                  break;

This needs to be read-only too.

>-            let filter = this._tabFilters.get(tab);
>-            let listener = this._tabListeners.get(tab);
>-
>-            browser.webProgress.removeProgressListener(filter);
>-            filter.removeProgressListener(listener);
>-            listener.destroy();
>+
>+            if (tab.linkedPanel) {
>+              let filter = this._tabFilters.get(tab);
>+              let listener = this._tabListeners.get(tab);
>+
>+              browser.webProgress.removeProgressListener(filter);
>+              filter.removeProgressListener(listener);
>+              listener.destroy();
>+            }

You can just null-check filter before calling browser.webProgress.removeProgressListener, and null-check listener before calling filter.removeProgressListener and listener.destroy.

>+    let restoreTabsLazily = this._prefBranch.getPrefType("sessionstore.restore_tabs_lazily") &&
>+                              this._prefBranch.getBoolPref("sessionstore.restore_tabs_lazily");

Please add this pref in browser/app/profile/firefox.js and get rid of the getPrefType call.

>+    if (tab.linkedPanel) {
>+      // Start a new epoch to discard all frame script messages relating to a
>+      // previous epoch. All async messages that are still on their way to chrome
>+      // will be ignored and don't override any tab data set when restoring.
>+      epoch = this.startNextEpoch(browser);
>+      // keep the data around to prevent dataloss in case
>+      // a tab gets closed before it's been properly restored
>+      browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE;
>+    }

Please add back the blank line in the middle.

I also think we should clarify the comment about __SS_restoreState according to what :mikedeboer said.

>+    if (!tab.linkedPanel) {
>+      let url = "about:blank";
>+      if (activeIndex in tabData.entries) {
>+        url = tabData.entries[activeIndex].url;
>+      }
>+
>+      tab.currentURI = Services.io.newURI(url, null, null);
>+    }

I'm not a fan of introducing this property on tabs. How is currentURI currently handled on pending tabs?
Attachment #8852675 - Flags: feedback?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #15)
> Comment on attachment 8852675 [details] [diff] [review]
> 1345090_patch_V3.exp.02.diff
> 
> As mconley said, reusing the 'remote' attribute is probably the sanest thing
> we can do here.

To be clear, so you're saying?:

+                case "isRemoteBrowser":
+                  getter = () => {
+                    return browser.getAttribute("remote");

> >+                  };
> >+                  setter = undefined;
> 
> It's already 'undefined' here, so you can drop this line.

Ugh, don't know what I was thinking.

> >+    let restoreTabsLazily = this._prefBranch.getPrefType("sessionstore.restore_tabs_lazily") &&
> >+                              this._prefBranch.getBoolPref("sessionstore.restore_tabs_lazily");
> 
> Please add this pref in browser/app/profile/firefox.js and get rid of the
> getPrefType call.

I just put that in there for WIP testing, but if you think the pref should be part of the patch I'll do that.

> >+    if (!tab.linkedPanel) {
> >+      let url = "about:blank";
> >+      if (activeIndex in tabData.entries) {
> >+        url = tabData.entries[activeIndex].url;
> >+      }
> >+
> >+      tab.currentURI = Services.io.newURI(url, null, null);
> >+    }
> 
> I'm not a fan of introducing this property on tabs. How is currentURI
> currently handled on pending tabs?

I'm not sure the answer you are looking for here.  In the browser binding:

      <property name="currentURI" readonly="true">
       <getter><![CDATA[
          if (this.webNavigation) {
            return this.webNavigation.currentURI;
          }
          return null;
       ]]>
       </getter>
      </property>

I suppose rather than using a property on the tab, we could use a weak map?
Dao, we have been testing for browser laziness indirectly through `tab.linkedPanel`.  More and more I am finding the need to test for laziness in different parts of the codebase where there is not direct access to the tab and I have to use a construct like this:

    let tabbrowser = browser.ownerGlobal.gBrowser;
    if (!tabbrowser.getTabForBrowser(browser).linkedPanel) {

I would like to introduce a `browser.isInserted` property that gets set upon browser insertion for simplicity.  Would you be opposed to that?

    if (!browser.isInserted) {
Flags: needinfo?(dao+bmo)
(In reply to Kevin Jones from comment #16)
> (In reply to Dão Gottwald [::dao] from comment #15)
> > Comment on attachment 8852675 [details] [diff] [review]
> > 1345090_patch_V3.exp.02.diff
> > 
> > As mconley said, reusing the 'remote' attribute is probably the sanest thing
> > we can do here.
> 
> To be clear, so you're saying?:
> 
> +                case "isRemoteBrowser":
> +                  getter = () => {
> +                    return browser.getAttribute("remote");

this way:

return browser.getAttribute("remote") == "true";

> > >+    if (!tab.linkedPanel) {
> > >+      let url = "about:blank";
> > >+      if (activeIndex in tabData.entries) {
> > >+        url = tabData.entries[activeIndex].url;
> > >+      }
> > >+
> > >+      tab.currentURI = Services.io.newURI(url, null, null);
> > >+    }
> > 
> > I'm not a fan of introducing this property on tabs. How is currentURI
> > currently handled on pending tabs?
> 
> I'm not sure the answer you are looking for here.  In the browser binding:
> 
>       <property name="currentURI" readonly="true">
>        <getter><![CDATA[
>           if (this.webNavigation) {
>             return this.webNavigation.currentURI;
>           }
>           return null;
>        ]]>
>        </getter>
>       </property>
> 
> I suppose rather than using a property on the tab, we could use a weak map?

Pending tabs don't have the actual content loaded, so something is faking webNavigation.currentURI for them. Anyway, I think that's not directly reusable here. Instead, I think the getter could call some sessionstore API to get the to-be-restored URI.

(In reply to Kevin Jones from comment #17)
> Dao, we have been testing for browser laziness indirectly through
> `tab.linkedPanel`.  More and more I am finding the need to test for laziness
> in different parts of the codebase where there is not direct access to the
> tab and I have to use a construct like this:
> 
>     let tabbrowser = browser.ownerGlobal.gBrowser;
>     if (!tabbrowser.getTabForBrowser(browser).linkedPanel) {
> 
> I would like to introduce a `browser.isInserted` property that gets set upon
> browser insertion for simplicity.  Would you be opposed to that?
> 
>     if (!browser.isInserted) {

Can you use browser.isConencted? I forgot again in what case the browser would already be in the document without tab.linkedPanel being set when _insertBrowser is called.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #18)
> 
> ...Anyway, I think that's not directly
> reusable here. Instead, I think the getter could call some sessionstore API
> to get the to-be-restored URI.

eg, TabStateCache?
(In reply to Kevin Jones from comment #19)
> (In reply to Dão Gottwald [::dao] from comment #18)
> > 
> > ...Anyway, I think that's not directly
> > reusable here. Instead, I think the getter could call some sessionstore API
> > to get the to-be-restored URI.
> 
> eg, TabStateCache?

More like SessionStore.getTabValue (but that's for extension data).
Depends on: 1352183
Attached patch 1345090_patch_V6.exp.diff (obsolete) — Splinter Review
Here's my answer to comment 20.  Using extData didn't seem appropriate here.  I considered using getTabState, but it returns serialized data which would need to be parsed, seems like a bit overkill just to get a couple of values.  Instead I exposed `SessionStore.getLatentTabData`.  See what you think.

Added `contentTitle` trap to prevent more unwanted lazy browser instantiation.  

Added/updated comments as requested.

Added browser.sessionstore.restore_tabs_lazily pref in browser/app/profile/firefox.js, but I couldn't get it to take.  Maybe you can point out my error.  For WIP I've just hardwired to restore lazily.
Attachment #8852675 - Attachment is obsolete: true
Attachment #8852675 - Flags: feedback?(mdeboer)
Attachment #8853245 - Flags: feedback?(mdeboer)
Attachment #8853245 - Flags: feedback?(dao+bmo)
Comment on attachment 8853245 [details] [diff] [review]
1345090_patch_V6.exp.diff

>+  getLatentTabData: function ssi_getLatentTabData(aTab) {
>+    let { latentUrl, latentContentTitle } = TabStateCache.get(aTab.linkedBrowser);
>+    return { latentUrl, latentContentTitle };
>+  },

We might want a better name for this, and mirror how getTabValue works (with a key rather than returning and object).

Also not sure if we want to use TabStateCache for this internally, but I'll defer to Mike here.
Attachment #8853245 - Flags: feedback?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #22)
> >+  getLatentTabData: function ssi_getLatentTabData(aTab) {
> >+    let { latentUrl, latentContentTitle } = TabStateCache.get(aTab.linkedBrowser);
> >+    return { latentUrl, latentContentTitle };
> >+  },
> 
> We might want a better name for this...

I knew that was coming :-)  Can you make a suggestion?
Flags: needinfo?(dao+bmo)
Dao, do you think bug 1352157 should block this?
(In reply to Kevin Jones from comment #23)
> (In reply to Dão Gottwald [::dao] from comment #22)
> > >+  getLatentTabData: function ssi_getLatentTabData(aTab) {
> > >+    let { latentUrl, latentContentTitle } = TabStateCache.get(aTab.linkedBrowser);
> > >+    return { latentUrl, latentContentTitle };
> > >+  },
> > 
> > We might want a better name for this...
> 
> I knew that was coming :-)  Can you make a suggestion?

Maybe getBuiltinTabValue, or something like that. We need to somehow differentiate it from getTabValue.

(In reply to Kevin Jones from comment #24)
> Dao, do you think bug 1352157 should block this?

Seems like they can be worked on independently.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #25) 
> Seems like they can be worked on independently.

That's what I thought, thanks.  It seems like once bug 1352183 lands, with the functionality in this patch, it should be safe to land this.  I think any undesirable surprises have been covered, at least natively.
Attached patch 1345090_patch_V7.exp.diff (obsolete) — Splinter Review
(In reply to Dão Gottwald [::dao] from comment #22)
> Comment on attachment 8853245 [details] [diff] [review]
> 1345090_patch_V6.exp.diff
> 
> >+  getLatentTabData: function ssi_getLatentTabData(aTab) {
> >+    let { latentUrl, latentContentTitle } = TabStateCache.get(aTab.linkedBrowser);
> >+    return { latentUrl, latentContentTitle };
> >+  },
> 
> We might want a better name for this, and mirror how getTabValue works (with
> a key rather than returning and object).
> 
> Also not sure if we want to use TabStateCache for this internally, but I'll
> defer to Mike here.

Is this more what you had in mind?
Attachment #8853245 - Attachment is obsolete: true
Attachment #8853245 - Flags: feedback?(mdeboer)
Attachment #8853482 - Flags: feedback?(dao+bmo)
Comment on attachment 8853482 [details] [diff] [review]
1345090_patch_V7.exp.diff

Yeah, roughly. __SS_builtin_data would need to be set regardless of tab.linkedPanel so that getBuiltinTabValue works consistently. Or we need to rename getBuiltinTabValue again to make it clear that it only exist for this very specific purpose. And then we should also remove __SS_builtin_data when it's not needed anymore as the data will go stale from that point on.

contentTitle should probably be just title so that SessionStore.jsm is internally consistent.
Attachment #8853482 - Flags: feedback?(dao+bmo) → feedback+
Attached patch 1345090_patch_V8.exp.diff (obsolete) — Splinter Review
Updated built-in lazy tab data API.

Original code in ssi_restoreTab is still in sequence.  I added some comments seeking advice on changing the sequence to reduce the number of `tab.linkedPanel` conditional blocks.
Attachment #8853482 - Attachment is obsolete: true
Attachment #8853725 - Flags: feedback?(dao+bmo)
Attachment #8853725 - Flags: feedback?(dao+bmo) → feedback?(mdeboer)
Comment on attachment 8853725 [details] [diff] [review]
1345090_patch_V8.exp.diff

Review of attachment 8853725 [details] [diff] [review]:
-----------------------------------------------------------------

Great stuff! I hope my largish comments don't scare you, though - but I wanted to give you all the info needed to get this patch into reviewable state.

Thanks for working on this!

::: browser/app/profile/firefox.js
@@ +1591,5 @@
>  pref("browser.formautofill.experimental", false);
>  pref("browser.formautofill.enabled", false);
>  pref("browser.formautofill.loglevel", "Warn");
>  
> +// Whether or not to restore a session with lazy-browser tabs

nit: missing dot.

::: browser/base/content/tabbrowser.xml
@@ +2114,5 @@
>                      return browser[name];
>                    };
>                    setter = (value) => {
>                      this._insertBrowser(aTab);
>                      return browser[name] = value;

May I recommend rewriting this to the following:

```js
let browser = aTab.linkedBrowser;
let getterSetterMap = {
  isRemoteBrowser: {
    get() { return browser.getAttribute("remote") == "true"; }
  },
  audioMuted: {
    get() { return false; }
  },
  permitUnload: {
    get() {
      return function() {
        return { permitUnload: true, timedOut: false };
      }
    }
  },
  currentURI: {
    get() { 
      let url = SessionStore.getBuiltinLazyTabValue(aTab, "url");
      return Services.io.newURI(url, null, null);
    }
  },
  contentTitle: {
    get() { return SessionStore.getBuiltinLazyTabValue(aTab, "title"); }
  },
  other: {
    get() {
      this._insertBrowser(aTab);
      return browser[name];
    },
    set() {
      this._insertBrowser(aTab);
      return browser[name] = value;
    }
  }
};

for (let name of this._browserBindingProperties) {
  let {get, set} = getterSetterMap[name] || getterSetterMap.other;
  Object.defineProperty(browser, name, {
    get, set,
    configurable: true,
    enumerable: true
  });
}
```

What do you think?

@@ +7387,5 @@
>          <parameter name="aMuteReason"/>
>          <body>
>          <![CDATA[
> +          // Do not attempt to toggle mute state if browser is lazy.
> +          if (!this.linkedPanel) {

I agree that at a certain point you might want to have a method dedicated to this feature and move the inferring of the tab state in there.
This way `x.linkedPanel` combined with a comment will be all over the place, whereas a single self-explanatory line of code would do the trick as well.
So please feel free to pursue your idea here!

::: browser/components/sessionstore/SessionStore.jsm
@@ +2533,5 @@
>      }
>    },
>  
> +  getBuiltinLazyTabValue: function ssi_getBuiltinLazyTabValue(aTab, aKey) {
> +    // Builtin lazy data only exists for lazy-browser tabs, otherwise

Please make this a proper JSDoc comment above the method definition. Also, new functions that you add here don't need to follow the old, obsolete signature [[name]: function [ssi_displayName]([[aArg1], [...aArgn]])], but in this case just like this:

```js
getBuiltInLazyTabValue(tab, key) {
  return (tab.__SS_builtin_lazy_data || {})[key];
},
```

nit: I'd advise the name 'getLazyTabValue'. I'm not sure what 'built-in' means or adds.

@@ +3548,5 @@
>                                   tabbrowser.selectedBrowser == browser ||
>                                   loadArguments;
>  
> +    // REVIEWER ADVISE: Can this be nested in final `tab.linkedPanel`
> +    // conditional block?[1]

No. I would like you to store 'tab.linkedBrowser' (or whatever method call result you end up with) in a local variable that is more explanatory, like 'let browserAlreadyInserted = ...' or something.

@@ +3620,5 @@
>      // Save the index in case we updated it above.
>      tabData.index = activeIndex + 1;
>  
> +    // REVIEWER ADVISE: Can this be nested in final `tab.linkedPanel`
> +    // conditional block?[1]

Everything that is done in order to collect data for the `sendAsyncMessage` call can be moved to the same conditional.

@@ +3686,5 @@
>        TabAttributes.set(tab, tabData.attributes);
>      }
>  
> +    // REVIEWER ADVISE:[1]
> +    // Only attempt to restore tab contents if browser is not lazy.

I agree with this comment, but if you move the entire block in here the restore operation will be halted! Have you tested this IRL?

@@ +3689,5 @@
> +    // REVIEWER ADVISE:[1]
> +    // Only attempt to restore tab contents if browser is not lazy.
> +    if (tab.linkedPanel) {
> +      browser.messageManager.sendAsyncMessage("SessionStore:restoreHistory",
> +                                              {tabData, epoch, loadArguments});

So this call will move to the unified conditional block for 'browserAlreadyInserted' that you will add just above here.

@@ +3693,5 @@
> +                                              {tabData, epoch, loadArguments});
> +
> +      // This could cause us to ignore MAX_CONCURRENT_TAB_RESTORES a bit, but
> +      // it ensures each window will have its selected tab loaded.
> +      if (willRestoreImmediately) {

So this will become: `if (browserAlreadyInserted && willRestoreImmediately) {` so that the else case is always invoked!

::: browser/components/sessionstore/TabStateFlusher.jsm
@@ +108,5 @@
>    flushWindow(window) {
> +    let promises = [];
> +    for (let browser of window.gBrowser.browsers) {
> +      let tabbrowser = browser.ownerGlobal.gBrowser;
> +      if (tabbrowser.getTabForBrowser(browser).linkedPanel) {

Again the linkedPanel :-/ I'm getting more and more enthusiastic about a dedicated method for this!

::: browser/modules/NetworkPrioritizer.jsm
@@ +94,5 @@
>    increasePriority: function NP_BH_increasePriority(aBrowser) {
> +    // Ignore if browser is lazy.  Once the browser is instantiated, this will
> +    // get taken care of by TabBrowserInserted and TabSelect handlers.
> +    let tabbrowser = aBrowser.ownerGlobal.gBrowser;
> +    if (!tabbrowser.getTabForBrowser(aBrowser).linkedPanel) {

I think you know what I was going to say here ;-)
Attachment #8853725 - Flags: feedback?(mdeboer) → feedback+
(In reply to Mike de Boer [:mikedeboer] from comment #30)
> Comment on attachment 8853725 [details] [diff] [review]
> 1345090_patch_V8.exp.diff
> 
> Review of attachment 8853725 [details] [diff] [review]:
> -----------------------------------------------------------------
> May I recommend rewriting this to the following:
> ...
> What do you think?

Yes, I like it.

> @@ +7387,5 @@
> >          <parameter name="aMuteReason"/>
> >          <body>
> >          <![CDATA[
> > +          // Do not attempt to toggle mute state if browser is lazy.
> > +          if (!this.linkedPanel) {
> 
> I agree that at a certain point you might want to have a method dedicated to
> this feature and move the inferring of the tab state in there.
> This way `x.linkedPanel` combined with a comment will be all over the place,
> whereas a single self-explanatory line of code would do the trick as well.
> So please feel free to pursue your idea here!

So you are saying something like:

browser.isInserted

?

> Everything that is done in order to collect data for the `sendAsyncMessage`
> call can be moved to the same conditional.

To be clear, you are only talking about the stuff that is already in exclusive blocks, correct?  For example, the TabState data would still need to be set even for lazy browsers, because that is from where we collect up the data again when we finally restore the tab once it is inserted.  (See onTabBrowserInserted)

> @@ +3686,5 @@
> >        TabAttributes.set(tab, tabData.attributes);
> >      }
> >  
> > +    // REVIEWER ADVISE:[1]
> > +    // Only attempt to restore tab contents if browser is not lazy.
> 
> I agree with this comment, but if you move the entire block in here the
> restore operation will be halted! Have you tested this IRL?

Actually, yes I have.  It seems to work fine, I am even using it in my personal profiles.  But if there is some edge case I am not seeing that you are anticipating, I wanted to know about it.

> @@ +3689,5 @@
> > +    // REVIEWER ADVISE:[1]
> > +    // Only attempt to restore tab contents if browser is not lazy.
> > +    if (tab.linkedPanel) {
> > +      browser.messageManager.sendAsyncMessage("SessionStore:restoreHistory",
> > +                                              {tabData, epoch, loadArguments});
> 
> So this call will move to the unified conditional block for
> 'browserAlreadyInserted' that you will add just above here.

I am not entirely clear on what you are wanting, but it seems to me what you are saying essentially is to move the `browser.messageManager.sendAsyncMessage(...)` call up to the middle block?  So it would end up something like this:

    let browserInserted = ...

    if (browserInserted) {
      if (!willRestoreImmediately && !forceOnDemand) {
        TabRestoreQueue.add(tab);
      }

      // Start a new epoch to discard all frame script messages relating to a
      // previous epoch. All async messages that are still on their way to chrome
      // will be ignored and don't override any tab data set when restoring.
      let epoch = this.startNextEpoch(browser);

      // Ensure that the tab will get properly restored in the event the tab
      // crashes while restoring.  But don't set this on lazy browsers as
      // restoreTab will get called again when the browser is instantiated.
      browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE;

      browser.messageManager.sendAsyncMessage("SessionStore:restoreHistory",
                                              {tabData, epoch, loadArguments});

    }

    browser.setAttribute("pending", "true");
    tab.setAttribute("pending", "true");

    .......
    
    // Restore tab attributes.
    if ("attributes" in tabData) {
      TabAttributes.set(tab, tabData.attributes);
    }

    // This could cause us to ignore MAX_CONCURRENT_TAB_RESTORES a bit, but
    // it ensures each window will have its selected tab loaded.
    if (browserInserted && willRestoreImmediately) {
      this.restoreTabContent(tab, loadArguments, reloadInFreshProcess,
                             restoreContentReason);
    } else if (!forceOnDemand) {
      this.restoreNextTab();
    }

The question I have is that with this construct we are sending the message before the data gets collected.  This seems to work okay, though I assume only because the following code runs before the message gets received.  However, I am guessing that you were meaning in your earlier comment (putting the data collection in the exclusive block) that would have proceeded the message propagation within the block.

So when you said that by moving the message sending code to the end the entire restore operation would be halted, I am wondering which part of the code needs to run after sending the message, which would prevent this?

To be clear, this is how the code would end up looking with my proposal (And again, I have tested this and it seems to work fine)

    // Restore tab attributes.
    if ("attributes" in tabData) {
      TabAttributes.set(tab, tabData.attributes);
    }

    // Only attempt to restore tab contents if browser is not lazy.
    if (browserInserted) {
      TabRestoreQueue.add(tab);

      // Start a new epoch to discard all frame script messages relating to a
      // previous epoch. All async messages that are still on their way to chrome
      // will be ignored and don't override any tab data set when restoring.
      let epoch = this.startNextEpoch(browser);

      // Ensure that the tab will get properly restored in the event the tab
      // crashes while restoring.  But don't set this on lazy browsers as
      // restoreTab will get called again when the browser is instantiated.
      browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE;

      browser.messageManager.sendAsyncMessage("SessionStore:restoreHistory",
                                              {tabData, epoch, loadArguments});

      // This could cause us to ignore MAX_CONCURRENT_TAB_RESTORES a bit, but
      // it ensures each window will have its selected tab loaded.
      if (willRestoreImmediately) {
        this.restoreTabContent(tab, loadArguments, reloadInFreshProcess,
                               restoreContentReason);
      } else if (!forceOnDemand) {
        this.restoreNextTab();
      }
    }

    // Decrease the busy state counter after we're done.
    this._setWindowStateReady(window);

(Sorry if my questions are hard to follow, but I'm sure we'll get to it.)

> Again the linkedPanel :-/ I'm getting more and more enthusiastic about a
> dedicated method for this!

Well, you all know I'm in agreement with that :-)
Flags: needinfo?(mdeboer)
(In reply to Kevin Jones from comment #31)
> So you are saying something like:
> 
> browser.isInserted
> 
> ?
> 

Or `tab.isBrowserInserted` or `gBrowser.isBrowserInserted(tab)`, whatever is most convenient to use across your project!

> To be clear, you are only talking about the stuff that is already in
> exclusive blocks, correct? 

Correct.

> Actually, yes I have.  It seems to work fine, I am even using it in my
> personal profiles.  But if there is some edge case I am not seeing that you
> are anticipating, I wanted to know about it.

Yes, for example when restore_on_demand = false and/ or when restore_pinned_tabs_on_demand = true.

> The question I have is that with this construct we are sending the message
> before the data gets collected.  This seems to work okay, though I assume
> only because the following code runs before the message gets received. 
> However, I am guessing that you were meaning in your earlier comment
> (putting the data collection in the exclusive block) that would have
> proceeded the message propagation within the block.
> 
> So when you said that by moving the message sending code to the end the
> entire restore operation would be halted, I am wondering which part of the
> code needs to run after sending the message, which would prevent this?

No, I didn't mean to have you focus on the `sendAsyncMessage` statement, but on its place in the `restoreTab()` method. I'd like you to cluster as much lazy tab specific logic together, without interrupting the flow and order of the method as it is now.

> To be clear, this is how the code would end up looking with my proposal (And
> again, I have tested this and it seems to work fine)

OK, allow me to come up with yet another proposal: :-P

```js
let willRestoreImmediately = // ... we start here.
let isBrowserInserted = tab.isBrowserInserted;

// The TabRestoreQueue.add(tab); code sitting here is confusing and unnecessary. Let's move it out of the way.
// Instead, the code will continue here with:

// Increase the busy state counter before modifying the tab.
this._setWindowStateBusy(window);

// It's important to set the window state to dirty so that
// we collect their data for the first time when saving state.
DirtyWindows.add(window);

// blahblah, stays the same.
// We move the `let epoch =` and other statement you put into a conditional, continuing with:

browser.setAttribute("pending", "true");
tab.setAttribute("pending", "true");

// If we're restoring this tab, it certainly shouldn't be in
// the ignored set anymore.
this._crashedBrowsers.delete(browser.permanentKey);

// Update the persistent tab state cache with |tabData| information.
TabStateCache.update(browser, {
  // blahblah...
});

// We move the `sendAsyncMessage` call to below.

// Update tab label and icon to show something
// while we wait for the messages to be processed.
this.updateTabLabelAndIcon(tab, tabData);

// Restore tab attributes.
if ("attributes" in tabData) {
  TabAttributes.set(tab, tabData.attributes);
}

if (isBrowserInserted) {
  // Start a new epoch to discard all frame script messages relating to a
  // previous epoch. All async messages that are still on their way to chrome
  // will be ignored and don't override any tab data set when restoring.
  let epoch = this.startNextEpoch(browser);

  // keep the data around to prevent dataloss in case
  // a tab gets closed before it's been properly restored
  browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE;

  browser.messageManager.sendAsyncMessage("SessionStore:restoreHistory",
                                          {tabData, epoch, loadArguments});

  // This could cause us to ignore MAX_CONCURRENT_TAB_RESTORES a bit, but
  // it ensures each window will have its selected tab loaded.
  if (willRestoreImmediately) {
    this.restoreTabContent(tab, loadArguments, reloadInFreshProcess,
                           restoreContentReason);
  } else if (!forceOnDemand) {
    TabRestoreQueue.add(tab);
    this.restoreNextTab();
  }
} else {
  // __SS_builtin_lazy_data holds data for lazy-browser tabs to proxy for
  // data unobtainable from the unbound browser.  This only applies to lazy
  // browsers and will be removed once the browser is inserted in the document.
  // This must preceed `updateTabLabelAndIcon` call for required data to be present.
  let url = "about:blank";
  let title = "";
  if (activeIndex in tabData.entries) {
    url = tabData.entries[activeIndex].url;
    title = tabData.entries[activeIndex].title || url;
  }
  tab.__SS_builtin_lazy_data = { url, title };
}

// Decrease the busy state counter after we're done.
this._setWindowStateReady(window);
```

How does this look?

> (Sorry if my questions are hard to follow, but I'm sure we'll get to it.)

They're ok! Most important thing is we can get towards a mutual understanding.
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #32)
> (In reply to Kevin Jones from comment #31)
> > So you are saying something like:
> > 
> > browser.isInserted
> > 
> > ?
> > 
> 
> Or `tab.isBrowserInserted` or `gBrowser.isBrowserInserted(tab)`, whatever is
> most convenient to use across your project!

Not in this bug. See the earlier discussion on isConnected.
comment 11 seems to suggest that this is going to have a heck of an impact on session restoration for heavy tab users.
Whiteboard: [qf]
(In reply to Dão Gottwald [::dao] from comment #33)
> Not in this bug. See the earlier discussion on isConnected.

Okidoke. My 2cts: `isConnected` sounds to me like the state captured of a network connection. 'IsInserted' sounds closer to me, but I'd like to steer clear from further bikeshedding today :-)
(In reply to Mike de Boer [:mikedeboer] from comment #32)
> OK, allow me to come up with yet another proposal: :-P
> 
> ```js
> let willRestoreImmediately = // ... we start here.
> let isBrowserInserted = tab.isBrowserInserted;
> 
> ....
> 
> // Decrease the busy state counter after we're done.
> this._setWindowStateReady(window);
> ```
> 
> How does this look?

This looks fine with one exception - you moved the lazy-browser-specific code to the end as an `else` for the restore code.
    
This code must run before the `this.updateTabLabelAndIcon(tab, tabData)` call, or it will fail on lazy browsers.

So we need to move `this.updateTabLabelAndIcon(tab, tabData)` to just after the block:

    } else {
      // __SS_builtin_lazy_data holds data for lazy-browser tabs to proxy for
      // data unobtainable from the unbound browser.  This only applies to lazy
      // browsers and will be removed once the browser is inserted in the document.
      // This must preceed `updateTabLabelAndIcon` call for required data to be present.
      let url = "about:blank";
      let title = "";

      if (activeIndex in tabData.entries) {
        url = tabData.entries[activeIndex].url;
        title = tabData.entries[activeIndex].title || url;
      }
      tab.__SS_builtin_lazy_data = { url, title };
    }

    // Update tab label and icon to show something
    // while we wait for the messages to be processed.
    this.updateTabLabelAndIcon(tab, tabData);

    // Decrease the busy state counter after we're done.
    this._setWindowStateReady(window);


I ran a quick test with that modification, and it seems to work fine.
comment 36
Flags: needinfo?(mdeboer)
(In reply to Kevin Jones from comment #36)
> I ran a quick test with that modification, and it seems to work fine.

Fine by me.
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #35)
> (In reply to Dão Gottwald [::dao] from comment #33)
> > Not in this bug. See the earlier discussion on isConnected.
> 
> Okidoke. My 2cts: `isConnected` sounds to me like the state captured of a
> network connection. 'IsInserted' sounds closer to me, but I'd like to steer
> clear from further bikeshedding today :-)

I truly hope I'm not falling into the bike-shedding category here, but before this comment was posted I was thinking the very same thing, and was thinking that this is the reason that when I tried using it as a test for browser laziness, that it was not reliable[1][2].  Dao, is it possible that this is the case, and that it is not really a bug at all?[2]  It seems like there just may truly be a valid reason not to use it as a test for laziness.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1352183#c3
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1352183#c4
Comment 39 (I seem to keep forgetting to needinfo today...)
Flags: needinfo?(dao+bmo)
Who should I ask to review this next patch?
Flags: needinfo?(mdeboer)
Dao, I sent you a private email with questions regarding frameLoader, did you receive that?  I didn't want to pollute the bug with those questions.
(In reply to Kevin Jones from comment #41)
> Who should I ask to review this next patch?

I see there are other issues that will need to be dealt with in this bug, eg, tooltips are not created properly on lazy-browser tabs.  So I will include that in the next patch.
See Also: → 1351677
(In reply to Mike de Boer [:mikedeboer] from comment #30)
> Comment on attachment 8853725 [details] [diff] [review]
> 1345090_patch_V8.exp.diff
> 
> Review of attachment 8853725 [details] [diff] [review]:
> -----------------------------------------------------------------

> May I recommend rewriting this to the following:
> 
> ```js
> let browser = aTab.linkedBrowser;
> let getterSetterMap = {
> 
> ....
> 
>   other: {
>     get() {
>       this._insertBrowser(aTab);
>       return browser[name];
>     },
>     set() {
>       this._insertBrowser(aTab);
>       return browser[name] = value;
>     }
>   }
> };

this._insertBrowser needs to be gBrowser._insertBrowser in this case.
Dao:

(In reply to Mike de Boer [:mikedeboer] from comment #30)
> 
> ```js
> getBuiltInLazyTabValue(tab, key) {
>   return (tab.__SS_builtin_lazy_data || {})[key];
> },
> ```
> 
> nit: I'd advise the name 'getLazyTabValue'. I'm not sure what 'built-in'
> means or adds.

Are you okay with `getLazyTabValue`?
(In reply to Kevin Jones from comment #43)
> (In reply to Kevin Jones from comment #41)
> > Who should I ask to review this next patch?
> 
> I see there are other issues that will need to be dealt with in this bug,
> eg, tooltips are not created properly on lazy-browser tabs.  So I will
> include that in the next patch.

Dao, Mike de Boer, can you guys tell me where you hang out and your IRC nicks, and what times you are available?  I'd like to discuss some stuff with you.  (I'm on the other side of the world so I'll have to keep up with you).

I'm sometimes getting calls that want a frameLoader, but on lazy tabs frameLoader is null.  I'm trying to find a strategy to deal with this without just forcing browser instantiation if frameLoader is called.  I have a few things in mind and I'd like to talk them over.

One of the culprits involves "browser-delayed-startup-finished" being sent and a very complicated sequence that takes place with devtools.  It is weird because it doesn't show up on most of the profiles I've tested with, but it occurs on one (even though all addons are disabled).

I don't believe we need to eliminate every single edge case that might insert browsers unnecessarily in order to land this, but I would like to try to take care of cases that that may be likely to cause mass-instantiation unexpectedly.

Anyway, if we could talk about these things it would be helpful.
Dao, Mike,

I've left a lot of n'info comments so just want to make sure they don't get missed:

Dao, comment 39, comment 41, comment 45, comment 46
Mike, comment 39, comment 41, comment 44, comment 46
(In reply to Kevin Jones from comment #39)
> (In reply to Mike de Boer [:mikedeboer] from comment #35)
> > (In reply to Dão Gottwald [::dao] from comment #33)
> > > Not in this bug. See the earlier discussion on isConnected.
> > 
> > Okidoke. My 2cts: `isConnected` sounds to me like the state captured of a
> > network connection. 'IsInserted' sounds closer to me, but I'd like to steer
> > clear from further bikeshedding today :-)
> 
> I truly hope I'm not falling into the bike-shedding category here, but
> before this comment was posted I was thinking the very same thing, and was
> thinking that this is the reason that when I tried using it as a test for
> browser laziness, that it was not reliable[1][2].  Dao, is it possible that
> this is the case, and that it is not really a bug at all?[2]  It seems like
> there just may truly be a valid reason not to use it as a test for laziness.
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1352183#c3
> [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1352183#c4

isConnected is a (poorly named) standard DOM property and doesn't have anything to do with the network status.

(In reply to Kevin Jones from comment #45)
> Dao:
> 
> (In reply to Mike de Boer [:mikedeboer] from comment #30)
> > 
> > ```js
> > getBuiltInLazyTabValue(tab, key) {
> >   return (tab.__SS_builtin_lazy_data || {})[key];
> > },
> > ```
> > 
> > nit: I'd advise the name 'getLazyTabValue'. I'm not sure what 'built-in'
> > means or adds.
> 
> Are you okay with `getLazyTabValue`?

I think Mike owns sessionstore these days, so if he's fine with that...

(In reply to Kevin Jones from comment #42)
> Dao, I sent you a private email with questions regarding frameLoader, did
> you receive that?  I didn't want to pollute the bug with those questions.

Received it but didn't get to it yet. I would generally recommend putting these kind of things in bugs so that you don't necessarily depend on one single person responding. Technical questions don't pollute bugs, it's what bugzilla is there for.

(In reply to Kevin Jones from comment #46)
> Dao, Mike de Boer, can you guys tell me where you hang out and your IRC
> nicks, and what times you are available?  I'd like to discuss some stuff
> with you.  (I'm on the other side of the world so I'll have to keep up with
> you).

Mike (mikedeboer) and I (dao) are in Central Europe, so I guess your morning would overlap with our evening.

> I don't believe we need to eliminate every single edge case that might
> insert browsers unnecessarily in order to land this, but I would like to try
> to take care of cases that that may be likely to cause mass-instantiation
> unexpectedly.

I would recommend keeping this bug focused on sessionstore and filing separate bugs for other stuff.
Flags: needinfo?(dao+bmo)
(In reply to Kevin Jones from comment #41)
> Who should I ask to review this next patch?

You can ask both of us.
(In reply to Kevin Jones from comment #44)
> this._insertBrowser needs to be gBrowser._insertBrowser in this case.

Or you can use .bind(this) on both the `get()` and `set()` definitions.
Flags: needinfo?(mdeboer)
(In reply to Kevin Jones from comment #31)
> (In reply to Mike de Boer [:mikedeboer] from comment #30)
> > Comment on attachment 8853725 [details] [diff] [review]
> > 1345090_patch_V8.exp.diff
> > 
> > Review of attachment 8853725 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > May I recommend rewriting this to the following:
> > ...
> > What do you think?
> 
> Yes, I like it.

I forgot this would be changing existing code.  I think I will keep things the way they are right now so this doesn't get too stirred up, and maybe when the dust settles change that in another bug.
Attached patch 1345090_patch_V10.diff (obsolete) — Splinter Review
Updated per comments.

I also included the devtools fix and tooltip fix in this patch that was discussed on IRC this morning.  The changes were very trivial and I didn't want to start a new blocking bug for this.  I hope that was okay.  I manually tested the devtools fix according to ochameau's recommendations on IRC this morning.

I used browser.frameLoader as a test in some places where it was more convenient than getting a tab.linkedPanel.

Added "frameLoader" to _browserBindingProperties to ensure breakage doens't occur on unforseen cases.

I'm assuming then we want lazy-browser restore pref default to be false?  No one mentioned on the last patch.
Attachment #8853725 - Attachment is obsolete: true
Attachment #8854928 - Flags: review?(mdeboer)
Attachment #8854928 - Flags: review?(dao+bmo)
Whiteboard: [qf] → [qf-]
(In reply to Kevin Jones from comment #53)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=67ceb507eba9c1d8237ba51380ca73ef7f9a2738&selectedJob=8
> 8911430
> 
> There are a couple of eslint errors.

I am unable to reproduce any of the test failures except these 3:

browser_UsageTelemetry_content.js
browser_urlBarHashChangeProxyState.js
browser_newtab_bug1178586.js
Attachment #8854928 - Attachment is obsolete: true
Attachment #8854928 - Flags: review?(mdeboer)
Attachment #8854928 - Flags: review?(dao+bmo)
Okay, so I have figured out the deal with `isConnected`.  `isConnected` is true on `_preloadBrowser`s because of course they have actually been inserted into the document.

It __seems__ like it might be safe to use `isConnected` as a test for laziness, AFAIK once a _preloadBrowser is associated with a tab, will always undergo `_insertBrowser()`.

What do you think about this, Dao?
Flags: needinfo?(dao+bmo)
Sounds good.
Flags: needinfo?(dao+bmo)
Attached patch 1345090_patch_P2_V4.diff (obsolete) — Splinter Review
Fixed bug which was causing test failures.

Again, just wanting to make sure the load_tabs_lazily pref default is what we want.
Attachment #8855645 - Flags: review?(mdeboer)
Attachment #8855645 - Flags: review?(dao+bmo)
Comment on attachment 8855645 [details] [diff] [review]
1345090_patch_P2_V4.diff

>+// Whether or not to restore a session with lazy-browser tabs.
>+pref("browser.sessionstore.restore_tabs_lazily", false);

Why not enable this now?

>@@ -4872,17 +4892,18 @@
>               label = this.mStringBundle.getString(stringID);
>             }
>           } else {
>             label = tab.getAttribute("label");
>             if (AppConstants.E10S_TESTING_ONLY &&
>                 tab.linkedBrowser &&
>                 tab.linkedBrowser.isRemoteBrowser) {
>               label += " - e10s";
>-              if (Services.prefs.getIntPref("dom.ipc.processCount") > 1) {
>+              if (tab.linkedBrowser.frameLoader &&
>+                    Services.prefs.getIntPref("dom.ipc.processCount") > 1) {
>                 label += " (" + tab.linkedBrowser.frameLoader.tabParent.osPid + ")";
>               }

nit: reduce indent by two spaces in the if-condition's second line

>--- a/devtools/server/actors/webbrowser.js
>+++ b/devtools/server/actors/webbrowser.js
>@@ -350,17 +350,17 @@ BrowserTabList.prototype.getTab = functi
>     }
>     return promise.reject({
>       error: "noTab",
>       message: "Unable to find tab with outerWindowID '" + outerWindowID + "'"
>     });
>   } else if (typeof tabId == "number") {
>     // Tabs OOP
>     for (let browser of this._getBrowsers()) {
>-      if (browser.frameLoader.tabParent &&
>+      if (browser.frameLoader && browser.frameLoader.tabParent &&
>           browser.frameLoader.tabParent.tabId === tabId) {

nit: add a new line for the second if-condition

r=me on the non-sessionstore bits
Attachment #8855645 - Flags: review?(dao+bmo) → review+
Attached patch 1345090_patch_P2_V5.diff (obsolete) — Splinter Review
Updated per Dao's requests.
Attachment #8855645 - Attachment is obsolete: true
Attachment #8855645 - Flags: review?(mdeboer)
Attachment #8855753 - Flags: review?(mdeboer)
Comment on attachment 8855753 [details] [diff] [review]
1345090_patch_P2_V5.diff

Review of attachment 8855753 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! Thanks for incorporating all our requests!

I assume test coverage is going to be added somewhere, sometime?

::: browser/components/sessionstore/SessionStore.jsm
@@ +1870,5 @@
>      }
> +
> +    // Only restore if browser has been lazy.
> +    if (aTab.__SS_lazy_data && !browser.__SS_restoreState &&
> +          TabStateCache.get(browser)) {

Please keep this one a single line. 80chars is not a _hard_ limit in my book.

@@ +3287,5 @@
>        tabbrowser.moveTabTo(tabbrowser.selectedTab, newTabCount - 1);
>  
>      let numVisibleTabs = 0;
>  
> +    let restoreTabsLazily = this._prefBranch.getBoolPref("sessionstore.restore_tabs_lazily");

I didn't notice this before! Please move it to `_initPrefs` -->

```js
this._restore_tabs_lazily = this._prefBranch.getBoolPref("sessionstore.restore_tabs_lazily");

// createLazyBrowser: this._restore_tabs_lazily,
```

There's no need to add an observer.
Attachment #8855753 - Flags: review?(mdeboer) → review+
Attached patch 1345090_patch_P2_V6.diff (obsolete) — Splinter Review
Here you go.  AFAIC this is ready to land.
Attachment #8855753 - Attachment is obsolete: true
(In reply to Mike de Boer [:mikedeboer] from comment #61)
> Comment on attachment 8855753 [details] [diff] [review]
> 1345090_patch_P2_V5.diff
> 
> Review of attachment 8855753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I assume test coverage is going to be added somewhere, sometime?

Indeed.
Attached patch 1345090_patch_P2_V7.diff (obsolete) — Splinter Review
Oops, try this one...
Attachment #8855791 - Attachment is obsolete: true
(In reply to Dão Gottwald [::dao] from comment #59)
> Comment on attachment 8855645 [details] [diff] [review]
> 1345090_patch_P2_V4.diff
> 
> >+// Whether or not to restore a session with lazy-browser tabs.
> >+pref("browser.sessionstore.restore_tabs_lazily", false);
> 
> Why not enable this now?

Kevin?
Attached patch 1345090_patch_P2_V8.diff (obsolete) — Splinter Review
Arrrrrrrgggg....
Attachment #8855793 - Attachment is obsolete: true
(In reply to Dão Gottwald [::dao] from comment #65)
> (In reply to Dão Gottwald [::dao] from comment #59)
> > Comment on attachment 8855645 [details] [diff] [review]
> > 1345090_patch_P2_V4.diff
> > 
> > >+// Whether or not to restore a session with lazy-browser tabs.
> > >+pref("browser.sessionstore.restore_tabs_lazily", false);
> > 
> > Why not enable this now?
> 
> Kevin?

Oh, sorry, I read you comment in reverse.  Fixing....
Attached patch 1345090_patch_P2_V9.diff (obsolete) — Splinter Review
Reversed pref.
Attachment #8855794 - Attachment is obsolete: true
(In reply to Kevin Jones from comment #58)
> Comment on attachment 8855645 [details] [diff] [review]
> 1345090_patch_P2_V4.diff
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b2667efb34e4eea441359db78273764d8beccf31

This was with the pref set to false, so not an overly useful Try run.
Attached patch 1345090_patch_P2_V11.diff (obsolete) — Splinter Review
Looks like we have to train some mochitests for lazy browser.  Also some updates in tabbrowser.xml and SessionStore.xml.

I have a test for browser.sessionstore.restore_on_demand within ssi_restoreWindow.  I needed to have it there instead of initPrefs otherwise tests weren't able to update it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74f372829c4224e05013053b06b383a296315377
Attachment #8855800 - Attachment is obsolete: true
Attachment #8856566 - Flags: feedback?(mdeboer)
Attachment #8856566 - Flags: feedback?(dao+bmo)
Mike, in the following code:

https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#4819-4837

If `browser.sessionstore.restore_on_demand` is true (default)  TabRestoreQueue.shift() would only be able to return a tab if there are pinned tabs (priority queue).  It would never be able to return visible or hidden tabs.

Is this intended?

I am asking because this comes up in a bug where inserting a browser doesn't always trigger a content restore.
Flags: needinfo?(mdeboer)
(In reply to Kevin Jones from comment #72)
> Mike, in the following code:
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/components/
> sessionstore/SessionStore.jsm#4819-4837
> 
> If `browser.sessionstore.restore_on_demand` is true (default) 
> TabRestoreQueue.shift() would only be able to return a tab if there are
> pinned tabs (priority queue).  It would never be able to return visible or
> hidden tabs.
> 
> Is this intended?
> 
> I am asking because this comes up in a bug where inserting a browser doesn't
> always trigger a content restore.

Okay, I think I am seeing that have not understood the role of `restoreNextTab` very well, and that TabRestoreQueue is for non-on-demand restoration.  It looks like I might have to rethink `restoreTab` for lazily inserted tabs a little bit.
Flags: needinfo?(mdeboer)
Attached patch 1345090_patch_P2_V16.diff (obsolete) — Splinter Review
Updated with code changes to fix test failures.

Added explicit "reload" and "reloadWithFlags" proxy to lazy browser.  See comments inline.  `userTypedValue` and `userTypedClear` proxys were also added.

Test for restore_on_demand was added to criteria for lazy browser restore; if !restore_on_demand, a lazy restore doesn't make sense[1].  This also fixed some test failures.

Some test failures could only be dealt with by modifying the tests to deal with lazy tabs.  In 2 cases of sessionstore tests, lazy browser restore was disabled, as with those particular tests lazy browser restore didn't really make sense[1].

In 2 test files, `yield promiseTabRestoring(tab);` was being used to ensure data would be available for tests, however with the lazy browser update it would never resolve.  It was simply removed as it does not seem necessary since AFAICT the `yield BrowserTestUtils.waitForEvent(newWin, "SSWindowStateReady");` just prior to it would ensure the data needed would be available.  IAE it fixed the  issue and the data updates properly.

These and other changes in tabbrowser.xml and SessionStore.jsm to help Firefox to navigate through the lazy-browser implemlentation without breakage.

[1] In order to allow tests to update those prefs for a restore, restore_on_demand and restore_tabs_lazily are polled directly in ssi_restoreWindow.  Mike, I removed the pref setting from ssi_initPrefs like you suggested for this reason.  I refrained from using the built-in observer since that always calls _notifyOfClosedObjectsChange, and I don't know if we want that in the cases of restore_on_demand or restore_tabs_lazily.  I took the explicit route for now to maintain functionality.  I suppose we could use a dedicated observer if that is preferred.  Please advise on this.

Tryserver results look good.  The current patch has been updated with some cleanup, syntax optimization, eslint corrections and comments since this run, but functionally is the same:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13e837ec6b6a4fe938c725c8f4524e778668b8ac

I was not able to reproduce failures in the two oranges that appear there.
Attachment #8856566 - Attachment is obsolete: true
Attachment #8856566 - Flags: feedback?(mdeboer)
Attachment #8856566 - Flags: feedback?(dao+bmo)
Attachment #8857697 - Flags: review?(mdeboer)
Attachment #8857697 - Flags: review?(dao+bmo)
Comment on attachment 8857697 [details] [diff] [review]
1345090_patch_P2_V16.diff

>+                case "reload":
>+                case "reloadWithFlags":
>+                  getter = () => {
>+                    return (params) => {
>+                      // Wait for load handler to be instantiated before
>+                      // initializing the reload.
>+                      aTab.addEventListener("SSTabRestoring", function() {
>+                        browser[name](params);
>+                      }, { once: true });
>+                      gBrowser._insertBrowser(aTab);
>+                    };
>+                  };
>+                  break;

Ugh.

nit: use an arrow function for the event listener

>--- a/browser/components/sessionstore/test/browser_599909.js
>+++ b/browser/components/sessionstore/test/browser_599909.js
>@@ -4,27 +4,32 @@
> 
> var stateBackup = ss.getBrowserState();
> 
> function cleanup() {
>   // Reset the pref
>   try {
>     Services.prefs.clearUserPref("browser.sessionstore.restore_on_demand");
>   } catch (e) {}
>+  try {
>+    Services.prefs.clearUserPref("browser.sessionstore.restore_tabs_lazily");
>+  } catch (e) {}

clearUserPref doesn't throw (not anymore)

>--- a/toolkit/modules/PrivateBrowsingUtils.jsm
>+++ b/toolkit/modules/PrivateBrowsingUtils.jsm

>   isBrowserPrivate(aBrowser) {
>     let chromeWin = aBrowser.ownerGlobal;
>-    if (chromeWin.gMultiProcessBrowser) {
>+    if (chromeWin.gMultiProcessBrowser || !aBrowser.isConnected) {
>       // In e10s we have to look at the chrome window's private
>       // browsing status since the only alternative is to check the
>       // content window, which is in another process.
>       return this.isWindowPrivate(chromeWin);

Should probably update the comment.

r=me on the non-sessionstore stuff
Attachment #8857697 - Flags: review?(dao+bmo) → review+
Attached patch 1345090_patch_P2_V17.diff (obsolete) — Splinter Review
Updated per Dao's requests.
Attachment #8857697 - Attachment is obsolete: true
Attachment #8857697 - Flags: review?(mdeboer)
Attachment #8857919 - Flags: review?(mdeboer)
Comment on attachment 8857919 [details] [diff] [review]
1345090_patch_P2_V17.diff

>   isBrowserPrivate(aBrowser) {
>     let chromeWin = aBrowser.ownerGlobal;
>-    if (chromeWin.gMultiProcessBrowser) {
>-      // In e10s we have to look at the chrome window's private
>-      // browsing status since the only alternative is to check the
>+    if (chromeWin.gMultiProcessBrowser || !aBrowser.isConnected) {
>+      // In e10s or if browser is lazy we have to look at the chrome window's
>+      // private browsing status since the only alternative is to check the
>       // content window, which is in another process.
>       return this.isWindowPrivate(chromeWin);

If e10s is disabled and the browser is lazy, the content window isn't in another process. It rather doesn't exist at all.
Attached patch 1345090_patch_P2_V18.diff (obsolete) — Splinter Review
Updated comment.
Attachment #8857919 - Attachment is obsolete: true
Attachment #8857919 - Flags: review?(mdeboer)
Attachment #8857945 - Flags: review?(mdeboer)
Comment on attachment 8857945 [details] [diff] [review]
1345090_patch_P2_V18.diff

Review of attachment 8857945 [details] [diff] [review]:
-----------------------------------------------------------------

OK, finally I was able to review your patch, Kevin! The delay was caused by Easter Monday and me being sick yesterday - otherwise it would've been much sooner. But, these things happen sometimes.
I'm very happy with how this turned out and I think this is a big step forward. Thanks much, Kevin, for holding out and for your perseverance!

r=me with comments addressed.

::: browser/base/content/tabbrowser.xml
@@ +2060,5 @@
> +                    };
> +                  };
> +                  break;
> +                case "isRemoteBrowser":
> +                  getter = () => {

nit: you can keep these simple ones one-liners, if you like: `getter = () => browser.getAttribute("remote") == "true";` and they still wouldn't exceed 80-120chars.

::: browser/components/sessionstore/SessionStore.jsm
@@ +3298,5 @@
>  
>      let numVisibleTabs = 0;
>  
> +    let createLazyBrowser =
> +          this._prefBranch.getBoolPref("sessionstore.restore_tabs_lazily") &&

nit: please format this properly:
```js
let createLazyBrowser = this._prefBranch.getBoolPref("sessionstore.restore_tabs_lazily") &&
  this._prefBranch.getBoolPref("sessionstore.restore_on_demand");
```

@@ +3760,5 @@
> +      if (activeIndex in tabData.entries) {
> +        url = tabData.entries[activeIndex].url;
> +        title = tabData.entries[activeIndex].title || url;
> +      }
> +      tab.__SS_lazy_data = { url,

nit: please format this correctly:
```js
tab.__SS_lazyData = {
  url, title,
  userTypedValue: tabData.userTypedValue || "",
  userTypedClear: tabData.userTypedClear || 0
};
```

Please also note I renamed the variable to `__SS_lazyData` to use camelCase as is common in sessionstore code.

::: browser/components/sessionstore/test/browser_522545.js
@@ +35,5 @@
> +
> +      // Change tabs to make sure address bar value gets updated.  If tab is
> +      // lazy, wait for SSTabRestored to ensure address bar has time to update.
> +      let tabToSelect = gBrowser.tabContainer.getItemAtIndex(0);
> +      if (tabToSelect.linkedPanel) {

Shouldn't this be changed to use `browser.isConnected` as well? Please change it if that's the case.

::: browser/components/sessionstore/test/head.js
@@ +386,5 @@
>            wasRestored++;
>          else if (browser.__SS_restoreState == TAB_STATE_RESTORING)
>            isRestoring++;
> +        else if (browser.__SS_restoreState == TAB_STATE_NEEDS_RESTORE ||
> +                 !browser.isConnected)

nit: You can keep this on one line.
Attachment #8857945 - Flags: review?(mdeboer) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #79)
> Comment on attachment 8857945 [details] [diff] [review]
> 1345090_patch_P2_V18.diff
> 
> Review of attachment 8857945 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK, finally I was able to review your patch, Kevin! The delay was caused by
> Easter Monday and me being sick yesterday - otherwise it would've been much
> sooner. But, these things happen sometimes.

No problem Mike.  I'm sorry you weren't feeling well.

> ::: browser/base/content/tabbrowser.xml
> @@ +2060,5 @@
> > +                    };
> > +                  };
> > +                  break;
> > +                case "isRemoteBrowser":
> > +                  getter = () => {
> 
> nit: you can keep these simple ones one-liners, if you like: `getter = () =>
> browser.getAttribute("remote") == "true";` and they still wouldn't exceed
> 80-120chars.

IIRC, Dao has asked me to use the indented braces even on simple stuff.
Attached patch 1345090_patch_P2_V19.diff (obsolete) — Splinter Review
Updated per Mike's comments.

If this looks okay to everyone, it is ready to land AFAICT.
Attachment #8857945 - Attachment is obsolete: true
Flags: needinfo?(mdeboer)
Flags: needinfo?(dao+bmo)
LGTM.
Flags: needinfo?(mdeboer)
Sure. Was there a recent enough Try run?
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #83)
> Sure. Was there a recent enough Try run?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c307e333633936ab560ab5d50df645edaece976
(In reply to Kevin Jones from comment #84)
> (In reply to Dão Gottwald [::dao] from comment #83)
> > Sure. Was there a recent enough Try run?
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=8c307e333633936ab560ab5d50df645edaece976

I think it's okay?  I don't know what the oranges are from, but I cannot replicate any of the failures locally with a fresh mc pull.
Those failures all look like known intermittents, yes. Please note that your Try push was on top of a nearly two week old parent revision, which is less than ideal, though.
(In reply to [Out of Office Until 24-April] Ryan VanderMeulen [:RyanVM] from comment #86)
> Those failures all look like known intermittents, yes. Please note that your
> Try push was on top of a nearly two week old parent revision, which is less
> than ideal, though.

Rebased:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0e4edb3d538cc0b4c0d252f947df04bb0d6086f
Rebased
Attachment #8859569 - Attachment is obsolete: true
Comment on attachment 8859833 [details] [diff] [review]
1345090_patch_P3_V2.diff

Interdiff is struggling.  Here is what changed:

tabbrowser.xml:

                label = this.mStringBundle.getString(stringID);
              }
            } else {
              label = tab.getAttribute("label");
              if (AppConstants.E10S_TESTING_ONLY &&
                  tab.linkedBrowser &&
                  tab.linkedBrowser.isRemoteBrowser) {
                label += " - e10s";
--              if (Services.prefs.getIntPref("dom.ipc.processCount") > 1) {
+-              if (Services.appinfo.maxWebProcessCount > 1) {
 +              if (tab.linkedBrowser.frameLoader &&
-+                  Services.prefs.getIntPref("dom.ipc.processCount") > 1) {
++                  Services.appinfo.maxWebProcessCount > 1) {
                  label += " (" + tab.linkedBrowser.frameLoader.tabParent.osPid + ")";
                }
              }
              if (tab.userContextId) {
                label = this.mStringBundle.getFormattedString("tabs.containers.tooltip", [label, ContextualIdentityService.getUserContextLabel(tab.userContextId)]);
              }
            }
  
SessionStore.jsm:

        // When that's done it will be removed from the cache and we always
        // collect it in TabState._collectBaseTabData().
        image: tabData.image || "",
        iconLoadingPrincipal: tabData.iconLoadingPrincipal || null,
        userTypedValue: tabData.userTypedValue || "",
        userTypedClear: tabData.userTypedClear || 0
      });
  
--    browser.messageManager.sendAsyncMessage("SessionStore:restoreHistory",
--                                            {tabData, epoch, loadArguments});
+-    this._sendRestoreHistory(browser, {tabData, epoch, loadArguments});
 +    // Restore tab attributes.
 +    if ("attributes" in tabData) {
 +      TabAttributes.set(tab, tabData.attributes);
 +    }
 +
 +    if (isBrowserInserted) {
 +      // Start a new epoch to discard all frame script messages relating to a
 +      // previous epoch. All async messages that are still on their way to chrome
 +      // will be ignored and don't override any tab data set when restoring.
 +      let epoch = this.startNextEpoch(browser);
 +
 +      // Ensure that the tab will get properly restored in the event the tab
 +      // crashes while restoring.  But don't set this on lazy browsers as
 +      // restoreTab will get called again when the browser is instantiated.
 +      browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE;
 +
-+      browser.messageManager.sendAsyncMessage("SessionStore:restoreHistory",
-+                                              {tabData, epoch, loadArguments});
++      this._sendRestoreHistory(browser, {tabData, epoch, loadArguments});
 +
 +      // This could cause us to ignore MAX_CONCURRENT_TAB_RESTORES a bit, but
 +      // it ensures each window will have its selected tab loaded.
 +      if (willRestoreImmediately) {
 +        this.restoreTabContent(tab, loadArguments, reloadInFreshProcess,
 +                               restoreContentReason);
 +      } else if (!forceOnDemand) {
 +        TabRestoreQueue.add(tab);
Attachment #8859833 - Flags: review?(mdeboer)
Attachment #8859833 - Flags: review?(dao+bmo)
Attachment #8859833 - Flags: review?(mdeboer)
Attachment #8859833 - Flags: review?(dao+bmo)
Attachment #8859833 - Flags: review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a03617861cb6
Modify SessionStore to restore tabs with lazy browsers. r=mikedeboer,dao
https://hg.mozilla.org/mozilla-central/rev/a03617861cb6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(In reply to Wes Kocher (:KWierso) from comment #91)
> https://hg.mozilla.org/mozilla-central/rev/a03617861cb6

Thanks a lot everyone, you're mentoring and patience has been invaluable.

:-)
Thanks, Kevin - your tireless efforts were outstanding and I'm looking forward to seeing happy users reporting in ;-)
Absolutely no breakage with my heavily customised profile (but that could be a result of oneman being prepared for this landing)

unloaded tabs spawn no content processes until loaded. ✓ 
Processes containing tabs that are forcefully unloaded with Auto unload tab are closed. ✓ 
significant reduction in memory through observed. ✓ 
Instant resume of a 523 tab session, as a result of not having to churn ipc restoring tabs to individual content processes. ✓ 

I would like to see this on the memshrink blog, it's a big win.
(In reply to Danial Horton from comment #94)
> Absolutely no breakage with my heavily customised profile (but that could be
> a result of oneman being prepared for this landing)
> 
> unloaded tabs spawn no content processes until loaded. ✓ 
> Processes containing tabs that are forcefully unloaded with Auto unload tab
> are closed. ✓ 
> significant reduction in memory through observed. ✓ 
> Instant resume of a 523 tab session, as a result of not having to churn ipc
> restoring tabs to individual content processes. ✓ 
> 
> I would like to see this on the memshrink blog, it's a big win.



Same thing here and it seems now loading sessions with nearly 600+ tabs does not make the fan start whirring and instead load instantly compared to 2 mins wait and then nearly 1 min to responsive UI.

The only problem face was while loading tabs from the session for a sec it turns into newtab just before it starts loading and is sometimes hitting this bug 1354796 with looses the data of the tabs.
new profile with e10s on/off & restore session from previous on.
Does this works on non e10s?

P.S. Great work kevin & hoping you are just getting warmed up ;) :P
(In reply to Sunil Kumar from comment #95)
> Does this works on non e10s?

Yes.
(In reply to Kevin Jones from comment #96)
> (In reply to Sunil Kumar from comment #95)
> > Does this works on non e10s?
> 
> Yes.

Great,did you look at the problem mentioned above?

Kudos.
Could this have caused bug 1358735?
(In reply to Mefoster from comment #95)
> The only problem face was while loading tabs from the session for a sec it
> turns into newtab just before it starts loading

That's probably bug 1054740.
seems auto unload tabs cannot determine what is and isn't a loaded tab anymore, it no longer goes to the nearest loaded tab when unloading the one you're currently on.
So far looks good. I see still this in browser console direct after startup.

TypeError: invalid 'in' operand browser  tabbrowser.xml:2137:1
_insertBrowser chrome://browser/content/tabbrowser.xml:2137:1
getRelatedElement chrome://browser/content/tabbrowser.xml:6459:11
set_selectedIndex chrome://global/content/bindings/tabbox.xml:403:31
tabs_XBL_Constructor chrome://global/content/bindings/tabbox.xml:272:13
Dont wanna to spam in your bugmail, folks, but i just cannot not to say how freaking fast session restore works now. I have extra heavy duty session and restarting was a PAIN for me, but now...its lightning fast. Awesome!
(In reply to Mefoster from comment #95)
> The only problem face was while loading tabs from the session for a sec it
> turns into newtab just before it starts loading and is sometimes hitting
> this bug 1354796 with looses the data of the tabs.

Are you actually seeing "newtab"?  There is a momentary blank appearance while the browser is being instantiated and the tab's history is being restored (since this is all done on-demand now).

Regarding bug 1354796, AFAICT this is not related to lazy tabs.
Depends on: 1358735
(In reply to Danial Horton from comment #94)
> Absolutely no breakage with my heavily customised profile (but that could be
> a result of oneman being prepared for this landing)
> 
> unloaded tabs spawn no content processes until loaded. ✓ 
> Processes containing tabs that are forcefully unloaded with Auto unload tab
> are closed. ✓ 
> significant reduction in memory through observed. ✓ 
> Instant resume of a 523 tab session, as a result of not having to churn ipc
> restoring tabs to individual content processes. ✓ 
> 
> I would like to see this on the memshrink blog, it's a big win.

(In reply to Mefoster from comment #95)

> Same thing here and it seems now loading sessions with nearly 600+ tabs does
> not make the fan start whirring and instead load instantly compared to 2
> mins wait and then nearly 1 min to responsive UI.
> 


Just upgraged to today's central build. Can't verify any of this.
All content processes (44) are started at startup and waste a humongous amount of RAM (3.1 GB).

It's nowhere near instant, but it might not be as excruciatingly slow to start as it was right after bug 1256472. I have browser.sessionstore.restore_tabs_lazily;true and dom.ipc.processPrelaunch.enabled;false.
Am I missing something?
(In reply to avada from comment #104)
> Just upgraged to today's central build. Can't verify any of this.
> All content processes (44) are started at startup and waste a humongous
> amount of RAM (3.1 GB).
> 
> It's nowhere near instant, but it might not be as excruciatingly slow to
> start as it was right after bug 1256472. I have
> browser.sessionstore.restore_tabs_lazily;true and
> dom.ipc.processPrelaunch.enabled;false.
> Am I missing something?

I assume you're testing on a fresh profile and browser.sessionstore.restore_on_demand is `true`?
(In reply to Kevin Jones from comment #105)
> (In reply to avada from comment #104)
> > Just upgraged to today's central build. Can't verify any of this.
> > All content processes (44) are started at startup and waste a humongous
> > amount of RAM (3.1 GB).
> > 
> > It's nowhere near instant, but it might not be as excruciatingly slow to
> > start as it was right after bug 1256472. I have
> > browser.sessionstore.restore_tabs_lazily;true and
> > dom.ipc.processPrelaunch.enabled;false.
> > Am I missing something?
> 
> I assume you're testing on a fresh profile and
> browser.sessionstore.restore_on_demand is `true`?

Nope. My main profile. browser.sessionstore.restore_on_demand is true however.
(In reply to avada from comment #104)
> Just upgraged to today's central build. Can't verify any of this.
> All content processes (44) are started at startup and waste a humongous
> amount of RAM (3.1 GB).
> 
> It's nowhere near instant, but it might not be as excruciatingly slow to
> start as it was right after bug 1256472. I have
> browser.sessionstore.restore_tabs_lazily;true and
> dom.ipc.processPrelaunch.enabled;false.
> Am I missing something?

I would recommend filing a new bug to investigate this.  Perhaps there is poor behavior with so many content processes, or something else in the config...  In any case, it makes sense to diagnose it separately in a new bug.
(In reply to avada from comment #106)
> (In reply to Kevin Jones from comment #105)
> > (In reply to avada from comment #104)
> > > Just upgraged to today's central build. Can't verify any of this.
> > > All content processes (44) are started at startup and waste a humongous
> > > amount of RAM (3.1 GB).
> > > 
> > > It's nowhere near instant, but it might not be as excruciatingly slow to
> > > start as it was right after bug 1256472. I have
> > > browser.sessionstore.restore_tabs_lazily;true and
> > > dom.ipc.processPrelaunch.enabled;false.
> > > Am I missing something?
> > 
> > I assume you're testing on a fresh profile and
> > browser.sessionstore.restore_on_demand is `true`?
> 
> Nope. My main profile. browser.sessionstore.restore_on_demand is true
> however.

You may be running into bug 1345098, and if you have addons enabled, one of them may be triggering this.  There was a patch that just landed to help monitor this, check your JS console for "[bug 1345098] Lazy browser prematurely inserted via '${name}' property access:".  It will give a stack trace showing the code that triggered it.

The patch just landed so I don't know if it made it into your version of Nightly or not, but if not I assume will make it into tonight's Nightly.
(In reply to Kevin Jones from comment #108)
> The patch just landed so I don't know if it made it into your version of
> Nightly or not, but if not I assume will make it into tonight's Nightly.

I've verified it is in the currently available Nightly.
Note to Nightly testers:

Bug 1345098 just landed with a patch that will help monitor if lazy browser tabs are being prematurely instantiated (and thus no longer lazy).  If this happens the message "[bug 1345098] Lazy browser prematurely inserted via '${name}' property access:" will be printed out in the JS console, along with a stack trace showing the code that triggered it.
I can't say there is any noticeable performance improvement after this bug landed. I tried to load a couple of sessions with 4+ windows and 500+ tabs using Session Manager addon, browser.sessionstore.restore_on_demand false, 8 content processes. All other extensions I have installed are e10s-compatible (Tab Mix Plus, uBlock Origin, NoScript).

I tried to search for "lazy" in the browser console but didn't find anything.
I filtered for "1345098". All forty something content processes were started at browser start.
I got this:

"[bug 1345098] Lazy browser prematurely inserted via 'messageManager' property access:
getter@chrome://browser/content/tabbrowser.xml:2115:45
TLCB_notifyConfigUpdatedMessage@chrome://textlink/content/globalOverlay.js:711:3
TLCB_init@chrome://textlink/content/globalOverlay.js:634:3
TextLinkContentBridge@chrome://textlink/content/globalOverlay.js:617:2
initTab@chrome://textlink/content/globalOverlay.js:326:36
handleEvent@chrome://textlink/content/globalOverlay.js:119:12
addTab@chrome://browser/content/tabbrowser.xml:2400:13
change_gBrowser/gBrowser.addTab@chrome://tabmixplus/content/minit/tablib.js:212:15
ssi_restoreWindow@resource:///modules/sessionstore/SessionStore.jsm:3309:33
Modules.LOADMODULE/<@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///C:/Users/Zsolt/AppData/Roaming/Mozilla/Firefox/Profiles/q74vpos5.teszt-teljes/extensions/tabgroups@quicksaver.xpi!/bootstrap.js -> resource://tabgroups/modules/utils/Modules.jsm -> resource://tabgroups/modules/Storage.jsm:155:3
ssi_restoreWindows@resource:///modules/sessionstore/SessionStore.jsm:3542:5
initializeWindow@resource:///modules/sessionstore/SessionStore.jsm:1170:11
onBeforeBrowserWindowShown/<@resource:///modules/sessionstore/SessionStore.jsm:1319:9
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:455:5
getAddonList@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:1050:5
getVisibleAddons@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:1133:5
getAddonsByTypes@resource://gre/modules/addons/XPIProvider.jsm:4254:5
callProviderAsync@resource://gre/modules/AddonManager.jsm:294:12
promiseCallProvider/<@resource://gre/modules/AddonManager.jsm:318:53
Promise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:390:5
promiseCallProvider@resource://gre/modules/AddonManager.jsm:317:10
getAddonsByTypes/<@resource://gre/modules/AddonManager.jsm:2492:36
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
Task_spawn@resource://gre/modules/Task.jsm:166:12
getAddonsByTypes@resource://gre/modules/AddonManager.jsm:2488:12
getAddonsByTypes@resource://gre/modules/AddonManager.jsm:3576:7
init@resource://tabmixplus/extensions/AddonManager.jsm:160:5
@resource://tabmixplus/extensions/AddonManager.jsm:170:1
Tabmix.beforeDelayedStartup@chrome://tabmixplus/content/tabmix.js:42:5
Tabmix.beforeBrowserInitOnLoad/gBrowserInit._delayedStartup@chrome://tabmixplus/content/links/setup.js:132:9
EventListener.handleEvent*onLoad@chrome://browser/content/browser.js:1239:5
onload@chrome://browser/content/browser.xul:1:1
"  tabbrowser.xml:2115
(In reply to avada from comment #112)
> I filtered for "1345098". All forty something content processes were started
> at browser start.

Filed bug 1360148.
(In reply to ajfhajf from comment #111)
> browser.sessionstore.restore_on_demand false

Lazy tabs only apply when you use restore on demand. Since you have that disabled you won't get any performance improvement.
(In reply to The 8472 from comment #114)
> (In reply to ajfhajf from comment #111)
> > browser.sessionstore.restore_on_demand false
> 
> Lazy tabs only apply when you use restore on demand. Since you have that
> disabled you won't get any performance improvement.

Yeah I mixed that up, sorry. I don't have tabs loading automatically.
I stumbled upon this bug from a link in an mconley newsletter.

What struck me is that comment #11 indicates that this change produced a >10x speedup for session restore yet no Talos alerts fired when this landed. This doesn't exactly inspire confidence that our performance testing of session restore is adequate and that changes to performance can be detected and pinpointed easily.
(In reply to Mike de Boer [:mikedeboer] from comment #12)
> (In reply to Kevin Jones from comment #11)
> > 330 tabs:
> > lazy: 1.5s
> > 
> > 502 tabs:
> > lazy: 2.4s
> > 
> > 1008 tabs:
> > lazy: 5.3s
> 
> Now you've done it - you made my eyes all teary! Please finish this work!
> It's awesome!!

Can you guys please uplift this in Firefox ESR 52 an this qualifies as a high impact/performance criteria and would help ESR users a lot who won't get Features only security updates.
This is a major perf win so should be uplifted.
Flags: needinfo?(mdeboer)
Flags: needinfo?(kevinhowjones)
No, we cannot reasonably uplift this to ESR 52.
Flags: needinfo?(mdeboer)
Flags: needinfo?(kevinhowjones)
A telemetry alert fired for this change: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/750/alerts/?from=2017-04-21&to=2017-04-21

(( Well, it fired for the build which included changes that included this change: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=27311156637f9b5d4504373967e01c4241902ae7&tochange=dd530a59750adcaa0d48fa4f69b0cdb52715852a ))

Looks like this moved the median needle down about 50% -- https://mzl.la/2pxBFWs

I'll just assume that this was an intentional improvement and that the probe continues to be relevant. Good work!
that improvement was definitely the whole point of lazy browsers (well that and reduced memory / process use)

updated nightly, and auto unload tab switches to the nearest active tab again when a tab is force unloaded!
"perf" on whiteboard?
Flags: needinfo?(dao+bmo)
Flags: needinfo?(dao+bmo)
Keywords: footprint, perf
Release Note Request (optional, but appreciated)
[Why is this notable]: Significant performance and memory improvement when restoring sessions with a lot of tabs
[Affects Firefox for Android]: no
[Suggested wording]: Browsing sessions with a high number of tabs are now restored in an instant
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
"Browsing sessions with a high number of tabs are now restored in an instant"
Worth mentioning that 'browser.sessionstore.restore_on_demand' must be set to 'true'?
(In reply to Ricardo from comment #123)
> "Browsing sessions with a high number of tabs are now restored in an instant"
> Worth mentioning that 'browser.sessionstore.restore_on_demand' must be set
> to 'true'?

As far as I can tell, true is the default value, so no need to mention it.
adding to the 55beta release notes
Depends on: 1396721
Depends on: 1402703
Depends on: 1446426
Depends on: 1459396
Performance Impact: --- → -
Whiteboard: [qf-]
You need to log in before you can comment on or make changes to this bug.