Last Comment Bug 723628 - speculative connect hint interface
: speculative connect hint interface
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: unspecified
: x86_64 Windows 7
: -- enhancement (vote)
: mozilla15
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
Depends on: 729133 749649
Blocks: 752143 808104 735543 737548 750445
  Show dependency treegraph
 
Reported: 2012-02-02 10:56 PST by Patrick McManus [:mcmanus]
Modified: 2012-11-02 11:33 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v0 (21.85 KB, patch)
2012-03-14 12:44 PDT, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review
patch v1 (22.51 KB, patch)
2012-03-16 09:52 PDT, Patrick McManus [:mcmanus]
mcmanus: review+
cbiesinger: superreview-
Details | Diff | Splinter Review
patch 2 (21.39 KB, patch)
2012-03-20 20:06 PDT, Patrick McManus [:mcmanus]
mcmanus: review+
cbiesinger: superreview+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2012-02-02 10:56:06 PST
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] 2012-03-14 12:44:28 PDT
Created attachment 605902 [details] [diff] [review]
patch v0
Comment 2 Honza Bambas (:mayhemer) 2012-03-15 19:39:41 PDT
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] 2012-03-16 09:45:02 PDT
(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] 2012-03-16 09:52:04 PDT
Created attachment 606610 [details] [diff] [review]
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) 2012-03-20 12:47:46 PDT
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) 2012-03-20 13:01:53 PDT
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] 2012-03-20 14:04:13 PDT
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] 2012-03-20 15:04:18 PDT
(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] 2012-03-20 20:06:59 PDT
Created attachment 607841 [details] [diff] [review]
patch 2
Comment 10 Patrick McManus [:mcmanus] 2012-04-25 06:12:54 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/57f7b1c6ae50
Comment 11 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-25 20:39:46 PDT
https://hg.mozilla.org/mozilla-central/rev/57f7b1c6ae50
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2012-04-26 04:36:49 PDT
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);

Note You need to log in before you can comment on or make changes to this bug.