Fix crash when opening Web Inspector after a WebSocket was blocked by content extensions
Created attachment 362392 [details] Patch
Comment on attachment 362392 [details] Patch Attachment 362392 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11205966 New failing tests: http/tests/inspector/network/contentextensions/blocked-websocket-crash.html
Created attachment 362403 [details] Archive of layout-test-results from ews100 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 362392 [details] Patch Attachment 362392 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11206082 New failing tests: http/tests/inspector/network/contentextensions/blocked-websocket-crash.html
Created attachment 362411 [details] Archive of layout-test-results from ews116 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 362415 [details] Patch
Patch updated to only enable the test on mac-wk2/gtk/wpe, like http/tests/contentextensions.
<rdar://problem/48211419>
Comment on attachment 362415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362415&action=review r=me, thanks for finding, fixing, and including a test!! Let me know if you need me to set the commit-queue flag. > LayoutTests/http/tests/inspector/network/contentextensions/blocked-websocket-crash.html:20 > + test(resolve, reject) { > + // The inspector won't receive anything, so there is nothing to expect. > + // The test just makes sure the inspector initialization is done > + resolve(); > + } Indentation got a little messed up here. But perhaps we should do something to just verify that things worked. How about: suite.addTestCase({ name: "Network.BlockedRequests.WebSocket", description: "Ensure Web Inspector works even with a blocked WebSocket connection", test(resolve, reject) { InspectorTest.pass("Web Inspector initialized."); resolve(); } });
(In reply to Joseph Pecoraro from comment #9) > Comment on attachment 362415 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362415&action=review > > r=me, thanks for finding, fixing, and including a test!! > > Let me know if you need me to set the commit-queue flag. If you are OK with the test, yes (I wasn't sure if it was the proper way, there isn't anything to test on the frontend side since blocked WebSockets are not displayed in the Network tab). > > > LayoutTests/http/tests/inspector/network/contentextensions/blocked-websocket-crash.html:20 > > + test(resolve, reject) { > > + // The inspector won't receive anything, so there is nothing to expect. > > + // The test just makes sure the inspector initialization is done > > + resolve(); > > + } > Oops, mixed tabs and spaces. > Indentation got a little messed up here. But perhaps we should do something > to just verify that things worked. How about: > > suite.addTestCase({ > name: "Network.BlockedRequests.WebSocket", > description: "Ensure Web Inspector works even with a blocked > WebSocket connection", > test(resolve, reject) { > InspectorTest.pass("Web Inspector initialized."); > resolve(); > } > }); OK, this is perhaps a little more clear like this, with a test status in the output (even if there already would be a timeout without the resolve()). I would still write "after a blocked WebSocket connection" instead of "with ...", since it's important : there was no issue if the inspector is opened before the connection is blocked.
Created attachment 362421 [details] Patch
Comment on attachment 362421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362421&action=review > LayoutTests/http/tests/inspector/network/contentextensions/blocked-websocket-crash.html:19 > + } Nit: Indentation on this line is still off
Created attachment 362496 [details] Patch
(In reply to Joseph Pecoraro from comment #12) > Comment on attachment 362421 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362421&action=review > > > LayoutTests/http/tests/inspector/network/contentextensions/blocked-websocket-crash.html:19 > > + } > > Nit: Indentation on this line is still off done
Comment on attachment 362496 [details] Patch Clearing flags on attachment: 362496 Committed r241824: <https://trac.webkit.org/changeset/241824>
All reviewed patches have been landed. Closing bug.
Thanks again! Are there any other issues you're aware of that you may want us to investigate?
Not on the inspector. I will push a first implementation of Content Extensions for Service Worker soon, where there will probably be discussions about the API and how to test it.