Summary: | WebSocket needs to suspend/resume as Active DOM object. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fumitoshi Ukai <ukai> | ||||||||
Component: | WebCore JavaScript | Assignee: | Fumitoshi Ukai <ukai> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, pfeldman, yutak | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Fumitoshi Ukai
2010-04-26 22:33:57 PDT
Created attachment 54390 [details]
Patch
(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 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.
Created attachment 55224 [details]
Patch
(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. + 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. (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. Created attachment 55521 [details]
Patch
Comment on attachment 55521 [details]
Patch
r=me
Committed r59116: <http://trac.webkit.org/changeset/59116> |