Bug 38171 – WebSocket needs to suspend/resume as Active DOM object.

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

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.

[Fumitoshi Ukai]

Created attachment 54390 [details]
Patch

[Yuta Kitamura]

(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.

[Alexey Proskuryakov]

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.

[Fumitoshi Ukai]

Created attachment 55224 [details]
Patch

[Yuta Kitamura]

(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.

[Alexey Proskuryakov]

+        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.

[Fumitoshi Ukai]

(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.

[Fumitoshi Ukai]

Created attachment 55521 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 55521 [details]
Patch

r=me

[Fumitoshi Ukai]

Committed r59116: <http://trac.webkit.org/changeset/59116>