790882 - [New Tab Page] Speculatively open connections for sites when hovering them

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

While reading about bug 789889 I thought we could maybe do the same for the new tab page when the user hovers one of the thumbnails. I don't think opening connections is particularly expensive and it's only for pages you've already visited or that you pinned yourself on the new tab page.

Comment 1

[Mardeg]

Would there be a need to send the DNT header on these speculative connections? The  tracking of users only on hover without the users' knowledge seems less than ideal.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

AFAIK, this just opens a connection to the server but does not yet send a GET request or the like. So I think we'd be fine with DNT header here as that is sent when the user actually clicks the page.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Checking with Wireshark confirms that this only opens a connection to the server without sending any HTTP requests. The server knows only about the user's IP address so far and shouldn't be able to really use that for tracking purposes.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 660744 [details] [diff] [review]
patch v1

Seems reasonable to me.

Comment 5

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

I don't see a privacy issue with this as no user data is being sent, as long as any DNT pref is sent on the actual connection we should be fine.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Curtis Koenig [:curtisk] from comment #5)
> I don't see a privacy issue with this as no user data is being sent

In a public network, for instance, you may want to visit only certain sites without letting others know about your other favorite sites. Moving the mouse over a thumbnail doesn't necessarily mean you're going to click it, so we're potentially leaking this.

> as long as any DNT pref is sent on the actual connection we should be fine.

See comment 2. DNT doesn't apply here.

Issues like this used to be discussed in security reviews, at a time when something like a privacy review didn't exist. Maybe this is still the case. Then again, there's seems to be no keyword for requesting a security review.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #6)
> Issues like this used to be discussed in security reviews, at a time when
> something like a privacy review didn't exist. Maybe this is still the case.
> Then again, there's seems to be no keyword for requesting a security review.

So there's the sec-review flag (invisible by default). Still, I don't know which review type applies here.

Comment 8

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

(In reply to Dão Gottwald [:dao] from comment #6)
> (In reply to Curtis Koenig [:curtisk] from comment #5)
> > I don't see a privacy issue with this as no user data is being sent
> 
> In a public network, for instance, you may want to visit only certain sites
> without letting others know about your other favorite sites. Moving the
> mouse over a thumbnail doesn't necessarily mean you're going to click it, so
> we're potentially leaking this.
>  
If a user actually loads the page it's going to be known as well. Is this really a threat we need to worry about? If so could we speculatively load several pages without much perf hit?

> > as long as any DNT pref is sent on the actual connection we should be fine.
> 
> See comment 2. DNT doesn't apply here.
> 
OK
> Issues like this used to be discussed in security reviews, at a time when
> something like a privacy review didn't exist. Maybe this is still the case.
> Then again, there's seems to be no keyword for requesting a security review.

(In reply to Dão Gottwald [:dao] from comment #7)
> (In reply to Dão Gottwald [:dao] from comment #6)
> > Issues like this used to be discussed in security reviews, at a time when
> > something like a privacy review didn't exist. Maybe this is still the case.
> > Then again, there's seems to be no keyword for requesting a security review.
> 
> So there's the sec-review flag (invisible by default). Still, I don't know
> which review type applies here.

So the keyword "privacy-review-needed" gets this tagged as needing a look over from a privacy only perspective. sec-review? flag gets this tagged as needing a security reivew (either of the code, the architecture, whatever). In security reviews we also cover privacy as needed.
In this case I don't think a security review is warranted, but a privacy review is still borderline IMO.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Curtis Koenig [:curtisk] from comment #8)
> If a user actually loads the page it's going to be known as well.

That's a call the user can make for that page. For sites the user isn't actually going to load, it's unexpected.

> Is this
> really a threat we need to worry about? If so could we speculatively load
> several pages without much perf hit?

You mean additional hosts from the user's history? How would that improve things?

Comment 10

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to Curtis Koenig [:curtisk] from comment #8)
> > If a user actually loads the page it's going to be known as well.
> 
> That's a call the user can make for that page. For sites the user isn't
> actually going to load, it's unexpected.
> 
> > Is this
> > really a threat we need to worry about? If so could we speculatively load
> > several pages without much perf hit?
> 
> You mean additional hosts from the user's history? How would that improve
> things?

Sounds like we need to make a decision about what the greater good is. Do we go for speed and sacrifice some privacy, or vice-verse? The only other way I can see to do this is with a pref that the user can enable if they want it or a door hanger that warns before first use.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Tom, can you make a call here based on the discussion above? I believe that we shouldn't be concerned about this leak, but we need to make a decision here so we can move this bug along.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #11)
> I believe that we shouldn't be concerned about this leak

Can you elaborate on why you think so?

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [:dao] from comment #12)
> (In reply to Jared Wein [:jaws] from comment #11)
> > I believe that we shouldn't be concerned about this leak
> 
> Can you elaborate on why you think so?

Because I think the user benefit of faster loading times is larger than the rare chance that a user is concerned about leaking a site.

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Jared Wein [:jaws] from comment #13)
> Because I think the user benefit of faster loading times is larger than the
> rare chance that a user is concerned about leaking a site.

Can we quantify the benefit and the chance of concern?  I think you're assuming the benefit is large and the concern is rare, which may or may not be the case.  People often surprise me (as does the series of tubes).

Given that this is just a connection open without pre-sending any HTTP request data whatsoever (comment 3), I don't think we should block this on privacy.  But this pre-connect is potentially surprising to users running a firewall or wireshark or something to see the extra traffic based on cursor movements, and we should be aware of what the benefit and chance of knee-jerk reactions might be.

Comment 15

[Tom Lowenthal]

I agree with Sid: this is a sensible performance enhancement, and the privacy impact is small. I think we're better off with it on by default.

This is definitely one of those features that some users will want off. I don't have good intuition about whether we're talking about an about:config setting, or a checkbox in the options interface. Who's the right UX lead for that call?

I especially agree that this might be an "unexpected" connection for some. I suspect that we want a note in the FF policy documenting it. Jishnu, if you agree, I'll open that bug.

I would also counsel adding a SUMO page documenting it. That could be very effective as a "lightning rod" for any concerns that power-users who independently notice it may have. Hopefully they see the SUMO article and grok the feature rather than tweeting in incomplete-information rage.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tom Lowenthal [:StrangeCharm] from comment #15)
> This is definitely one of those features that some users will want off. I
> don't have good intuition about whether we're talking about an about:config
> setting, or a checkbox in the options interface. Who's the right UX lead for
> that call?

I'll make that call: pref yes, checkbox no. I don't think this rises to the level of something we'd actually expose a visible pref for, but a pref makes plenty of sense for the reasons you mention.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

One other thing to consider: we'll likely have many cases of this kind of behavior (bug 808104 is another example, and I'm sure there are more). It probably makes sense to have the pref apply to all speculative connections in general - I don't think fine-grained control is needed here.

If it becomes a big enough user concern, we could then consider exposing that pref in the prefs UI, but I'd only want to do that if there's significant user feedback that warrants it (and I don't expect there to be).

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Sounds like we've made a call on privacy here. I agree that a user doc on SUMO would be valuable (though I think it should cover speculative connections in general, not just this one), flagging that with user-doc-needed.

Comment 19

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #17)
> One other thing to consider: we'll likely have many cases of this kind of
> behavior (bug 808104 is another example, and I'm sure there are more).

It's not the same kind of behavior. Just opening a new tab makes me randomly hover over thumbnails that I'm not actually going to click on. Bug 808104 and similar bugs propose that we speculatively connect when we're pretty much sure that we'll really end up loading the URI.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

There are varying levels of certainty about whether we will actually load the sites that we're speculatively connecting to, yes. But I don't think we need to split that hair with the user-exposed preference.

Comment 21

[Dão Gottwald [:dao]]

Splitting that hair would make sense since the motivation for adding that pref is privacy, which isn't in danger when we're sure about the about-to-be-requested URI. A binary choice between speed and privacy doesn't make much sense when we can really give users both in /most/ cases.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

You may be right that the specific example I gave is a case where we have 100% certainty. We can omit that one from being controlled by the pref, if you're right. But I think having one pref that controls all cases where the certainty is less than 100% is sufficient, and that's what I was trying to suggest.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

(ref: bug 781006, bug 795303, bug 752143, etc.)

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Attachment: speculatively open connections for sites when hovering them

Comment 25

[:Gavin Sharp [email: [e-mail]]]

We decided against a pref in bug 814169. Is there reason to revisit that decision?

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #25)
> We decided against a pref in bug 814169. Is there reason to revisit that
> decision?

No, sorry I completely missed that discussion.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Attachment: speculatively open connections for sites when hovering them

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 727586 [details] [diff] [review]
speculatively open connections for sites when hovering them

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

::: browser/base/content/newtab/sites.js
@@ +156,5 @@
> +   */
> +  _speculativeConnect: function Site_speculativeConnect() {
> +    let sc = Services.io.QueryInterface(Ci.nsISpeculativeConnect);
> +    let uri = Services.io.newURI(this.url, null, null);
> +    sc.speculativeConnect(uri, null, null);

I couldn't find any usages of speculativeConnect that takes a third argument. This should be:
> sc.speculativeConnect(uri, null);

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Jared Wein [:jaws] from comment #28)
> I couldn't find any usages of speculativeConnect that takes a third
> argument. This should be:
> > sc.speculativeConnect(uri, null);

Huh, back when I wrote the first patch there still was one:
http://hg.mozilla.org/mozilla-central/rev/57f7b1c6ae50

Anyway, thanks for catching that!

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/d6a51ac10751

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d6a51ac10751

Comment 32

[Chris Peterson [:cpeterson]]

I believe this patch triggers the following log messages warning about using mouseenter/mouseleave events. I see these warnings when launching a debug build, first appearing in Nightly build 2013-03-22.

WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file content/events/src/nsEventListenerManager.cpp, line 337
WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file content/events/src/nsEventListenerManager.cpp, line 337
WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file content/events/src/nsEventListenerManager.cpp, line 337
WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file content/events/src/nsEventListenerManager.cpp, line 337

Comment 33

[:Gavin Sharp [email: [e-mail]]]

(In reply to Chris Peterson (:cpeterson) from comment #32)
(fix in bug 854075)

Comment 34

[Virgil Dicu [:virgil] [QA]]

Is there any telemetry data I could use to manually test and compare performance before and after the patch landed in Nightly? I assume just loading a few sites after hovering in the New tab page would do.

Comment 35

[Alex Keybl [:akeybl]]

This didn't end up making the cut for the user facing release notes, although it's a very cool fix :)

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Virgil Dicu [:virgil] [QA] from comment #34)
> Is there any telemetry data I could use to manually test and compare
> performance before and after the patch landed in Nightly? I assume just
> loading a few sites after hovering in the New tab page would do.

You could use Wireshark to see if we open connections when hovering a thumbnail. Whether we see any speed-up or not is probably quite dependent on a whole bunch of variables. It's definitely not getting worse :)

Comment 37

[Virgil Dicu [:virgil] [QA]]

Thanks, Tim. I'll mark this as qa- as this is dependent on other variables and can't be verified accurately.

Comment 38

[Verdi [:verdi] Best to slack me]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #18)
> Sounds like we've made a call on privacy here. I agree that a user doc on
> SUMO would be valuable (though I think it should cover speculative
> connections in general, not just this one), flagging that with
> user-doc-needed.

It looks like this is the article we'd add this information to: https://support.mozilla.org/en-US/kb/how-stop-firefox-automatically-making-connections

Is there a way for someone to turn this off if they want?