Bug 219510 – Web Inspector: REGRESSION(r266467): viewing a WebSocket created before Web Inspector was opened doesn't show any message frames

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

# STEPS TO REPRODUCE
1. inspect <http://devinrousso.com:8080/dots/>
2. select the "websocket" resource
3. click on "Play AI" and then click anywhere in the grid

# EXPECTED
there would be message frames of the format `{"type":"move","player":1,"line":{"r":3,"c":6,"side":"b"},"current":2}` 

# ACTUAL
nothing is shown after "WebSocket Connection Established"

# NOTES
[Error] Assertion Failed
	_webSocketFrameReceivedOrSent (NetworkManager.js:750)
	webSocketFrameSent (NetworkManager.js:728)
	webSocketFrameSent (NetworkObserver.js:109)
	_dispatchEvent (Connection.js:210)
	dispatch (Connection.js:79)
	dispatchMessageFromTarget (TargetManager.js:176)
	dispatchMessageFromTarget (TargetObserver.js:47)
	_dispatchEvent (Connection.js:210)
	dispatch (Connection.js:79)
	dispatch (InspectorBackend.js:232)
	(anonymous function) (MessageDispatcher.js:42)

[Radar WebKit Bug Importer]

<rdar://problem/71953639>

[Patrick Angle]

Created attachment 416367 [details]
Patch v1.0

[Blaze Burg]

Comment on attachment 416367 [details]
Patch v1.0

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

LGTM, let's wait for Youenn to take a peek too.

> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.h:163
> +    // Dummy implementation of inspector related APIs.

Comment is probably not needed.

[Patrick Angle]

Comment on attachment 416367 [details]
Patch v1.0

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

>> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.h:163
>> +    // Dummy implementation of inspector related APIs.
> 
> Comment is probably not needed.

I could go either way with this - this comment was removed in r266467, and actually appears meant to describe not only `requestIdentifier()` but also `hasCreatedHandshake()` and `isConnected()`, although I suppose it's pretty obvious these are stubs given their values.

[Devin Rousso]

Comment on attachment 416367 [details]
Patch v1.0

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

r=me as well, nice work!

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:74
> -        m_identifier = page->progress().createUniqueIdentifier();
> +        m_requestIdentifier = page->progress().createUniqueIdentifier();

This doesn't appear to have been touched in r266467.  Is this renaming just for clarity?

> Source/WebCore/Modules/websockets/WebSocketChannel.h:212
> +    unsigned m_requestIdentifier { 0 }; // m_identifier == 0 means that we could not obtain a request identifier.

NIT: Should the comment also change `m_identifier` to `m_requestIdentifier`?

>> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.h:163
>> +    // Dummy implementation of inspector related APIs.
> 
> Comment is probably not needed.

Yeah I'd just move it below the FIXME comment.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:66
> +                    data: "Hello World! ÐÑÐ¸Ð²ÐµÑ ÐиÑ!",

What are these characters?

> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:78
> +

Style: unnecessary newline

> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:86
> +                    resolve();

NIT: Just in case, please remove this event listener so that if more test cases are added later it won't interfere with the output.

[Patrick Angle]

Comment on attachment 416367 [details]
Patch v1.0

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

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:74
>> +        m_requestIdentifier = page->progress().createUniqueIdentifier();
> 
> This doesn't appear to have been touched in r266467.  Is this renaming just for clarity?

Yes, the rename is for clarity so that it is clear that the `m_requestIdentifier` is not the same as the `m_identifier`.

>> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:66
>> +                    data: "Hello World! ÐÑÐ¸Ð²ÐµÑ ÐиÑ!",
> 
> What are these characters?

This is the same data from http/tests/websocket/tests/hybi/inspector/send-and-receive.html - Those characters are supposed to be `Hello World!` in Russian. I'm going to drop them, as the non-ascii case is already tested elsewhere, and the encoding of these appear to be more trouble than they are worth to have in more than one place.

[Patrick Angle]

Created attachment 416369 [details]
Patch v1.1 - BJ and Devin review notes

[youenn fablet]

Comment on attachment 416369 [details]
Patch v1.1 - BJ and Devin review notes

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

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:74
> +        m_requestIdentifier = page->progress().createUniqueIdentifier();

Is it necessary to be a progress identifier so that it might not be ambiguous with other resource loads?
If so, we should do the same strategy in the new code path (WebKit::WebSocketChannel) to resolve the same bug in the new code path.
New code path can be enabled by enabling IsNSURLSessionWebSocketEnabled flag.

Ideally we would reuse identifier() here, or do m_requestIdentifier = identifier().toUInt64() if we want to have null identifiers in case of no page.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:207
>      if (m_document) {

As a side note, we call InspectorInstrumentation::didXX methods in various places with different pre conditions.
Here m_document, below m_document && m_requestIdentifier, or m_requestIdentifier && InspectorInstrumentation::hasFrontends(), sometimes no check.
Should we try to use the same check everywhere?

[Patrick Angle]

Comment on attachment 416369 [details]
Patch v1.1 - BJ and Devin review notes

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

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:74
>> +        m_requestIdentifier = page->progress().createUniqueIdentifier();
> 
> Is it necessary to be a progress identifier so that it might not be ambiguous with other resource loads?
> If so, we should do the same strategy in the new code path (WebKit::WebSocketChannel) to resolve the same bug in the new code path.
> New code path can be enabled by enabling IsNSURLSessionWebSocketEnabled flag.
> 
> Ideally we would reuse identifier() here, or do m_requestIdentifier = identifier().toUInt64() if we want to have null identifiers in case of no page.

Yeah, the identifier needs to not be ambiguous with other resource loads. I'll correct `WebKit::WebSocketChannel` by getting the progressIdentifier from its `m_inspector`'s `m_progressIdentifier`. I'm also going to change this member name from `m_requestIdentifer` to `m_progressIdentifier` in order to be slightly consistent.

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:207
>>      if (m_document) {
> 
> As a side note, we call InspectorInstrumentation::didXX methods in various places with different pre conditions.
> Here m_document, below m_document && m_requestIdentifier, or m_requestIdentifier && InspectorInstrumentation::hasFrontends(), sometimes no check.
> Should we try to use the same check everywhere?

That's a great question. Looking at what checks are implemented for `WebKit::WebSocketChannel`, all calls are actually handled through and intermediate `WebCore::WebSocketChannelInspector` which checks for the presence of a progress identifier and the document. The one check of `InspectorInstrumentation::hasFrontends()` makes sense in order to avoid additional work of getting the request header when we aren't inspecting. My first instinct here is that `WebCore::WebSocketChannel` should adopt the same use of `WebCore::WebSocketChannelInspector` as `WebKit::WebSocketChannel`. If so, that's probably best handled as a follow-up enhancement since it isn't directly related to the issue at hand.

[Patrick Angle]

Comment on attachment 416369 [details]
Patch v1.1 - BJ and Devin review notes

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

>>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:207
>>>      if (m_document) {
>> 
>> As a side note, we call InspectorInstrumentation::didXX methods in various places with different pre conditions.
>> Here m_document, below m_document && m_requestIdentifier, or m_requestIdentifier && InspectorInstrumentation::hasFrontends(), sometimes no check.
>> Should we try to use the same check everywhere?
> 
> That's a great question. Looking at what checks are implemented for `WebKit::WebSocketChannel`, all calls are actually handled through and intermediate `WebCore::WebSocketChannelInspector` which checks for the presence of a progress identifier and the document. The one check of `InspectorInstrumentation::hasFrontends()` makes sense in order to avoid additional work of getting the request header when we aren't inspecting. My first instinct here is that `WebCore::WebSocketChannel` should adopt the same use of `WebCore::WebSocketChannelInspector` as `WebKit::WebSocketChannel`. If so, that's probably best handled as a follow-up enhancement since it isn't directly related to the issue at hand.

Meant to add that for all of these InspectorInstrumentation calls, they immediately return if there are no frontends.

[youenn fablet]

OK, let's go then, we should at least fix the current code path.
It would be nice to do some clean-up.
And we also need to update the code to support the new code path.

[youenn fablet]

(In reply to Patrick Angle from comment #9)
> Comment on attachment 416369 [details]
> Patch v1.1 - BJ and Devin review notes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416369&action=review
> 
> >> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:74
> >> +        m_requestIdentifier = page->progress().createUniqueIdentifier();
> > 
> > Is it necessary to be a progress identifier so that it might not be ambiguous with other resource loads?
> > If so, we should do the same strategy in the new code path (WebKit::WebSocketChannel) to resolve the same bug in the new code path.
> > New code path can be enabled by enabling IsNSURLSessionWebSocketEnabled flag.
> > 
> > Ideally we would reuse identifier() here, or do m_requestIdentifier = identifier().toUInt64() if we want to have null identifiers in case of no page.
> 
> Yeah, the identifier needs to not be ambiguous with other resource loads.
> I'll correct `WebKit::WebSocketChannel` by getting the progressIdentifier
> from its `m_inspector`'s `m_progressIdentifier`. I'm also going to change
> this member name from `m_requestIdentifer` to `m_progressIdentifier` in
> order to be slightly consistent.

Thanks!

It would be nice to do a patch that makes the identifiers consistent.
Either we can update inspector code to not require web socket identifiers to not clash with load identifiers.
Or we could introduce a thread-safe ProgressIdentifier and use it for both WebSocket and regular loads.

[Patrick Angle]

Created attachment 416424 [details]
Patch v1.2 - Youenn review notes

[youenn fablet]

Comment on attachment 416424 [details]
Patch v1.2 - Youenn review notes

LGTM.
It would be good to check the test is passing with the new code path.
This can be done locally by changing WKPreferencesSetIsNSURLSessionWebSocketEnabled(preferences, false) to WKPreferencesSetIsNSURLSessionWebSocketEnabled(preferences, true) in TestController.cpp than running the new test with WTR.
If it does not pass, let me know and I'll look at the issue.

[Patrick Angle]

Just verified the test passes when enabling the NSURLSession WebSockets

[EWS]

Committed r270937: <https://trac.webkit.org/changeset/270937>

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