The default bug view has changed. See this FAQ.

speculative connect hint interface

RESOLVED FIXED in mozilla15

Status

()

Core
Networking: HTTP
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla15
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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..
(Assignee)

Updated

5 years ago
Depends on: 729133
(Assignee)

Updated

5 years ago
Assignee: nobody → mcmanus
Summary: allow preconnect to search provider when search box is anticipated to be used → speculative connect hint interface
(Assignee)

Updated

5 years ago
Blocks: 735543
(Assignee)

Comment 1

5 years ago
Created attachment 605902 [details] [diff] [review]
patch v0
Attachment #605902 - Flags: review?(honzab.moz)
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.
Attachment #605902 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 606610 [details] [diff] [review]
patch v1

Christian, can you take a sr look at the interface here?
Attachment #605902 - Attachment is obsolete: true
Attachment #606610 - Flags: superreview?(cbiesinger)
Attachment #606610 - Flags: review+
(Assignee)

Updated

5 years ago
Blocks: 737548
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 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 :)
Attachment #606610 - Flags: superreview?(cbiesinger) → superreview-
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
Created attachment 607841 [details] [diff] [review]
patch 2
Attachment #606610 - Attachment is obsolete: true
Attachment #607841 - Flags: superreview?(cbiesinger)
Attachment #607841 - Flags: review+
Attachment #607841 - Flags: superreview?(cbiesinger) → superreview+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/57f7b1c6ae50
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/57f7b1c6ae50
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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);

Updated

5 years ago
Depends on: 749649
Blocks: 750445
Blocks: 752143

Updated

5 years ago
Blocks: 808104
You need to log in before you can comment on or make changes to this bug.