Fetch's Response.statusText is unexpectedly the full http status line for HTTP/2 responses

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Fetch's Response.statusText is unexpectedly the full http status line for HTTP/2 responses

* Steps to reproduce
1. Inspect "http://www.google.com"
2. fetch("/").then(console.log)
3. Expand the resulting Response
  => statusText is "HTTP/2.0 200", expected "" or "OK"

* Notes

HTTP/2 doesn't really have a status message, just a status code.
https://tools.ietf.org/html/rfc7540#section-8.1.2.4
> 8.1.2.4.  Response Pseudo-Header Fields
> 
>    For HTTP/2 responses, a single ":status" pseudo-header field is
>    defined that carries the HTTP status code field (see [RFC7231],
>    Section 6).  This pseudo-header field MUST be included in all
>    responses; otherwise, the response is malformed (Section 8.1.2.6).
> 
>    HTTP/2 does not define a way to carry the version or reason phrase
>    that is included in an HTTP/1.1 status line.

Spec is ambiguous:
https://github.com/whatwg/fetch/issues/599

Lets start by moving to empty string.

[Joseph Pecoraro]

Created attachment 320079 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 320079 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/platform/network/HTTPParsers.cpp:486
>      size_t spacePos = view.find(' ');

I considered making this:

    view.find(' ', strlen("HTTP"))

since we expect this to always start with "HTTP... ### ...". But I left it alone since who knows what the future holds. And it isn't exactly related to what this bug is fixing.

[Sam Weinig]

Comment on attachment 320079 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/ChangeLog:8
> +        No test. HTTP/2 test server is not yet available.

Are you sure WPT can't do this? It can send pretty much arbitrary responses.

[Joseph Pecoraro]

> > Source/WebCore/ChangeLog:8
> > +        No test. HTTP/2 test server is not yet available.
> 
> Are you sure WPT can't do this? It can send pretty much arbitrary responses.

I spent some time looking both on the web and in tests and did not find a way.

The closest I came to something promising was:
https://github.com/w3c/web-platform-tests/pull/5102/files/7dcde16821ed2bec7ee05e401bf0d3327c08bcf4#diff-d2596ee33b586549641da55dbd43e131

Which points out that HTTP/2 and HTTP/1.1 are wire-incompatible. So while WPT can send an arbitrary responses that claim to be HTTP/2 that may not be good enough.

I ended up filing:
<https://webkit.org/b/176483> Provide an HTTP/2 LayoutTest server

---

However, you did just help me realize that I probably don't need HTTP/2. In the case this change is addressing, we just need to test that we correctly handle the data we get from CFNetwork (CFHTTPMessageCopyResponseStatusLine) for a response without a status message.

HTTP/2 just always causes this to happen because there is no status message. However I should be able to send an HTTP/1.1 response that has no status message, and verify that we fallback to an empty string instead of the full status line.

I'll do that.

[Joseph Pecoraro]

Created attachment 320092 [details]
[PATCH] Proposed Fix

[Alexey Proskuryakov]

Comment on attachment 320092 [details]
[PATCH] Proposed Fix

Is this how other browsers actually behave, or do they generate a synthetic status for backward compatibility?

[Joseph Pecoraro]

(In reply to Alexey Proskuryakov from comment #6)
> Comment on attachment 320092 [details]
> [PATCH] Proposed Fix
> 
> Is this how other browsers actually behave, or do they generate a synthetic
> status for backward compatibility?

See: https://github.com/whatwg/fetch/issues/599

This behavior makes us match Chrome. Firefox seems to synthesize the default text for common status codes.

[WebKit Commit Bot]

Comment on attachment 320092 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 320092

Committed r221716: <http://trac.webkit.org/changeset/221716>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

(In reply to Sam Weinig from comment #3)
> Comment on attachment 320079 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320079&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No test. HTTP/2 test server is not yet available.
> 
> Are you sure WPT can't do this? It can send pretty much arbitrary responses.

There is a GitHub issue for that but no work towards it AFAIK.
Doing it through Apache in the short term sounds feasible.

[Ryan Haddad]

This change caused assertion failures on macOS Debug WK2:

ASSERTION FAILED: m_isConstructed
/Volumes/Data/slave/sierra-debug/build/WebKitBuild/Debug/usr/local/include/wtf/NeverDestroyed.h(119) : PointerType WTF::LazyNeverDestroyed<WTF::AtomicString>::storagePointer() const [T = WTF::AtomicString]
1   0x12196f8ad WTFCrash
2   0x11349cab6 WTF::LazyNeverDestroyed<WTF::AtomicString>::storagePointer() const
3   0x11349ca65 WTF::LazyNeverDestroyed<WTF::AtomicString>::get()
4   0x113625860 WTF::emptyAtom()
5   0x114312357 WebCore::extractReasonPhraseFromHTTPStatusLine(WTF::String const&)
6   0x115ae32f4 WebCore::extractHTTPStatusText(__CFHTTPMessage*)
7   0x115ae2efb WebCore::ResourceResponse::platformLazyInit(WebCore::ResourceResponseBase::InitLevel)
8   0x115adf79b WebCore::ResourceResponseBase::lazyInit(WebCore::ResourceResponseBase::InitLevel) const
9   0x10ec032f8 void WebCore::ResourceResponseBase::encode<IPC::Encoder>(IPC::Encoder&) const
10  0x10ec032a8 IPC::ArgumentCoder<WebCore::ResourceResponse>::encode(IPC::Encoder&, WebCore::ResourceResponse const&)
11  0x10ec03275 std::__1::enable_if<!(std::is_enum<std::__1::remove_const<std::__1::remove_reference<WebCore::ResourceResponse const&>::type>::type>::value), void>::type IPC::Encoder::encode<WebCore::ResourceResponse const&>(WebCore::ResourceResponse const&&&)
12  0x10ec02da7 IPC::Encoder& IPC::Encoder::operator<<<WebCore::ResourceResponse const&>(WebCore::ResourceResponse const&&&)
13  0x10ef39d64 IPC::TupleCoder<1ul, WebCore::ResourceRequest const&, WebCore::ResourceResponse const&>::encode(IPC::Encoder&, std::__1::tuple<WebCore::ResourceRequest const&, WebCore::ResourceResponse const&> const&)
14  0x10ef39d21 IPC::TupleCoder<2ul, WebCore::ResourceRequest const&, WebCore::ResourceResponse const&>::encode(IPC::Encoder&, std::__1::tuple<WebCore::ResourceRequest const&, WebCore::ResourceResponse const&> const&)
15  0x10ef39ccd IPC::ArgumentCoder<std::__1::tuple<WebCore::ResourceRequest const&, WebCore::ResourceResponse const&> >::encode(IPC::Encoder&, std::__1::tuple<WebCore::ResourceRequest const&, WebCore::ResourceResponse const&> const&)
16  0x10ef39c95 std::__1::enable_if<!(std::is_enum<std::__1::remove_const<std::__1::remove_reference<std::__1::tuple<WebCore::ResourceRequest const&, WebCore::ResourceResponse const&> const&>::type>::type>::value), void>::type IPC::Encoder::encode<std::__1::tuple<WebCore::ResourceRequest const&, WebCore::ResourceResponse const&> const&>(std::__1::tuple<WebCore::ResourceRequest const&, WebCore::ResourceResponse const&> const&&&)
17  0x10ef39899 bool IPC::MessageSender::send<Messages::WebResourceLoader::WillSendRequest>(Messages::WebResourceLoader::WillSendRequest const&, unsigned long long, WTF::OptionSet<IPC::SendOption>)
18  0x10ef338c3 bool IPC::MessageSender::send<Messages::WebResourceLoader::WillSendRequest>(Messages::WebResourceLoader::WillSendRequest const&)
19  0x10ef33471 WebKit::NetworkResourceLoader::willSendRedirectedRequest(WebCore::ResourceRequest&&, WebCore::ResourceRequest&&, WebCore::ResourceResponse&&)
20  0x10eedf9ff WebKit::NetworkLoad::sharedWillSendRedirectedRequest(WebCore::ResourceRequest&&, WebCore::ResourceResponse&&)
21  0x10eedff75 WebKit::NetworkLoad::willPerformHTTPRedirection(WebCore::ResourceResponse&&, WebCore::ResourceRequest&&, WTF::CompletionHandler<void (WebCore::ResourceRequest const&)>&&)
22  0x10eed783a WebKit::NetworkDataTaskCocoa::willPerformHTTPRedirection(WebCore::ResourceResponse&&, WebCore::ResourceRequest&&, WTF::CompletionHandler<void (WebCore::ResourceRequest const&)>&&)
23  0x10ef617c4 -[WKNetworkSessionDelegate URLSession:task:willPerformHTTPRedirection:newRequest:completionHandler:]
24  0x7fffc683c92d __86-[NSURLSession delegate_task:willPerformHTTPRedirection:newRequest:completionHandler:]_block_invoke
25  0x7fffc8f581a9 __NSBLOCKOPERATION_IS_CALLING_OUT_TO_A_BLOCK__
26  0x7fffc8f57e8c -[NSBlockOperation main]
27  0x7fffc8f565b4 -[__NSOperationInternal _start:]
28  0x7fffc8f5246b __NSOQSchedule_f
29  0x7fffdd2078fc _dispatch_client_callout
30  0x7fffdd214aac _dispatch_main_queue_callback_4CF
31  0x7fffc7544d69 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__

https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20%28Tests%29/builds/2914

[Ryan Haddad]

Reverted r221716 for reason:

This change caused assertion failures on macOS Debug WK2.

Committed r221732: <http://trac.webkit.org/changeset/221732>

[Joseph Pecoraro]

(In reply to Ryan Haddad from comment #11)
> This change caused assertion failures on macOS Debug WK2:
> 
> ASSERTION FAILED: m_isConstructed

This is because we are using one of the global AtomicStrings (emptyAtom) and those must be initialized with AtomicString::init. These assertions were all in the NetworkProcess, which apparently was never initializing atomic strings.

Two solutions come to mind:

1. Don't use `emptyAtom()` and instead just normal AtomicString construction for the empty string. For example returning `emptyString()` here will get converted to an AtomicString without using the emptyAtom static.

2. Initialize AtomicStrings at some point in the NetworkProcess.

After talking to a few folks I think (2) is the better solution. If anyone comes along and uses the AtomicString statics (nullAtom, emptyAtom, etc) in the NetworkProcess they would need atomic strings to be initialized or they will encounter the exact same issue I encountered.

[Joseph Pecoraro]

Created attachment 320316 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 320316 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 320316

Committed r221804: <http://trac.webkit.org/changeset/221804>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693670>