Guarantee order of WebSocket events in case of being resumed
Created attachment 441050 [details] Patch
What motivated this change?
Comment on attachment 441050 [details] Patch Odds are that we should update WebSocket to dispatch events asynchronously and use the HTML event loop. Then we wouldn't need such to worry about suspending / resuming (since the HTML event loop properly suspends when in bfcache).
(In reply to Chris Dumez from comment #3) > Comment on attachment 441050 [details] > Patch > > Odds are that we should update WebSocket to dispatch events asynchronously > and use the HTML event loop. Then we wouldn't need such to worry about > suspending / resuming (since the HTML event loop properly suspends when in > bfcache). Good point, I'll do that instead.
Created attachment 441203 [details] Patch
Created attachment 441205 [details] Patch
Created attachment 441336 [details] Patch
Created attachment 441590 [details] Patch
Created attachment 441597 [details] Patch
Created attachment 441702 [details] Patch
Comment on attachment 441702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441702&action=review Nice, r=me with fixes. > Source/WebCore/Modules/websockets/WebSocket.cpp:554 > + ASSERT(scriptExecutionContext()); Please do an early return, there is no such guarantee with the event loop as the event loop is shared with other contexts of the same origin. > Source/WebCore/Modules/websockets/WebSocket.cpp:568 > + ASSERT(scriptExecutionContext()); ditto. > Source/WebCore/Modules/websockets/WebSocket.cpp:598 > + ASSERT(scriptExecutionContext()); ditto. > Source/WebCore/Modules/websockets/WebSocket.cpp:632 > + ASSERT(scriptExecutionContext()); ditto.
(In reply to Chris Dumez from comment #11) > Comment on attachment 441702 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=441702&action=review > > Nice, r=me with fixes. > > > Source/WebCore/Modules/websockets/WebSocket.cpp:554 > > + ASSERT(scriptExecutionContext()); > > Please do an early return, there is no such guarantee with the event loop as > the event loop is shared with other contexts of the same origin. > > > Source/WebCore/Modules/websockets/WebSocket.cpp:568 > > + ASSERT(scriptExecutionContext()); > > ditto. > > > Source/WebCore/Modules/websockets/WebSocket.cpp:598 > > + ASSERT(scriptExecutionContext()); > > ditto. > > > Source/WebCore/Modules/websockets/WebSocket.cpp:632 > > + ASSERT(scriptExecutionContext()); > > ditto. I think all these ASSERTs are correct since we either do early return on m_state being closed or m_channel being null and WebSocket::stop does set m_state to closed and m_channel to nullptr.
I guess I can remove these ASSERTs since we are not doing them consistently now after enqueuing a task.
(In reply to youenn fablet from comment #12) > (In reply to Chris Dumez from comment #11) > > Comment on attachment 441702 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=441702&action=review > > > > Nice, r=me with fixes. > > > > > Source/WebCore/Modules/websockets/WebSocket.cpp:554 > > > + ASSERT(scriptExecutionContext()); > > > > Please do an early return, there is no such guarantee with the event loop as > > the event loop is shared with other contexts of the same origin. > > > > > Source/WebCore/Modules/websockets/WebSocket.cpp:568 > > > + ASSERT(scriptExecutionContext()); > > > > ditto. > > > > > Source/WebCore/Modules/websockets/WebSocket.cpp:598 > > > + ASSERT(scriptExecutionContext()); > > > > ditto. > > > > > Source/WebCore/Modules/websockets/WebSocket.cpp:632 > > > + ASSERT(scriptExecutionContext()); > > > > ditto. > > I think all these ASSERTs are correct since we either do early return on > m_state being closed or m_channel being null and WebSocket::stop does set > m_state to closed and m_channel to nullptr. Maybe I misread your code. I just wanted to point out that tasks posted on the HTML event loop can fire after the script execution is gone (and thus after stop() has been called). So in general, asserting that the script execution context exists in a task posted on the HTML event loop is incorrect. It may be correct here in your code due to some other logic but in general it isn't.
(In reply to Chris Dumez from comment #14) > (In reply to youenn fablet from comment #12) > > (In reply to Chris Dumez from comment #11) > > > Comment on attachment 441702 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=441702&action=review > > > > > > Nice, r=me with fixes. > > > > > > > Source/WebCore/Modules/websockets/WebSocket.cpp:554 > > > > + ASSERT(scriptExecutionContext()); > > > > > > Please do an early return, there is no such guarantee with the event loop as > > > the event loop is shared with other contexts of the same origin. > > > > > > > Source/WebCore/Modules/websockets/WebSocket.cpp:568 > > > > + ASSERT(scriptExecutionContext()); > > > > > > ditto. > > > > > > > Source/WebCore/Modules/websockets/WebSocket.cpp:598 > > > > + ASSERT(scriptExecutionContext()); > > > > > > ditto. > > > > > > > Source/WebCore/Modules/websockets/WebSocket.cpp:632 > > > > + ASSERT(scriptExecutionContext()); > > > > > > ditto. > > > > I think all these ASSERTs are correct since we either do early return on > > m_state being closed or m_channel being null and WebSocket::stop does set > > m_state to closed and m_channel to nullptr. > > Maybe I misread your code. I just wanted to point out that tasks posted on > the HTML event loop can fire after the script execution is gone (and thus > after stop() has been called). > So in general, asserting that the script execution context exists in a task > posted on the HTML event loop is incorrect. It may be correct here in your > code due to some other logic but in general it isn't. OK, let's keep the code like this then. I wonder whether we should add an option in queueTaskKeepingObjectAlive to actually not call the callback if the ActiveDOMObject is stopped, it seems that is the behavior most call sites will want.
(In reply to youenn fablet from comment #15) > (In reply to Chris Dumez from comment #14) > > (In reply to youenn fablet from comment #12) > > > (In reply to Chris Dumez from comment #11) > > > > Comment on attachment 441702 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=441702&action=review > > > > > > > > Nice, r=me with fixes. > > > > > > > > > Source/WebCore/Modules/websockets/WebSocket.cpp:554 > > > > > + ASSERT(scriptExecutionContext()); > > > > > > > > Please do an early return, there is no such guarantee with the event loop as > > > > the event loop is shared with other contexts of the same origin. > > > > > > > > > Source/WebCore/Modules/websockets/WebSocket.cpp:568 > > > > > + ASSERT(scriptExecutionContext()); > > > > > > > > ditto. > > > > > > > > > Source/WebCore/Modules/websockets/WebSocket.cpp:598 > > > > > + ASSERT(scriptExecutionContext()); > > > > > > > > ditto. > > > > > > > > > Source/WebCore/Modules/websockets/WebSocket.cpp:632 > > > > > + ASSERT(scriptExecutionContext()); > > > > > > > > ditto. > > > > > > I think all these ASSERTs are correct since we either do early return on > > > m_state being closed or m_channel being null and WebSocket::stop does set > > > m_state to closed and m_channel to nullptr. > > > > Maybe I misread your code. I just wanted to point out that tasks posted on > > the HTML event loop can fire after the script execution is gone (and thus > > after stop() has been called). > > So in general, asserting that the script execution context exists in a task > > posted on the HTML event loop is incorrect. It may be correct here in your > > code due to some other logic but in general it isn't. > > OK, let's keep the code like this then. > I wonder whether we should add an option in queueTaskKeepingObjectAlive to > actually not call the callback if the ActiveDOMObject is stopped, it seems > that is the behavior most call sites will want. We used to make this assumption and it was wrong. It is not uncommon that frames script each other and APIs usually still work in detached iframes if the caller is still in a valid script execution context. This is the reason why we made this change (and it helped pass more WPT tests).
Committed r284472 (243231@main): <https://commits.webkit.org/243231@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 441702 [details].
<rdar://problem/84425230>