Bug 231664 – Guarantee order of WebSocket events in case of being resumed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Guarantee order of WebSocket events in case of being resumed

[youenn fablet]

Created attachment 441050 [details]
Patch

[Alex Christensen]

What motivated this change?

[Chris Dumez]

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).

[youenn fablet]

(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.

[youenn fablet]

Created attachment 441203 [details]
Patch

[youenn fablet]

Created attachment 441205 [details]
Patch

[youenn fablet]

Created attachment 441336 [details]
Patch

[youenn fablet]

Created attachment 441590 [details]
Patch

[youenn fablet]

Created attachment 441597 [details]
Patch

[youenn fablet]

Created attachment 441702 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

(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.

[youenn fablet]

I guess I can remove these ASSERTs since we are not doing them consistently now after enqueuing a task.

[Chris Dumez]

(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.

[youenn fablet]

(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.

[Chris Dumez]

(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).

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/84425230>