184987 2018-04-25 11:55:17 -0700 [Cocoa] ResourceRequest constructors should retain 'isTopSite' (and other) state 2019-02-21 10:15:53 -0800 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED CONFIGURATION CHANGED https://bugs.webkit.org/show_bug.cgi?id=194906 P2 Normal --- 1 bfulgham dbates achristensen bfulgham cdumez csaavedra dbates ews-watchlist japhet sam youennf oldest_to_newest 1417663 0 bfulgham 2018-04-25 11:55:17 -0700 When we convert a ResourceRequest to a cocoa type (e.g., CFURLRequest/NSURLRequest) for Cocoa/CFNetwork API use, and then attempt to build new ResourceRequest objects (perhaps during redirect delegate calls from CFNetwork) using the constructors from CFURLRequest and NSURLRequest, we end up losing useful states (e.g., 'isTopSite()'). We should try to retain this state somewhere, perhaps by using properties on the CFNetwork objects (e.g., _propertyForKey:@"_kCFHTTPCookiePolicyPropertyIsTopLevelNavigation"). 1417778 1 dbates 2018-04-25 14:32:32 -0700 (In reply to Brent Fulgham from comment #0) > When we convert a ResourceRequest to a cocoa type (e.g., > CFURLRequest/NSURLRequest) for Cocoa/CFNetwork API use, and then attempt to > build new ResourceRequest objects (perhaps during redirect delegate calls > from CFNetwork) using the constructors from CFURLRequest and NSURLRequest, > we end up losing useful states (e.g., 'isTopSite()'). > As it turns out, we only set ResourceRequest::isTopSite() if the ResourceRequest::isSameSiteUnspecified(). So, the result you are seeing of ResourceRequest::isTopSite() returning false in -[WKNetworkSessionDelegate URLSession:task:willPerformHTTPRedirection:newRequest:completionHandler:] is because we conditionally set ResourceRequest::isTopSite(). Instead we should uncondtionally set ResourceRequest::isTopSite() whenever we are loading a main resource. 1417788 2 338798 dbates 2018-04-25 14:46:05 -0700 Created attachment 338798 Patch 1417807 3 338798 youennf 2018-04-25 15:15:10 -0700 Comment on attachment 338798 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338798&action=review > Source/WebCore/ChangeLog:12 > + commit. I am not sure ResourceRequest is the right place for this kind of information. Maybe this should be a loader option instead? As an example, we create new requests in the case of redirections. In that case, we would need to make sure isTopSite sticks to the new request, which I am not sure is guaranteed in all code paths. Having this information as a loader option might be more robust. 1417820 4 dbates 2018-04-25 15:29:51 -0700 (In reply to youenn fablet from comment #3) > Comment on attachment 338798 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338798&action=review > > > Source/WebCore/ChangeLog:12 > > + commit. > > I am not sure ResourceRequest is the right place for this kind of > information. > Maybe this should be a loader option instead? Do you feel it is necessary to address this in this patch? 1417904 5 sam 2018-04-25 17:26:08 -0700 Is "top site" as a term relatively new to WebKit? I don't remember seeing it before. I find it a bit confusing. "Site" is a term we don't traditionally use (outside of the term of art "site-specific quirks"). If what it is referring to is a main frame navigation, can we call it that, or match the CFNetwork terminology and it call it a "top level navigation"? 1418036 6 csaavedra 2018-04-26 07:56:49 -0700 For what it's worth, I was investigating whether I could use this for the libsoup network backend's implementation of HSTS (knowing whether the request is not for a toplevel frame would allow us to mitigate HSTS abuse) and came across this same bug, and the patch looks right to me. I suspect that "top site" makes sense in the context of same-site cookies, but as described above there are more cases in which this information is useful. 1418112 7 achristensen 2018-04-26 10:28:11 -0700 (In reply to Sam Weinig from comment #5) > Is "top site" as a term relatively new to WebKit? I don't remember seeing it > before. I find it a bit confusing. "Site" is a term we don't traditionally > use (outside of the term of art "site-specific quirks"). > > If what it is referring to is a main frame navigation, can we call it that, > or match the CFNetwork terminology and it call it a "top level navigation"? I agree. When I see "top site" I think it must be a very popular website. 1418122 8 youennf 2018-04-26 10:49:08 -0700 This parameter is very close to FetchOptions::Mode. As it is immutable, I would consider moving it to a loader option, plus this would be platform-agnostic. 1423997 9 csaavedra 2018-05-15 04:59:21 -0700 Regardless of the criticism that you might have for the method's name, the code is not working reliably and the discussion is stalled. Can we please fix it at at least so that it's not inconsistent? 1424124 10 youennf 2018-05-15 14:20:59 -0700 In NetworkProcess side, there is NetworkResourceLoadParameters.options.destination and NetworkLoadParameters.isMainFrameNavigation. That tells whether the load is for a document and whether this is for the main frame. Claudio, would that be sufficient for your use case? 1482380 11 csaavedra 2018-11-28 00:34:54 -0800 (In reply to youenn fablet from comment #10) > In NetworkProcess side, there is > NetworkResourceLoadParameters.options.destination and > NetworkLoadParameters.isMainFrameNavigation. > That tells whether the load is for a document and whether this is for the > main frame. > Claudio, would that be sufficient for your use case? It might, I have to check it. I will during these days, now that I've retaken the HSTS work. However, this bug stands valid. You have internal API that is not working consistently. We either remove it or fix it. Having it broken because of a posteriori disagreement is not a good idea. 1508562 12 338798 dbates 2019-02-21 10:14:58 -0800 Comment on attachment 338798 Patch Declaring bankruptcy on this bug. Fix is correct and Claudio Saavedra is the only person that could figure this out. I suspect the confusion arose because I thought I could kill two birds with one stone with this bug: 1) fix the correctness issue myself and Claudio noticed and 2) provide an enhancement for Brent to use (see the request in comment #0). It would later turn out that Brent would not need this enhancement as he forged his own equivalent, NetworkLoadParameters.isMainFrameNavigation (as pointed out in comment #10). Untangling this mess by reverting the name of the bug back to its original name and closing it. 1508563 13 dbates 2019-02-21 10:15:25 -0800 I will fix the correctness issue in bug #194906. 338798 2018-04-25 14:46:05 -0700 2019-02-21 10:14:58 -0800 Patch bug-184987-20180425144604.patch text/plain 3013 dbates U3VidmVyc2lvbiBSZXZpc2lvbjogMjMwOTc3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOGQ3NTczNzczMTJkNzI3 MDE3ZWQwNDZmODViYTliN2YyZWViOGZiMC4uNTUwMjk4YjdjY2YxOWY2MWFhM2QyYmUzZTg2Zjk2 Y2M0YmU1N2I4YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDE4LTA0LTI1ICBEYW5p ZWwgQmF0ZXMgIDxkYWJhdGVzQGFwcGxlLmNvbT4KKworICAgICAgICBVbmNvbmRpdGlvbmFsbHkg c2V0IFJlc291cmNlUmVxdWVzdDo6aXNUb3BTaXRlKCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE4NDk4NworCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgIEFzIGl0IHR1cm5zIG91dCBSZXNvdXJjZVJlcXVlc3Q6 OmlzVG9wU2l0ZSgpIGlzIHVzZWZ1bCBvdXRzaWRlIG9mIFNhbWUtU2l0ZSBjb29raWVzCisgICAg ICAgIHRvIGtub3cgaWYgYSByZXF1ZXN0IGlzIGEgdG9wLWxldmVsIG5hdmlnYXRpb24uIFdlIHNo b3VsZCBzdXBwb3J0IHRyYWNraW5nIHRoaXMgc3RhdGUKKyAgICAgICAgaW5kZXBlbmRlbnQgb2Yg dGhlIHN0YXRlIG9mIHRoZSByZXF1ZXN0J3MgInNhbWUtc2l0ZSItbmVzcy4gV2UgYXJlIG5vdCBj dXJyZW50bHkgdXNpbmcKKyAgICAgICAgUmVzb3VyY2VSZXF1ZXN0Ojppc1RvcFNpdGUoKSBvdXRz aWRlIG9mIHRoZSBTYW1lLVNpdGUgY29va2llIGNvZGUuIFdlIHdpbGwgaW4gYSBzdWJzZXF1ZW50 CisgICAgICAgIGNvbW1pdC4KKworICAgICAgICAqIGxvYWRlci9GcmFtZUxvYWRlci5jcHA6Cisg ICAgICAgIChXZWJDb3JlOjpGcmFtZUxvYWRlcjo6YWRkRXh0cmFGaWVsZHNUb1JlcXVlc3QpOiBV bmNvbmRpdGlvbmFsbHkgc2V0IHRoZSByZXF1ZXN0J3MgdG9wLQorICAgICAgICBzaXRlIHN0YXRl LgorICAgICAgICAqIHBsYXRmb3JtL25ldHdvcmsvUmVzb3VyY2VSZXF1ZXN0QmFzZS5jcHA6Cisg ICAgICAgIChXZWJDb3JlOjpSZXNvdXJjZVJlcXVlc3RCYXNlOjpzZXRBc0lzb2xhdGVkQ29weSk6 IFVuY29uZGl0aW9uYWxseSBjb3B5IHRoZSByZXF1ZXN0J3MKKyAgICAgICAgdG9wLXNpdGUgc3Rh dGUuCisKIDIwMTgtMDQtMjQgIFNpbW9uIEZyYXNlciAgPHNpbW9uLmZyYXNlckBhcHBsZS5jb20+ CiAKICAgICAgICAgc2hhcGUtb3V0c2lkZSBhbmQgZmlsdGVyIHN0eWxlcyBvY2N1ciB0d2ljZSBp biB0aGUgcmVzdWx0IG9mIGdldENvbXB1dGVkU3R5bGUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJD b3JlL2xvYWRlci9GcmFtZUxvYWRlci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvRnJhbWVM b2FkZXIuY3BwCmluZGV4IDk3Y2YwYjY4ZTExNGYxN2E1NmYwNDFhYTdhYjA4NGYzNTAwMmYwZDUu LjcyM2EzNTJhZjdlNWJjZTdjOTkzZmJjYmEzOTA5MjM2YTcyMDEyMzYgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJDb3JlL2xvYWRlci9GcmFtZUxvYWRlci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUv bG9hZGVyL0ZyYW1lTG9hZGVyLmNwcApAQCAtMjcwNyw4ICsyNzA3LDggQEAgdm9pZCBGcmFtZUxv YWRlcjo6YWRkRXh0cmFGaWVsZHNUb1JlcXVlc3QoUmVzb3VyY2VSZXF1ZXN0JiByZXF1ZXN0LCBG cmFtZUxvYWRUeXAKICAgICAgICAgICAgIEFTU0VSVChvd25lckZyYW1lIHx8IG1fZnJhbWUuaXNN YWluRnJhbWUoKSk7CiAgICAgICAgIH0KICAgICAgICAgYWRkU2FtZVNpdGVJbmZvVG9SZXF1ZXN0 SWZOZWVkZWQocmVxdWVzdCwgaW5pdGlhdG9yKTsKLSAgICAgICAgcmVxdWVzdC5zZXRJc1RvcFNp dGUoaXNNYWluUmVzb3VyY2UgJiYgbV9mcmFtZS5pc01haW5GcmFtZSgpKTsKICAgICB9CisgICAg cmVxdWVzdC5zZXRJc1RvcFNpdGUoaXNNYWluUmVzb3VyY2UgJiYgbV9mcmFtZS5pc01haW5GcmFt ZSgpKTsKIAogICAgIFBhZ2UqIHBhZ2UgPSBmcmFtZSgpLnBhZ2UoKTsKICAgICBib29sIGhhc1Nw ZWNpZmljQ2FjaGVQb2xpY3kgPSByZXF1ZXN0LmNhY2hlUG9saWN5KCkgIT0gVXNlUHJvdG9jb2xD YWNoZVBvbGljeTsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsv UmVzb3VyY2VSZXF1ZXN0QmFzZS5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3Jr L1Jlc291cmNlUmVxdWVzdEJhc2UuY3BwCmluZGV4IDJiNjUwY2JiNGVlZjFlZWZjNGE0ZDliZjE2 NWU1MmJkNzJjMWY2YTkuLjRmYmI1NzQyZGNmNTJjOWM1OWQ5YmY0YzEzYjY1NmY0NWEwZTU2NjAg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvUmVzb3VyY2VSZXF1 ZXN0QmFzZS5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9SZXNvdXJj ZVJlcXVlc3RCYXNlLmNwcApAQCAtNjYsMTAgKzY2LDkgQEAgdm9pZCBSZXNvdXJjZVJlcXVlc3RC YXNlOjpzZXRBc0lzb2xhdGVkQ29weShjb25zdCBSZXNvdXJjZVJlcXVlc3QmIG90aGVyKQogICAg IHNldEluaXRpYXRvcklkZW50aWZpZXIob3RoZXIuaW5pdGlhdG9ySWRlbnRpZmllcigpLmlzb2xh dGVkQ29weSgpKTsKICAgICBzZXRDYWNoZVBhcnRpdGlvbihvdGhlci5jYWNoZVBhcnRpdGlvbigp Lmlzb2xhdGVkQ29weSgpKTsKIAotICAgIGlmICghb3RoZXIuaXNTYW1lU2l0ZVVuc3BlY2lmaWVk KCkpIHsKKyAgICBpZiAoIW90aGVyLmlzU2FtZVNpdGVVbnNwZWNpZmllZCgpKQogICAgICAgICBz ZXRJc1NhbWVTaXRlKG90aGVyLmlzU2FtZVNpdGUoKSk7Ci0gICAgICAgIHNldElzVG9wU2l0ZShv dGhlci5pc1RvcFNpdGUoKSk7Ci0gICAgfQorICAgIHNldElzVG9wU2l0ZShvdGhlci5pc1RvcFNp dGUoKSk7CiAKICAgICB1cGRhdGVSZXNvdXJjZVJlcXVlc3QoKTsKICAgICBtX2h0dHBIZWFkZXJG aWVsZHMgPSBvdGhlci5odHRwSGVhZGVyRmllbGRzKCkuaXNvbGF0ZWRDb3B5KCk7Cg==