215456 2020-08-13 09:30:50 -0700 Set Sec-WebSocket-Protocol for WebSocket NSURLSession code path 2020-08-17 05:21:21 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 youennf youennf achristensen jmehta webkit-bug-importer oldest_to_newest 1680005 0 youennf 2020-08-13 09:30:50 -0700 Set Sec-WebSocket-Protocol for WebSocket NSURLSession code path 1680006 1 webkit-bug-importer 2020-08-13 09:31:43 -0700 <rdar://problem/66996369> 1680309 2 406601 youennf 2020-08-14 10:10:13 -0700 Created attachment 406601 Patch 1680311 3 406601 achristensen 2020-08-14 10:12:06 -0700 Comment on attachment 406601 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406601&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1571 > + [requestWithProtocols addValue: StringView(protocol).createNSStringWithoutCopying().get() forHTTPHeaderField:@"Sec-WebSocket-Protocol"]; This seems unsafe. Why not just make a copy? 1680314 4 406601 achristensen 2020-08-14 10:25:22 -0700 Comment on attachment 406601 Patch This seems like a good workaround for the fact that there is webSocketTaskWithURL:protocols: but no webSocketTaskWithRequest:protocols:. Let's introduce that API and do this on operating system versions that don't have it. 1680320 5 youennf 2020-08-14 10:33:35 -0700 (In reply to Alex Christensen from comment #4) > Comment on attachment 406601 [details] > Patch > > This seems like a good workaround for the fact that there is > webSocketTaskWithURL:protocols: but no webSocketTaskWithRequest:protocols:. > Let's introduce that API and do this on operating system versions that don't > have it. Actually there is, let's use that. 1680321 6 achristensen 2020-08-14 10:34:18 -0700 There isn't on Catalina. 1680326 7 youennf 2020-08-14 10:35:39 -0700 (In reply to Alex Christensen from comment #6) > There isn't on Catalina. We might actually want to bump HAVE_NSURLSESSION_WEBSOCKET to Big Sur. 1680334 8 youennf 2020-08-14 10:43:14 -0700 (In reply to Alex Christensen from comment #6) > There isn't on Catalina. I looked too quickly, there is no such thing given we also want to set other headers like User-Agent. 1680344 9 406606 youennf 2020-08-14 10:49:07 -0700 Created attachment 406606 Patch 1680352 10 jmehta 2020-08-14 10:52:52 -0700 There is no variant of WebSocket task with request and protocols. The only variant with protocols is the one that takes a url. The client can add the protocol headers manually on the request which would achieve the same result. I think in WebKit's case we want to pass along the request since other information about main document etc will also be needed to lookup cookies etc, so moving to the url variant might not work. 1680355 11 youennf 2020-08-14 10:54:38 -0700 > I think in WebKit's case we want to pass along the request since other > information about main document etc will also be needed to lookup cookies > etc, so moving to the url variant might not work. Right, I do not think offering webSocketTaskWithRequest:protocols: provides much really. If you are using webSocketTaskWithRequest, you are already out of WebSocket standard API. 1680357 12 jmehta 2020-08-14 10:56:59 -0700 (In reply to youenn fablet from comment #11) > > I think in WebKit's case we want to pass along the request since other > > information about main document etc will also be needed to lookup cookies > > etc, so moving to the url variant might not work. > > Right, I do not think offering webSocketTaskWithRequest:protocols: provides > much really. > If you are using webSocketTaskWithRequest, you are already out of WebSocket > standard API. Yup! 1680364 13 406606 achristensen 2020-08-14 11:06:19 -0700 Comment on attachment 406606 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406606&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1569 > + auto *nsRequest = request.nsURLRequest(WebCore::HTTPBodyUpdatePolicy::DoNotUpdateHTTPBody); I think this returns an NSMutableURLRequest, so we could avoid a copy. Probably not too important here. > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1571 > + NSMutableURLRequest *requestWithProtocols = [[nsRequest mutableCopy] autorelease]; adoptNS could avoid putting a reference in the autorelease pool. 1680427 14 406606 achristensen 2020-08-14 14:04:37 -0700 Comment on attachment 406606 Patch ok 1680765 15 youennf 2020-08-17 05:17:00 -0700 > > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1569 > > + auto *nsRequest = request.nsURLRequest(WebCore::HTTPBodyUpdatePolicy::DoNotUpdateHTTPBody); > > I think this returns an NSMutableURLRequest, so we could avoid a copy. > Probably not too important here. It does not seem so from ResourceRequest::nsURLRequest. 1680766 16 ews-feeder 2020-08-17 05:21:20 -0700 Committed r265752: <https://trac.webkit.org/changeset/265752> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406606. 406601 2020-08-14 10:10:13 -0700 2020-08-14 10:49:03 -0700 Patch bug-215456-20200814191012.patch text/plain 2578 youennf U3VidmVyc2lvbiBSZXZpc2lvbjogMjY1NjY2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDVjOGM0N2Q4ZmQyZmFhNjQ0 ZGMyNmI2NjBhOTExZmU2YzNjNmQ0NWQuLjgzNDBhNTc3Y2RkMjg4NmJhNDc0YWQzMjA4NTMwZDI3 OWQwYzMwNzkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTkgQEAKKzIwMjAtMDgtMTQgIFlvdWVubiBG YWJsZXQgIDx5b3Vlbm5AYXBwbGUuY29tPgorCisgICAgICAgIFNldCBTZWMtV2ViU29ja2V0LVBy b3RvY29sIGZvciBXZWJTb2NrZXQgTlNVUkxTZXNzaW9uIGNvZGUgcGF0aAorICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjE1NDU2CisgICAgICAgIDxyZGFy Oi8vcHJvYmxlbS82Njk5NjM2OT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICBDb3ZlcmVkIGJ5IGV4aXN0aW5nIHByb3RvY29sIHRlc3RzIHdpdGggTlNV UkxTZXNzaW9uIGNvZGUgcGF0aCBlbmFibGVkLgorICAgICAgICBodHRwL3Rlc3RzL3dlYnNvY2tl dC90ZXN0cy9oeWJpL3NldC1wcm90b2NvbC5odG1sLCBodHRwL3Rlc3RzL3dlYnNvY2tldC90ZXN0 cy9oeWJpL3dvcmtlcnMvbXVsdGlwbGUtc3VicHJvdG9jb2xzLmh0bWwKKyAgICAgICAgQ29kZSBj aGFuZ2Ugb3JpZ2luYXRpbmcgZnJvbSBKaXRlbiBNZWh0YS4KKworICAgICAgICAqIE5ldHdvcmtQ cm9jZXNzL2NvY29hL05ldHdvcmtTZXNzaW9uQ29jb2EubW06CisgICAgICAgIChXZWJLaXQ6Ok5l dHdvcmtTZXNzaW9uQ29jb2E6OmNyZWF0ZVdlYlNvY2tldFRhc2spOgorICAgICAgICBTZXQgU2Vj LVdlYlNvY2tldC1Qcm90b2NvbCB0byBzdXBwb3J0IFdlYlNvY2tldCBwcm90b2NvbCBuZWdvdGlh dGlvbi4KKwogMjAyMC0wOC0xMyAgQ2FybG9zIEdhcmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxp YS5jb20+CiAKICAgICAgICAgW0dUSzRdIE5vdGlmeSB0aGUgd2ViIHByb2Nlc3Mgb24gZHJhZyBs ZWF2ZQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9OZXR3b3JrUHJvY2Vzcy9jb2NvYS9OZXR3 b3JrU2Vzc2lvbkNvY29hLm1tIGIvU291cmNlL1dlYktpdC9OZXR3b3JrUHJvY2Vzcy9jb2NvYS9O ZXR3b3JrU2Vzc2lvbkNvY29hLm1tCmluZGV4IDJjYWU2NzU5Y2MwNGMxNmQwZjMzMGI4ZGQ3MTNk ZTNiYzdkNjM3OGUuLjgwNzcyMjEwNTdiMjRkNGZhNDIzZDdkZTQxODc5ZjU2OTNmMDhmNzQgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3MvY29jb2EvTmV0d29ya1Nlc3Np b25Db2NvYS5tbQorKysgYi9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9jZXNzL2NvY29hL05ldHdv cmtTZXNzaW9uQ29jb2EubW0KQEAgLTE1NjUsOCArMTU2NSwxMyBAQCBETUZXZWJzaXRlUG9saWN5 TW9uaXRvciAqTmV0d29ya1Nlc3Npb25Db2NvYTo6ZGV2aWNlTWFuYWdlbWVudFBvbGljeU1vbml0 b3IoKQogI2lmIEhBVkUoTlNVUkxTRVNTSU9OX1dFQlNPQ0tFVCkKIHN0ZDo6dW5pcXVlX3B0cjxX ZWJTb2NrZXRUYXNrPiBOZXR3b3JrU2Vzc2lvbkNvY29hOjpjcmVhdGVXZWJTb2NrZXRUYXNrKE5l dHdvcmtTb2NrZXRDaGFubmVsJiBjaGFubmVsLCBjb25zdCBXZWJDb3JlOjpSZXNvdXJjZVJlcXVl c3QmIHJlcXVlc3QsIGNvbnN0IFN0cmluZyYgcHJvdG9jb2wpCiB7Ci0gICAgLy8gRklYTUU6IFVz ZSBwcm90b2NvbC4KLSAgICBSZXRhaW5QdHI8TlNVUkxTZXNzaW9uV2ViU29ja2V0VGFzaz4gdGFz ayA9IFttX3Nlc3Npb25XaXRoQ3JlZGVudGlhbFN0b3JhZ2Uuc2Vzc2lvbiB3ZWJTb2NrZXRUYXNr V2l0aFJlcXVlc3Q6cmVxdWVzdC5uc1VSTFJlcXVlc3QoV2ViQ29yZTo6SFRUUEJvZHlVcGRhdGVQ b2xpY3k6OkRvTm90VXBkYXRlSFRUUEJvZHkpXTsKKyAgICBhdXRvICpuc1JlcXVlc3QgPSByZXF1 ZXN0Lm5zVVJMUmVxdWVzdChXZWJDb3JlOjpIVFRQQm9keVVwZGF0ZVBvbGljeTo6RG9Ob3RVcGRh dGVIVFRQQm9keSk7CisgICAgaWYgKCFwcm90b2NvbC5pc051bGwoKSkgeworICAgICAgICBOU011 dGFibGVVUkxSZXF1ZXN0ICpyZXF1ZXN0V2l0aFByb3RvY29scyA9IFtbbnNSZXF1ZXN0IG11dGFi bGVDb3B5XSBhdXRvcmVsZWFzZV07CisgICAgICAgIFtyZXF1ZXN0V2l0aFByb3RvY29scyBhZGRW YWx1ZTogU3RyaW5nVmlldyhwcm90b2NvbCkuY3JlYXRlTlNTdHJpbmdXaXRob3V0Q29weWluZygp LmdldCgpIGZvckhUVFBIZWFkZXJGaWVsZDpAIlNlYy1XZWJTb2NrZXQtUHJvdG9jb2wiXTsKKyAg ICAgICAgbnNSZXF1ZXN0ID0gcmVxdWVzdFdpdGhQcm90b2NvbHM7CisgICAgfQorICAgIFJldGFp blB0cjxOU1VSTFNlc3Npb25XZWJTb2NrZXRUYXNrPiB0YXNrID0gW21fc2Vzc2lvbldpdGhDcmVk ZW50aWFsU3RvcmFnZS5zZXNzaW9uIHdlYlNvY2tldFRhc2tXaXRoUmVxdWVzdDpuc1JlcXVlc3Rd OwogICAgIHJldHVybiBtYWtlVW5pcXVlPFdlYlNvY2tldFRhc2s+KGNoYW5uZWwsIFdURk1vdmUo dGFzaykpOwogfQogCg== 406606 2020-08-14 10:49:07 -0700 2020-08-17 05:21:21 -0700 Patch bug-215456-20200814194905.patch text/plain 2653 youennf U3VidmVyc2lvbiBSZXZpc2lvbjogMjY1NjY2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDVjOGM0N2Q4ZmQyZmFhNjQ0 ZGMyNmI2NjBhOTExZmU2YzNjNmQ0NWQuLjgzNDBhNTc3Y2RkMjg4NmJhNDc0YWQzMjA4NTMwZDI3 OWQwYzMwNzkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTkgQEAKKzIwMjAtMDgtMTQgIFlvdWVubiBG YWJsZXQgIDx5b3Vlbm5AYXBwbGUuY29tPgorCisgICAgICAgIFNldCBTZWMtV2ViU29ja2V0LVBy b3RvY29sIGZvciBXZWJTb2NrZXQgTlNVUkxTZXNzaW9uIGNvZGUgcGF0aAorICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjE1NDU2CisgICAgICAgIDxyZGFy Oi8vcHJvYmxlbS82Njk5NjM2OT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICBDb3ZlcmVkIGJ5IGV4aXN0aW5nIHByb3RvY29sIHRlc3RzIHdpdGggTlNV UkxTZXNzaW9uIGNvZGUgcGF0aCBlbmFibGVkLgorICAgICAgICBodHRwL3Rlc3RzL3dlYnNvY2tl dC90ZXN0cy9oeWJpL3NldC1wcm90b2NvbC5odG1sLCBodHRwL3Rlc3RzL3dlYnNvY2tldC90ZXN0 cy9oeWJpL3dvcmtlcnMvbXVsdGlwbGUtc3VicHJvdG9jb2xzLmh0bWwKKyAgICAgICAgQ29kZSBj aGFuZ2Ugb3JpZ2luYXRpbmcgZnJvbSBKaXRlbiBNZWh0YS4KKworICAgICAgICAqIE5ldHdvcmtQ cm9jZXNzL2NvY29hL05ldHdvcmtTZXNzaW9uQ29jb2EubW06CisgICAgICAgIChXZWJLaXQ6Ok5l dHdvcmtTZXNzaW9uQ29jb2E6OmNyZWF0ZVdlYlNvY2tldFRhc2spOgorICAgICAgICBTZXQgU2Vj LVdlYlNvY2tldC1Qcm90b2NvbCB0byBzdXBwb3J0IFdlYlNvY2tldCBwcm90b2NvbCBuZWdvdGlh dGlvbi4KKwogMjAyMC0wOC0xMyAgQ2FybG9zIEdhcmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxp YS5jb20+CiAKICAgICAgICAgW0dUSzRdIE5vdGlmeSB0aGUgd2ViIHByb2Nlc3Mgb24gZHJhZyBs ZWF2ZQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9OZXR3b3JrUHJvY2Vzcy9jb2NvYS9OZXR3 b3JrU2Vzc2lvbkNvY29hLm1tIGIvU291cmNlL1dlYktpdC9OZXR3b3JrUHJvY2Vzcy9jb2NvYS9O ZXR3b3JrU2Vzc2lvbkNvY29hLm1tCmluZGV4IDJjYWU2NzU5Y2MwNGMxNmQwZjMzMGI4ZGQ3MTNk ZTNiYzdkNjM3OGUuLmZiZjFjNWVlNzk2OTRlZDFlMjY3OTllNTg5MzU1YTFiYmY2YmZiMzQgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3MvY29jb2EvTmV0d29ya1Nlc3Np b25Db2NvYS5tbQorKysgYi9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9jZXNzL2NvY29hL05ldHdv cmtTZXNzaW9uQ29jb2EubW0KQEAgLTE1NjUsOCArMTU2NSwxNCBAQCBETUZXZWJzaXRlUG9saWN5 TW9uaXRvciAqTmV0d29ya1Nlc3Npb25Db2NvYTo6ZGV2aWNlTWFuYWdlbWVudFBvbGljeU1vbml0 b3IoKQogI2lmIEhBVkUoTlNVUkxTRVNTSU9OX1dFQlNPQ0tFVCkKIHN0ZDo6dW5pcXVlX3B0cjxX ZWJTb2NrZXRUYXNrPiBOZXR3b3JrU2Vzc2lvbkNvY29hOjpjcmVhdGVXZWJTb2NrZXRUYXNrKE5l dHdvcmtTb2NrZXRDaGFubmVsJiBjaGFubmVsLCBjb25zdCBXZWJDb3JlOjpSZXNvdXJjZVJlcXVl c3QmIHJlcXVlc3QsIGNvbnN0IFN0cmluZyYgcHJvdG9jb2wpCiB7Ci0gICAgLy8gRklYTUU6IFVz ZSBwcm90b2NvbC4KLSAgICBSZXRhaW5QdHI8TlNVUkxTZXNzaW9uV2ViU29ja2V0VGFzaz4gdGFz ayA9IFttX3Nlc3Npb25XaXRoQ3JlZGVudGlhbFN0b3JhZ2Uuc2Vzc2lvbiB3ZWJTb2NrZXRUYXNr V2l0aFJlcXVlc3Q6cmVxdWVzdC5uc1VSTFJlcXVlc3QoV2ViQ29yZTo6SFRUUEJvZHlVcGRhdGVQ b2xpY3k6OkRvTm90VXBkYXRlSFRUUEJvZHkpXTsKKyAgICBBU1NFUlQoIXJlcXVlc3QuaGFzSFRU UEhlYWRlckZpZWxkKFdlYkNvcmU6OkhUVFBIZWFkZXJOYW1lOjpTZWNXZWJTb2NrZXRQcm90b2Nv bCkpOworICAgIGF1dG8gKm5zUmVxdWVzdCA9IHJlcXVlc3QubnNVUkxSZXF1ZXN0KFdlYkNvcmU6 OkhUVFBCb2R5VXBkYXRlUG9saWN5OjpEb05vdFVwZGF0ZUhUVFBCb2R5KTsKKyAgICBpZiAoIXBy b3RvY29sLmlzTnVsbCgpKSB7CisgICAgICAgIE5TTXV0YWJsZVVSTFJlcXVlc3QgKnJlcXVlc3RX aXRoUHJvdG9jb2xzID0gW1tuc1JlcXVlc3QgbXV0YWJsZUNvcHldIGF1dG9yZWxlYXNlXTsKKyAg ICAgICAgW3JlcXVlc3RXaXRoUHJvdG9jb2xzIGFkZFZhbHVlOiBTdHJpbmdWaWV3KHByb3RvY29s KS5jcmVhdGVOU1N0cmluZygpLmdldCgpIGZvckhUVFBIZWFkZXJGaWVsZDpAIlNlYy1XZWJTb2Nr ZXQtUHJvdG9jb2wiXTsKKyAgICAgICAgbnNSZXF1ZXN0ID0gcmVxdWVzdFdpdGhQcm90b2NvbHM7 CisgICAgfQorICAgIFJldGFpblB0cjxOU1VSTFNlc3Npb25XZWJTb2NrZXRUYXNrPiB0YXNrID0g W21fc2Vzc2lvbldpdGhDcmVkZW50aWFsU3RvcmFnZS5zZXNzaW9uIHdlYlNvY2tldFRhc2tXaXRo UmVxdWVzdDpuc1JlcXVlc3RdOwogICAgIHJldHVybiBtYWtlVW5pcXVlPFdlYlNvY2tldFRhc2s+ KGNoYW5uZWwsIFdURk1vdmUodGFzaykpOwogfQogCg==