[webkit-reviews] review denied: [Bug 33248] WebSocket close the connection when unloading the document. : [Attachment 46655] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Fumitoshi Ukai
<ukai at chromium.org>'s request for review:
Bug 33248: WebSocket close the connection when unloading the document.
https://bugs.webkit.org/show_bug.cgi?id=33248

Attachment 46655: Patch
https://bugs.webkit.org/attachment.cgi?id=46655&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.