723628 - speculative connect hint interface

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Patrick McManus [:mcmanus]]

we take un-necessary delay in generating search results from the search box and address bar by not connecting until we know the contents of the search.

hopefully we're making an https connecttion - so that's possibly 3 RTTs of latency (~300ms) that can be overlapped with the typing of the query. I have second hand information chrome does something similar.

I want to start by making a "hints" API for necko where a caller can tell necko that it thinks it will need a connection soon - necko can then check what's available and make one if appropriate. It probably has other uses cases too (e.g. in parallel with disk cache revalidation retrieval).

Then we can work with awesomebar/searchbox implementors to see if this is a good idea for them, clearing any rules we might have with search box sponsors, etc..

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch v0

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 605902 [details] [diff] [review]
patch v0

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

Patrick, you read my mind.

r=honzab

::: netwerk/base/public/nsIIOService.idl
@@ +166,5 @@
> +     *
> +     */
> +    void speculativeConnect(in nsIURI aURI,
> +                            in nsIInterfaceRequestor aCallbacks,
> +                            in nsIEventTarget aTarget);

Are you assuming this method may change in the future?  I presume yes, since otherwise I'd expect nsIOService just implement nsISpeculativeConnect.

::: netwerk/base/public/nsISpeculativeConnect.idl
@@ +42,5 @@
> +interface nsIInterfaceRequestor;
> +interface nsIEventTarget;
> +
> +[scriptable, uuid(b3c53863-1313-480a-90a2-5b0da651ee5e)]
> +interface nsISpeculativeConnect : nsISupports

nsISpeculativeConnectHandler ?

::: netwerk/base/src/nsIOService.cpp
@@ +540,5 @@
> +    if (NS_FAILED(rv))
> +        return rv;
> +
> +    if (!scheme.EqualsLiteral("http") && !scheme.EqualsLiteral("https"))
> +        return NS_OK;

Really necessary to filter these out?  I think extension protocols should be able to get this too.  I then don't see a need to interface this feature on handler implementations that effectively "filters" here already.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1613,5 @@
> +        }
> +    }
> +    // Ensure that this is HTTP or HTTPS, otherwise we don't do preconnect here
> +    else if (!scheme.EqualsLiteral("http"))
> +        return NS_ERROR_UNEXPECTED;

Maybe worth to check strict transport security service here for potential redirect to https and open https rather.

@@ +1632,5 @@
> +    if (NS_FAILED(rv))
> +        return rv;
> +
> +    nsHttpConnectionInfo *ci =
> +        new nsHttpConnectionInfo(host, port, nsnull, usingSSL);

Could we just create a regular entry and info for this tuple?

::: netwerk/test/unit/test_speculative_connect.js
@@ +19,5 @@
> +	do_check_true(true);
> +	do_test_finished();
> +    } ,
> +
> +    onStopListening: function(socket) {}

I think you need a QI implementation.

Comment 3

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #2)

> Are you assuming this method may change in the future?  

yep.. I don't feel like I understand all the ramifications of this space yet - but that's not a good reason to not press forward with things I am confident about. So just trying to keep it flexible where it is easy to do so.

> 
> ::: netwerk/protocol/http/nsHttpHandler.cpp
> @@ +1613,5 @@
> > +        }
> > +    }
> > +    // Ensure that this is HTTP or HTTPS, otherwise we don't do preconnect here
> > +    else if (!scheme.EqualsLiteral("http"))
> > +        return NS_ERROR_UNEXPECTED;
> 
> Maybe worth to check strict transport security service here for potential
> redirect to https and open https rather.

I think this is a good idea and I implemented it. Part of me thinks it isn't worth the check - STS rewrites are a matter of safety/correctness and shouldn't happen hardly ever.. and since this is just speculation we could skip it.

otoh I was convinced by the thought of some piece of persistent chrome or js that used the wrong url, rather than an attempt at subversion, which basically nullified the optimization. So I went with it :)

> 
> @@ +1632,5 @@
> > +    if (NS_FAILED(rv))
> > +        return rv;
> > +
> > +    nsHttpConnectionInfo *ci =
> > +        new nsHttpConnectionInfo(host, port, nsnull, usingSSL);
> 
> Could we just create a regular entry and info for this tuple?

a CI has generally been the interface for anything outside of connection manager.. so that's why I left it that way.

> 
> ::: netwerk/test/unit/test_speculative_connect.js
>
> I think you need a QI implementation.

ok. offhand do you have any documenation or pointers on that topic? There are lots of examples without a QI, and what I had worked fine in try so I just want to understand what is going on better.

Comment 4

[Patrick McManus [:mcmanus]]

Attachment: patch v1

Christian, can you take a sr look at the interface here?

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

xpconnect will autogenerate a QI impl for you as long as you only need to QI to those interfaces (and their base interfaces) that you try to pass it through xpconnect as

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

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

+++ b/netwerk/base/public/nsIIOService.idl
+     * @param aTarget the event target thread for callbacks

This needs better documentation (or removal). It is not at all clear what callbacks this method would produce (and where it sends them).

But better than fixing the documentation would be to just call NS_GetCurrentThread() in the implementation to get an event target.

+++ b/netwerk/base/public/nsISpeculativeConnectHandler.idl

since this method is identical to the one on nsIIOService, have you considered just making the IO service implement this interface instead of adding to nsIIOService?

+++ b/netwerk/base/src/nsIOService.cpp
+    // Check for proxy information. If there is a proxy configured then a
+    // speculative connect should not be performed because the potential
+    // reward is slim with tcp peers closely located to the browser.

It is also likely that we still have a keepalive connection to the proxy :)

Comment 7

[Patrick McManus [:mcmanus]]

biesi - is this more what you had in mind?

     * @param aURI the URI of the hinted transaction
     * @param aCallbacks any security callbacks for use with SSL for interfaces
     *        such as nsIBadCertListener. May be null.
     * @param aTarget the thread on which the release of the callbacks will
     *        occur. May be null for "any thread"

(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #6)

> +++ b/netwerk/base/public/nsISpeculativeConnectHandler.idl
> 
> since this method is identical to the one on nsIIOService, have you
> considered just making the IO service implement this interface instead of
> adding to nsIIOService?
> 

yes - I think we need them both. It can't really be consolidated in the protocol handler because they would all need the centralized proxy lookup code, and consolidating it in io-service means introducing http specific gunk like httpconnectioninfo into that module.

Comment 8

[Patrick McManus [:mcmanus]]

(05:54:20 PM) biesi: mcmanus, I'm saying keep the code in the io service and the code in the protocol handlers, but make nsIOService implement nsISpeculativeConnectHandler
(05:54:26 PM) biesi: instead of adding the declaration to nsIIOService
(05:54:29 PM) biesi: make sense?
(05:56:33 PM) mcmanus: and have callers QI their io-service?
(05:58:40 PM) mcmanus: I put it on nsIIOService because it is sort of an entry point analagous to NewChannel (though obviously of much less utility), so they seemed paired.
(05:58:55 PM) biesi: yeah, have callers QI
(05:59:01 PM) biesi: I guess I don't like duplicating the declaration
(05:59:09 PM) biesi: you could make IIOService inherit from the interface
(06:01:14 PM) mcmanus: I guess I can live with having the caller QI.

Comment 9

[Patrick McManus [:mcmanus]]

Attachment: patch 2

Comment 10

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57f7b1c6ae50

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/57f7b1c6ae50

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 607841 [details] [diff] [review]
patch 2

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

::: netwerk/base/src/nsIOService.cpp
@@ +641,5 @@
> +            pi = nsnull;
> +    }
> +    *outPI = pi;
> +    if (pi)
> +        pi.forget();

pi.forget(outPI);