Summary: | Regression(r182517): WebSocket::suspend() causes error event to be fired | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||
Component: | Page Loading | Assignee: | Chris Dumez <cdumez> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ap, bfulgham, buildbot, commit-queue, rniwa | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=144057 | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 143505 | ||||||||||||||
Attachments: |
|
Description
Chris Dumez
2015-04-15 15:43:05 PDT
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. |