Bug 176479

Created attachment 320079 [details] [PATCH] Proposed Fix

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.

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.

> > 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.

Created attachment 320092 [details] [PATCH] Proposed Fix

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?

(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.

Comment on attachment 320092 [details] [PATCH] Proposed Fix Clearing flags on attachment: 320092 Committed r221716: <http://trac.webkit.org/changeset/221716>

All reviewed patches have been landed. Closing bug.

(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.

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

Reverted r221716 for reason: This change caused assertion failures on macOS Debug WK2. Committed r221732: <http://trac.webkit.org/changeset/221732>

(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.

Created attachment 320316 [details] [PATCH] Proposed Fix

Comment on attachment 320316 [details] [PATCH] Proposed Fix Clearing flags on attachment: 320316 Committed r221804: <http://trac.webkit.org/changeset/221804>

All reviewed patches have been landed. Closing bug.

<rdar://problem/34693670>