WebSocket::suspend() causes error event to be fired after r182517. This is not allowed as firing the event could trigger arbitrary JS execution, which is no longer allowed at this point. We should delay the error event firing until after WebSocket::resume() is called, similarly to what we already do for the close event. Radar: <rdar://problem/20559812>
Created attachment 250892 [details] Patch
Comment on attachment 250892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250892&action=review > Source/WebCore/Modules/websockets/WebSocket.cpp:507 > + for (auto& event : pendingEvents) > + dispatchEvent(WTF::move(event)); Anything can happen while firing the first event from pendingEvents, so some protection is likely needed. At the very least, the WebSocket object should be protected with a Ref, but we also may need to check that we didn't enter suspend again. This gets confusing quickly.
Created attachment 250897 [details] Patch
(In reply to comment #2) > Comment on attachment 250892 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250892&action=review > > > Source/WebCore/Modules/websockets/WebSocket.cpp:507 > > + for (auto& event : pendingEvents) > > + dispatchEvent(WTF::move(event)); > > Anything can happen while firing the first event from pendingEvents, so some > protection is likely needed. > > At the very least, the WebSocket object should be protected with a Ref, but > we also may need to check that we didn't enter suspend again. This gets > confusing quickly. I protected |this| and handled the case where suspend() is called while we're firing the events (by queueing the events again).
Comment on attachment 250897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250897&action=review > Source/WebCore/Modules/websockets/WebSocket.cpp:511 > + dispatchOrQueueEvent(WTF::move(event)); I think that this results in a wrong order of events. The remaining events should be prepended (not one by one, but all a once). It would be good to have test cases for these situations.
(In reply to comment #5) > Comment on attachment 250897 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250897&action=review > > > Source/WebCore/Modules/websockets/WebSocket.cpp:511 > > + dispatchOrQueueEvent(WTF::move(event)); > > I think that this results in a wrong order of events. The remaining events > should be prepended (not one by one, but all a once). > > It would be good to have test cases for these situations. Hmm. Good point. I'll see what happens if I navigate away from the error event handler.
Comment on attachment 250897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250897&action=review >>> Source/WebCore/Modules/websockets/WebSocket.cpp:511 >>> + dispatchOrQueueEvent(WTF::move(event)); >> >> I think that this results in a wrong order of events. The remaining events should be prepended (not one by one, but all a once). >> >> It would be good to have test cases for these situations. > > Hmm. Good point. I'll see what happens if I navigate away from the error event handler. I thought about this more and I don't think there can be any event in the Vector at this point. There is no API to reopen a WebSocket once it is closed. Therefore, the user would have to create a new WebSocket to reopen the connection. For events to be queued, the WebSocket needs to be open when suspend() is called, which can only happen once. I'll add a test for this. Also, creating a Vector copy here is not necessary. I can probably use a Dequeue and do something like this: while (!m_pendingEvents.isEmpty() && !m_shouldDelayEventFiring) dispatchEvent(m_pendingEvents.takeFirst());
Created attachment 250933 [details] Patch
Comment on attachment 250933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250933&action=review > Source/WebCore/Modules/websockets/WebSocket.cpp:509 > + while (!m_pendingEvents.isEmpty() && !m_shouldDelayEventFiring) Check for !m_shouldDelayEventFiring to stop iterating if suspend() is called while we fire the events. I am not sure this is possible but it will be handled if it is. > Source/WebCore/Modules/websockets/WebSocket.cpp:520 > + m_pendingEvents.clear(); We now clear the pending events in stop() so that resumeTimerFired() stops firing events if it is currently doing so. > LayoutTests/ChangeLog:15 > + * http/tests/websocket/tests/hybi/resources/page-cache-websocket.html: Added. Added layout test to cover the case where WebSocket::stop() is called while firing the pending events upon restoring the page from PageCache. I wasn't able to write one that causes WebSocket::suspend() to be called while firing the pending events as I wasn't able to trigger a synchronous navigation from the error event handler.
Comment on attachment 250933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250933&action=review > Source/WebCore/Modules/websockets/WebSocket.cpp:471 > + NoEventDispatchAssertion assertNoEventDispatch; Don't we have a NoEventDispatchAssertion in the caller of suspend()? > Source/WebCore/Modules/websockets/WebSocket.cpp:489 > + NoEventDispatchAssertion assertNoEventDispatch; Ditto.
Comment on attachment 250933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250933&action=review >> Source/WebCore/Modules/websockets/WebSocket.cpp:471 >> + NoEventDispatchAssertion assertNoEventDispatch; > > Don't we have a NoEventDispatchAssertion in the caller of suspend()? ScriptExecutionContext::suspendActiveDOMObjects() is the call site. I don't see any NoEventDispatchAssertion there but it sounds like a good idea though. I'll look into this in a follow-up patch. >> Source/WebCore/Modules/websockets/WebSocket.cpp:489 >> + NoEventDispatchAssertion assertNoEventDispatch; > > Ditto. Ditto for ScriptExecutionContext::resumeActiveDOMObjects() call site. There are assertions in place to make sure that ActiveDOMObjects are no removed or added. However, I don't see any check for event firing. Will look into this in a follow-up patch.
Comment on attachment 250933 [details] Patch Rejecting attachment 250933 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 250933, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.appspot.com/results/5240127588663296
Created attachment 250936 [details] Patch
Comment on attachment 250936 [details] Patch Attachment 250936 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5924837516115968 New failing tests: compositing/scrolling/touch-scroll-to-clip.html
Created attachment 250938 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 250936 [details] Patch Clearing flags on attachment: 250936 Committed r182901: <http://trac.webkit.org/changeset/182901>
All reviewed patches have been landed. Closing bug.
Looks like the new test is crashing on Windows: https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r182909%20(51130)/results.html However, there is no stack trace so I have no idea what's wrong with it.
Brent, is there any way to get a crash log more reliably? The test crashes almost every time.