Bug 39340 - WebSocket: resume should not process buffer if already processing.
Summary: WebSocket: resume should not process buffer if already processing.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Fumitoshi Ukai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-18 20:41 PDT by Fumitoshi Ukai
Modified: 2010-06-18 01:29 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.89 KB, patch)
2010-05-18 22:55 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (8.55 KB, patch)
2010-05-19 19:41 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (10.12 KB, patch)
2010-05-21 02:17 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-05-18 20:41:15 PDT
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.
Comment 1 Fumitoshi Ukai 2010-05-18 22:47:22 PDT
(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.
Comment 2 Fumitoshi Ukai 2010-05-18 22:55:23 PDT
Created attachment 56459 [details]
Patch
Comment 3 Fumitoshi Ukai 2010-05-19 19:40:11 PDT
(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.
Comment 4 Fumitoshi Ukai 2010-05-19 19:41:54 PDT
Created attachment 56548 [details]
Patch
Comment 5 Fumitoshi Ukai 2010-05-21 02:17:35 PDT
Created attachment 56684 [details]
Patch
Comment 6 Alexey Proskuryakov 2010-06-17 10:58:24 PDT
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
Comment 7 Fumitoshi Ukai 2010-06-17 21:30:41 PDT
Committed r61375: <http://trac.webkit.org/changeset/61375>
Comment 8 Fumitoshi Ukai 2010-06-17 21:37:01 PDT
> (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.
Comment 9 WebKit Review Bot 2010-06-18 00:35:21 PDT
http://trac.webkit.org/changeset/61375 might have broken Leopard Intel Debug (Tests)
Comment 10 Fumitoshi Ukai 2010-06-18 01:29:45 PDT
(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>