| Summary: | Web Inspector: REGRESSION(r266467): viewing a WebSocket created before Web Inspector was opened doesn't show any message frames | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||
| Component: | Web Inspector | Assignee: | Patrick Angle <pangle> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | bburg, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, pangle, toyoshim, webkit-bug-importer, youennf, yutak | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Devin Rousso
2020-12-03 14:48:58 PST
Created attachment 416367 [details]
Patch v1.0
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. 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. 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. 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. Created attachment 416369 [details]
Patch v1.1 - BJ and Devin review notes
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? 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. 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. 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. (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. Created attachment 416424 [details]
Patch v1.2 - Youenn review notes
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.
Just verified the test passes when enabling the NSURLSession WebSockets Committed r270937: <https://trac.webkit.org/changeset/270937> All reviewed patches have been landed. Closing bug and clearing flags on attachment 416424 [details]. |