Bug 194819 - Fix crash when opening Web Inspector after a WebSocket was blocked by content extensions
Summary: Fix crash when opening Web Inspector after a WebSocket was blocked by content...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-19 10:15 PST by Loïc Yhuel
Modified: 2019-02-21 00:30 PST (History)
12 users (show)

See Also:


Attachments
Patch (5.88 KB, patch)
2019-02-19 10:28 PST, Loïc Yhuel
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (2.45 MB, application/zip)
2019-02-19 11:31 PST, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-highsierra (2.06 MB, application/zip)
2019-02-19 12:14 PST, Build Bot
no flags Details
Patch (8.60 KB, patch)
2019-02-19 12:35 PST, Loïc Yhuel
no flags Details | Formatted Diff | Diff
Patch (8.59 KB, patch)
2019-02-19 13:36 PST, Loïc Yhuel
no flags Details | Formatted Diff | Diff
Patch (8.59 KB, patch)
2019-02-20 06:30 PST, Loïc Yhuel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Loïc Yhuel 2019-02-19 10:15:19 PST
Fix crash when opening Web Inspector after a WebSocket was blocked by content extensions
Comment 1 Loïc Yhuel 2019-02-19 10:28:27 PST
Created attachment 362392 [details]
Patch
Comment 2 Build Bot 2019-02-19 11:31:15 PST
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
Comment 3 Build Bot 2019-02-19 11:31:17 PST
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 4 Build Bot 2019-02-19 12:14:04 PST
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
Comment 5 Build Bot 2019-02-19 12:14:06 PST
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
Comment 6 Loïc Yhuel 2019-02-19 12:35:14 PST
Created attachment 362415 [details]
Patch
Comment 7 Loïc Yhuel 2019-02-19 12:38:34 PST
Patch updated to only enable the test on mac-wk2/gtk/wpe, like http/tests/contentextensions.
Comment 8 Radar WebKit Bug Importer 2019-02-19 12:56:55 PST
<rdar://problem/48211419>
Comment 9 Joseph Pecoraro 2019-02-19 13:02:39 PST
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();
        }
    });
Comment 10 Loïc Yhuel 2019-02-19 13:34:29 PST
(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.
Comment 11 Loïc Yhuel 2019-02-19 13:36:22 PST
Created attachment 362421 [details]
Patch
Comment 12 Joseph Pecoraro 2019-02-19 17:22:00 PST
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
Comment 13 Loïc Yhuel 2019-02-20 06:30:49 PST
Created attachment 362496 [details]
Patch
Comment 14 Loïc Yhuel 2019-02-20 06:32:03 PST
(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 15 WebKit Commit Bot 2019-02-20 11:23:35 PST
Comment on attachment 362496 [details]
Patch

Clearing flags on attachment: 362496

Committed r241824: <https://trac.webkit.org/changeset/241824>
Comment 16 WebKit Commit Bot 2019-02-20 11:23:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Joseph Pecoraro 2019-02-20 11:24:53 PST
Thanks again!

Are there any other issues you're aware of that you may want us to investigate?
Comment 18 Loïc Yhuel 2019-02-20 13:48:21 PST
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.