Bug 184987 - [Cocoa] ResourceRequest constructors should retain 'isTopSite' (and other) state
Summary: [Cocoa] ResourceRequest constructors should retain 'isTopSite' (and other) state
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-04-25 11:55 PDT by Brent Fulgham
Modified: 2019-02-21 10:15 PST (History)
9 users (show)

See Also:


Attachments
Patch (2.94 KB, patch)
2018-04-25 14:46 PDT, Daniel Bates
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-25 11:55:17 PDT
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").
Comment 1 Daniel Bates 2018-04-25 14:32:32 PDT
(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.
Comment 2 Daniel Bates 2018-04-25 14:46:05 PDT
Created attachment 338798 [details]
Patch
Comment 3 youenn fablet 2018-04-25 15:15:10 PDT
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?

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.
Comment 4 Daniel Bates 2018-04-25 15:29:51 PDT
(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?
Comment 5 Sam Weinig 2018-04-25 17:26:08 PDT
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"?
Comment 6 Claudio Saavedra 2018-04-26 07:56:49 PDT
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.
Comment 7 Alex Christensen 2018-04-26 10:28:11 PDT
(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.
Comment 8 youenn fablet 2018-04-26 10:49:08 PDT
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.
Comment 9 Claudio Saavedra 2018-05-15 04:59:21 PDT
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?
Comment 10 youenn fablet 2018-05-15 14:20:59 PDT
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?
Comment 11 Claudio Saavedra 2018-11-28 00:34:54 PST
(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.
Comment 12 Daniel Bates 2019-02-21 10:14:58 PST
Comment on attachment 338798 [details]
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.
Comment 13 Daniel Bates 2019-02-21 10:15:25 PST
I will fix the correctness issue in bug #194906.