Bug 33248 – WebSocket should close the connection when unloading the document

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

http://www.whatwg.org/specs/web-apps/current-work/#unloading-documents says:

6.11.10 Unloading documents

When a user agent is to unload a document, it must run the following steps. These steps are passed an argument, recycle, which is either true or false, indicating whether the Document object is going to be re-used. (This is set by the document.open() method.)
...
7. Close the Web Socket connection of any WebSocket objects that were created by the WebSocket() constructor visible on the Document's Window object. If this affected any WebSocket objects, set salvageable to false.

But current implementation doesn't close the Web Socket connection when unloading the document.

[Fumitoshi Ukai]

It looks Safari closes the connection when unloaded, but chromium doesn't.
Is it v8 binding issue?

[Fumitoshi Ukai]

Created attachment 46649 [details]
Patch

[Eric Seidel (no email)]

Attachment 46649 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/187985

[WebKit Review Bot]

Attachment 46649 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/189025

[WebKit Review Bot]

Attachment 46649 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/189030

[Fumitoshi Ukai]

Created attachment 46655 [details]
Patch

[David Levin]

Do you know why Safari closes the connection but chromium doesn't?

Anyway, since this looks like a generic websocket patch and ap has been reviewing these and working in the same area, it would probably make sense for him to review this .

[Alexey Proskuryakov]

> Do you know why Safari closes the connection but chromium doesn't?

Yes, that's a very interesting question - I don't see anything in CF-specific code that would do that. In particular, it's not clear what guarantees WebSocketChannel::m_context pointer not becoming stale.

[Fumitoshi Ukai]

(In reply to comment #8)
> > Do you know why Safari closes the connection but chromium doesn't?
> 
> Yes, that's a very interesting question - I don't see anything in CF-specific
> code that would do that. In particular, it's not clear what guarantees
> WebSocketChannel::m_context pointer not becoming stale.

Should we override ActiveDOMObject::contextDestroyed() and calls websocket disconnect() in it?

[Alexey Proskuryakov]

Comment on attachment 46655 [details]
Patch

On the Mac, connections are closed due to garbage collection:

#0	0x05b3c3f5 in WebCore::SocketStreamHandle::platformClose at SocketStreamHandleCFNet.cpp:604
#1	0x05b3af84 in WebCore::SocketStreamHandleBase::close at SocketStreamHandleBase.cpp:83
#2	0x05cd5ee2 in WebCore::WebSocketChannel::disconnect at WebSocketChannel.cpp:112
#3	0x05cd5460 in WebCore::WebSocket::~WebSocket at WebSocket.cpp:106
#4	0x058de89b in WTF::RefCounted<WebCore::WebSocket>::deref at RefCounted.h:109
#5	0x058de8c0 in WTF::RefPtr<WebCore::WebSocket>::~RefPtr at RefPtr.h:53
#6	0x058de6cc in WebCore::JSWebSocket::~JSWebSocket at JSWebSocket.cpp:124
#7	0x00d1c274 in JSC::Heap::sweep at Collector.cpp:1084
#8	0x00d1db95 in JSC::Heap::collectAllGarbage at Collector.cpp:1264

I'm surprised this doesn't work with v8, but anyway, relying on GC is insufficient. The right way to kill WebSocket connections is likely from an overridden AciveDOMObject::stop() method - similarly to how it's done for XMLHttpRequest.

> Should we override ActiveDOMObject::contextDestroyed() and calls websocket
> disconnect() in it?

Overriding ActiveDOMObject::contextDestroyed() to avoid having a dangling pointer in WebSocketChannel sounds reasonable, but I don't see how calling disconnect() achieves that goal. Even if it does somehow, that's probably too indirect to be reliable.

+//    ws = new WebSocket("ws://127.0.0.1:8880/websocket/tests/close-when-unload");
+    ws = new WebSocket("ws://172.30.85.230:8880/websocket/tests/close-when-unload");

URLs in the test are not correct.

+    setTimeout('unload()', 500);

You can test unloading by removing an iframe - that way, the test wouldn't need to be timing dependent.

[Alexey Proskuryakov]

I tried to make test cases for WebSocket lifetime, expecting to get some crashes. But it turns out that WebSocket lifetime is broken in ways that prevent making such tests - I filed bug 34014 for one such issue, but there could be more. It's good in the sense that we don't crash, but layering further improvements upon current behavior can be problematic.

[Fumitoshi Ukai]

Created attachment 47998 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 47998 [details]
Patch

Nice test! r=me

> +        WebSocket should close the connection when unloading the document

One thing I'm not quite sure about - how does the HTML5 "unload the document" concept correspond to detaching a frame? Maybe we should make one or two more variations of this test - in one, we'd force garbage collection after detaching subframe; in another, we'd keep a reference to frames[0].document, making sure that it outlives the frame.

[Alexey Proskuryakov]

Also, what happens when navigating the frame to a different location (without removing it). I think that works just fine, because we'll call ActiveDOMObject::stop as usual, but this doesn't seem to be covered by any tests.

[Fumitoshi Ukai]

Committed r54319: <http://trac.webkit.org/changeset/54319>

[Fumitoshi Ukai]

(In reply to comment #13)
> (From update of attachment 47998 [details])
> Nice test! r=me
> 
> > +        WebSocket should close the connection when unloading the document
> 
> One thing I'm not quite sure about - how does the HTML5 "unload the document"
> concept correspond to detaching a frame? 

I think iframe constructs a nested browsing context, so removing it would cause "unload the document"..

> Maybe we should make one or two more
> variations of this test - in one, we'd force garbage collection after detaching
> subframe;

I'm not sure what it is trying to test. set event handler to websocket in frame (like websocket-pending-activity.html?) and detaching subframe and force garbage collection and see it would get onmessage ?

> in another, we'd keep a reference to frames[0].document, making sure
> that it outlives the frame.

this means websocket object is kept in parent document of the frame and see it won't be closed after detaching the frame?

[Fumitoshi Ukai]

(In reply to comment #14)
> Also, what happens when navigating the frame to a different location (without
> removing it). I think that works just fine, because we'll call
> ActiveDOMObject::stop as usual, but this doesn't seem to be covered by any
> tests.

Sure. https://bugs.webkit.org/show_bug.cgi?id=34557

[Alexey Proskuryakov]

> I'm not sure what it is trying to test. set event handler to websocket in frame
> (like websocket-pending-activity.html?) and detaching subframe and force
> garbage collection and see it would get onmessage ?

I think it will get stopped, just testing that it won't crash.

>> in another, we'd keep a reference to frames[0].document, making sure
>> that it outlives the frame.

> this means websocket object is kept in parent document of the frame and see it
> won't be closed after detaching the frame?

I was thinking about a reference to an object from child context. Again, just to see that it's not crashing (which is of course so, since detaching the frame will stop network activity, and nothing will happen to the object).

Another variation:

// in subframe
top.ws = new WebSocket;

// in parent frame
// ...detach subframe
ws.open(...)

[Fumitoshi Ukai]

(In reply to comment #18)
> > I'm not sure what it is trying to test. set event handler to websocket in frame
> > (like websocket-pending-activity.html?) and detaching subframe and force
> > garbage collection and see it would get onmessage ?
> 
> I think it will get stopped, just testing that it won't crash.

Ah, I see.
 
> >> in another, we'd keep a reference to frames[0].document, making sure
> >> that it outlives the frame.
> 
> > this means websocket object is kept in parent document of the frame and see it
> > won't be closed after detaching the frame?
> 
> I was thinking about a reference to an object from child context. Again, just
> to see that it's not crashing (which is of course so, since detaching the frame
> will stop network activity, and nothing will happen to the object).
> 
> Another variation:
> 
> // in subframe
> top.ws = new WebSocket;
> 
> // in parent frame
> // ...detach subframe
> ws.open(...)

Ok.  https://bugs.webkit.org/show_bug.cgi?id=34562

[Fumitoshi Ukai]

(In reply to comment #18)
> > I'm not sure what it is trying to test. set event handler to websocket in frame
> > (like websocket-pending-activity.html?) and detaching subframe and force
> > garbage collection and see it would get onmessage ?
> 
> I think it will get stopped, just testing that it won't crash.
> 
> >> in another, we'd keep a reference to frames[0].document, making sure
> >> that it outlives the frame.
> 
> > this means websocket object is kept in parent document of the frame and see it
> > won't be closed after detaching the frame?
> 
> I was thinking about a reference to an object from child context. Again, just
> to see that it's not crashing (which is of course so, since detaching the frame
> will stop network activity, and nothing will happen to the object).
> 
> Another variation:
> 
> // in subframe
> top.ws = new WebSocket;
> 
> // in parent frame
> // ...detach subframe
> ws.open(...)

Ah, there is no open method in WebSocket object.  What do you want to do in the test?

[Alexey Proskuryakov]

Oops, sorry. Perhaps testing send() makes sense in this situation?

[Fumitoshi Ukai]

(In reply to comment #21)
> Oops, sorry. Perhaps testing send() makes sense in this situation?

Hmm, after detaching frame, ws is CLOSED, so send() should return false.
https://bugs.webkit.org/show_bug.cgi?id=34630