874533 - Use speculative connect for pending tabs in the restore queue

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Nick and his team came up with the idea of speeding up the process of restoring a session by speculatively connecting to hosts of tabs that are currently pending to be restored and that will be restored automatically (not on demand).

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: use speculative connect for pending tabs in the restore queue

Nick, I know that the final patch will not use nsISpeculativeConnect but a new module with a couple of heuristics - nevertheless I spent some time on the weekend to implement what we talked about.

I think this basically should do the trick and all left to do is switch to the new API when it's ready. Please feel free to take over the patch and test or adjust it to your needs!

Comment 2

[Mike de Boer [:mikedeboer]]

Hi Tim! I'm curious, which Nick did you mean and what new module are you talking about? Would using speculative connect here be reasonable still? (My guess: yes)

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Probably still reasonable. I'm pretty sure I meant Nick Hurley, who is CC'ed. No idea what module I was talking about... is there a spec connect JSM?

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Tim Taubert [:ttaubert] from comment #3)
> No idea what module I was talking about... is there a spec connect
> JSM?

No, just nsISpeculativeConnect, which is simple enough to use as-is...

Comment 5

[u408661]

I suspect the module Tim was talking about is nsINetworkPredictor, not a JSM, but what was being worked on when this bug was filed. This particular bit of functionality never made it into a production version of that service, and after now multiple years of experience with the predictor, I'm not certain that's actually the right place.

I think the patch that Tim put up "back in the day" is probably the right way to do this - we don't need the "smarts" of the predictor to do this, so it seems silly to increase the amount of data the predictor is storing just to make this work.

Another thing that might be useful is to speculatively connect to the host for unloaded tabs when the user hovers their mouse over them (we might already do this, I don't know). We do the same thing for links on a page through the predictor (that way we can avoid doing it if the predictor is disabled), so we might want to do the same thing for this use case. For connecting to hosts we already know are just waiting to be loaded, though, nsISpeculativeConnect is fine.

Comment 6

[Bao Quan [:beekill]]

Attachment: spec.v1.patch

I implement this based on the patch by Tim and Nicholas's suggestion.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8876165 [details] [diff] [review]
spec.v1.patch

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

Still a few things to do here, but looks promising.

::: browser/components/sessionstore/SessionStore.jsm
@@ +960,4 @@
>            Services.obs.notifyObservers(browser, NOTIFY_TAB_RESTORED);
>          }
>  
> +        // Remove the mouse hover listener after the tab has loaded

nit: missing dot.

@@ +960,5 @@
>            Services.obs.notifyObservers(browser, NOTIFY_TAB_RESTORED);
>          }
>  
> +        // Remove the mouse hover listener after the tab has loaded
> +        if (tab.__connection_prepared) {

I don't think you need to do this at all, since the tab _will_ be hover once in the near future. And when it fires and the connection has already been made, it'll be a no-op.

@@ +3293,4 @@
>        let select = t == selectTab - 1;
>        let createLazyBrowser = restoreTabsLazily && !select && !tabData.pinned;
>  
> +      let activeIndex = (tabData.index || tabData.entries.length) - 1;

You don't need to move this code.

@@ +3326,4 @@
>            tabbrowser.removeTab(tabbrowser.tabs[0]);
>          }
>          tabsToRemove = 0;
> +        // Prepare connection to the host when users hover mouse over this

nit: You need to align this comment properly with the closing curly brace below.

@@ +3329,5 @@
> +        // Prepare connection to the host when users hover mouse over this
> +        // tab. If we're not restoring on demand, we'll prepare connection
> +        // when we're restoring next tab.
> +      } else if ((!tabData.pinned && this._prefBranch.getBoolPref("sessionstore.restore_on_demand")) ||
> +                 (tabData.pinned && this._prefBranch.getBoolPref("sessionstore.restore_pinned_tabs_on_demand"))) {

Why the two clauses? It seems to me like you can leave `tabData.pinned` out of it.
Getting a pref value is not free, so you need to 'cache' it before its first use at `let restoreTabsLazily`.
Then you can make this simply `if (restoreOnDemand) {`

@@ +3330,5 @@
> +        // tab. If we're not restoring on demand, we'll prepare connection
> +        // when we're restoring next tab.
> +      } else if ((!tabData.pinned && this._prefBranch.getBoolPref("sessionstore.restore_on_demand")) ||
> +                 (tabData.pinned && this._prefBranch.getBoolPref("sessionstore.restore_pinned_tabs_on_demand"))) {
> +          tab.__onMouseOver = () => {

Please make this look better by adding a method `speculativeConnectOnHover(tab, url)`, where you setup this listener and make the connection on tab hover.

@@ +3337,5 @@
> +              let url = tabData.entries[activeIndex].url;
> +              this.prepareConnectionToHost(url);
> +            }
> +          };
> +          tab.addEventListener("mouseover", tab.__onMouseOver);

In that function, please add `, {once: true}` to cleanup the listener properly.

@@ +3431,4 @@
>    },
>  
>    /**
> +   * Prepare connection to host beforehand

nit: missing dot.

@@ +3436,5 @@
> +   * @param url
> +   *        URL of the host
> +   */
> +  prepareConnectionToHost(url) {
> +    if (url) {

There's no need to guard like this; URL is always set, right?
What _would_ be useful here is to check if it's not an 'about:' URL and return if so.

@@ +3711,5 @@
>          this.restoreNextTab();
> +
> +        // Check if the current tab is currently queued and will be restored
> +        // after the currently loading tabs. If so, prepare the connection
> +        // to its current URL's host to speed up page loading.

nit: you're saying 'current' quite a bit here ;-)

Comment 8

[Bao Quan [:beekill]]

Attachment: spec.v2.patch

Changed as you requested.

Mike, how should I write a test for this?

Comment 9

[Mike de Boer [:mikedeboer]]

(In reply to Bao Quan [:beekill] from comment #8)
> Mike, how should I write a test for this?

Not a clue. I think we'll just have to assume it works... the only thing I can think of is that you can test this by stubbing the `prepareConnectionToHost` method in the test to set a flag and in the test you'd assert that the flag was set when you simulate hovering the mouse.

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8878292 [details] [diff] [review]
spec.v2.patch

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

Yeah, this looks good to me, I think it'd be ready for reviews in this state.

Comment 11

[Bao Quan [:beekill]]

Attachment: Bug 874533 - Part 1: Speculative connect when users hover mouse on a tab or when restoring tabs automatically.

Review commit: https://reviewboard.mozilla.org/r/152018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152018/

Comment 12

[Bao Quan [:beekill]]

Attachment: Bug 874533 - Part 2: Test connections prepared when users hover mouse on tabs or when restoring automatically.

Review commit: https://reviewboard.mozilla.org/r/152020/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152020/

Comment 13

[Mike de Boer [:mikedeboer]]

What happened to the approach with the mock 'nsISpeculativeConnect' and/ or 'nsIIOService2' service? `Services.io` is a lazy-loaded instance of the 'nsIIOService2' service, which you probably can mock if you do it before doing anything else in that test.
Because you need to write too much boilerplate code to be able to test it and that's not maintainable.

Comment 14

[Bao Quan [:beekill]]

Attachment: browser_874533.js

I misunderstood you. Stupid me!

I tried to mock the nsISpeculativeConnect, that required me to mock Services.io as well. However, I mocked both of them but it crashed the browser. The browser crashed when nsIClassInfo is queried from Services.io - probably from C++ code.

I attached the test file so you can check it out.

After a search in searchfox, I found out some places use speculative connect like bug 1006103, bug 789889 and bug 790882. These bugs don't have tests for me to follow. So I guess they just assumed it worked.

Comment 15

[Mike de Boer [:mikedeboer]]

I'll take a look at this test today.

Comment 16

[Mike de Boer [:mikedeboer]]

You know what, now that I've taken a good look at how the sessionstore.debug pref is used here, I think it's a nice find and is probably the cheapest way of testing this at the moment. Please continue with the test as-is.
Again, apology for the delay, but that was inevitable.

The code has bitrotted a bit, so please rebase your patches on top of m-c tip. Thanks!

Comment 17

[Bao Quan [:beekill]]

Comment on attachment 8880683 [details]
Bug 874533 - Part 1: Speculative connect when users hover mouse on a tab or when restoring tabs automatically.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152018/diff/1-2/

Comment 18

[Bao Quan [:beekill]]

Comment on attachment 8880684 [details]
Bug 874533 - Part 2: Test connections prepared when users hover mouse on tabs or when restoring automatically.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152020/diff/1-2/

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880683 [details]
Bug 874533 - Part 1: Speculative connect when users hover mouse on a tab or when restoring tabs automatically.

https://reviewboard.mozilla.org/r/152018/#review162810

Comment 20

[Mike de Boer [:mikedeboer]]

Comment on attachment 8880684 [details]
Bug 874533 - Part 2: Test connections prepared when users hover mouse on tabs or when restoring automatically.

https://reviewboard.mozilla.org/r/152020/#review162812

r=me with the test and the test asset renamed to be more descriptive, instead of just the bug number.

Comment 21

[Bao Quan [:beekill]]

Comment on attachment 8880684 [details]
Bug 874533 - Part 2: Test connections prepared when users hover mouse on tabs or when restoring automatically.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152020/diff/2-3/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36fe0b0b3b4c
Part 1: Speculative connect when users hover mouse on a tab or when restoring tabs automatically. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/b194f4fb6731
Part 2: Test connections prepared when users hover mouse on tabs or when restoring automatically. r=mikedeboer

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failures, e.g. in browser/components/sessionstore/SessionStore.jsm:3437:

https://hg.mozilla.org/integration/autoland/rev/6542a177cd648b30394821f1055e548af7b3ae1a
https://hg.mozilla.org/integration/autoland/rev/462ef83227d51df19bf541532cfc1f457a097703

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b194f4fb673102e0606b347a4bc19a201657173f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=115290847&repo=autoland

TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/sessionstore/SessionStore.jsm:3437:17 | newURI's last parameters are optional. (mozilla/no-useless-parameters)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/sessionstore/test/browser_speculative_connect.html:8:8 | Newline required at end of file but not found. (eol-last)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/sessionstore/test/browser_speculative_connect.js:15:2 | Unnecessary semicolon. (no-extra-semi)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/sessionstore/test/browser_speculative_connect.js:38:26 | Strings must use doublequote. (quotes)

Comment 25

[Bao Quan [:beekill]]

Comment on attachment 8880683 [details]
Bug 874533 - Part 1: Speculative connect when users hover mouse on a tab or when restoring tabs automatically.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152018/diff/2-3/

Comment 26

[Bao Quan [:beekill]]

Comment on attachment 8880684 [details]
Bug 874533 - Part 2: Test connections prepared when users hover mouse on tabs or when restoring automatically.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152020/diff/3-4/

Comment 27

[Bao Quan [:beekill]]

Fixed eslint error. Mike, can you land this for me? Thanks!

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d0bcaaf7695
Part 1: Speculative connect when users hover mouse on a tab or when restoring tabs automatically. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/b9710c3f87d5
Part 2: Test connections prepared when users hover mouse on tabs or when restoring automatically. r=mikedeboer

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6d0bcaaf7695
https://hg.mozilla.org/mozilla-central/rev/b9710c3f87d5