Summary: | WebSocket should close the connection when unloading the document | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fumitoshi Ukai <ukai> | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, dglazkov, eric, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Fumitoshi Ukai
2010-01-05 22:25:48 PST
It looks Safari closes the connection when unloaded, but chromium doesn't. Is it v8 binding issue? Created attachment 46649 [details]
Patch
Attachment 46649 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/187985 Attachment 46649 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/189025 Attachment 46649 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/189030 Created attachment 46655 [details]
Patch
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 . > 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.
(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? 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. 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. Created attachment 47998 [details]
Patch
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. 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. Committed r54319: <http://trac.webkit.org/changeset/54319> (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? (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 > 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(...) (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 (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? Oops, sorry. Perhaps testing send() makes sense in this situation? (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 |