Bug 33248

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 Flags
Patch
none
Patch
none
Patch ap: review+

Description Fumitoshi Ukai 2010-01-05 22:25:48 PST
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.
Comment 1 Fumitoshi Ukai 2010-01-06 00:53:55 PST
It looks Safari closes the connection when unloaded, but chromium doesn't.
Is it v8 binding issue?
Comment 2 Fumitoshi Ukai 2010-01-15 00:48:45 PST
Created attachment 46649 [details]
Patch
Comment 3 Eric Seidel (no email) 2010-01-15 00:53:41 PST
Attachment 46649 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/187985
Comment 4 WebKit Review Bot 2010-01-15 00:55:12 PST
Attachment 46649 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/189025
Comment 5 WebKit Review Bot 2010-01-15 00:57:25 PST
Attachment 46649 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/189030
Comment 6 Fumitoshi Ukai 2010-01-15 01:45:57 PST
Created attachment 46655 [details]
Patch
Comment 7 David Levin 2010-01-21 22:36:47 PST
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 .
Comment 8 Alexey Proskuryakov 2010-01-21 23:00:04 PST
> 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.
Comment 9 Fumitoshi Ukai 2010-01-21 23:07:12 PST
(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 10 Alexey Proskuryakov 2010-01-22 11:26:21 PST
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.
Comment 11 Alexey Proskuryakov 2010-01-22 12:25:05 PST
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.
Comment 12 Fumitoshi Ukai 2010-02-02 23:10:09 PST
Created attachment 47998 [details]
Patch
Comment 13 Alexey Proskuryakov 2010-02-03 08:29:33 PST
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.
Comment 14 Alexey Proskuryakov 2010-02-03 08:32:23 PST
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.
Comment 15 Fumitoshi Ukai 2010-02-03 18:24:22 PST
Committed r54319: <http://trac.webkit.org/changeset/54319>
Comment 16 Fumitoshi Ukai 2010-02-03 20:13:17 PST
(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?
Comment 17 Fumitoshi Ukai 2010-02-03 20:18:05 PST
(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
Comment 18 Alexey Proskuryakov 2010-02-03 22:59:24 PST
> 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(...)
Comment 19 Fumitoshi Ukai 2010-02-03 23:11:17 PST
(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
Comment 20 Fumitoshi Ukai 2010-02-04 00:44:26 PST
(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?
Comment 21 Alexey Proskuryakov 2010-02-04 08:50:28 PST
Oops, sorry. Perhaps testing send() makes sense in this situation?
Comment 22 Fumitoshi Ukai 2010-02-04 22:28:43 PST
(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