Bug 165120 - Web Inspector: Null ResourceResponse Preflight requests cause crash
Summary: Web Inspector: Null ResourceResponse Preflight requests cause crash
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
Keywords: InRadar
Depends on:
Reported: 2016-11-28 15:47 PST by Joseph Pecoraro
Modified: 2016-11-30 10:02 PST (History)
11 users (show)

See Also:

[PATCH] Proposed Fix (2.78 KB, patch)
2016-11-28 15:51 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-11-28 15:47:29 PST
Null ResourceResponse Preflight requests cause crash

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x00000001021ddb66 Inspector::InspectorObjectBase::writeJSON(WTF::StringBuilder&) const + 342
1   com.apple.JavaScriptCore      	0x00000001021ddb6f Inspector::InspectorObjectBase::writeJSON(WTF::StringBuilder&) const + 351
2   com.apple.JavaScriptCore      	0x00000001021dcab3 Inspector::InspectorValue::toJSONString() const + 83
3   com.apple.JavaScriptCore      	0x00000001021cbdf8 Inspector::NetworkFrontendDispatcher::responseReceived(WTF::String const&, WTF::String const&, WTF::String const&, double, Inspector::Protocol::Page::ResourceType, WTF::RefPtr<Inspector::Protocol::Network::Response>) + 2136
4   com.apple.WebCore             	0x0000000103107693 WebCore::InspectorNetworkAgent::didReceiveResponse(unsigned long, WebCore::DocumentLoader&, WebCore::ResourceResponse const&, WebCore::ResourceLoader*) + 995
5   com.apple.WebCore             	0x00000001029c3901 WebCore::InspectorInstrumentation::didReceiveResourceResponseImpl(WebCore::InspectorInstrumentationCookie const&, unsigned long, WebCore::DocumentLoader*, WebCore::ResourceResponse const&, WebCore::ResourceLoader*) + 49
6   com.apple.WebCore             	0x0000000102d21bd5 WebCore::CrossOriginPreflightChecker::validatePreflightResponse(WebCore::DocumentThreadableLoader&, WebCore::ResourceRequest&&, unsigned long, WebCore::ResourceResponse const&) + 117
7   com.apple.WebCore             	0x00000001029de379 WebCore::CachedResource::checkNotify() + 153
8   com.apple.WebCore             	0x0000000102c8a430 WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*) + 224
9   com.apple.WebCore             	0x00000001029de1ab WebCore::SubresourceLoader::didFinishLoading(double) + 1163
10  com.apple.WebCore             	0x00000001038f6526 WebCore::SubresourceLoader::willSendRequestInternal(WebCore::ResourceRequest&, WebCore::ResourceResponse const&) + 694
11  com.apple.WebCore             	0x00000001037a41a6 WebCore::ResourceLoader::willSendRequest(WebCore::ResourceRequest&&, WebCore::ResourceResponse const&, std::__1::function<void (WebCore::ResourceRequest&&)>&&) + 22
12  com.apple.WebKit              	0x000000010141f7ca WebKit::WebResourceLoader::willSendRequest(WebCore::ResourceRequest&&, WebCore::ResourceResponse&&) + 148

- Unsure how to reproduce.
- Crashes always have CrossOriginPreflightChecker::validatePreflightResponse => NetworkFrontendDispatcher::responseReceived
- InspectorValue::toJSONString choking on a NULL value here means that the ResourceResponse given to InspectorInstrumentation was a "null response"
Comment 1 Joseph Pecoraro 2016-11-28 15:47:41 PST
Comment 2 Joseph Pecoraro 2016-11-28 15:51:26 PST
Created attachment 295545 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2016-11-28 15:53:10 PST
Youeen if you have any ideas how we can get a Null Response here, please provide pointers and I can try and add a LayoutTest to cover it. The LayoutTests (http/tests/xmlhttprequest) seem to cover quite a few cases, and even after reading the code I'm unsure how we could get a Null response here without the Request being a failure or cancelled.
Comment 4 Joseph Pecoraro 2016-11-28 17:19:59 PST
For these crashes to happen Web Inspector would have to have been open, otherwise we wouldn't get into WebCore::InspectorNetworkAgent at all.
Comment 5 Daniel Bates 2016-11-28 18:45:16 PST
Have you tested with a CORS preflight request that is redirected?
Comment 6 Daniel Bates 2016-11-28 18:47:09 PST
(In reply to comment #5)
> Have you tested with a CORS preflight request that is redirected?

When handling such a case we get here <http://trac.webkit.org/browser/trunk/Source/WebCore/loader/SubresourceLoader.cpp?rev=208919#L190>.
Comment 7 Joseph Pecoraro 2016-11-28 18:59:20 PST
(In reply to comment #5)
> Have you tested with a CORS preflight request that is redirected?

I ran a LayoutTest that should cover this with Web Inspector open and didn't encounter any issues:

However looking at that code:

>    ResourceResponse opaqueRedirectedResponse;
>    opaqueRedirectedResponse.setURL(redirectResponse.url());
>    opaqueRedirectedResponse.setType(ResourceResponse::Type::Opaqueredirect);
>    m_resource->responseReceived(opaqueRedirectedResponse);

ResourceResponseBase::setURL would make this response non-null.

I'll try putting an early ASSERT in and seeing if a Debug build can catch this with any LayoutTests.
Comment 8 youenn fablet 2016-11-29 01:28:51 PST
Preflight requests redirect fetch mode is set to Manual so that no redirection is followed.
For simplicity and efficiency currently, we return a null response at the "application level" (i.e. the preflight checker).

But it seems useful to pass the information to the inspector (and service worker in the future). So maybe we should make a copy of the redirect response in manual-redirect mode, still setting the response type to opaqueRedirect.
Comment 9 BJ Burg 2016-11-30 09:36:17 PST
Comment on attachment 295545 [details]
[PATCH] Proposed Fix

Comment 10 WebKit Commit Bot 2016-11-30 10:01:56 PST
Comment on attachment 295545 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 295545

Committed r209134: <http://trac.webkit.org/changeset/209134>
Comment 11 WebKit Commit Bot 2016-11-30 10:02:01 PST
All reviewed patches have been landed.  Closing bug.