Bug 38171 - WebSocket needs to suspend/resume as Active DOM object.
Summary: WebSocket needs to suspend/resume as Active DOM object.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Fumitoshi Ukai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-26 22:33 PDT by Fumitoshi Ukai
Modified: 2010-05-10 18:51 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.56 KB, patch)
2010-04-27 00:06 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (20.12 KB, patch)
2010-05-06 03:34 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (24.17 KB, patch)
2010-05-09 23:29 PDT, Fumitoshi Ukai
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fumitoshi Ukai 2010-04-26 22:33:57 PDT
WebSocket needs to suspend/resume as Active DOM object.
For example, javascript debugger would suspend Active DOM objects by PageGroupLoadDeferrer, and run message loops.
Message loop would pump up new events from platform network code and process message frames.
If debugger runs in onmessage event handler, reentrant WebSocketChannel::didReceiveMessage and cause trouble.
(see http://code.google.com/p/chromium/issues/detail?id=42533. and possibly http://code.google.com/p/chromium/issues/detail?id=28562)

WebSocket needs to suspend and queue event up until it is resumed.
Comment 1 Fumitoshi Ukai 2010-04-27 00:06:08 PDT
Created attachment 54390 [details]
Patch
Comment 2 Yuta Kitamura 2010-04-27 01:45:20 PDT
(In reply to comment #1)
> Created an attachment (id=54390) [details]
> Patch

It seems like this patch is a non-solution; the bug is still reproducible with this patch applied. I can get almost identical stack trace.

Furthermore, I cannot reproduce the bug on Mac WebKit (ToT) either.
Comment 3 Alexey Proskuryakov 2010-04-27 10:10:42 PDT
Comment on attachment 54390 [details]
Patch

+    , m_suspend(false)

Probably should be m_suspended.

+bool WebSocket::canSuspend() const
+{
+    return true;
+}

There are currently three uses for suspend/resume that I know of:

1. Suspend behind modal dialogs and alerts, so that program state wouldn't change during an alert() call. WebSocket definitely needs that.

2. Suspend when a page goes into back/forward cache. One can't practically suspend a socket for that long, so open WebSocket objects should prevent a page from going to b/f cache. Fo this, canSuspend should return false - see how it's done for XMLHttpRequest.

3. Suspend during a pause in debugger. I don't know much about how that works.

So in short, canSuspend() should return !m_channel.

+void WebSocket::suspend()
+{
+    m_suspend = true;
+}

I don't think it's sufficient. We need to make sure that readyState and other properties don't change during an alert or modalDialog() calls.

-    dispatchEvent(Event::create(eventNames().openEvent, false, false));
+    enqueueEvent(Event::create(eventNames().openEvent, false, false));

The enqueueEvent() name makes it sound like some kind of deferred dispatch, like "post a task to fire a simple event" in HTML5 parlance. But that's not how it generally behaves.

+        Deque<RefPtr<Event> > m_pendingEvents;

Deque is not a great container to use here. We don't do random append/remove in normal case - we fill the whole container first, and then empty it. See similar patterns in Document and HTMLMediaElement.

Of these comments, the most difficult to address is of course insufficiency of a boolean flag. I don't know how to best fix that, perhaps one needs to rely of platform specific code in SocketStreamHandle, similarly to what we do for XMLHttpRequest.
Comment 4 Fumitoshi Ukai 2010-05-06 03:34:42 PDT
Created attachment 55224 [details]
Patch
Comment 5 Yuta Kitamura 2010-05-06 05:04:18 PDT
(In reply to comment #4)
> Created an attachment (id=55224) [details]
> Patch

I confirmed this patch fixes the issue http://crbug.com/42533. I have not looked at the code in detail, though.
Comment 6 Alexey Proskuryakov 2010-05-07 10:11:34 PDT
+        While WebSocketChannel is suspended, it only adds received data in
+        m_buffer or record the handle was closed, and report no event to
+        WebSocket.

One problem with this approach is that remote side will think that we're consuming the data, and TCP/IP throttling mechanisms will be ineffective. But it's probably the lesser evil.

+        Since suspend/resume would be called while processing JavaScript event
+        handler, WebSocketChannel methods that would fire an event need to be
+        reentrant

I fail to understand this scenario. Could you please explain it in more detail?

     , m_bufferSize(0)
+    , m_suspended(false)
+    , m_closed(false)
+    , m_unhandledBufferedAmount(0)

What is the difference between m_bufferSize and m_unhandledBufferedAmount?

+void WorkerThreadableWebSocketChannel::Bridge::suspend()
+{
+    ASSERT(m_peer);
+    m_loaderProxy.postTaskToLoader(createCallbackTask(&WorkerThreadableWebSocketChannel::mainThreadSuspend, m_peer));
+}

Is there a window of opportunity between the suspend message is posted, and the loader thread receives it? The loader may be sending data to worker thread at this very moment.
Comment 7 Fumitoshi Ukai 2010-05-09 23:18:47 PDT
(In reply to comment #6)
> +        While WebSocketChannel is suspended, it only adds received data in
> +        m_buffer or record the handle was closed, and report no event to
> +        WebSocket.
> 
> One problem with this approach is that remote side will think that we're consuming the data, and TCP/IP throttling mechanisms will be ineffective. But it's probably the lesser evil.

We might need to change SocketStream interface.  I'll address this in different bug.

> 
> +        Since suspend/resume would be called while processing JavaScript event
> +        handler, WebSocketChannel methods that would fire an event need to be
> +        reentrant
> 
> I fail to understand this scenario. Could you please explain it in more detail?

Suppose that alert() is called in onmessage event handler.  I think the following scenario.

 WebSocketChannel::didReceiveData is called.
 process buffer and extract a message in a websocket frame.
 WebSocket dispatch message event.
 JavaScript engine runs onmessage event handler.
 to run alert(), it suspends Active DOMs, so WebSocket is suspended.
   run message loop until alert() is finished.
   at this point, network event would be processed, so might reenter WebSocketChannel::didReceiveData()
 alert() finished. resumes Active DOMs, including WebSocket.

So, this patch changed WebSocketChannel::didReceiveData() to add data in buffer and not to process buffer when suspended.

> 
>      , m_bufferSize(0)
> +    , m_suspended(false)
> +    , m_closed(false)
> +    , m_unhandledBufferedAmount(0)
> 
> What is the difference between m_bufferSize and m_unhandledBufferedAmount?

m_bufferSize is size of m_buffer.
m_unhandledBufferedAmount is amount of buffer that was pushed to SocketStream, but not sent before close.

> +void WorkerThreadableWebSocketChannel::Bridge::suspend()
> +{
> +    ASSERT(m_peer);
> +    m_loaderProxy.postTaskToLoader(createCallbackTask(&WorkerThreadableWebSocketChannel::mainThreadSuspend, m_peer));
> +}
> 
> Is there a window of opportunity between the suspend message is posted, and the loader thread receives it? The loader may be sending data to worker thread at this very moment.

Ah, yes.  I'll fix.
Comment 8 Fumitoshi Ukai 2010-05-09 23:29:47 PDT
Created attachment 55521 [details]
Patch
Comment 9 Alexey Proskuryakov 2010-05-10 16:28:56 PDT
Comment on attachment 55521 [details]
Patch

r=me
Comment 10 Fumitoshi Ukai 2010-05-10 18:51:00 PDT
Committed r59116: <http://trac.webkit.org/changeset/59116>