Bug 184764 - Limit cookie header access to the Network process
Summary: Limit cookie header access to the Network process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-18 16:54 PDT by Brent Fulgham
Modified: 2018-04-20 21:22 PDT (History)
7 users (show)

See Also:


Attachments
Patch (40.25 KB, patch)
2018-04-18 16:57 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (40.01 KB, patch)
2018-04-18 19:58 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (41.51 KB, patch)
2018-04-18 20:18 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (41.60 KB, patch)
2018-04-18 20:51 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.24 MB, application/zip)
2018-04-18 22:00 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews200 for win-future (12.79 MB, application/zip)
2018-04-18 22:04 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.87 MB, application/zip)
2018-04-18 22:20 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (53.46 MB, application/zip)
2018-04-18 22:40 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.01 MB, application/zip)
2018-04-18 22:52 PDT, EWS Watchlist
no flags Details
Patch v2 (Does not take Youenn's comments into account) (41.79 KB, patch)
2018-04-19 09:04 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.23 MB, application/zip)
2018-04-19 10:10 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (19.87 MB, application/zip)
2018-04-19 10:53 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews114 for mac-sierra (2.98 MB, application/zip)
2018-04-19 10:59 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.94 MB, application/zip)
2018-04-19 11:28 PDT, EWS Watchlist
no flags Details
Patch (with Youenn's suggestions) (43.03 KB, patch)
2018-04-19 17:49 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v3 (with Youenn's comments) (43.04 KB, patch)
2018-04-19 17:56 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (53.84 KB, patch)
2018-04-20 16:00 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch for landing (54.38 KB, patch)
2018-04-20 16:43 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch for landing (54.42 KB, patch)
2018-04-20 17:10 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Speculative Windows build fix (1.50 KB, patch)
2018-04-20 19:45 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Speculative Windows build fix (1.16 KB, patch)
2018-04-20 20:12 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2018-04-18 16:54:29 PDT
Revise the handling of cookie request headers so that we don't interact with them in the WebContent process. They are only needed for interaction with the server and the network process, so we should limit their scope to just the Network process.

Instead, we should handle a token that represents the cookie headers in the WebContent process, which can be converted to the relevant cookie data in the network process when needed.
Comment 1 Brent Fulgham 2018-04-18 16:55:13 PDT
<rdar://problem/36785285>
Comment 2 Brent Fulgham 2018-04-18 16:57:59 PDT
Created attachment 338282 [details]
Patch
Comment 3 Brent Fulgham 2018-04-18 19:58:35 PDT
Created attachment 338298 [details]
Patch
Comment 4 Brent Fulgham 2018-04-18 20:18:55 PDT
Created attachment 338300 [details]
Patch
Comment 5 Brent Fulgham 2018-04-18 20:51:19 PDT
Created attachment 338304 [details]
Patch
Comment 6 EWS Watchlist 2018-04-18 22:00:04 PDT
Comment on attachment 338304 [details]
Patch

Attachment 338304 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7367060

New failing tests:
http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html
http/tests/websocket/tests/hybi/secure-cookie-insecure-connection.pl
http/tests/websocket/tests/hybi/httponly-cookie.pl
http/tests/websocket/tests/hybi/secure-cookie-secure-connection.pl
Comment 7 EWS Watchlist 2018-04-18 22:00:05 PDT
Created attachment 338306 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 8 EWS Watchlist 2018-04-18 22:04:11 PDT
Comment on attachment 338304 [details]
Patch

Attachment 338304 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7367008

New failing tests:
http/tests/websocket/tests/hybi/secure-cookie-insecure-connection.pl
http/tests/websocket/tests/hybi/secure-cookie-secure-connection.pl
http/tests/websocket/tests/hybi/httponly-cookie.pl
Comment 9 EWS Watchlist 2018-04-18 22:04:22 PDT
Created attachment 338307 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 10 EWS Watchlist 2018-04-18 22:20:21 PDT
Comment on attachment 338304 [details]
Patch

Attachment 338304 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7367087

New failing tests:
http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html
http/tests/websocket/tests/hybi/secure-cookie-insecure-connection.pl
http/tests/websocket/tests/hybi/httponly-cookie.pl
http/tests/websocket/tests/hybi/simple-wss.html
http/tests/websocket/tests/hybi/secure-cookie-secure-connection.pl
Comment 11 EWS Watchlist 2018-04-18 22:20:23 PDT
Created attachment 338308 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 12 EWS Watchlist 2018-04-18 22:40:02 PDT
Comment on attachment 338304 [details]
Patch

Attachment 338304 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7367147

New failing tests:
http/tests/websocket/tests/hybi/secure-cookie-insecure-connection.pl
http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html
http/tests/websocket/tests/hybi/httponly-cookie.pl
http/tests/websocket/tests/hybi/simple-wss.html
http/tests/websocket/tests/hybi/secure-cookie-secure-connection.pl
Comment 13 EWS Watchlist 2018-04-18 22:40:05 PDT
Created attachment 338310 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 14 EWS Watchlist 2018-04-18 22:52:00 PDT
Comment on attachment 338304 [details]
Patch

Attachment 338304 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7367213

New failing tests:
http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html
http/tests/websocket/tests/hybi/secure-cookie-insecure-connection.pl
http/tests/websocket/tests/hybi/httponly-cookie.pl
http/tests/websocket/tests/hybi/secure-cookie-secure-connection.pl
Comment 15 EWS Watchlist 2018-04-18 22:52:01 PDT
Created attachment 338312 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 16 youenn fablet 2018-04-18 23:04:22 PDT
Comment on attachment 338304 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:287
> +    handle.sendData(handshakeMessage.data(), handshakeMessage.length(), WTFMove(cookieRequestHeaderFieldProxy), [this, protectedThis = makeRef(*this)] (bool success, bool didAccessSecureCookies) {

I know this is preexisting code but it seems strange to use sendData to actually do the handshake.
Ideally, we would follow the spec, this is probably difficult and not the right time to do it.

Would it be possible instead to add a sendHandshake() or establishConnection() method that would be specialized for WK2?
That might also allow moving the proxy to WK2 specific code, maybe as parameters to the related IPC message.

On the reverse side, we could also add a specific message to notify that the connection was established or not and whether secure cookies were accessed.
That would remove the need to modify the sendData instead of using the completion handler.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:758
> +                processSecureCookies(didAccessSecureCookies);

I think that didAccessSecureCookies can only be false here.
So this call is probably not needed.
If we add a specific message to notify that cookies are used, we would not need those modifications.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:826
> +void WebSocketChannel::sendFrame(WebSocketFrame::OpCode opCode, const char* data, size_t dataLength, WTF::Function<void(bool, bool)> completionHandler)

No need for WTF:: any longer.
Also we could use CompletionHandler instead of Function, modulo some modifications to the WK2 specialization. Maybe for a later patch.
Comment 17 Brent Fulgham 2018-04-19 09:04:20 PDT
Created attachment 338325 [details]
Patch v2 (Does not take Youenn's comments into account)
Comment 18 Brent Fulgham 2018-04-19 09:05:05 PDT
Comment on attachment 338325 [details]
Patch v2 (Does not take Youenn's comments into account)

Uploading fix for EWS to confirm proper test output. Will refactor per Youenn's suggestions once I confirm the code is working properly.
Comment 19 EWS Watchlist 2018-04-19 10:10:09 PDT
Comment on attachment 338325 [details]
Patch v2 (Does not take Youenn's comments into account)

Attachment 338325 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7372091

New failing tests:
http/tests/websocket/tests/hybi/httponly-cookie.pl
http/tests/websocket/tests/hybi/secure-cookie-secure-connection.pl
Comment 20 EWS Watchlist 2018-04-19 10:10:10 PDT
Created attachment 338335 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 21 EWS Watchlist 2018-04-19 10:53:17 PDT
Comment on attachment 338325 [details]
Patch v2 (Does not take Youenn's comments into account)

Attachment 338325 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7372313

New failing tests:
http/tests/websocket/tests/hybi/httponly-cookie.pl
http/tests/websocket/tests/hybi/simple-wss.html
http/tests/websocket/tests/hybi/secure-cookie-secure-connection.pl
Comment 22 EWS Watchlist 2018-04-19 10:53:20 PDT
Created attachment 338341 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 23 EWS Watchlist 2018-04-19 10:59:44 PDT
Comment on attachment 338325 [details]
Patch v2 (Does not take Youenn's comments into account)

Attachment 338325 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7372223

New failing tests:
http/tests/websocket/tests/hybi/httponly-cookie.pl
http/tests/websocket/tests/hybi/secure-cookie-secure-connection.pl
Comment 24 EWS Watchlist 2018-04-19 10:59:45 PDT
Created attachment 338342 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 25 EWS Watchlist 2018-04-19 11:27:59 PDT
Comment on attachment 338325 [details]
Patch v2 (Does not take Youenn's comments into account)

Attachment 338325 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7373019

New failing tests:
http/tests/websocket/tests/hybi/secure-cookie-secure-connection.pl
http/tests/websocket/tests/hybi/simple-wss.html
http/tests/websocket/tests/hybi/httponly-cookie.pl
Comment 26 EWS Watchlist 2018-04-19 11:28:01 PDT
Created attachment 338346 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 27 Brent Fulgham 2018-04-19 17:49:23 PDT
Created attachment 338386 [details]
Patch (with Youenn's suggestions)
Comment 28 EWS Watchlist 2018-04-19 17:51:49 PDT
Attachment 338386 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/mac/CookieJarMac.mm:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Brent Fulgham 2018-04-19 17:56:39 PDT
Created attachment 338387 [details]
Patch v3 (with Youenn's comments)
Comment 30 EWS Watchlist 2018-04-19 17:58:49 PDT
Attachment 338387 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/mac/CookieJarMac.mm:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 youenn fablet 2018-04-19 21:52:16 PDT
Comment on attachment 338387 [details]
Patch v3 (with Youenn's comments)

Patch looks almost ready to me.
Some comments and suggestions below.

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

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:284
> +            m_document->setSecureCookiesAccessed();

Shouldn't we check for m_document being null?
Cannot we have something like socket is disconnecting and we receive just after the IPC call that we did the handshake?

> Source/WebCore/loader/CookieJar.cpp:81
> +    auto includeSecureCookies = (url.protocolIs("https") && !document.foundMixedContent().contains(SecurityContext::MixedContentType::Active)) ? IncludeSecureCookies::Yes : IncludeSecureCookies::No;

This routine is found in other places of the code base.
Maybe it would be better to have a utility function here, something like computeIncludeSecureCookies(document, url)?

> Source/WebCore/loader/CookieJar.cpp:84
> +    if (frame)

if (auto* frame = document.frame()) maybe

> Source/WebCore/loader/CookieJar.cpp:87
> +    return { storageSession(document).sessionID(), document.firstPartyForCookies().isolatedCopy(), url.isolatedCopy(), std::nullopt, std::nullopt, includeSecureCookies };

Why should they be isolatedCopy()?
Isn't this method always called from the main thread?

> Source/WebCore/platform/network/CookieRequestHeaderFieldProxy.h:53
> +    CookieRequestHeaderFieldProxy isolatedCopy() const

Do we need this one?
It seems that sockets on workers will not use it, right?

> Source/WebCore/platform/network/CookieRequestHeaderFieldProxy.h:79
> +    encoder.encodeEnum(m_includeSecureCookies);

Ideally we would use  encoder << m_includeSecureCookies with
template<> struct EnumTraits<WebCore::IncludeSecureCookies> {
    using values = EnumValues<
        WebCore::IncludeSecureCookies,
        WebCore::IncludeSecureCookies::No,
        WebCore::IncludeSecureCookies::Yes
    >;
};

> Source/WebCore/platform/network/SocketStreamHandle.h:57
> +    void sendHandshake(CString&& handshake, std::optional<CookieRequestHeaderFieldProxy>&&, Function<void(bool, bool)>);

Should be Function<>&& probably.
Also Function<void(bool, bool)> is not very easy to read.
Not sure whether it is worth adding some enums to improve readability.

> Source/WebCore/platform/network/SocketStreamHandleImpl.cpp:80
> +            data.shrink(data.size() - 2);

We could add an assertion that data is terminated by '\r\n'

> Source/WebCore/platform/network/SocketStreamHandleImpl.cpp:85
> +            builder.appendLiteral("\r\n\r\n");

We might not need a StringBuilder here, since we create a String for a very temporary time and we do utf8 conversion.
Can we try appending literal strings directly to data and utf8 convert cookieData only?
Or maybe directly to m_buffer?

> Source/WebKit/WebProcess/Network/WebSocketStream.cpp:109
> +void WebSocketStream::platformSendHandshake(const char* data, size_t length, const std::optional<CookieRequestHeaderFieldProxy>& headerFieldProxy, Function<void(bool, bool)>&& completionHandler)

It seems we should try to migrate this code from char to uint8_t.
Can we make it a const uint8_t*?

> Source/WebKit/WebProcess/Network/WebSocketStream.cpp:115
> +    m_sendHandshakeCallbacks.set(dataIdentifier, WTFMove(completionHandler));

s/add/set/
Comment 32 Brent Fulgham 2018-04-20 15:54:51 PDT
Comment on attachment 338387 [details]
Patch v3 (with Youenn's comments)

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

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:284
>> +            m_document->setSecureCookiesAccessed();
> 
> Shouldn't we check for m_document being null?
> Cannot we have something like socket is disconnecting and we receive just after the IPC call that we did the handshake?

Good point. If the document has gone away, there's no need to note that we accessed secure cookies, so we should just do nothing. I'll remove the assertion and do a null check.

>> Source/WebCore/loader/CookieJar.cpp:81
>> +    auto includeSecureCookies = (url.protocolIs("https") && !document.foundMixedContent().contains(SecurityContext::MixedContentType::Active)) ? IncludeSecureCookies::Yes : IncludeSecureCookies::No;
> 
> This routine is found in other places of the code base.
> Maybe it would be better to have a utility function here, something like computeIncludeSecureCookies(document, url)?

Sure. I prefer 'shouldIncludeSecureCookies(document, url)', but otherwise totally agree.

>> Source/WebCore/loader/CookieJar.cpp:84
>> +    if (frame)
> 
> if (auto* frame = document.frame()) maybe

OK!

>> Source/WebCore/loader/CookieJar.cpp:87
>> +    return { storageSession(document).sessionID(), document.firstPartyForCookies().isolatedCopy(), url.isolatedCopy(), std::nullopt, std::nullopt, includeSecureCookies };
> 
> Why should they be isolatedCopy()?
> Isn't this method always called from the main thread?

That's true. I'll fix that.

>> Source/WebCore/platform/network/CookieRequestHeaderFieldProxy.h:53
>> +    CookieRequestHeaderFieldProxy isolatedCopy() const
> 
> Do we need this one?
> It seems that sockets on workers will not use it, right?

Removed!

>> Source/WebCore/platform/network/CookieRequestHeaderFieldProxy.h:79
>> +    encoder.encodeEnum(m_includeSecureCookies);
> 
> Ideally we would use  encoder << m_includeSecureCookies with
> template<> struct EnumTraits<WebCore::IncludeSecureCookies> {
>     using values = EnumValues<
>         WebCore::IncludeSecureCookies,
>         WebCore::IncludeSecureCookies::No,
>         WebCore::IncludeSecureCookies::Yes
>     >;
> };

OK!

>> Source/WebCore/platform/network/SocketStreamHandle.h:57
>> +    void sendHandshake(CString&& handshake, std::optional<CookieRequestHeaderFieldProxy>&&, Function<void(bool, bool)>);
> 
> Should be Function<>&& probably.
> Also Function<void(bool, bool)> is not very easy to read.
> Not sure whether it is worth adding some enums to improve readability.

Should 'sendData' also be using Function<>&&? Both sites create a Function object on the stack as an argument to the 'send...' method, so that seems movable.

>> Source/WebCore/platform/network/SocketStreamHandleImpl.cpp:80
>> +            data.shrink(data.size() - 2);
> 
> We could add an assertion that data is terminated by '\r\n'

Good idea.

>> Source/WebCore/platform/network/SocketStreamHandleImpl.cpp:85
>> +            builder.appendLiteral("\r\n\r\n");
> 
> We might not need a StringBuilder here, since we create a String for a very temporary time and we do utf8 conversion.
> Can we try appending literal strings directly to data and utf8 convert cookieData only?
> Or maybe directly to m_buffer?

Yeah -- I wasn't happy with this section because of all the data copying. I'll revise it to be more efficient.

> Source/WebCore/platform/network/SocketStreamHandleImpl.cpp:103
> +        if (auto result = platformSendInternal(data.data(), data.size()))

Sadly, it looks like the entire handshake buffer needs to be sent in one shot through this method. Calling it twice (once with data, one with cookie data) causes the handshake to fail and the tests to hang.

>> Source/WebKit/WebProcess/Network/WebSocketStream.cpp:109
>> +void WebSocketStream::platformSendHandshake(const char* data, size_t length, const std::optional<CookieRequestHeaderFieldProxy>& headerFieldProxy, Function<void(bool, bool)>&& completionHandler)
> 
> It seems we should try to migrate this code from char to uint8_t.
> Can we make it a const uint8_t*?

Sure!

>> Source/WebKit/WebProcess/Network/WebSocketStream.cpp:115
>> +    m_sendHandshakeCallbacks.set(dataIdentifier, WTFMove(completionHandler));
> 
> s/add/set/

Done (for both places).
Comment 33 Brent Fulgham 2018-04-20 16:00:09 PDT
Created attachment 338477 [details]
Patch
Comment 34 EWS Watchlist 2018-04-20 16:02:16 PDT
Attachment 338477 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/mac/CookieJarMac.mm:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 35 youenn fablet 2018-04-20 16:08:10 PDT
Comment on attachment 338477 [details]
Patch

r=me as long as the bots are happy.

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

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:283
> +            if (m_document)

one if liner?

> Source/WebCore/platform/network/SocketStreamHandleImpl.cpp:94
> +    data.append('\n');

We could append a Vector of '\r\n\r\n' maybe
Comment 36 Brent Fulgham 2018-04-20 16:43:07 PDT
Created attachment 338488 [details]
Patch for landing
Comment 37 EWS Watchlist 2018-04-20 17:00:54 PDT
Attachment 338488 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/mac/CookieJarMac.mm:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 38 Brent Fulgham 2018-04-20 17:10:57 PDT
Created attachment 338498 [details]
Patch for landing
Comment 39 Brent Fulgham 2018-04-20 17:11:45 PDT
Comment on attachment 338477 [details]
Patch

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

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:283
>> +            if (m_document)
> 
> one if liner?

Doh!

>> Source/WebCore/platform/network/SocketStreamHandleImpl.cpp:94
>> +    data.append('\n');
> 
> We could append a Vector of '\r\n\r\n' maybe

I tried this but it didn't work, so I settled on the current code. I'm not sure if it's worth creating a static Vector of this.


data.append({ '\r', '\n', '\r', '\n' });

I'm bummed that didn't work. Maybe there is better syntax that would fix it...

> Source/WebCore/platform/network/SocketStreamHandleImpl.cpp:95
> +

I figured it out!
    data.appendVector(Vector<uint8_t>({ '\r', '\n', '\r', '\n' }));
Comment 40 WebKit Commit Bot 2018-04-20 18:51:43 PDT
Comment on attachment 338498 [details]
Patch for landing

Clearing flags on attachment: 338498

Committed r230875: <https://trac.webkit.org/changeset/230875>
Comment 41 WebKit Commit Bot 2018-04-20 18:51:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 42 Chris Dumez 2018-04-20 19:33:32 PDT
Broke the Windows build:
WebKitSystemInterface.lib(WebKitSystemInterface.obj) : warning LNK4099: PDB 'WebKitSystemInterface.pdb' was not found with 'WebKitSystemInterface.lib(WebKitSystemInterface.obj)' or at 'C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking object as if no debug info [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
WebKitSystemInterface.lib(MediaUI.obj) : warning LNK4099: PDB 'WebKitSystemInterface.pdb' was not found with 'WebKitSystemInterface.lib(MediaUI.obj)' or at 'C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking object as if no debug info [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
WebKitSystemInterface.lib(WebKitSystemInterfacePrefix.obj) : warning LNK4099: PDB 'WebKitSystemInterface.pdb' was not found with 'WebKitSystemInterface.lib(WebKitSystemInterfacePrefix.obj)' or at 'C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking object as if no debug info [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
WebKitSystemInterface.lib(SharedMediaUI.obj) : warning LNK4099: PDB 'WebKitSystemInterface.pdb' was not found with 'WebKitSystemInterface.lib(SharedMediaUI.obj)' or at 'C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking object as if no debug info [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
     Creating library C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/TestWebCoreLib.lib and object C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/TestWebCoreLib.exp
WebCore.lib(UnifiedSource348.obj) : error LNK2019: unresolved external symbol "struct std::pair<class WTF::String,bool> __cdecl WebCore::cookieRequestHeaderFieldValue(class WebCore::NetworkStorageSession const &,struct WebCore::CookieRequestHeaderFieldProxy const &)" (?cookieRequestHeaderFieldValue@WebCore@@YA?AU?$pair@VString@WTF@@_N@std@@ABVNetworkStorageSession@1@ABUCookieRequestHeaderFieldProxy@1@@Z) referenced in function "struct std::pair<class WTF::Vector<unsigned char,0,class WTF::CrashOnOverflow,16,struct WTF::FastMalloc>,bool> __cdecl WebCore::cookieDataForHandshake(struct WebCore::CookieRequestHeaderFieldProxy const &)" (?cookieDataForHandshake@WebCore@@YA?AU?$pair@V?$Vector@E$0A@VCrashOnOverflow@WTF@@$0BA@UFastMalloc@2@@WTF@@_N@std@@ABUCookieRequestHeaderFieldProxy@1@@Z) [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin32\TestWebCoreLib.dll : fatal error LNK1120: 1 unresolved externals [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
WebKitSystemInterface.lib(WebKitSystemInterface.obj) : warning LNK4099: PDB 'WebKitSystemInterface.pdb' was not found with 'WebKitSystemInterface.lib(WebKitSystemInterface.obj)' or at 'C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking object as if no debug info [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKitLegacy\WebKitLegacy.vcxproj]
WebKitSystemInterface.lib(MediaUI.obj) : warning LNK4099: PDB 'WebKitSystemInterface.pdb' was not found with 'WebKitSystemInterface.lib(MediaUI.obj)' or at 'C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking object as if no debug info [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKitLegacy\WebKitLegacy.vcxproj]
WebKitSystemInterface.lib(WebKitSystemInterfacePrefix.obj) : warning LNK4099: PDB 'WebKitSystemInterface.pdb' was not found with 'WebKitSystemInterface.lib(WebKitSystemInterfacePrefix.obj)' or at 'C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking object as if no debug info [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKitLegacy\WebKitLegacy.vcxproj]
WebKitSystemInterface.lib(SharedMediaUI.obj) : warning LNK4099: PDB 'WebKitSystemInterface.pdb' was not found with 'WebKitSystemInterface.lib(SharedMediaUI.obj)' or at 'C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking object as if no debug info [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKitLegacy\WebKitLegacy.vcxproj]
     Creating library C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/WebKit.lib and object C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/WebKit.exp
WebCore.lib(UnifiedSource348.obj) : error LNK2019: unresolved external symbol "struct std::pair<class WTF::String,bool> __cdecl WebCore::cookieRequestHeaderFieldValue(class WebCore::NetworkStorageSession const &,struct WebCore::CookieRequestHeaderFieldProxy const &)" (?cookieRequestHeaderFieldValue@WebCore@@YA?AU?$pair@VString@WTF@@_N@std@@ABVNetworkStorageSession@1@ABUCookieRequestHeaderFieldProxy@1@@Z) referenced in function "struct std::pair<class WTF::Vector<unsigned char,0,class WTF::CrashOnOverflow,16,struct WTF::FastMalloc>,bool> __cdecl WebCore::cookieDataForHandshake(struct WebCore::CookieRequestHeaderFieldProxy const &)" (?cookieDataForHandshake@WebCore@@YA?AU?$pair@V?$Vector@E$0A@VCrashOnOverflow@WTF@@$0BA@UFastMalloc@2@@WTF@@_N@std@@ABUCookieRequestHeaderFieldProxy@1@@Z) [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKitLegacy\WebKitLegacy.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin32\WebKit.dll : fatal error LNK1120: 1 unresolved externals [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKitLegacy\WebKitLegacy.vcxproj]
Comment 43 Chris Dumez 2018-04-20 19:42:10 PDT
(In reply to Chris Dumez from comment #42)
> Broke the Windows build:
> WebKitSystemInterface.lib(WebKitSystemInterface.obj) : warning LNK4099: PDB
> 'WebKitSystemInterface.pdb' was not found with
> 'WebKitSystemInterface.lib(WebKitSystemInterface.obj)' or at
> 'C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking
> object as if no debug info
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
> WebKitSystemInterface.lib(MediaUI.obj) : warning LNK4099: PDB
> 'WebKitSystemInterface.pdb' was not found with
> 'WebKitSystemInterface.lib(MediaUI.obj)' or at
> 'C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking
> object as if no debug info
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
> WebKitSystemInterface.lib(WebKitSystemInterfacePrefix.obj) : warning
> LNK4099: PDB 'WebKitSystemInterface.pdb' was not found with
> 'WebKitSystemInterface.lib(WebKitSystemInterfacePrefix.obj)' or at
> 'C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking
> object as if no debug info
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
> WebKitSystemInterface.lib(SharedMediaUI.obj) : warning LNK4099: PDB
> 'WebKitSystemInterface.pdb' was not found with
> 'WebKitSystemInterface.lib(SharedMediaUI.obj)' or at
> 'C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking
> object as if no debug info
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
>      Creating library
> C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/
> TestWebCoreLib.lib and object
> C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/
> TestWebCoreLib.exp
> WebCore.lib(UnifiedSource348.obj) : error LNK2019: unresolved external
> symbol "struct std::pair<class WTF::String,bool> __cdecl
> WebCore::cookieRequestHeaderFieldValue(class WebCore::NetworkStorageSession
> const &,struct WebCore::CookieRequestHeaderFieldProxy const &)"
> (?cookieRequestHeaderFieldValue@WebCore@@YA?AU?$pair@VString@WTF@@_N@std@@ABV
> NetworkStorageSession@1@ABUCookieRequestHeaderFieldProxy@1@@Z) referenced in
> function "struct std::pair<class WTF::Vector<unsigned char,0,class
> WTF::CrashOnOverflow,16,struct WTF::FastMalloc>,bool> __cdecl
> WebCore::cookieDataForHandshake(struct
> WebCore::CookieRequestHeaderFieldProxy const &)"
> (?cookieDataForHandshake@WebCore@@YA?AU?$pair@V?$Vector@E$0A@VCrashOnOverflow
> @WTF@@$0BA@UFastMalloc@2@@WTF@@_N@std@@ABUCookieRequestHeaderFieldProxy@1@@Z)
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
> C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\bin32\TestWebCoreLib.dll : fatal error
> LNK1120: 1 unresolved externals
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
> WebKitSystemInterface.lib(WebKitSystemInterface.obj) : warning LNK4099: PDB
> 'WebKitSystemInterface.pdb' was not found with
> 'WebKitSystemInterface.lib(WebKitSystemInterface.obj)' or at
> 'C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking
> object as if no debug info
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Source\WebKitLegacy\WebKitLegacy.vcxproj]
> WebKitSystemInterface.lib(MediaUI.obj) : warning LNK4099: PDB
> 'WebKitSystemInterface.pdb' was not found with
> 'WebKitSystemInterface.lib(MediaUI.obj)' or at
> 'C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking
> object as if no debug info
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Source\WebKitLegacy\WebKitLegacy.vcxproj]
> WebKitSystemInterface.lib(WebKitSystemInterfacePrefix.obj) : warning
> LNK4099: PDB 'WebKitSystemInterface.pdb' was not found with
> 'WebKitSystemInterface.lib(WebKitSystemInterfacePrefix.obj)' or at
> 'C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking
> object as if no debug info
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Source\WebKitLegacy\WebKitLegacy.vcxproj]
> WebKitSystemInterface.lib(SharedMediaUI.obj) : warning LNK4099: PDB
> 'WebKitSystemInterface.pdb' was not found with
> 'WebKitSystemInterface.lib(SharedMediaUI.obj)' or at
> 'C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\bin32\WebKitSystemInterface.pdb'; linking
> object as if no debug info
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Source\WebKitLegacy\WebKitLegacy.vcxproj]
>      Creating library
> C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/
> WebKit.lib and object
> C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/
> WebKit.exp
> WebCore.lib(UnifiedSource348.obj) : error LNK2019: unresolved external
> symbol "struct std::pair<class WTF::String,bool> __cdecl
> WebCore::cookieRequestHeaderFieldValue(class WebCore::NetworkStorageSession
> const &,struct WebCore::CookieRequestHeaderFieldProxy const &)"
> (?cookieRequestHeaderFieldValue@WebCore@@YA?AU?$pair@VString@WTF@@_N@std@@ABV
> NetworkStorageSession@1@ABUCookieRequestHeaderFieldProxy@1@@Z) referenced in
> function "struct std::pair<class WTF::Vector<unsigned char,0,class
> WTF::CrashOnOverflow,16,struct WTF::FastMalloc>,bool> __cdecl
> WebCore::cookieDataForHandshake(struct
> WebCore::CookieRequestHeaderFieldProxy const &)"
> (?cookieDataForHandshake@WebCore@@YA?AU?$pair@V?$Vector@E$0A@VCrashOnOverflow
> @WTF@@$0BA@UFastMalloc@2@@WTF@@_N@std@@ABUCookieRequestHeaderFieldProxy@1@@Z)
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Source\WebKitLegacy\WebKitLegacy.vcxproj]
> C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\bin32\WebKit.dll : fatal error LNK1120: 1
> unresolved externals
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Source\WebKitLegacy\WebKitLegacy.vcxproj]

cookieRequestHeaderFieldValue() probably needs an implementation in CookieJarCFNet.cpp
Comment 44 Chris Dumez 2018-04-20 19:44:42 PDT
Reopen to fix window build.
Comment 45 Chris Dumez 2018-04-20 19:45:56 PDT
Created attachment 338511 [details]
Speculative Windows build fix
Comment 46 Chris Dumez 2018-04-20 20:12:16 PDT
Created attachment 338513 [details]
Speculative Windows build fix
Comment 47 Chris Dumez 2018-04-20 21:22:49 PDT
Landed speculative Windows build fix in <https://trac.webkit.org/changeset/230880>.