640213 - Implement RFC 2616 "Upgrade" (Section 14.42)

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Patrick McManus [:mcmanus]]

HTTP provides a mechanism, using the "upgrade" request header, to bootstrap a new TCP protocol from HTTP. Basically, an upgrade token is sent in the request and if the server understands the protocol specified by the token it responds with a a 101, and after that HTTP response is complete the bidi TCP stream is speaking the new protocol.

The websocket protocol uses this approach.

The patch here provides a new interface for upgradable http channels, and teaches our http stack to implement it.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: http upgrade v1

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: http upgrade v2

Update this patch to deal with data past the 101 response that might have accidentally been read by the transaction instead of being passed to the upgraded stream.

Comment 3

[Honza Bambas (:mayhemer)]

Few nits:
- couldn't we use pipe [1] for what nsPreloadedStream is used?  I assume it will do the same for you.  No need to duplicate a code we already have.
- please add a good documentation to your new interfaces, it is not clear which one is for what
- you are missing tests for all the new code paths you add

[1] http://mxr.mozilla.org/mozilla-central/ident?i=NS_NewPipe2

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to comment #3)
> Few nits:
> - couldn't we use pipe [1] for what nsPreloadedStream is used?  I assume it
> will do the same for you.  No need to duplicate a code we already have.

I'm not seeing it - maybe you can help me. I could use pipe but then I would be responsible for producing _all_ of the data that is read out of the inputstream inside websockets.

I don't really want to do that - it is generally fine to read it right off the socket except for the first few bytes which might be replayed because we read-too-much from the http layer. The preloadedstream allows me to replay that hand full of bytes and then just get out of the way instead of having one service reading from the socket, writing to the pipe and then another reading it back out of the pipe.. but maybe I misunderstand the way the pipe works?

> - you are missing tests for all the new code paths you add

the websocket mochitests actually exercise this thoroughly though it would be nice to specify some specific unit tests, yes. but it is exercised by the test base.

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to comment #4)
> (In reply to comment #3)
> > Few nits:
> > - couldn't we use pipe [1] for what nsPreloadedStream is used?  I assume it
> > will do the same for you.  No need to duplicate a code we already have.
> 
> I'm not seeing it - 
Ah, I missed the stream you read after the buffer is consumed.  Then you really need a new class, there is nothing known to me that could do this for you.  We have a multiplex input stream, but it is not async.

You might want to move this new class and the interface to xpcom/io, side by side with nsIAsync*Streams.

> the websocket mochitests actually exercise this thoroughly 
Cool.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to comment #5)
> You might want to move this new class and the interface to xpcom/io, side by
> side with nsIAsync*Streams.

Hmm.  This is just a class, maybe create an interface as this could be useful for other as well and put it to xpcom?  Up to you.

Comment 7

[Patrick McManus [:mcmanus]]

Attachment: http upgrade v3

udpate for bitrot only

Comment 8

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

How does this related to bug 276813?

Comment 9

[Patrick McManus [:mcmanus]]

(In reply to comment #8)
> How does this related to bug 276813?

I have my doubts about implementing that bug as its written..  but setting that aside, this patch enables our HTTP stack to negotiate other protocols to be bootstrapped out of HTTP and hand over a naked stream to the new protocol handler if the negotiation completes.

That is something 276813 needs and therefore, I guess this is a necessary component of that.

It is of course also something websockets needs.

Interestingly, it is something SPDY needs too.

Comment 10

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

Comment on attachment 526522 [details] [diff] [review]
http upgrade v3

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

::: netwerk/base/src/nsPreloadedStream.cpp
@@ +42,5 @@
> +#include "nsThreadUtils.h"
> +#include "nsAlgorithm.h"
> +#include "prmem.h"
> +   
> +namespace mozilla { namespace net {

Looks like netwerk puts the two declarations on two separate lines, please follow that style.

@@ +55,5 @@
> +      mOffset(0),
> +      mLen(datalen)
> +{
> +    mBuf = (char *) moz_xmalloc(datalen);
> +    memcpy (mBuf, data, datalen);

Please remove the space before (

@@ +60,5 @@
> +}
> +
> +nsPreloadedStream::~nsPreloadedStream()
> +{
> +    moz_free (mBuf);

same

@@ +91,5 @@
> +    if (!mLen)
> +        return mStream->Read(aBuf, aCount, _retval);
> +    
> +    PRUint32 toRead = NS_MIN(mLen, aCount);
> +    memcpy (aBuf, mBuf + mOffset, toRead);

No space before (

@@ +108,5 @@
> +        return mStream->ReadSegments(aWriter, aClosure, aCount, result);
> +
> +    *result = 0;
> +    while (mLen > 0) {
> +        PRUint32 toRead = NS_MIN(mLen, aCount);

If mLen > aCount, your loop does the wrong thing, calling aWriter with 0 bytes. The loop condition should probably be mLen > 0 && aCount > 0.

@@ +115,5 @@
> +
> +        rv = aWriter(this, aClosure, mBuf + mOffset, *result, toRead, &didRead);
> +
> +        if (NS_FAILED(rv)) {
> +            return (*result > 0) ? NS_OK : rv;

This should always return NS_OK

@@ +140,5 @@
> +    mLen = 0;
> +    return mStream->CloseWithStatus(aStatus);
> +}
> +
> +class RunOnThread : public nsIRunnable

You could inherit from nsRunnable instead and save the XPCOM boilerplate
http://mxr.mozilla.org/seamonkey/source/xpcom/glue/nsThreadUtils.h#229

@@ +176,5 @@
> +                                  aEventTarget);
> +
> +    nsCOMPtr<nsIRunnable> event =
> +        new RunOnThread(this, aCallback);
> +    return aEventTarget->Dispatch(event, nsIEventTarget::DISPATCH_NORMAL);

aEventTarget may be null:
http://mxr.mozilla.org/seamonkey/source/xpcom/io/nsIAsyncInputStream.idl#103

I think you want:

  if (!aEventTarget) {
    aCallback->OnInputStreamReady(this);
  } else {
    // your existing code
  }

::: netwerk/base/src/nsPreloadedStream.h
@@ +42,5 @@
> +
> +#include "nsIAsyncInputStream.h"
> +#include "nsCOMPtr.h"
> +
> +namespace mozilla { namespace net {

Same as in the .h file - two different lines, please.

@@ +44,5 @@
> +#include "nsCOMPtr.h"
> +
> +namespace mozilla { namespace net {
> +
> +class nsPreloadedStream : public nsIAsyncInputStream

Please, add a comment about what this class does and how it is meant to be used.

@@ +53,5 @@
> +    NS_DECL_NSIASYNCINPUTSTREAM
> +
> +    nsPreloadedStream(nsIAsyncInputStream *aStream, 
> +                      const char *data, PRUint32 datalen);
> +    virtual ~nsPreloadedStream();

Make this private and nonvirtual

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +665,5 @@
>  
> +    if (mUpgradeProtocolCallback) {
> +        mRequestHead.SetHeader(nsHttp::Upgrade, mUpgradeProtocol, PR_FALSE);
> +        mRequestHead.SetHeader(nsHttp::Connection,
> +                               nsCAutoString(nsHttp::Upgrade.get()), PR_TRUE);

nsCAutoString -> nsDependentCString

@@ +4071,3 @@
>          // authentication request over it.  this applies to connection based
>          // authentication schemes only.  for request based schemes, conn is not
>          // needed, so it may be null.

You really should rewrite this comment now that the connection is always initialized

@@ +4855,5 @@
> +nsHttpChannel::HTTPUpgrade(const nsACString &aProtocolName,
> +                           nsIHttpUpgradeChannelListener *aListener)
> +{
> +    if (!aListener || aProtocolName.IsEmpty())
> +        return NS_ERROR_NOT_INITIALIZED;

NOT_INITIALIZED -> INVALID_ARG?

I might also use NS_ENSURE_ARG(_POINTER) so that there's a warning message on the console when this happens.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +440,5 @@
> +        if (!upgradeReq || !upgradeResp ||
> +            !nsHttp::FindToken(upgradeResp, upgradeReq,
> +                               HTTP_HEADER_VALUE_SEPS)) {
> +            LOG(("HTTP 101 Upgrade header mismatch req = %s, resp = %s\n",
> +                 upgradeReq ? upgradeReq : "N/A",

You don't really need the check, PR_LOG can handle a null string:
http://mxr.mozilla.org/seamonkey/source/nsprpub/pr/src/io/prprf.c#384

@@ +516,5 @@
> +{
> +    LOG(("nsHttpConnection::PushBack [this=%p, length=%d]\n", this, length));
> +
> +    if (mInputOverflow) {
> +        LOG(("nsHttpConnection::PushBack only one buffer supported"));

Maybe make this an NS_ERROR/NS_ABORT_IF_FALSE?

::: netwerk/protocol/http/nsHttpPipeline.cpp
@@ +264,5 @@
>      
>      NS_ASSERTION(PR_GetCurrentThread() == gSocketThread, "wrong thread");
>      NS_ASSERTION(mPushBackLen == 0, "push back buffer already has data!");
>  
> +    // If we have no chance for a pipeline (e.g. due to an Upgrade)

Why do you need this code here? You disable pipelining for the upgrade case, right?

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +210,5 @@
>      PRPackedBool                    mStatusEventPending;
>      PRPackedBool                    mHasRequestBody;
>      PRPackedBool                    mSSLConnectFailed;
>      PRPackedBool                    mHttpResponseMatched;
> +    PRPackedBool                    mPreserveStream     : 1; // in case of Upgrade

I'd prefer you not to use the :1 for just one variable here. It won't save you any space, anyway.

::: netwerk/protocol/http/nsIHttpUpgradeChannel.idl
@@ +44,5 @@
> +
> +[scriptable, uuid(5644af88-09e1-4fbd-83da-f012b3b30180)]
> +interface nsIHttpUpgradeChannelListener : nsISupports
> +{
> +    void OnTransportAvailable(in nsISocketTransport   aTransport,

lowercase O

@@ +51,5 @@
> +};
> +
> +
> +[scriptable, uuid(9363fd96-af59-47e8-bddf-1d5e91acd336)]
> +interface nsIHttpUpgradeChannel : nsISupports

Please, add a comment about how this interface can be used.

Comment 11

[Patrick McManus [:mcmanus]]

Thank you for the review, I greatly appreciate it. There is nothing in here very structural at all so I can get it turned around asap.

> > +    PRPackedBool                    mPreserveStream     : 1; // in case of Upgrade
> 
> I'd prefer you not to use the :1 for just one variable here. It won't save
> you any space, anyway.
> 

ha! that's a botched merge of my out-of-tree patches with a recently landed patch that changed all those other flags from int foo:1 to be prpackedbool. Thanks for catching it!

> @@ +264,5 @@
> >      
> >      NS_ASSERTION(PR_GetCurrentThread() == gSocketThread, "wrong thread");
> >      NS_ASSERTION(mPushBackLen == 0, "push back buffer already has data!");
> >  
> > +    // If we have no chance for a pipeline (e.g. due to an Upgrade)
> 
> Why do you need this code here? You disable pipelining for the upgrade case,
> right?

My inclination is strongly that this is the correct code for the class, which I suppose could be changed to an assert of some sort. There is no logical reason you cannot pipeline an upgrade (though I wouldn't pipeline after one!), though its true we don't for reasons of conservatism. I didn't make it an assert pretty much because I have an otherwise unrelated changeset that relies on the nshttppipeline structure a lot more often (to preserve the option of pipelining at a wider variety of points in time) and that change makes sense in that context.. but the change you quote is due to the logic of this patch, so carrying it over there is a bit of a catch-22 with them both unlanded. I'd prefer to just leave it in the upgrade patch as a harmless belt-plus-suspenders thing for now.

Comment 12

[Patrick McManus [:mcmanus]]

> @@ +4071,3 @@
> >          // authentication request over it.  this applies to connection based
> >          // authentication schemes only.  for request based schemes, conn is not
> >          // needed, so it may be null.
> 
> You really should rewrite this comment now that the connection is always
> initialized
> 

you lost me on that one. Can you rephrase?

Comment 13

[Patrick McManus [:mcmanus]]

Attachment: http upgrade v4

Websockets on the move - I love it!

Patch updated to reflect review comments in comment 10, modulo my small reservations in comment 11 and comment 12.

I'm not a process expert, but as I understand it this needs a sr because it adds an idl? Can you provide that service as well?

Comment 14

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

(In reply to comment #12)
> > You really should rewrite this comment now that the connection is always
> > initialized
> > 
> 
> you lost me on that one. Can you rephrase?

The comment implies that conn can be null sometimes. But with your change, it won't be null. So you should change the comment so that it still makes sense.


(In reply to comment #13)
> I'm not a process expert, but as I understand it this needs a sr because it
> adds an idl? Can you provide that service as well?

I can't; the superreview must be done by someone else. Quoting http://www.mozilla.org/hacking/reviewers.html:
"This means that one reviewer cannot provide both review and super-review on a single patch."

Comment 15

[Patrick McManus [:mcmanus]]

Attachment: http upgrade v5

updated comment as per comment 14

Comment 16

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

Comment on attachment 530800 [details] [diff] [review]
http upgrade v5

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

Seems like there really should be unit tests for this, but I guess our frameworks wouldn't support that too well?

::: netwerk/base/src/nsPreloadedStream.cpp
@@ +155,5 @@
> +        return NS_OK;
> +    }
> +
> +private:
> +    ~RunOnThread() {}

so this will work, but since it does inherit from nsRunnable, this destructor will be virtual anyway, so I'd make it public and virtual here.

::: netwerk/protocol/http/nsIHttpUpgradeChannel.idl
@@ +51,5 @@
> + * used as the bootstrapping channel and provide an implementation of 
> + * nsIHttpUpgradeChannelListener that will have its onTransportAvailable()
> + * method invoked if a matching 101 is processed. The arguments to
> + * onTransportAvailable provide the new protocol the low level tranport
> + * streams that are no longer used by HTTP.

Please also add a note that onStartRequest/onStopRequest will be called even for the upgrade case, but then the listener gets full control over the socket. (That is what happens, right?)

Comment 17

[Patrick McManus [:mcmanus]]

Attachment: http upgrade v6

updates from comment 16, carrying forward r=biesi.. asking for sr?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 531385 [details] [diff] [review]
http upgrade v6

So the only reason you didn't use multiplex input stream is that it doesn't implement nsIAsyncInputStream?  Could we file a followup to consider fixing that and using it here?  I'm not sure how painful that would be...

Please put the big comment describing what the class does _above_ the include guard, and formatted like so:

  /**
   * Comment 
   * here
   */

so that it will show up in mxr.

Is there a good reason to add the new upgrade channel interface instead of just putting a new method on nsIHttpChannel or nsIHttpChannelInternal?

In the interface comments in the upgrade channel idl, s/nsIReqestObserver/nsIRequestObserver/ (missing 'u').

sr=me given an adequate explanation for the new upgrade channel interface.

Comment 19

[Patrick McManus [:mcmanus]]

Boris - thanks for taking the time for the review!

(In reply to comment #18)
> Comment on attachment 531385 [details] [diff] [review] [review]
> http upgrade v6
> 
> So the only reason you didn't use multiplex input stream is that it doesn't
> implement nsIAsyncInputStream?  Could we file a followup to consider fixing
> that and using it here?  I'm not sure how painful that would be...

It's probably more accurate to say I wasn't away of multiplex input stream because I worked my way through the possibilities by seeing what classes implemented nsIAsyncInputStream... other than the "async" issue the other mismatch is that the multiplexed input stream takes N streams, while I really had 1 stream and 1 small buffer.. that buffer certainly could be turned into a stream but it is just adding work.

> Is there a good reason to add the new upgrade channel interface instead of
> just putting a new method on nsIHttpChannel or nsIHttpChannelInternal?

I have a strong bias against changing interfaces when just adding things. The fact that COM lets me extend implementations with multiple interfaces is something (maybe the only thing) I really like about it. This is all doubly true when dealing with a prominent interface like nsIHttpChannel.

That being said, I've received feedback recently from a number of sources along the lines of "just break it". So I'll probably incorporate that pov going forward ;).. 

As for "Internal", the interface has applicability beyond internal things.. so I didn't think it apropos.

> sr=me given an adequate explanation for the new upgrade channel interface.

I'll update the patch for the changes in comments. Please let me know if I should proceed or you would like changes in the idl arrangement. Thanks!

Comment 20

[Patrick McManus [:mcmanus]]

Attachment: http upgrade v7

updated for sr comments in comment 18 and comment 19

carry forward r=biesi sr=bz

Comment 21

[Boris Zbarsky [:bzbarsky]]

> As for "Internal", the interface has applicability beyond internal things

Ah, ok.  We've been using nsIHttpChannelInternal as effectively nsIHttpChannel2.

There's a cost to adding new interfaces: it makes objects bigger, makes QI on them slower, and increases cognitive load for future developers.  I agree about willy-nilly interface changes being bad, but I think in this case it would make some sense to add to one of the existing http channel interfaces.  Probably nsIHttpChannelInternal, honestly.

Comment 22

[Patrick McManus [:mcmanus]]

Attachment: 640213-http-upgrade v8

ok, I've reorganized it a bit to get rid of the nsIHttpUpgradeChannel.idl and merge that into the existing nsIHttpChannelInternal.. let me know if that is what you were envisioning..

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 532262 [details] [diff] [review]
640213-http-upgrade v8

sr=me

I just realized that the new stream doesn't actually read all the available data in Read/ReadSegments; this is probably not an issue since consumers shouldn't rely on that anyway.... though some OnDataAvailable implementations do, so I hope it's not passed there.

Comment 24

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

(In reply to comment #23)
> I just realized that the new stream doesn't actually read all the available
> data in Read/ReadSegments; this is probably not an issue since consumers
> shouldn't rely on that anyway.... though some OnDataAvailable
> implementations do, so I hope it's not passed there.

Is that really true? ODA should not read all available data, it should only read as much data as was passed into it as the length, and this impl still alows that.

Comment 25

[Boris Zbarsky [:bzbarsky]]

nsStreamLoader, for example, just does:

  return inStr->ReadSegments(WriteSegmentFun, this, count, &countRead);

and assumes that this will read |count| data. Same thing for nsDownloader.  nsHtml5StreamParser does equivalent things with a Read() call.

So more precisely, if any of those consumers are passed this stream then whoever is doing the passing should not use Available() to determine the length to pass in.

Perhaps Available() should return mLen if mLen is not 0 and call through to mStream otherwise instead of adding the two together?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Or put another way, if someone is to pass this stream to OnDataAvailable, how would they do that right?

Now as far as I can tell we never do that with mSocketIn, so we should be ok, I think...

Comment 27

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/40d44ce2377e

For future reference Patrick, it's much appreciated if when you use checkin-needed you create a final patch using |hg export| containing a "User" line.

Comment 28

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/mozilla-central/rev/42f1cd502298 - within the next four pushes, we filed bug 657177 and bug 657185, so I don't think video networking on Windows likes this patch very much.

Comment 29

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1305429860.1305430275.4865.gz from a retriggered run on this bug's push (so much for it being all-green), a nsHttpConnectionMgr::nsConnectionHandle::Release() crash in content/base/test/test_CrossSiteXHR_origin.html, so apparently it was just coincidence that the first few were in media tests.

Comment 30

[Phil Ringnalda (:philor)]

Though causing the failures in

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1305429870.1305430648.6076.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1305429883.1305430664.6104.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1305429883.1305430645.6061.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1305429887.1305430648.6075.gz

was a handy service, since it pointed out bug 657190.

Comment 31

[Patrick McManus [:mcmanus]]

Attachment: 640213-http-upgrade v9

Fix the backout issue - the bug caused an AuthRetry cycle to be passed a sticky connection that was not always sticky. The fix is to split the connection references into two - one for the stickyAuthRetry (that may be null depending on sticky and keep-alive) like it used to be and a separate one for the http upgrade.

carrying forward the sr+ because this fix doesn't touch any interfaces. Let me know if that's wrong.

try was (eventually) able to reproduce the bug that got it backed out on mochi-1 on windows, and has passed 8+ runs of that config with this change.

Comment 32

[Patrick McManus [:mcmanus]]

Comment on attachment 532946 [details] [diff] [review]
640213-http-upgrade v9

removing r? as try has rejected this (intermittently on xp only). puzzling.

Comment 33

[Patrick McManus [:mcmanus]]

Attachment: 640213-http-upgrade v10

Try likes this one.

The missing insight was that the connection reference can only be meaningfully held across the transaction release if the connection is sticky (which upgrades are).

The interdiff against 8 is really simple, I'd like to get this landed quickly as it is the only websockets piece unlanded that really meaningfully tocuches non-ws things.

Comment 34

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

Comment on attachment 533282 [details] [diff] [review]
640213-http-upgrade v10

+        if (mUpgradeProtocolCallback && stickyConn  &&

just one space before &&, please

Comment 35

[Patrick McManus [:mcmanus]]

http://hg.mozilla.org/mozilla-central/rev/9197b91fd9f4

Comment 36

[Eric Shepherd [:sheppy]]

Documentation added:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIHttpChannelInternal#HTTPUpgrade()
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIHttpUpgradeListener

And mentioned on Firefox 6 for developers.