1360932 - Lazy tabs do not appear in Awesome Bar's "switch-to" history

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Comment 1

[u462496]

Attachment: 1360932_lazy_switch_to_tabs_V1.diff

Tabs get registered as "switch-to-tabs" in onLocationChange progress listener, thus lazy tabs never get registered.  This patch registers them explicitly in SessionStore.restoreTab().

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 8863508 [details] [diff] [review]
1360932_lazy_switch_to_tabs_V1.diff

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

::: browser/components/sessionstore/SessionStore.jsm
@@ +3769,5 @@
> +      // browser must be explicitly registered so tab will appear as
> +      // a switch-to-tab candidate in autocomplete.
> +      let location = Services.io.newURI(url);
> +      tabbrowser._unifiedComplete.registerOpenPage(location, tabData.userContextId || 0);
> +      browser.registeredOpenURI = location;

Dão will have better feedback for you on the _how_, but I can say that I think this doesn't belong in SessionStore.jsm.
Instead, this needs to move to tabbrowser.xml where the other unifiedComplete mechanics live as well. Like I said; the where exactly and how I leave for Dão to hint at. ;-)

Comment 3

[u462496]

(In reply to Mike de Boer [:mikedeboer] from comment #2)
> Comment on attachment 8863508 [details] [diff] [review]
> 1360932_lazy_switch_to_tabs_V1.diff
> 
> Review of attachment 8863508 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/sessionstore/SessionStore.jsm
> @@ +3769,5 @@
> > +      // browser must be explicitly registered so tab will appear as
> > +      // a switch-to-tab candidate in autocomplete.
> > +      let location = Services.io.newURI(url);
> > +      tabbrowser._unifiedComplete.registerOpenPage(location, tabData.userContextId || 0);
> > +      browser.registeredOpenURI = location;
> 
> Dão will have better feedback for you on the _how_, but I can say that I
> think this doesn't belong in SessionStore.jsm.
> Instead, this needs to move to tabbrowser.xml where the other
> unifiedComplete mechanics live as well. Like I said; the where exactly and
> how I leave for Dão to hint at. ;-)

Yeah, I thought that might be the case.  We can get the url from getLazyTabValue, the only problem I see is, that data is only available after the lazy tab has gone through restoreTab, and we need some way of knowing when that has happened.  SSTabRestoring/SSTabRestored only get fired if the tab is not lazy.  Is there another event you can think of we can use, or can we create a new one eg, SSLazyTabRestored?  Unless there is some other way of handling this.

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Kevin Jones from comment #3)
> or can we create a new one eg, SSLazyTabRestored?  Unless there is some
> other way of handling this.

I think it's a very good idea to add this event - also useful for other bugs in the future!

Comment 5

[u462496]

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> (In reply to Kevin Jones from comment #3)
> > or can we create a new one eg, SSLazyTabRestored?  Unless there is some
> > other way of handling this.
> 
> I think it's a very good idea to add this event - also useful for other bugs
> in the future!

Do you prefer a helper function for that, ie, imitate _sendTabRestoredNotification, or to just put the event code right in tabRestore?

Comment 6

[u462496]

Attachment: 1360932_lazy_switch_to_tabs_V2.diff

Add event for lazy tab restore, handle autocomplete register in tabbrowser.xml.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8863721 [details] [diff] [review]
1360932_lazy_switch_to_tabs_V2.diff

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

LGTM! If it were me, I'd use 'SSTabRestoredLazily', because now it might be understood as 'a lazy tab/ browser was fully restored', which is not what this is about. Your choice, just nitpickin'.

::: browser/base/content/tabbrowser.xml
@@ +4976,5 @@
>                break;
> +            case "SSTabLazyRestored":
> +              // Lazy browser must be explicitly registered so tab will appear as
> +              // a switch-to-tab candidate in autocomplete.
> +              let tab = aEvent.target;

If you're using block-scoped variables in a switch-case-block, please enclose it with braces to avoid potential foot-gun style issues `case "SSTabLazyRestored": { ...; break; }`

Comment 8

[Dão Gottwald [::dao]]

Comment on attachment 8863721 [details] [diff] [review]
1360932_lazy_switch_to_tabs_V2.diff

>+            case "SSTabLazyRestored":
>+              // Lazy browser must be explicitly registered so tab will appear as
>+              // a switch-to-tab candidate in autocomplete.
>+              let tab = aEvent.target;
>+              let browser = tab.linkedBrowser;
>+              let location = browser.currentURI;
>+              this._unifiedComplete.registerOpenPage(location, tab.userContextId);
>+              browser.registeredOpenURI = location;
>+              break;

I'd prefer letting SessionStore.jsm pass the URI to addTab (we can probably overload the aURI argument for this), and addTab doing the above.

Comment 9

[u462496]

(In reply to Dão Gottwald [::dao] from comment #8)
> Comment on attachment 8863721 [details] [diff] [review]
> 1360932_lazy_switch_to_tabs_V2.diff
> 
> >+            case "SSTabLazyRestored":
> >+              // Lazy browser must be explicitly registered so tab will appear as
> >+              // a switch-to-tab candidate in autocomplete.
> >+              let tab = aEvent.target;
> >+              let browser = tab.linkedBrowser;
> >+              let location = browser.currentURI;
> >+              this._unifiedComplete.registerOpenPage(location, tab.userContextId);
> >+              browser.registeredOpenURI = location;
> >+              break;
> 
> I'd prefer letting SessionStore.jsm pass the URI to addTab (we can probably
> overload the aURI argument for this), and addTab doing the above.

Did you mean overload `aReferrerURI` (arguments[1]) ?

And do something like:

SessionStore.jsm:

  restoreWindow() {

    ....

    for (var t = 0; t < newTabCount; t++) {

      ....

      let switchToURL = "about:blank";
      if (activeIndex in winData.tabs[t].entries) {
        switchToURL = winData.tabs[t].entries[activeIndex].url;
      }

      let tab = reuseExisting ? this._maybeUpdateBrowserRemoteness(tabbrowser.tabs[t])
                              : tabbrowser.addTab("about:blank",
                                                  { createLazyBrowser,
                                                    skipAnimation: true,
                                                    userContextId,
                                                    skipBackgroundNotify: true,
                                                    switchToURL });

tabbrowser.xml:

      <method name="addTab">
    
        ....
    
              aSwitchToURL          = params.switchToURL;

              ....

              let location = Services.io.newURI(aSwitchToURL);;
              this._unifiedComplete.registerOpenPage(location, tab.userContextId);
              browser.registeredOpenURI = location;

Comment 10

[Dão Gottwald [::dao]]

(In reply to Kevin Jones from comment #9)
> > I'd prefer letting SessionStore.jsm pass the URI to addTab (we can probably
> > overload the aURI argument for this), and addTab doing the above.
> 
> Did you mean overload `aReferrerURI` (arguments[1]) ?

No, I meant aURI. This could get special treatment from addTab in the createLazyBrowser case.

Comment 11

[u462496]

Mike, is the last 2 lines of this code still necessary in ssi_restoreTab:

    let activeIndex = (tabData.index || tabData.entries.length) - 1;
    activeIndex = Math.min(activeIndex, tabData.entries.length - 1);
    activeIndex = Math.max(activeIndex, 0);

I am wondering because Dao would like for me to move passing the url to the tab in restoreWindow so that the switch-to stuff can be handled in addTab.  If I do that then I will need to move the above code up there.  But I am wondering if the last 2 lines are really necessary - is there ever a case where tabData.index would be out of range (other than being absent)?

Comment 12

[u462496]

(In reply to Kevin Jones from comment #11)
> Mike, is the last 2 lines of this code still necessary in ssi_restoreTab:
> 
>     let activeIndex = (tabData.index || tabData.entries.length) - 1;
>     activeIndex = Math.min(activeIndex, tabData.entries.length - 1);
>     activeIndex = Math.max(activeIndex, 0);
> 
> I am wondering because Dao would like for me to move passing the url to the
> tab in restoreWindow so that the switch-to stuff can be handled in addTab. 
> If I do that then I will need to move the above code up there.  But I am
> wondering if the last 2 lines are really necessary - is there ever a case
> where tabData.index would be out of range (other than being absent)?

Actually I don't think it can be "moved" because restoreTab can be called from setTabState, so it would have to appear in both places.

Comment 13

[u462496]

Attachment: 1360932_lazy_switch_to_tabs_V3.diff

Update per Dao's recommendations.

Comment 14

[Dão Gottwald [::dao]]

Comment on attachment 8864984 [details] [diff] [review]
1360932_lazy_switch_to_tabs_V3.diff

this needs to be rebased for bug 1054740

Comment 15

[u462496]

Attachment: 1360932_lazy_switch_to_tabs_rebased_P1_V1.diff

Rebased.  Mike, also please be sure to take a look at comment 11 regarding this.

Comment 16

[Mike de Boer [:mikedeboer]]

(In reply to Kevin Jones from comment #11)
> I am wondering because Dao would like for me to move passing the url to the
> tab in restoreWindow so that the switch-to stuff can be handled in addTab. 
> If I do that then I will need to move the above code up there.  But I am
> wondering if the last 2 lines are really necessary - is there ever a case
> where tabData.index would be out of range (other than being absent)?

Well, I don't think so, but this code is very defensively written. As in, try to be as stable here as we can humans can make it.

I won't be on the fence if you decide to remove these two lines.

Comment 17

[u462496]

Attachment: 1360932_lazy_switch_to_tabs_P1_V2.diff

Removed the "out of bounds" test for tabdata.activeIndex.  I decided it would be better to deal with "out of bounds" test in restoreTab in a separate bug as there will be some other code involved.

Comment 18

[Dão Gottwald [::dao]]

Comment on attachment 8865677 [details] [diff] [review]
1360932_lazy_switch_to_tabs_P1_V2.diff

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -2279,16 +2279,21 @@
>             }
> 
>             // if we're adding tabs, we're past interrupt mode, ditch the owner
>             if (this.mCurrentTab.owner)
>               this.mCurrentTab.owner = null;
> 
>             var t = document.createElementNS(NS_XUL, "tab");
> 
>+            let switchToURI = aURI;
>+            if (aCreateLazyBrowser) {
>+              aURI = "about:blank";
>+            }

I'd prefer:

            let lazyBrowserURI;
            if (aCreateLazyBrowser) {
              lazyBrowserURI = aURI;
              aURI = "about:blank";
            }

Comment 19

[Dão Gottwald [::dao]]

(In reply to Kevin Jones from comment #1)
> Tabs get registered as "switch-to-tabs" in onLocationChange progress
> listener, thus lazy tabs never get registered.

Actually, how does this work with pending but not lazy tabs?

Comment 20

[u462496]

(In reply to Dão Gottwald [::dao] from comment #19)
> (In reply to Kevin Jones from comment #1)
> > Tabs get registered as "switch-to-tabs" in onLocationChange progress
> > listener, thus lazy tabs never get registered.
> 
> Actually, how does this work with pending but not lazy tabs?

For pending but not lazy tabs:

onLocationChange@chrome://browser/content/tabbrowser.xml:830:16
_callProgressListeners@resource://gre/modules/RemoteWebProgress.jsm:179:11
receiveMessage@resource://gre/modules/RemoteWebProgress.jsm:265:7

Originates from a series of message hand-offs but is basically from a history restore originating in ssi_restoreTab.  Anyway, this could never happen for lazy tabs.

Comment 21

[u462496]

Attachment: 1360932_lazy_switch_to_tabs_P1_V3.diff

Comment 22

[Mike de Boer [:mikedeboer]]

Comment on attachment 8865733 [details] [diff] [review]
1360932_lazy_switch_to_tabs_P1_V3.diff

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

LGTM

Comment 23

[Dão Gottwald [::dao]]

Comment on attachment 8865733 [details] [diff] [review]
1360932_lazy_switch_to_tabs_P1_V3.diff

>               this._createLazyBrowser(t);
>+              // Lazy browser must be explicitly registered so tab will appear as
>+              // a switch-to-tab candidate in autocomplete.
>+              let location = Services.io.newURI(lazyBrowserURI);
>+              this._unifiedComplete.registerOpenPage(location, aUserContextId);
>+              b.registeredOpenURI = location;

nit: add blank line above this new block of code

>+      if (createLazyBrowser) {
>+        // For lazy browser tabs, url param will be overidden in addTab in
>+        // order to set switch-to-tab data for url bar autocomplete.
>+        let activeIndex = (tabData.index || tabData.entries.length) - 1;
>+        url = tabData.entries[activeIndex].url;

Don't need to go into details about what addTab will do with the URI, just mention that it's useful for tabbrowser to know the future URI and that progress listeners won't get the onLocationChange notification before the browser is inserted.

Comment 24

[u462496]

Attachment: 1360932_lazy_switch_to_tabs_P1_V4.diff

Update with recommendations.

Comment 25

[Dão Gottwald [::dao]]

Comment on attachment 8866438 [details] [diff] [review]
1360932_lazy_switch_to_tabs_P1_V4.diff

Thanks :)

Comment 26

[Dão Gottwald [::dao]]

Comment on attachment 8866438 [details] [diff] [review]
1360932_lazy_switch_to_tabs_P1_V4.diff

One last thing:

>+            let lazyBrowserURI;
>+            if (aCreateLazyBrowser) {
>+              lazyBrowserURI = aURI;
>+              aURI = "about:blank";
>+            }
>+
>             var uriIsAboutBlank = !aURI || aURI == "about:blank";

Do this before your new code: aURI = aURI || "about:blank";
And check if (aCreateLazyBrowser && aURI != "about:blank") {...

>+              let location = Services.io.newURI(lazyBrowserURI);
>+              this._unifiedComplete.registerOpenPage(location, aUserContextId);
>+              b.registeredOpenURI = location;

And null-check lazyBrowserURI here.

Comment 27

[u462496]

Attachment: 1360932_lazy_switch_to_tabs_P1_V5.diff

Maybe this time...

Comment 28

[Dão Gottwald [::dao]]

Comment on attachment 8866456 [details] [diff] [review]
1360932_lazy_switch_to_tabs_P1_V5.diff

>             var uriIsAboutBlank = !aURI || aURI == "about:blank";
> 
>+            let lazyBrowserURI;
>+            if (aCreateLazyBrowser && aURI != "about:blank") {
>+              lazyBrowserURI = aURI;
>+              aURI = "about:blank";
>+            }

Nope, sorry, you need to reshuffle this like I described, e.g.:

aURI = aURI || "about:blank";

let lazyBrowserURI;
if (aCreateLazyBrowser && aURI != "about:blank") {
  lazyBrowserURI = aURI;
  aURI = "about:blank";
}

var uriIsAboutBlank = aURI == "about:blank";

(untested)

>             if (!aURI || isBlankPageURL(aURI)) {

... and then !aURI can also go away here.

Comment 29

[u462496]

Attachment: 1360932_lazy_switch_to_tabs_P1_V6.diff

Sorry, I didn't look at your comment carefully enough.

Comment 30

[u462496]

Comment on attachment 8866600 [details] [diff] [review]
1360932_lazy_switch_to_tabs_P1_V6.diff

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fe6d682df4e537cc18f4a8c887f6b37826213fb

Comment 31

[Dão Gottwald [::dao]]

Comment on attachment 8866600 [details] [diff] [review]
1360932_lazy_switch_to_tabs_P1_V6.diff

Patch doesn't apply cleanly on mozilla-central tip :(

Needs rebasing for https://hg.mozilla.org/mozilla-central/rev/c1cde4075380

Comment 32

[u462496]

Attachment: 1360932_lazy_switch_to_tabs_P2_V1.diff

Rebased.

Comment 33

[Dão Gottwald [::dao]]

Comment on attachment 8866908 [details] [diff] [review]
1360932_lazy_switch_to_tabs_P2_V1.diff

>+            if (aCreateLazyBrowser && aURI != "about:blank") {
>+              lazyBrowserURI = aURI;

Could also just call Services.io.newURI here and avoid declaring 'location' later.

Comment 34

[u462496]

Attachment: 1360932_lazy_switch_to_tabs_P2_V2.diff

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76408d97fe7a
Registered lazy tabs as switch-to-tab candidates. r=dao

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76408d97fe7a

Comment 37

[Daniel Holbert [:dholbert]]

This seems to have introduced bug 1365933 which is a pretty serious dataloss-on-session-restore bug -- it's forced me to stop using Nightly for the past week, because otherwise I seem to lose all my tabs every time I restart Firefox.

Could this be backed out for the time being and then re-landed once we're confident it won't introduce bug 1365933?  (Or alternately, could we attack bug 1365933 ASAP and land a fix within a couple days? I think the regression is bad enough that we have to do one or the other, which makes me lean towards a backout as the simplest short-term solution.)

Comment 38

[Daniel Holbert [:dholbert]]

Maybe nevermind about the backout -- a fix might be coming sooner than I thought, per recent activity over on bug 1365933... (Thanks, Kevin!)