765738 - Race condition allows ghost WebSocket connections to live after tab closed

Status: RESOLVED DUPLICATE of bug 983243

(Core :: Networking: WebSockets, defect)

Description

[Martyn Loughran]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_4) AppleWebKit/534.57.2 (KHTML, like Gecko) Version/5.1.7 Safari/534.57.2

Steps to reproduce:

I tried changing the timeout in my `onclose` handler to control when the WebSocket reconnect happens while investigating the effectiveness of the workaround for issue #696085. This exposed what looks like a race condition in Firefox.


Actual results:

This is a summary of what happens with different timeout values:

Timeout 0 case: `onclose` is called (with `wasClean = true`) when you click to navigate away from the page, then a new connection is opened (by the `onclose` handler). This connection then appears to be closed by firefox almost instantly (presumably this is the workaround to this bug, so that the ghost connection is not left lying about).

Timeout of 90ms in `onclose`: Exactly the same behaviour as case 0ms above.

Timeout of 100ms in `onclose`: Sometimes behaves as 0ms, sometimes as 103ms.

Timeout of 103ms in `onclose`: Inconsistent behaviour - there is definitely a race condition in firefox here. Usually this works perfectly (i.e. existing connection is closed and new one isn't opened), but sometimes a new connection is established, and it's left around as a ghost connection after exiting the page. This is basically the behaviour I saw when we initially reported issue #696085, but it's even worse: **The TCP connection actually stays around even when the browser tab is closed, and is only closed when Firefox quits!**

My hunch based on all this experimentation is that if a WebSocket connection is in the process of opening when the page is finally cleaned up by firefox (which seems to be after ~ 105ms), that the connection is orphaned forever. This could potentially leave Firefox amassing large numbers of ghost WebSocket connections.

Full details, including code to replicate in this gist: https://gist.github.com/1308932


Expected results:

WebSocket connection should have been closed. [also, in my opinion, it should not be possible to create new websocket connections when a page is in the process of unloading, but this is a separate issue.

Comment 1

[Phil Leggetter]

I've raised a query related to this in StackOverflow.
http://stackoverflow.com/questions/10965720/should-websocket-onclose-be-triggered-by-user-navigation-or-refresh

It seems to me that any issue related to this could be resolved by stopping the WebSocket.onclose handler from being called at all if the page is unloading. I can't see any value in knowing about the WebSocket.onclose when the page is unloading anyway and the browser is going to close the WebSocket connection. Can't we just stop the onclose being called which then means that it's no longer possible to try and create a new WebSocket connection in the `onclose` event handler and therefore 'ghost' connections can't be created.

Order of events at the moment:

1. window.onbeforeunload
2. WebSocket.onclose
3. window.onunload

Comment 2

[Martyn Loughran]

Hi guys. Have you managed to reproduce this bug your end? Is there anything I can do to expedite the process? Cheers.

Comment 3

[Jason Duell]

In particular, we fixed some "ghost" websocket connections in bug 696085.  What version of firefox are you using?

> It seems to me that any issue related to this could be resolved by stopping 
> the WebSocket.onclose handler from being called at all if the page is unloading.

Indeed, and that was the fix I was hoping to make in bug 696085, but it turns out DocShell doesn't make it easy/possible to do that right now, alas.

Comment 4

[Martyn Loughran]

(In reply to Jason Duell (:jduell) from comment #3)
> In particular, we fixed some "ghost" websocket connections in bug 696085. 
> What version of firefox are you using?

I tested using version 13. I was actually testing to see if the workaround for that bug was successful and ended up finding more issues - see my comment there https://bugzilla.mozilla.org/show_bug.cgi?id=696085#c28

Comment 5

[Jason Duell]

Thanks Martyn:  I'll look into what's going here.

Comment 6

[Martyn Loughran]

I've just tried to replicate with Firefox 17 and am seeing the same issues as I saw with version 13. To reiterate, I can get the browser into a situation where WebSockets are kept open for closed browser tabs indefinitely.

I've updated the gist which has full instructions to replicate - it should only take you a couple of minutes. I'd really appreciate if someone could look into this.

https://gist.github.com/1308932

Comment 7

[Jason Duell]

Thanks for the report and steps to reproduce.  This is high on my list after B2G v1 work is finished.

Comment 8

[Jason Duell]

Smaug: I may need some DOM expertise to figure this out.  The issue is that

1) when tab is closed, existing websocket's onclose() is called
2) if onclose tries to create another websocket, the new websocket is closed (correctly)
3) if onclose sets a short timer (<100ms for the user here) that when fired creates a new websocket, the new websocket is closed (correctly)
4) but if the timer is >100ms, the websocket gets created, and seemingly doesn't belong to any window (it doesn't close when the tab is closed, and the TCP connection exists until the browser is closed.)

I would have expected the checkInnerWindowCorrectness(), etc, code here to catch this.

  http://mxr.mozilla.org/mozilla-central/source/content/base/src/WebSocket.cpp#663

Any idea what could be going on here?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Jason, what does aOwnerWindow end up being in WebSocket::Init() in this case?  Specifically, what is its mDoc->mDocumentURI.mRawPtr->mSpec.mData?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Hmm, why doesn't innerwindowcorrectness check help here. Is this somehow related to bug 823196?
Does the bug happen with old build which didn't use WebIDL binding for WebSocket?

Comment 11

[Ian]

Has this been seen on any other platforms?  I'm having similar problems with MJPEG streaming over HTTP on Windows 7.

Comment 12

[Valentin Gosu [:valentin] (he/him)]

I think Bug 983243 should have probably fixed this use case as well.
Marking as duplicate.