Bug 39340 – WebSocket: resume should not process buffer if already processing.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Fumitoshi Ukai]

Reported on chromium issue tracker: http://code.google.com/p/chromium/issues/detail?id=44309

I investigated this and found this happens as follows:

- didReceiveData called for handshake response message and initial websocket frame
  - processBuffer found handshake response header and WebSocket connection is established.
    - didConnect
      - dispatch open event
        ..
        - while processing handler code, it might issue some API which will send IPC to browser process
          - RenderThread::Send will enter modal loop, which will suspend active DOM objects
            - WebSocket and its WebSocketChannel is suspended
              ..
          - RenderThread::Send exit modal loop, which will resume active DOM objects
            - WebSocket and its WebSocketChannel is resumed
              processBuffer found initial websocket frame and fire message event
              - didReceiveMessage
                - dispatch message event

Thus, message event is dispatched while open event handler is running, which is wrong behavior.
This issue is in Chromium, but not in chromium test_shell nor in WebKit nightly build.

[Fumitoshi Ukai]

(In reply to comment #0)
> Reported on chromium issue tracker: http://code.google.com/p/chromium/issues/detail?id=44309
> 
> I investigated this and found this happens as follows:
> 
> - didReceiveData called for handshake response message and initial websocket frame
>   - processBuffer found handshake response header and WebSocket connection is established.
>     - didConnect
>       - dispatch open event
>         ..
>         - while processing handler code, it might issue some API which will send IPC to browser process
>           - RenderThread::Send will enter modal loop, which will suspend active DOM objects
>             - WebSocket and its WebSocketChannel is suspended
>               ..
>           - RenderThread::Send exit modal loop, which will resume active DOM objects
>             - WebSocket and its WebSocketChannel is resumed
>               processBuffer found initial websocket frame and fire message event
>               - didReceiveMessage
>                 - dispatch message event
> 
> Thus, message event is dispatched while open event handler is running, which is wrong behavior.
> This issue is in Chromium, but not in chromium test_shell nor in WebKit nightly build.

If alert() is called in event handler, it would happens in chromium test_shell and WebKit nightly build as well.

[Fumitoshi Ukai]

Created attachment 56459 [details]
Patch

[Fumitoshi Ukai]

(In reply to comment #1)
> (In reply to comment #0)

> If alert() is called in event handler, it would happens in chromium test_shell and WebKit nightly build as well.

We should not process pending event in resume().
alert() may be called in other context than websocket's event handler.
While in alert(), network pushed data to WebSocketChannel, but it is not good to fire websocket event just after alert().
I think websocket event should be fired after the current javascript task has been finished.

[Fumitoshi Ukai]

Created attachment 56548 [details]
Patch

[Fumitoshi Ukai]

Created attachment 56684 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 56684 [details]
Patch

Nice catch!

+++ b/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.h

Do we even need to suspend WebSocket message processing below an alert() in workers?

+    if (!m_resumeTimer.isActive())
+        m_resumeTimer.startOneShot(0);

So, a timer will be started for all WebSocketChannel objects, even those that have no pending data? This doesn't look great.

+    RefPtr<WebSocketChannel> protect(this); // The client can close the channel, potentially removing the last reference.

Is this covered by regression tests?

+++ b/LayoutTests/websocket/tests/script-tests/alert-in-event-handler.js

I still hate it that I can't see test content when I open .html, and have to dig inside a subdirectory for matching .js.

r=me

[Fumitoshi Ukai]

Committed r61375: <http://trac.webkit.org/changeset/61375>

[Fumitoshi Ukai]

> (From update of attachment 56684 [details])
> Nice catch!
Thanks for review!

> +++ b/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.h
> 
> Do we even need to suspend WebSocket message processing below an alert() in workers?

Hmm, I'm not sure, but in chrome, it will suspend/resume active DOM objects when sending IPC message, so it might be safer to do as main thread.

> 
> +    if (!m_resumeTimer.isActive())
> +        m_resumeTimer.startOneShot(0);
> 
> So, a timer will be started for all WebSocketChannel objects, even those that have no pending data? This doesn't look great.

Ah, sure. I've add check if there are pending event before starting timer.

> +    RefPtr<WebSocketChannel> protect(this); // The client can close the channel, potentially removing the last reference.
> 
> Is this covered by regression tests?

not sure. but didClose() will deref(), so it would be safe to keep reference here.

[WebKit Review Bot]

http://trac.webkit.org/changeset/61375 might have broken Leopard Intel Debug (Tests)

[Fumitoshi Ukai]

(In reply to comment #9)
> http://trac.webkit.org/changeset/61375 might have broken Leopard Intel Debug (Tests)

Oops.

I realized that we couldn't use Timer in worker thread.
And I noticed MessageLoop runs in main thread, so we don't need to use Timer for resume in worker thread.

Fixed in r61385: <http://trac.webkit.org/changeset/61385>