Bug 237280 – REGRESSION (r284472): [ Monterey ] http/tests/websocket/tests/hybi/inspector/send-and-recieve-debugger.html is failing

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Truitt Savell]

http/tests/websocket/tests/hybi/inspector/send-and-recieve-debugger.html

Possibly this test just needs a new baseline for Monterey, seems like there is a minor timing difference. 

History:
https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Finspector%2Fsend-and-recieve-debugger.html

Diff:
--- /Volumes/Data/worker/monterey-release-applesilicon-tests-wk2/build/layout-test-results/http/tests/websocket/tests/hybi/inspector/send-and-recieve-debugger-expected.txt
+++ /Volumes/Data/worker/monterey-release-applesilicon-tests-wk2/build/layout-test-results/http/tests/websocket/tests/hybi/inspector/send-and-recieve-debugger-actual.txt
@@ -9,10 +9,10 @@
 PASS: Frame should be outgoing.
 PASS: Message is walltime.
 Pausing script execution with `debugger` statement.
-Resuming script execution.
 PASS: Resource size should double.
 PASS: Frame data should be 'Hello World! Привет Мир!'
 PASS: Frame should be text.
 PASS: Frame should be incoming.
 PASS: Frame walltime should be greater than the previous one.
+Resuming script execution.

[Radar WebKit Bug Importer]

<rdar://problem/89569036>

[Alexey Proskuryakov]

This looks like an outright failure, not a timing difference. The purpose of the test is to verify that messages aren't being handled while JS execution is disabled, and that's exactly what is happening here.

[Patrick Angle]

Created attachment 453635 [details]
Patch v1.0 - For verification in EWS before review

[Patrick Angle]

Created attachment 453638 [details]
Patch v1.1 - For verification in EWS before review

[Patrick Angle]

Created attachment 453660 [details]
Patch v1.2 - For verification in EWS before review

[Patrick Angle]

Created attachment 453748 [details]
Patch v1.3

[Devin Rousso]

Comment on attachment 453748 [details]
Patch v1.3

View in context: https://bugs.webkit.org/attachment.cgi?id=453748&action=review

r=me, nice fix! =D

> Source/WebCore/ChangeLog:20
> +        correct this the `WebKit::WebSocketChannel` now provides a hook into its helper `WebInspectorChannelInspector`

Did you mean `WebSocketChannelInspector`?

> Source/WebCore/Modules/websockets/WebSocket.cpp:614
> +        if (auto* inspector = m_channel->channelInspector())

NIT: Should this also be wrapped in a `if (UNLIKELY(InspectorInstrumentation::hasFrontends())) {`?

[youenn fablet]

Comment on attachment 453748 [details]
Patch v1.3

View in context: https://bugs.webkit.org/attachment.cgi?id=453748&action=review

> Source/WebCore/ChangeLog:19
> +        in the frontend "in the future" (e.g. before the paused script would have been able to be aware of them). To

Isn't it a good thing to have inspector showing what happens in the network, instead of what happens in JS?

> Source/WebCore/Modules/websockets/ThreadableWebSocketChannelClientWrapper.cpp:222
> +    m_pendingTasks.append(makeUnique<ScriptExecutionContext::Task>([this, protectedThis = Ref { *this }, reason] (ScriptExecutionContext&) {

didClose is isolating copy reason, which might not be needed.
We should probably be consistent though.

> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:330
> +    m_loaderProxy.postTaskForModeToWorkerOrWorkletGlobalScope([workerClientWrapper = m_workerClientWrapper, reason](ScriptExecutionContext& context) mutable {

We need to isolateCopy here since we are hoping to another thread.

[Patrick Angle]

Comment on attachment 453748 [details]
Patch v1.3

View in context: https://bugs.webkit.org/attachment.cgi?id=453748&action=review

>> Source/WebCore/ChangeLog:19
>> +        in the frontend "in the future" (e.g. before the paused script would have been able to be aware of them). To
> 
> Isn't it a good thing to have inspector showing what happens in the network, instead of what happens in JS?

The web socket is a viewable "source" with a list of events in the Sources Tab of Web Inspector. The behavior before r284472 was that we only showed these messages when they logically happened in relationship to JS.  This is also how Web Socket messages in Web Inspector are still represented pre-Monterey, since the instrumentation isn't called until script execution is resumed and the event loop has its next chance to fire the appropriate events in JS - otherwise the socket may show messages that the author would have wanted to handle, leaving them wondering why their handler was not called as they step through their code since the handler will be informed some time in the future of the message.

[Patrick Angle]

Created attachment 453850 [details]
Patch v1.4

[Patrick Angle]

Comment on attachment 453850 [details]
Patch v1.4

Oops - meant to set cq+, not r+

[EWS]

Committed r290856 (248088@main): <https://commits.webkit.org/248088@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453850 [details].