Bug 190046 – Web Inspector: crash in InspectorNetworkAgent::didReceiveResponse when loading denied x-frame resources

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

[Devin Rousso]

Create a test file "crash.html" and add the following to it:

    <iframe src="http://devinrousso.com"></iframe>

Open the file in release/debug, open WebInspector, and refresh the page.

This is caused by the `ResourceResponse` being "null", which means we generate a `nullptr` value for our `Inspector::Protocol::Network::Response`, which isn't JSON serializable, so crash.

[Devin Rousso]

Created attachment 350982 [details]
Patch

[EWS Watchlist]

Comment on attachment 350982 [details]
Patch

Attachment 350982 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9370787

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/navigation-redirect-to-http.https.html
http/wpt/service-workers/header-filtering.https.html
imported/w3c/web-platform-tests/xhr/access-control-basic-cors-safelisted-response-headers.htm
http/tests/fetch/redirectmode-and-preload.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-mode-worker.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-redirect.https.html
http/tests/fetch/caching-with-different-options.html
imported/w3c/web-platform-tests/service-workers/service-worker/redirected-response.https.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-mode.html
http/tests/xmlhttprequest/access-control-basic-whitelist-response-headers.html
http/tests/webarchive/cross-origin-stylesheet-crash.html

[EWS Watchlist]

Created attachment 350985 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 350982 [details]
Patch

Attachment 350982 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9373286

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/navigation-redirect-to-http.https.html
http/wpt/service-workers/header-filtering.https.html
imported/w3c/web-platform-tests/xhr/access-control-basic-cors-safelisted-response-headers.htm
http/tests/fetch/redirectmode-and-preload.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-mode-worker.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-redirect.https.html
http/tests/fetch/caching-with-different-options.html
imported/w3c/web-platform-tests/service-workers/service-worker/redirected-response.https.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-mode.html
http/tests/xmlhttprequest/access-control-basic-whitelist-response-headers.html

[EWS Watchlist]

Created attachment 351021 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Devin Rousso]

Created attachment 351058 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 350982 [details]
Patch

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

Oops apparently I never submitted these comments.

> Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:190
> -    m_coreLoader->documentLoader()->stopLoadingAfterXFrameOptionsOrContentSecurityPolicyDenied(m_coreLoader->identifier(), ResourceResponse { });
> +    m_coreLoader->documentLoader()->stopLoadingAfterXFrameOptionsOrContentSecurityPolicyDenied(m_coreLoader->identifier(), response);

My guess is that this now non-null response is going to cause differences in WebCore handling of the response separate from Web Inspector code. That might be a good thing or a bad thing. It looks like some tests saw differences.

> LayoutTests/http/tests/inspector/network/x-frame-options.html:18
> +        description: "Ensure that x-frame/CSP denials are recieved and don't crash.",

Nit: "X-Frame-Options" instead of "x-frame" is clearer and easier to search for. Also drop the "don't crash" part, which is pretty much always our expectation. This fix eliminates the crash but then also provides inspector with the correct data it wasn't getting before and the description of the test can be the latter which.

> LayoutTests/http/tests/inspector/network/x-frame-options.html:35
> +<p>Tests for various x-frame-options headers.</p>

Nit: X-Frame-Options

Did you want to add another test for a different option, like "sameorigin"?

[Joseph Pecoraro]

Comment on attachment 351058 [details]
Patch

r=me, but the same comments apply regarding the test.

[Devin Rousso]

Created attachment 351117 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 351117 [details]
Patch

Clearing flags on attachment: 351117

Committed r236627: <https://trac.webkit.org/changeset/236627>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44879692>