| Summary: | WebSocket: MessageEvent fired during send() on workers | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yuta Kitamura <yutak> | ||||
| Component: | WebCore Misc. | Assignee: | Yuta Kitamura <yutak> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | levin, levin+threading, tkent, toyoshim, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Attachments: |
|
||||||
|
Description
Yuta Kitamura
2012-01-18 00:44:41 PST
Created attachment 122886 [details]
Patch
Comment on attachment 122886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122886&action=review > Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:190 > + if (!m_syncMethodDone) { Why go through all of this? If you post a task without a mode, then it won't run until the nested loop is done. Comment on attachment 122886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122886&action=review >> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:190 >> + if (!m_syncMethodDone) { > > Why go through all of this? > > If you post a task without a mode, then it won't run until the nested loop is done. Sorry, I digged the code a bit but I still don't get the idea of what the "mode" is. WorkerThreadableWebSocketChannel posts tasks with m_taskMode specified as the mode (which is generated within ThreadableWebSocketChannel::create()). Is this wrong? (In reply to comment #3) > (From update of attachment 122886 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122886&action=review > > >> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:190 > >> + if (!m_syncMethodDone) { > > > > Why go through all of this? > > > > If you post a task without a mode, then it won't run until the nested loop is done. > > Sorry, I digged the code a bit but I still don't get the idea of what the "mode" is. > > WorkerThreadableWebSocketChannel posts tasks with m_taskMode specified as the mode (which is generated within ThreadableWebSocketChannel::create()). Is this wrong? When a nested loop runs in a worker, it runs in a "mode". The mode is just a string but that string acts as a filter. It only allows tasks that are posted using that "mode" (the same string) to run during that nested loop. If a task is posted with no mode, then the task isn't run until the nested loop is done. In general, the code should posts tasks using the m_taskMode (or else they won't run at all during this nested message loop.). However, in this case, for this one task, it sounds like you want to run a task but not until the nested loop is done, so if the code uses "postTask" instead of "postTaskForMode" (or uses "" as the mode if necessary), then you will get the desired result. Hope that helps. Comment on attachment 122886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122886&action=review >>>> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:190 >>>> + if (!m_syncMethodDone) { >>> >>> Why go through all of this? >>> >>> If you post a task without a mode, then it won't run until the nested loop is done. >> >> Sorry, I digged the code a bit but I still don't get the idea of what the "mode" is. >> >> WorkerThreadableWebSocketChannel posts tasks with m_taskMode specified as the mode (which is generated within ThreadableWebSocketChannel::create()). Is this wrong? > > When a nested loop runs in a worker, it runs in a "mode". The mode is just a string but that string acts as a filter. It only allows tasks that are posted using that "mode" (the same string) to run during that nested loop. If a task is posted with no mode, then the task isn't run until the nested loop is done. > > In general, the code should posts tasks using the m_taskMode (or else they won't run at all during this nested message loop.). However, in this case, for this one task, it sounds like you want to run a task but not until the nested loop is done, so if the code uses "postTask" instead of "postTaskForMode" (or uses "" as the mode if necessary), then you will get the desired result. > > Hope that helps. Okay, I think I got your point. I'm going to update the patch. Comment on attachment 122886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122886&action=review >>>>> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:190 >>>>> + if (!m_syncMethodDone) { >>>> >>>> Why go through all of this? >>>> >>>> If you post a task without a mode, then it won't run until the nested loop is done. >>> >>> Sorry, I digged the code a bit but I still don't get the idea of what the "mode" is. >>> >>> WorkerThreadableWebSocketChannel posts tasks with m_taskMode specified as the mode (which is generated within ThreadableWebSocketChannel::create()). Is this wrong? >> >> When a nested loop runs in a worker, it runs in a "mode". The mode is just a string but that string acts as a filter. It only allows tasks that are posted using that "mode" (the same string) to run during that nested loop. If a task is posted with no mode, then the task isn't run until the nested loop is done. >> >> In general, the code should posts tasks using the m_taskMode (or else they won't run at all during this nested message loop.). However, in this case, for this one task, it sounds like you want to run a task but not until the nested loop is done, so if the code uses "postTask" instead of "postTaskForMode" (or uses "" as the mode if necessary), then you will get the desired result. >> >> Hope that helps. > > Okay, I think I got your point. I'm going to update the patch. After some investigation, I started to think postTask'ing pending tasks (e.g. did*Callback()) is infeasible because we can't call these callbacks when the worker is suspended. We need to process these events in the chronological order, and if we postTask'ed each event it would be difficult to handle suspension correctly. That's why there's m_pendingTasks, I guess. (Maybe the responsibility of suspension handling should exist in WebSocket class, not here; but fixing it would require another work.) Comment on attachment 122886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122886&action=review Thanks for the explanation. (I think I led you astray.) > Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:188 > + if (m_suspended) Since you are adding this check, perhaps in another patch (not this patch), you could change the many places that do this if (!m_suspended) processPendingTasks(); to processPendingTasks(); Thanks. I somehow missed you r+ed my patch (sorry!), so I'm trying CQ now... Comment on attachment 122886 [details] Patch Clearing flags on attachment: 122886 Committed r107788: <http://trac.webkit.org/changeset/107788> All reviewed patches have been landed. Closing bug. |