Bug 184987 – [Cocoa] ResourceRequest constructors should retain 'isTopSite' (and other) state

• Component: WebCore Misc.
• Status: RESOLVED CONFIGURATION CHANGED

[Brent Fulgham]

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

[Daniel Bates]

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

[Daniel Bates]

Created attachment 338798 [details]
Patch

[youenn fablet]

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.

[Daniel Bates]

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

[Sam Weinig]

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"?

[Claudio Saavedra]

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.

[Alex Christensen]

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

[youenn fablet]

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.

[Claudio Saavedra]

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?

[youenn fablet]

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?

[Claudio Saavedra]

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

[Daniel Bates]

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.

[Daniel Bates]

I will fix the correctness issue in bug #194906.