Bug 194819 – Fix crash when opening Web Inspector after a WebSocket was blocked by content extensions

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Loïc Yhuel]

Fix crash when opening Web Inspector after a WebSocket was blocked by content extensions

[Loïc Yhuel]

Created attachment 362392 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Loïc Yhuel]

Created attachment 362415 [details]
Patch

[Loïc Yhuel]

Patch updated to only enable the test on mac-wk2/gtk/wpe, like http/tests/contentextensions.

[Radar WebKit Bug Importer]

<rdar://problem/48211419>

[Joseph Pecoraro]

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();
        }
    });

[Loïc Yhuel]

(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.

[Loïc Yhuel]

Created attachment 362421 [details]
Patch

[Joseph Pecoraro]

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

[Loïc Yhuel]

Created attachment 362496 [details]
Patch

[Loïc Yhuel]

(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

[WebKit Commit Bot]

Comment on attachment 362496 [details]
Patch

Clearing flags on attachment: 362496

Committed r241824: <https://trac.webkit.org/changeset/241824>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Thanks again!

Are there any other issues you're aware of that you may want us to investigate?

[Loïc Yhuel]

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.