Closed Bug 1360932 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: u462496, Assigned: u462496)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 10 obsolete files)

No description provided.
Blocks: lazytabs
Component: Tabbed Browser → Untriaged
Component: Untriaged → Location Bar
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().
Attachment #8863508 - Flags: feedback?(mdeboer)
Attachment #8863508 - Flags: feedback?(dao+bmo)
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. ;-)
Attachment #8863508 - Flags: feedback?(mdeboer) → feedback-
(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.
Flags: needinfo?(mdeboer)
(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!
Flags: needinfo?(mdeboer)
(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?
Flags: needinfo?(mdeboer)
Add event for lazy tab restore, handle autocomplete register in tabbrowser.xml.
Attachment #8863508 - Attachment is obsolete: true
Attachment #8863508 - Flags: feedback?(dao+bmo)
Attachment #8863721 - Flags: feedback?(mdeboer)
Attachment #8863721 - Flags: feedback?(dao+bmo)
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; }`
Attachment #8863721 - Flags: feedback?(mdeboer) → feedback+
Flags: needinfo?(mdeboer)
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.
Attachment #8863721 - Flags: feedback?(dao+bmo) → feedback-
(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;
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
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)?
Flags: needinfo?(mdeboer)
(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.
Update per Dao's recommendations.
Attachment #8863721 - Attachment is obsolete: true
Attachment #8864984 - Flags: feedback?(mdeboer)
Attachment #8864984 - Flags: feedback?(dao+bmo)
Comment on attachment 8864984 [details] [diff] [review]
1360932_lazy_switch_to_tabs_V3.diff

this needs to be rebased for bug 1054740
Attachment #8864984 - Flags: feedback?(mdeboer)
Attachment #8864984 - Flags: feedback?(dao+bmo)
Rebased.  Mike, also please be sure to take a look at comment 11 regarding this.
Attachment #8864984 - Attachment is obsolete: true
Attachment #8865208 - Flags: feedback?(mdeboer)
Attachment #8865208 - Flags: feedback?(dao+bmo)
(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.
Flags: needinfo?(mdeboer)
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.
Attachment #8865208 - Attachment is obsolete: true
Attachment #8865208 - Flags: feedback?(mdeboer)
Attachment #8865208 - Flags: feedback?(dao+bmo)
Attachment #8865677 - Flags: feedback?(mdeboer)
Attachment #8865677 - Flags: feedback?(dao+bmo)
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";
            }
Attachment #8865677 - Flags: feedback?(dao+bmo)
(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?
(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.
Attachment #8865677 - Attachment is obsolete: true
Attachment #8865677 - Flags: feedback?(mdeboer)
Attachment #8865733 - Flags: feedback?(mdeboer)
Attachment #8865733 - Flags: feedback?(dao+bmo)
Comment on attachment 8865733 [details] [diff] [review]
1360932_lazy_switch_to_tabs_P1_V3.diff

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

LGTM
Attachment #8865733 - Flags: feedback?(mdeboer) → feedback+
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.
Attachment #8865733 - Flags: feedback?(dao+bmo) → feedback+
Update with recommendations.
Attachment #8865733 - Attachment is obsolete: true
Attachment #8866438 - Flags: review?(dao+bmo)
Comment on attachment 8866438 [details] [diff] [review]
1360932_lazy_switch_to_tabs_P1_V4.diff

Thanks :)
Attachment #8866438 - Flags: review?(dao+bmo) → review+
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.
Assignee: nobody → kevinhowjones
Component: Location Bar → Tabbed Browser
Maybe this time...
Attachment #8866438 - Attachment is obsolete: true
Attachment #8866456 - Flags: review?(dao+bmo)
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.
Attachment #8866456 - Flags: review?(dao+bmo) → review-
Sorry, I didn't look at your comment carefully enough.
Attachment #8866456 - Attachment is obsolete: true
Attachment #8866600 - Flags: review?(dao+bmo)
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
Attachment #8866600 - Flags: review?(dao+bmo)
Rebased.
Attachment #8866600 - Attachment is obsolete: true
Attachment #8866908 - Flags: review?(dao+bmo)
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.
Attachment #8866908 - Flags: review?(dao+bmo) → review+
Attachment #8866908 - Attachment is obsolete: true
Attachment #8866944 - Flags: review?(dao+bmo)
Attachment #8866944 - Flags: review?(dao+bmo) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76408d97fe7a
Registered lazy tabs as switch-to-tab candidates. r=dao
https://hg.mozilla.org/mozilla-central/rev/76408d97fe7a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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.)
Flags: needinfo?(dao+bmo)
Maybe nevermind about the backout -- a fix might be coming sooner than I thought, per recent activity over on bug 1365933... (Thanks, Kevin!)
Flags: needinfo?(dao+bmo)
Depends on: 1385698
You need to log in before you can comment on or make changes to this bug.