Bug 190046 - Web Inspector: crash in InspectorNetworkAgent::didReceiveResponse when loading denied x-frame resources
Summary: Web Inspector: crash in InspectorNetworkAgent::didReceiveResponse when loadin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-27 11:08 PDT by Devin Rousso
Modified: 2018-09-28 17:23 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.65 KB, patch)
2018-09-27 11:33 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.27 MB, application/zip)
2018-09-27 12:32 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.51 MB, application/zip)
2018-09-27 16:58 PDT, EWS Watchlist
no flags Details
Patch (9.22 KB, patch)
2018-09-27 23:37 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (9.88 KB, patch)
2018-09-28 14:58 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-09-27 11:08:32 PDT
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.
Comment 1 Devin Rousso 2018-09-27 11:33:20 PDT
Created attachment 350982 [details]
Patch
Comment 2 EWS Watchlist 2018-09-27 12:32:14 PDT Comment hidden (obsolete)
Comment 3 EWS Watchlist 2018-09-27 12:32:15 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2018-09-27 16:58:18 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2018-09-27 16:58:20 PDT Comment hidden (obsolete)
Comment 6 Devin Rousso 2018-09-27 23:37:50 PDT
Created attachment 351058 [details]
Patch
Comment 7 Joseph Pecoraro 2018-09-28 10:24:09 PDT
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"?
Comment 8 Joseph Pecoraro 2018-09-28 10:25:13 PDT
Comment on attachment 351058 [details]
Patch

r=me, but the same comments apply regarding the test.
Comment 9 Devin Rousso 2018-09-28 14:58:09 PDT
Created attachment 351117 [details]
Patch
Comment 10 WebKit Commit Bot 2018-09-28 17:22:15 PDT
Comment on attachment 351117 [details]
Patch

Clearing flags on attachment: 351117

Committed r236627: <https://trac.webkit.org/changeset/236627>
Comment 11 WebKit Commit Bot 2018-09-28 17:22:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2018-09-28 17:23:38 PDT
<rdar://problem/44879692>