Closed Bug 1348278 Opened 7 years ago Closed 7 years ago

Speculatively connect on mousedown on links

Categories

(Core :: Networking, enhancement)

Product:
Core
Component:
Networking
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: phlsa, Assigned: u408661)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(2 files)

Bug 1348278 - Speculatively connect when mousedown happens on a link
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1348278 - Limit speculative connections to http[s] only.
59 bytes, text/x-review-board-request
mcmanus
: review+
Details 
There is about a 50-300ms (sometimes even higher) timeframe between the mousedown and mouseup events when a user clicks on a link.

We could use this intent signal to start loading the new page earlier (in the background) and thereby shorten the perceived load process.

The performance gain here might be particularly useful, since it is shortening the part of the page load where the new page isn't visible yet. Our research has shown that users perceive a browser to be faster if they see the beginning of the page rendering sooner. So those 50-300ms might be perceived as even more of a speedup than they are technically.

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
There is already a bug for this.  Just need to find it.
I can see value in doing this to get to a much better user-perceived input latency during page request as phlsa described.

@phlsa, do you have data that shows the 50-300ms delay in mousedown/up events? Is this something that we track?
Flags: needinfo?(philipp)
If we track it right now, then I'm not aware of it.

Those numbers come from a very limited trial I did using this quick prototype:
https://phlsa.github.io/photon-performance/clicktest.html

It shows you the number of ms between mousedown and mouseup below the giant button.
I tested with various input devices on a couple of systems. Tap-click on a Mac trackpad was the only device where I could get the value below 30ms. Everything else was much higher. Since this measure depends mostly on human motor skills and less on the machine, I think that this simple test can give us a good direction.

It would, of course, be interesting to get a larger sample on this to see where the click durations cluster (I suspect that I'm a fast clicker ;)
Flags: needinfo?(philipp)
Whiteboard: [necko-backlog]
The same issues of bug 1348275 comment 9 and 10 apply here, too.

Sebastian
Blocks: photon-performance-triage
Whiteboard: [necko-backlog] → [necko-backlog][qf]
How would we deal with users changing their mind (e.g. mouse down, then move away) or dragging links, considering that some links perform actions (that could even make the URL useless afterwards; I'm thinking of https://privnote.com/)?
Summary: Preload pages on mousedown on links → Speculatively preload pages on mousedown on links
(In reply to Dão Gottwald [::dao] from comment #5)
> How would we deal with users changing their mind (e.g. mouse down, then move
> away) or dragging links, considering that some links perform actions (that
> could even make the URL useless afterwards; I'm thinking of
> https://privnote.com/)?

My assumption is that we would speculatively connect (ie. prepare the socket and start the ssl negotiation), but not send the actual HTTP request.
Okay, that sounds much less risky.
Summary: Speculatively preload pages on mousedown on links → Speculatively connect pages on mousedown on links
Summary: Speculatively connect pages on mousedown on links → Speculatively connect on mousedown on links
If I read the code and bugzilla history correctly, we already speculatively connect on hovering http links since bug 881804, but it's disabled for https (http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/modules/libpref/init/all.js#1908).
Florian is semi-correct in comment 8 - we speculatively connect to *any* link (http or https) on hover as long as that link appears on a page served via http. We do not speculatively connect on hover to any link appearing on a page served via https. The idea being, we don't want to leak the content of a page served over https, but http is already as good as leaked, so we may as well do it there :)

I think doing this on mousedown even on https pages is more acceptable, as that's a much more explicit user action than link hover.
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #9)
> Florian is semi-correct in comment 8 - we speculatively connect to *any*
> link (http or https) on hover as long as that link appears on a page served
> via http. We do not speculatively connect on hover to any link appearing on
> a page served via https. The idea being, we don't want to leak the content
> of a page served over https, but http is already as good as leaked, so we
> may as well do it there :)
>
> I think doing this on mousedown even on https pages is more acceptable, as
> that's a much more explicit user action than link hover.

Nicholas, are these the tasks necessary to complete this bug?

1. Change the current "connect on mouse hover" behavior to "connect on mousedown".
2. Allow "connect on mousedown" on both HTTP and HTTPS pages (network.predictor.enable-hover-on-ssl) for mousedown on either HTTP and HTTPS links.

Is there anything we still want to do differently for HTTPS pages?
Is there any eager behavior we would still want to do for "connect on mouse hover"?
What does Chrome do for mouse hover or mousedown on links?
Flags: needinfo?(hurley)
Prefetch on mouse-down is privacy friendly, prefetch on hover not. So maybe a popular Ad & Tracking Blocking Addon like uBlock Origin would no longer deactivate prefetching then which would lead to a better UX.

If you more like the hover-prefetching, evaluate this: prefetch on hover for same-host resources/pages and on mousedown for resources/pages from 3rd party hosts
Whiteboard: [necko-backlog][qf] → [necko-backlog][qf:p1]
(In reply to Chris Peterson [:cpeterson] from comment #10)
> Nicholas, are these the tasks necessary to complete this bug?
> 
> 1. Change the current "connect on mouse hover" behavior to "connect on
> mousedown".
> 2. Allow "connect on mousedown" on both HTTP and HTTPS pages
> (network.predictor.enable-hover-on-ssl) for mousedown on either HTTP and
> HTTPS links.
> 
> Is there anything we still want to do differently for HTTPS pages?
> Is there any eager behavior we would still want to do for "connect on mouse
> hover"?
> What does Chrome do for mouse hover or mousedown on links?

I don't know that we need to change the "connect on hover" behaviour so much as just add a "connect on mousedown" behaviour that works on pages served over both http and https. We don't even need to go through the predictor for this - we can just use nsISpeculativeConnect directly.
Flags: needinfo?(hurley)
this is a qf1 bug - so I want to make sure its on your radar.. if it needs to be reassigned let jason know.
Assignee: nobody → hurley
Whiteboard: [necko-backlog][qf:p1] → [necko-active][qf:p1]
Comment on attachment 8862456 [details]
Bug 1348278 - Speculatively connect when mousedown happens on a link

https://reviewboard.mozilla.org/r/134382/#review137336

::: commit-message-c0d35:3
(Diff revision 1)
> +Bug 1348278 - Speculatively connect on mousedown r?bz
> +
> +Try to speed up pageload by opening a connection as soon as the user

This commit message matches the code (mostly; see below), but not the discussion in the bug or the bug summary.

Is the intent to preconnect on mousedown, or on click?

Right now what the code is doing is preconnecting after (1) click has happened and (2) we have taken a trip through the event loop and gotten to the runnable the click posted.  Even if the intent is to do this on click, we could at least do it before that trip through the event loop...  As things stand, we're doing the preconnect pretty much right before we call AsyncOpen anyway.

::: docshell/base/nsDocShell.cpp:14252
(Diff revision 1)
>                           : aContent->NodePrincipal();
>  
> +  // OK, we're pretty sure we're going to load, so warm up a speculative
> +  // connection to be sure we have one ready when we open the channel.
> +  nsCOMPtr<nsIIOService> ios = do_GetIOService();
> +  nsCOMPtr<nsISpeculativeConnect> sc = do_QueryInterface(ios);

do_QueryInterface(nsContentUtils::GetIOService())
Attachment #8862456 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #15)
> Is the intent to preconnect on mousedown, or on click?

mousedown because we want to use the 50-300 ms between the user physically mousing down and up to get a head start on the connection.

Should the SpeculativeConnect happen somewhere like Element::PostHandleEventForLinks()?

Is calling SpeculativeConnect unnecessary for non-HTTPS link mousedown when we are already calling Predictor::PredictForLink() for non-HTTPS link hover?
(In reply to Chris Peterson [:cpeterson] from comment #16)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no
> decent message, automatic r-) from comment #15)
> > Is the intent to preconnect on mousedown, or on click?
> 
> mousedown because we want to use the 50-300 ms between the user physically
> mousing down and up to get a head start on the connection.
> 
> Should the SpeculativeConnect happen somewhere like
> Element::PostHandleEventForLinks()?

This is all fine, makes sense. (I'm not 100% sure on the location for calling SpeculativeConnect, but that's because I'm unfamiliar with the code.)

> Is calling SpeculativeConnect unnecessary for non-HTTPS link mousedown when
> we are already calling Predictor::PredictForLink() for non-HTTPS link hover?

It may be strictly unnecessary, but it's also not harmful. SpeculativeConnect will, by default, not open a connection if a free one already exists. So, the one the predictor creates will cause the raw SpeculativeConnect to be a no-op.
Yes, doing this in the mousedown case in Element::PostHandleEventForLinks would probably make more sense.
Comment on attachment 8862456 [details]
Bug 1348278 - Speculatively connect when mousedown happens on a link

https://reviewboard.mozilla.org/r/134382/#review137724

r=me

::: commit-message-c0d35:1
(Diff revision 2)
> +Bug 1348278 - Speculatively connect on mousedown r?bz

s/on mousedown/when mousedown happens on a link/ ?

::: dom/base/Element.cpp:3075
(Diff revision 2)
> +
> +          // OK, we're pretty sure we're going to load, so warm up a speculative
> +          // connection to be sure we have one ready when we open the channel.
> +          nsCOMPtr<nsISpeculativeConnect> sc =
> +            do_QueryInterface(nsContentUtils::GetIOService());
> +          sc->SpeculativeConnect2(absURI, NodePrincipal(), nullptr);

You could probably QI the nsILinkHandler to nsIInterfaceRequestor here (it's the docshell) and pass it as the third arg.
Attachment #8862456 - Flags: review?(bzbarsky) → review+
So this is causing crashes with (what I think are) blob URIs in marionette: https://treeherder.mozilla.org/logviewer.html#?job_id=95263088&repo=try&lineNumber=2207

I'm going to try limiting to http[s] in Event.cpp to see if that fixes the issue. If so, I suspect we should limit speculative connections to http[s] (and perhaps ftp?) at a lower layer (somewhere in nsISpeculativeConnect) to avoid this issue with other schemes in other places.
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #21)
> So this is causing crashes with (what I think are) blob URIs in marionette:
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=95263088&repo=try&lineNumber=2207
> 
> I'm going to try limiting to http[s] in Event.cpp to see if that fixes the
> issue. If so, I suspect we should limit speculative connections to http[s]
> (and perhaps ftp?) at a lower layer (somewhere in nsISpeculativeConnect) to
> avoid this issue with other schemes in other places.

yes - good catch. just http[s] plz
Yep, that was the issue. New patch (prereq for the original) limits our speculative connections in the IO service.
Comment on attachment 8863015 [details]
Bug 1348278 - Limit speculative connections to http[s] only.

https://reviewboard.mozilla.org/r/134858/#review138086
Attachment #8863015 - Flags: review?(mcmanus) → review+
Pushed by hurley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae9743c32bbb
Limit speculative connections to http[s] only. r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/dc2095a94b5e
Speculatively connect when mousedown happens on a link r=bz
https://hg.mozilla.org/mozilla-central/rev/ae9743c32bbb
https://hg.mozilla.org/mozilla-central/rev/dc2095a94b5e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Sorry for noising up a closed bug and sorry if I missed it when skimming, but does this bug also cover CTRL + Click and middle click situations?
(In reply to Caspy7 from comment #29)
> Sorry for noising up a closed bug and sorry if I missed it when skimming,
> but does this bug also cover CTRL + Click and middle click situations?

The mousedown event is fired for those situations, too. So looking at the patch it does work regardless of how you click the link.

Check Bug 1363772 comment 1 for follow-ups (right click, touch) to this bug, though.
Performance Impact: --- → P1
Whiteboard: [necko-active][qf:p1] → [necko-active]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: