675784 - WebSockets - problem cancelling pre-bootstrapped connection

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Patrick McManus [:mcmanus]]

A canceled request can hang if it is canceled prior to the websockets bootstrap handshake being issued.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch 1

if you isolate test 2 of test_websocket.html and click reload a couple times while the test is outstanding (the test involves a multiple second connection timeout - reload during that) you can see the issue.

Comment 2

[Jason Duell]

Comment on attachment 550143 [details] [diff] [review]
patch 1

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

Took me a while to grok it all, but I think it's good.

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +379,5 @@
> +                          "transaction already running");
> +
> +        (mData[index])->mChannel->mOpenBlocked = 0;
> +        (mData[index])->mChannel->mOpenRunning = 1;
> +        (mData[index])->mChannel->BeginOpen();

So if the page is in the middle of being Canceled by the LoadGroup, and the LG cancels this cxn but there's another WS in same page that's mOpenBlocked, it's going to get found by this logic and have BeginOpen->AsyncOpen called, just to then get Cancelled by the LG.  Right?  I guess that's fine--a little messy, but correct.

There's a big block of shared logic here with Complete--move to a new function called "ConnectNext" or whatever (and please declare a channel * variable so you don't need to repeat mData[index]>mChannel 5 times).

::: netwerk/protocol/websocket/WebSocketChannel.h
@@ +240,5 @@
>    PRUint32                        mAutoFollowRedirects       : 1;
>    PRUint32                        mReleaseOnTransmit         : 1;
>    PRUint32                        mTCPClosed                 : 1;
> +  PRUint32                        mOpenBlocked               : 1;
> +  PRUint32                        mOpenStarted               : 1;

Unlike mOpenBlocked and mOpenRunning, this one isn't temporary--it stays set once the http chan's asyncOpen has been called.  For parity with nsHttpChannel, let's call it mChannelWasOpened, unless you object.

Comment 3

[Jason Duell]

Attachment: Some fixups to original patch (as additional patch, not replacement).

Patrick, let's include these as well unless you object.

Comment 4

[Patrick McManus [:mcmanus]]

> So if the page is in the middle of being Canceled by the LoadGroup, and the
> LG cancels this cxn but there's another WS in same page that's mOpenBlocked,
> it's going to get found by this logic and have BeginOpen->AsyncOpen called,
> just to then get Cancelled by the LG.  Right?  I guess that's fine--a little
> messy, but correct.
> 

right.. the basic issue is that the websocket protocol defines a period of time (a potentially long async period of time) between calling websocketchannel->asyncopen() and when the http connection it uses is opened. The spec requires a dns lookup and potentially a queued wait before the http stuff can begin. If a cancel comes in during that time, we don't see it.

I went with this approach. The other approach is to implement nsirequest just to get the cancel, but that's a whole slew of code that is needless/dup'd-with-http to solve a very small problem even if it is a little cleaner in terms of the model. (i.e. it fits the model well, but it is annoying in practice).

Comment 5

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

http://hg.mozilla.org/mozilla-central/rev/1586f2d51fac
http://hg.mozilla.org/mozilla-central/rev/d28a4c15096c

Comment 6

[Jason Duell]

> The other approach is to implement nsirequest just to get the cancel, 
> but that's a whole slew of code that is needless/dup'd-with-http

Right.  But with the cleanup to eliminate WebsocketEstablishedConnection, it probably will make sense to more the nsIRequest code it implements into WebsocketChannel rather than nsWebSocket.  (I haven't looked at Wellington's patch to see if he's doing that already or not).