Bug 190046

Summary: Web Inspector: crash in InspectorNetworkAgent::didReceiveResponse when loading denied x-frame resources
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dbates, ews-watchlist, inspector-bugzilla-changes, joepeck, mkwst, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Patch
none
Patch none

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>