Closed Bug 1348275 Opened 7 years ago Closed 7 years ago

speculatively connect to web server(s) on autocomplete when typing in awesomebar

Categories

(Firefox :: Address Bar, enhancement, P1)

Product:
Firefox
Component:
Address Bar
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Iteration:
56.2 - Jul 10
Tracking Flags:
Tracking Status
firefox56 --- disabled
firefox57 --- fixed

People

(Reporter: phlsa, Assigned: jj.evelyn)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [reserve-photon-performance] [fxsearch])

Attachments

(1 file, 3 obsolete files)

Bug 1348275 - speculative connect to an autocomplete url.
59 bytes, text/x-review-board-request
mak
: review+
Details 
When the awesomebar autocompletes to a URL, that is a strong signal that the user has an intent to visit that page. We have a hypothesis that we could speed up the perceived load time of pages if we start preloading pages in the background once we get that intent signal.

This could be particularly valuable, since a behavior that we see in user tests all the time is that users actually type out entire domains, even though it is getting autocompleted. For those users, we could make pages appear to load almost instantly. For users who accept autocomplete results faster, we'd still get a good speed gain in most cases.

We'd probably have to insert some delay before starting the load process, especially when only very few characters have been typed.

This bug is part of a number of efforts to use intent signals in order to kick off processes faster. They are documented here:
https://docs.google.com/document/d/1U-n-RJSEjsRGnNZz6hInZjAYA2TibMDUeYZevy2KWOs/edit#heading=h.d2ozn4fomtnz
ni? :jduell for input on what preload options necko currently has.

This also seems like something we could easily experiment on to measure hit rate, impact on bandwidth and page load performance.
Flags: needinfo?(jduell.mcbugs)
Just a note this idea raises from time to time.  Last time I remember it was denied for privacy issues.  It leaks your history results to the world, which is pretty much undesired.  Would have to find the bug#...
(In reply to Honza Bambas (:mayhemer) from comment #2)
> Just a note this idea raises from time to time.  Last time I remember it was
> denied for privacy issues.  It leaks your history results to the world,
> which is pretty much undesired.

Would DNS prefetch of autocomplete suggestions' hostnames be a privacy leak?
Blocks: Quantum
(In reply to Chris Peterson [:cpeterson] from comment #3)
> (In reply to Honza Bambas (:mayhemer) from comment #2)
> > Just a note this idea raises from time to time.  Last time I remember it was
> > denied for privacy issues.  It leaks your history results to the world,
> > which is pretty much undesired.
> 
> Would DNS prefetch of autocomplete suggestions' hostnames be a privacy leak?

sure, it would be (that's why it has been rejected in the past). I think it might be a reasonable tradeoff though - but be explicit.

so dns and tcp and tls handshakes are pretty easy to do optimistically - they are basically latnency bound and don't cost much. I'm a big fan of doing a lot of this (where we are satisfied on the privacy side).

downloading stuff is harder - and its not just the cost of the bandwidth - it can be hard to shift rapidly from one download to another (as you might want to do as the awesomebar partial results shift around) because they fill buffers (both those in firefox, but also those in the kernel, and those in various routers.. adsl routers tend to have huge buffers) and for the most part you just have to let those buffers drain before you can get anything else through them.. that's a real problem if you are not blocking confirmed content behind speculative content. So you might actually find you make responsiveness worse if you're over aggressive.

honza has been running into some of this in an effort to improve pageload time in the face of concurrent background downloads. It can be tough to make the system react quickly when downloads have filled all those buffers (tho some of the buffering is under our control and we want to do better, but again TCP needs some buffering in order to fill the whole pipe - so its a very tricky balance).

the takeaway here might just be 'be conservative' - I'm not saying it can't work.. but perhaps measuring what you can get with the setup stuff (dns/tcp/tls) would be a good start. nsISpeculativeConnect already provides an interface for that.. it wouldn't really need core::netwokring work.
I'm not sure who makes the call about privacy here, but I could certainly see us doing a mode where by default we do speculative dns/tcp/tls and maybe sometimes prefetch-into-cache, but we don't do it in private browsing or tracking protection or TOR, and possibly we have UI in settings (or just an about:config pref) to manually tweak the pref.  My gut sense is that we shouldn't avoid this optimization by default out of privacy concerns.

I think Patrick is right that the first step here is to use nsISpeculativeConnect. That should give us the low-hanging fruit fairly easily and quickly.  Longer term I could see a possible case for doing a full prefetch-into-cache at times: if I've gone to facebook.com the last 99 times I start typing "fa" into the awesomebar, it could make sense. (OTOH even in this case it's pretty likely that the user will quickly hit return and navigate to facebook once the awesomebar suggests it, and as long as they do that more quickly than it takes to set up TLS, there's no win by doing prefetch-into-cache--we won't talk to the server any sooner).  I'm not sure the network predictor has the info we'd need to do this--might need to keep a new database in the awesomebar or something like that.

Anyway, for the 57/Quantum timeframe I think just calling nsISpeculativeConnect from the awesomebar code once it has a solid prediction is the way to go.  I'm not sure how to quantify "solid prediction" for you, but connection setup is fairly cheap.
Flags: needinfo?(jduell.mcbugs)
Summary: Preload web pages on autocomplete when typing in awesomebar → speculatively connect to web server(s) on autocomplete when typing in awesomebar
We should get this implemented behind a pref to run it as field trial. Which team would own implementation, is Networking the right component?
awesomebar team should own it - the necessary interface exists (nsISpeculativeConnect)
Moving this bug to the Firefox::Location Bar component.
Component: Networking → Location Bar
Product: Core → Firefox
Whiteboard: [fxsearch]
Blocks: photon-ui-refresh
Whiteboard: [fxsearch] → [photon] [fxsearch]
Blocks: photon-performance-triage
No longer blocks: 1349424
No longer blocks: photon-ui-refresh
How would this prefetching affect the data shown for the requests in the DevTools' Network Monitor? Or could prefetching be disabled when the DevTools are open?

Sebastian
Also, users should be able to turn off the prefetching when they have a slow internet connection or pay by transferred data.

Sebastian
Hey bsmedberg, know who we'd talk to about the potential privacy implications for this project? Is speculative connection from things typed in the URL bar something we'd ever consider shipping to our release population?
Flags: needinfo?(benjamin)
I am not the decision-maker for this: data steward is explicitly about data we send back to Mozilla, not privacy risks in general. I expect that the review of this needs to take the following form:

* benefit/risk analysis - I'm not sure who the right person is for this, but I'll try to find somebody by EOD
* user control decision - If UX has already determined that this behavior will be under user control, what will that look like? If not, control options may be a factor in...
* final product decision. At the end of the day, this is a product decision and needs to be made by a PM who is well informed about risks and options. I expect that PM is either Jeff or pdol.
Flags: needinfo?(benjamin)
Tanvi Vyas would normally be the privacy lead, but since she's away for a while Dan Veditz will be the lead to assist with risk analysis. Please coordinate directly with him.
Thanks. dveditz, how should we go about having this idea evaluated?
Flags: needinfo?(dveditz)
This could be more privacy-friendly:
I would like to draw attention on bug 1158191 comment 41. Better to have some telemetry earlier than none at all. I suggested to take HSTS as signal because HSTS preloading is hard to get out from, but everyone can set a HSTS header. (I've seen many banks having HSTS without preloading.)
See Also: → 1355451
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [photon] [fxsearch] → [photon-performance] [fxsearch]
(In reply to Mike Conley (:mconley) from comment #14)
> Thanks. dveditz, how should we go about having this idea evaluated?

I suppose formally it could be a "pi-request" (https://mana.mozilla.org/wiki/display/PI/PI+Request). Informally:

* we shouldn't do this in private browsing mode (leaks your non-private autocompletes into your private context)

* people/distros/TorBrowser need a way to turn this off (about:config is fine)

* DNS lookups and connections seem fine

* I'd want to spend more time/talk to more people if you want to pre-fetch URLs (though from comment 4 sounds like that might raise technical problems anyway).
Flags: needinfo?(dveditz)
Thank for your input, dveditz. I am going to follow up here and make a WIP so we could firstly verify if that really gain "perceived performance". If it works well, I will send a pi-request for privacy review.
Blocks: photon-perf-spec-connect
No longer blocks: photon-performance-triage
Priority: P2 → P3
Whiteboard: [photon-performance] [fxsearch] → [reserve-photon-performance] [fxsearch]
Comment on attachment 8878446 [details]
Bug 1348275 - speculative connect to an autocomplete url.

Hi Florian, 
this wip patch inserts speculative connection setup on the point of fill-in text to urlbar. I'm not sure if this is the best place though. May I have your feedback? Thank you!
Attachment #8878446 - Flags: feedback?(florian)
Assignee: nobody → ehung
Priority: P3 → P1
Comment on attachment 8878446 [details]
Bug 1348275 - speculative connect to an autocomplete url.

https://reviewboard.mozilla.org/r/149794/#review154466

::: browser/base/content/urlbarBindings.xml:248
(Diff revision 1)
> +          // We shouldn't leak autocomplete result in the private context.
> +          if (!Services.prefs.getBoolPref("browser.urlbar.speculativeConnection.enabled") ||
> +              this.inPrivateContext) {
> +            return;
> +          }
> +          let sc = Services.io.QueryInterface(Components.interfaces.nsISpeculativeConnect);

When calling QueryInterface in JS, it adds the interface to the existing object, so we don't need the sc variable here.

Given that we intend to use speculativeConnect in lots of places in the UI, I think it would make sense to do the QueryInterface call in Services.jsm, like http://searchfox.org/mozilla-central/rev/20d16dadd336e0c6b97e3e19dc4ff907744b5734/toolkit/modules/Services.jsm#57 and then just use Services.io.speculativeConnect...

::: browser/base/content/urlbarBindings.xml:254
(Diff revision 1)
> +          try {
> +            let uri = makeURI("http://" + aUriString);
> +            let originAttr = document.docShell.getOriginAttributes();
> +            let principal = Services.scriptSecurityManager.createCodebasePrincipal(uri, originAttr);
> +            sc.speculativeConnect2(uri, principal, null);
> +          } catch(e) { }

Which error is this catching? Why are we dropping it on the floor?

::: toolkit/content/widgets/autocomplete.xml:198
(Diff revision 1)
>          <parameter name="aReason"/>
>          <body><![CDATA[
>            if (aReason == Components.interfaces.nsIAutoCompleteInput
>                                     .TEXTVALUE_REASON_COMPLETEDEFAULT) {
>              this._disableTrim = true;
> +            // In the cause of default complete the result on the urlbar.

I don't understand this comment.

::: toolkit/content/widgets/autocomplete.xml:200
(Diff revision 1)
>            if (aReason == Components.interfaces.nsIAutoCompleteInput
>                                     .TEXTVALUE_REASON_COMPLETEDEFAULT) {
>              this._disableTrim = true;
> +            // In the cause of default complete the result on the urlbar.
> +            // we may do speculative connection as a performance optimization.
> +            if (typeof this.maybeSetupSpeculativeConnection == "function") {

Why the typeof check here? Isn't a simple
  if (this.maybeSetupSpeculativeConnection) {
enough?
Comment on attachment 8878446 [details]
Bug 1348275 - speculative connect to an autocomplete url.

I'm not very familiar with the autocomplete code, I would suggest requesting feedback from Marco (::mak) on the next version of the patch.

Have you thought about writing a test for this?
Attachment #8878446 - Flags: feedback?(florian) → feedback+
Comment on attachment 8878446 [details]
Bug 1348275 - speculative connect to an autocomplete url.

https://reviewboard.mozilla.org/r/149794/#review154466

> Which error is this catching? Why are we dropping it on the floor?

Both makeURI and speculativeConnect2 will throw errors, and I think in this case we just don't care. 
I have made a comment in the `catch(e) {}`.

> Why the typeof check here? Isn't a simple
>   if (this.maybeSetupSpeculativeConnection) {
> enough?

Since the method is defined in another file (urlbarBindings.xml) so it's better to check if it's a function.
(In reply to Florian Quèze [:florian] [:flo] from comment #21)
> Comment on attachment 8878446 [details]
> Bug 1348275 - set up speculative connection for a frequently visit sites.
> 
> I'm not very familiar with the autocomplete code, I would suggest requesting
> feedback from Marco (::mak) on the next version of the patch.
> 
> Have you thought about writing a test for this?

Thank you for the feedback.
It's always good to write a test but I'm not sure how to verify that we've successfully set up a _real_ network connection. I guess we could just mock `speculativceConnect2` and ensure it is(or isn't) called. Is it good enough?
Flags: needinfo?(florian)
(In reply to Evelyn Hung [:evelyn] from comment #23)
> Comment on attachment 8878446 [details]
> Bug 1348275 - set up speculative connection for a frequently visit sites.
> 
> https://reviewboard.mozilla.org/r/149794/#review154466
> 
> > Which error is this catching? Why are we dropping it on the floor?
> 
> Both makeURI and speculativeConnect2 will throw errors, and I think in this
> case we just don't care. 
> I have made a comment in the `catch(e) {}`.

In which case do you see speculativeConnect2 throw errors? http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/netwerk/base/nsIOService.cpp#1829 looks a lot like it'll not throw.
(it actually crashes when given a null URI, I just filed bug 1374238 on this, but a try/catch won't help with that)

Can we catch only the specific error we care about?

Also, why are you adding "http://"? How do you know it's not an https connection that we need?

> > Why the typeof check here? Isn't a simple
> >   if (this.maybeSetupSpeculativeConnection) {
> > enough?
> 
> Since the method is defined in another file (urlbarBindings.xml) so it's
> better to check if it's a function.

I think a nullcheck is enough. I don't see why another file would define a symbol with the same name that wouldn't be a function.
Flags: needinfo?(florian)
(In reply to Evelyn Hung [:evelyn] from comment #24)

> > Have you thought about writing a test for this?
> 
> It's always good to write a test but I'm not sure how to verify that we've
> successfully set up a _real_ network connection. I guess we could just mock
> `speculativceConnect2` and ensure it is(or isn't) called. Is it good enough?

I would prefer if we could setup an http server, and verify that it receives a connection.

I know some search xpcshell tests use an http server (http://searchfox.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/head_search.js#345). It looks like some browser chrome tests also do: http://searchfox.org/mozilla-central/search?q=new+HttpServer&case=false&regexp=false&path=browser_
Comment on attachment 8878446 [details]
Bug 1348275 - speculative connect to an autocomplete url.

https://reviewboard.mozilla.org/r/149794/#review154996

::: browser/base/content/urlbarBindings.xml:249
(Diff revision 2)
> +          if (!Services.prefs.getBoolPref("browser.urlbar.speculativeConnection.enabled") ||
> +              this.inPrivateContext) {
> +            return;
> +          }
> +          try {
> +            let uri = makeURI("http://" + aUriString);

Is there a way that I can get the correct scheme here? I tried `Services.uriFixup.getFixupURIInfo` but not working. 

For HTTP/2 connection, I was told that the implemation of speculativeConnect2 will automiatically upgrade in the networking layer if the server tells us to use https instead of http.

::: browser/base/content/urlbarBindings.xml:251
(Diff revision 2)
> +            return;
> +          }
> +          try {
> +            let uri = makeURI("http://" + aUriString);
> +            Services.io.speculativeConnect2(uri, gBrowser.contentPrincipal, null);
> +          } catch(ex) {

Sorry for a lint error here. I will fix it before landing.
(In reply to Florian Quèze [:florian] [:flo] from comment #25)
> 
> In which case do you see speculativeConnect2 throw errors?
> http://searchfox.org/mozilla-central/rev/
> 7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/netwerk/base/nsIOService.cpp#1829
> looks a lot like it'll not throw.
> (it actually crashes when given a null URI, I just filed bug 1374238 on
> this, but a try/catch won't help with that)
> 

No, every NS_ENSURE_SUCCESS has a chance to return error, and 
`nsProtocolProxyService::AsyncResolve` might return NS_ERROR too.

> Can we catch only the specific error we care about?
> 
> Also, why are you adding "http://"? How do you know it's not an https
> connection that we need?

You are right, I don't think adding "http://" here is right. An issue is opened on reviewboard for getting ::mak's feedback.

> 
> > > Why the typeof check here? Isn't a simple
> > >   if (this.maybeSetupSpeculativeConnection) {
> > > enough?
> > 
> > Since the method is defined in another file (urlbarBindings.xml) so it's
> > better to check if it's a function.
> 
> I think a nullcheck is enough. I don't see why another file would define a
> symbol with the same name that wouldn't be a function.

I think it's better to not assume it must be a function because autocomplete.xml and urlbarBindings.xml are two different modules. I also want to follow the convention in this file.
Attached patch test-wip.patch (obsolete) — — Splinter Review 
Try to hack httpd.js to get connection number so we know the networking has set up our speculative connection. Not a working wip though. :(
Status: NEW → ASSIGNED
I found that the auto-completing value ignores _port_ part, which makes it hard to test with httpd.js. I need to launch a testing http server and bind it on a random port (or fixed but can't be 80). The problem I meet is that, even though I could manipulate visited Places, I couldn't get the correct value (host:port) from the urlbar.

I also noticed that my speculative connection is always failed with some reasons when connecting to a local testing server that I setup for. I got logs but don't really know what it tells. I'll paste it here in the next comment.
Attached file network.log (obsolete) — 
This was a failed case that I tried to setup a speculative connection to "http://moz.local:8000". It seemed that the connection was created but got closed immediately. From the server side I didn't see any incoming connection.
Thanks to :schien, I figured out the networking issue. In bug 992611, we turned off the speculative connection in mochitest by a pref. Also we can't use any host name other than 'localhost', even the name is on the testing domain list.

So the only problem in my test now is "how can I get the correct uri {scheme://host:port}" from the autocomplete result. The http server I run for the test is listening to a random port. I use `PlacesTestUtils.addVisits` to add a visited uri with something like "http://localhost:57147". However, the autocomplete result always gives me "localhost/" and I can see "http://localhost:57147" in the popup menu (as the 2nd item). I wonder why the schema and port information are missing. :mak, is this by design?
Flags: needinfo?(mak77)
Updated test case.
Attachment #8879510 - Attachment is obsolete: true
Attachment #8880085 - Attachment is obsolete: true
(In reply to Evelyn Hung [:evelyn] from comment #32)
> So the only problem in my test now is "how can I get the correct uri
> {scheme://host:port}" from the autocomplete result. The http server I run
> for the test is listening to a random port. I use
> `PlacesTestUtils.addVisits` to add a visited uri with something like
> "http://localhost:57147". However, the autocomplete result always gives me
> "localhost/" and I can see "http://localhost:57147" in the popup menu (as
> the 2nd item). I wonder why the schema and port information are missing.
> :mak, is this by design?

I was looking at the test.
You can likely use promiseAutocompleteResultPopup(searchtext); instead of
  gURLBar.inputField.value = searchtext.substr(0, searchtext.length - 1);
  EventUtils.synthesizeKey(searchtext.substr(-1, 1), {});
  await promiseSearchComplete();

Regarding the autocomplete result, autoFill at this time doesn't properly support port numbers (bug 764062), and that means that you indeed will get just localhost/ even if the visit was to localhost:some_port.
Flags: needinfo?(mak77)
Comment on attachment 8878446 [details]
Bug 1348275 - speculative connect to an autocomplete url.

https://reviewboard.mozilla.org/r/149794/#review156628

Please, try these suggestions and let me know if things don't work as expected, I didn't try it and I'm mostly going by memory.

::: browser/app/profile/firefox.js:302
(Diff revision 2)
>  #endif
>  
>  // Control autoFill behavior
>  pref("browser.urlbar.autoFill", true);
>  pref("browser.urlbar.autoFill.typed", true);
> +pref("browser.urlbar.speculativeConnection.enabled", true);

nit: What about we s/Connection/Connect/ both here and in urlbarBindings? it seems easier to read and discuss about in general.

::: toolkit/content/widgets/autocomplete.xml:203
(Diff revision 2)
> +            // The default completing url must be the user frequently visiting
> +            // website, so we could do speculative connection as a performance
> +            // optimization.
> +            if (typeof this.maybeSetupSpeculativeConnection == "function") {
> +              this.maybeSetupSpeculativeConnection(aValue);
> +            }

I'm wondering if you couldn't instead hook up in urlbarbindings onResultsAdded, inside the
if (this.selectedIndex == -1 && this._isFirstResultHeuristic) {
so we leave autocomplete.xml alone (it already contains too much location bar related stuff).

In particular, you may change _isFirstResultHeuristic to _firstResultStyles, that would return an array of the styles, and replace all calls to _isFirstResultHeuristic with _firstResultStyles.includes("heuristic").

Then here you could (pseudocode)
if (this.selectedIndex == -1) {
  let styles = _firstResultStyles;
  if (styles.includes("heuristic")) {
    ...
    if (styles.includes("autofill")) {
      // Speculative connect.
      let href = this.input.mController.getfinalCompleteValueAt(0);
      ...
    }

Note that the finalCompleteValue will include any scheme but http. So this solves your "how to get the right scheme", but you'll have to detect if you need to add the "http://" prefix (maybe could just look for "://" in this case, since all the schemes we autofill have it).

Another alternative, may be to hook into UnifiedComplete.js:
http://searchfox.org/mozilla-central/source/toolkit/components/places/UnifiedComplete.js#1707
The only advantage is that it would likely start a few milliseconds earlier BUT you wouldn't have the contentPrincipal, so maybe it's a no-go.
Attachment #8878446 - Flags: review?(mak77)
Keywords: perf
Blocks: 481503
Comment on attachment 8878446 [details]
Bug 1348275 - speculative connect to an autocomplete url.

https://reviewboard.mozilla.org/r/149794/#review157148

The approach looks good, there's still a couple things to do yet:
1. include the fixed test in the patch for review (I think I suggested some changes to the test as well in previous comments)
2. Likely you want to set browser.urlbar.speculativeConnect.enabled to false in testing/profiles/prefs_general.js because the test harness is not allowed to connect to the outside world, and I can imagine this may end up doing unexpected connections. You should then enable the pref only in your test.
3. A Green-ish Try Run with all of this

::: browser/base/content/urlbarBindings.xml:2140
(Diff revision 3)
> +            // If this is the first time we get the result from the current
> +            // search, and the result is an "autofill" result, that means it's
> +            // the site that user frequently visits. Then we could speculatively
> +            // connect to this site as a performance optimization.
> +            if (!this.input.gotResultForCurrentQuery &&
> +                this.input.mController.getStyleAt(0).includes("autofill")) {

you should at least also check this.input.mController.matchCount > 0

That's why previously I suggested making _isFirstResultHeuristic more generic, since it does that check already, and we'd pay the xpcom crossing cost only once here.
Attachment #8878446 - Flags: review?(mak77)
Update the offline discussion with ::mak: since it's hard to manage a http server for testing real connection being set up, I am going to write my test by mocking `maybeSetupSpeculativeConnect` and ensure we call it. I will also make some comments in the test to write down issues I met to test with a real http server so we could improve the test one day.
Comment on attachment 8878446 [details]
Bug 1348275 - speculative connect to an autocomplete url.

https://reviewboard.mozilla.org/r/149794/#review158296

::: browser/base/content/test/urlbar/browser_urlbar_search_speculative_connect.js:29
(Diff revision 4)
> +    transition: Ci.nsINavHistoryService.TRANSITION_TYPED,
> +  }]);
> +
> +  let oldSpeculativeConnect = gURLBar.popup.maybeSetupSpeculativeConnect.bind(gURLBar.popup);
> +  gURLBar.popup.maybeSetupSpeculativeConnect = (uriString) => {
> +    is(true, true, "maybeSetupSpeculativeConnect has been called");

I think this is not enough. If this should not be invoked, the only thing that would happen is one less logged PASS in the output. But it wouldn't fail the test.
Instead you should probably resolve a promise here, and wait for the promise resolution after the value has been autofilled, so that, if this isn't invoked, at least the test will timeout.

::: browser/base/content/test/urlbar/browser_urlbar_search_speculative_connect.js:42
(Diff revision 4)
> +
> +async function promiseTestResult(test) {
> +  info(`Searching for '${test.search}'`);
> +
> +  await promiseAutocompleteResultPopup(test.search);
> +  info(`Result: ${gURLBar.popup.richlistbox.firstChild}`);

does this actually print something useful? I suspect it just prints out xul stuff?
I don't think this info is useful, in general.

::: browser/base/content/urlbarBindings.xml:2152
(Diff revision 4)
> +            // If this is the first time we get the result from the current
> +            // search, and the result is an "autofill" result, that means it's
> +            // the site that user frequently visits. Then we could speculatively
> +            // connect to this site as a performance optimization.
> +            if (!this.input.gotResultForCurrentQuery &&
> +                this.input.mController.getStyleAt(0).includes("autofill")) {

This still needs to check this.input.mController.matchCount > 0 before the getStyleAt call.
Indeed we may get an empty result and then the getStyleArt would likely throw.
Attachment #8878446 - Flags: review?(mak77)
There was a problem in the test so my mock `maybeSetupSpeculativeConnect` wasn't invoked. In this version I fix it and also go back to test with a real http server (it failed because it did not do the speculative connect, sorry!) This time it works on try server! \o/
A little trick here is that we should turn IPv6 off to avoid some weird networking issue, and also because the http server only accept IPv4 connection.
Attachment #8880282 - Attachment is obsolete: true
Comment on attachment 8878446 [details]
Bug 1348275 - speculative connect to an autocomplete url.

https://reviewboard.mozilla.org/r/149794/#review158800

::: browser/base/content/test/urlbar/browser_urlbar_search_speculative_connect.js:27
(Diff revision 5)
> +  }
> +  is(gHttpServer.identity.has(gScheme, gHost, gPort), true, "make sure we have this domain listed");
> +
> +  const SPECULATIVE_CONNECT_PREF = "browser.urlbar.speculativeConnection.enabled";
> +  // In mochitest this number is 0 by default but we have to turn it on.
> +  const SPECULATVIE_CONNECT_LIMIT_PREF = "network.http.speculative-parallel-limit";

there's a typo in SPECULATVIE that is copy/pasted around in the test.

::: browser/base/content/test/urlbar/browser_urlbar_search_speculative_connect.js:34
(Diff revision 5)
> +  // problem.
> +  const DISABLE_IPv6_PREF = "network.dns.disableIPv6";
> +
> +  Services.prefs.setBoolPref(SPECULATIVE_CONNECT_PREF, true);
> +  Services.prefs.setIntPref(SPECULATVIE_CONNECT_LIMIT_PREF, 6);
> +  Services.prefs.setBoolPref(DISABLE_IPv6_PREF, true);

nit: you may use await SpecialPowers.pushPrefEnv to set temporary prefs for the test, it will automatically undo them at the end of the test and may be more robust then clearUserPref in registerCleanupFunction

::: browser/base/content/test/urlbar/browser_urlbar_search_speculative_connect.js:83
(Diff revision 5)
> +      is(gHttpServer.connectionNumber, 1,
> +         `${gHttpServer.connectionNumber} speculative connection has been setup.`)
> +      return gHttpServer.connectionNumber == 1;
> +    }
> +    return false;
> +  }, "Waiting for connection setup", 1000, 500);

The interval and maxTries here look a bit too large (this is requesting to try every second for 500 seconds...). Aren't the default values good for this? (that is try every 100ms for a maximum of 5 seconds)
I honestly think you can just remove both values from here and let waitForCondition fallback to the defaults.

::: browser/base/content/urlbarBindings.xml:2154
(Diff revision 5)
> +            // the site that user frequently visits. Then we could speculatively
> +            // connect to this site as a performance optimization.
> +            if (!this.input.gotResultForCurrentQuery &&
> +                this.input.mController.matchCount > 0 &&
> +                this.input.mController.getStyleAt(0).includes("autofill")) {
> +                let uri = this.input.mController.getFinalCompleteValueAt(0);

The body of the if should only be indented by 2 spaces, not 4, per coding style.
Attachment #8878446 - Flags: review?(mak77) → review+
I'm not sure if this still need a PI request for reviewing anything, I've done what dveditz said in comment 16 (avoid leakage in private browsing mode and a pref in about:config). I'd like to land it first and we can back it out if there is any concern.
Pushed by ehung@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/071beab1c31e
speculative connect to an autocomplete url. r=mak
https://hg.mozilla.org/mozilla-central/rev/071beab1c31e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Iteration: --- → 56.2 - Jul 10
Depends on: 1384335
Depends on: 910207
No longer depends on: 1384335
Since bug 910207 has been backout and probably can't uplift to fx56 in time, I guess it's better to backout this one on _Fx56_. (For 57, I assume we still have some time.)

Ryan, I am not sure how to flag here in this case... could you help? Thanks! :)
Flags: needinfo?(ryanvm)
Redirecting to Liz. Happy to do it before we build 56b12 once she signs off.
Flags: needinfo?(ryanvm) → needinfo?(lhenry)
Yes, please go ahead and back this out today. Thanks very much for catching this!
Flags: needinfo?(lhenry) → needinfo?(ryanvm)
wait, why are we backing out patches, we have a pref for the feature!
Depends on: 1399599
Filed bug 1399599 to flip the pref
Flags: needinfo?(ryanvm)
Bug 1399599 disabled speculative connections from the address bar in 56. It remains enabled in 57.
status-firefox56: fixed → disabled
status-firefox57: --- → fixed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: