Bug 176628 – [WK2] Add API to get the redirect chain of a WKDownload

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Add API to get the redirect chain of a download. This should include redirects that happened during initial load, before the load was converted into a download.

[Radar WebKit Bug Importer]

<rdar://problem/34338279>

[Chris Dumez]

Created attachment 320295 [details]
Patch

[Build Bot]

Attachment 320295 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebPageProxy.cpp:3195:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/_WKDownload.mm:64:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 320295 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/WKDownload.cpp:70
> +    Vector<RefPtr<API::Object>> urls;

You can do the urls.reserveIinitialCapacity(toImpl(download)->redirectChain().length()) + appendUnchecked() stuff here.

[Chris Dumez]

Created attachment 320309 [details]
Patch

[Build Bot]

Attachment 320309 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebPageProxy.cpp:3195:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/_WKDownload.mm:64:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 320309 [details]
Patch

I think this is a bad idea.  What ever happened to just informing the client about each redirect as they happen?

[Chris Dumez]

(In reply to Alex Christensen from comment #7)
> Comment on attachment 320309 [details]
> Patch
> 
> I think this is a bad idea.  What ever happened to just informing the client
> about each redirect as they happen?

I am pretty sure you are in cc of the mail where this was discussed. The navigation client only informs the client of top level navigations and redirects. The client is not notified at subframe level and this is an intentional design. Downloads can be triggered by navigating a subframe, for which the client is not notified and for which we do not create a WKNavigation object. Please read mail thread for more info.

[Geoffrey Garen]

Comment on attachment 320309 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Add API to get the redirect chain of a WKDwnload. The redirect chain includes redirects

WKDownload

[Geoffrey Garen]

> I think this is a bad idea.

Can you explain your rationale? What are some disadvantages of this approach?

[Geoffrey Garen]

Here is my rationale from our email discussion:

It’s a goal of the design of WKNavigationDelegate not to force clients to worry too much about individual sub frames and sub resources. Previous WebKit API was verbose about those things, adding API surface complexity and tight coupling without much benefit.

(The two policy decisions, decidePolicyForNavigationAction and decidePolicyForNavigationResponse, can fire for any frame. And didReceiveAuthenticationChallenge can fire for any resource. Those are exceptions to the general rule. Everything with a WKNavigation represents a full page transition.)

Considering this in detail, I think [a redirectChain property on WKDownload] might be best. It’s a goal of the WKNavigationDelegate API to hide or at least smooth over sub frame and sub resource details. The theory is that WebKit can usually handle these details internally and/or expose them in a more purpose-specific — and therefore clearer — way.

This case seems to fit the theory. We *could* expose lots of sub frame information to clients. That would give them the freedom to implement this feature and many other features we haven’t thought of yet. But if our goal is just redirect tracking on downloads, WebKit can handle that internally, and it’s nicer for clients if WebKit provides a purpose-built API on WKDownload.

It was a neat idea that our existing delegate messages could be enough to implement this feature. But if they’re not, they’re not. No biggie.

[Alex Christensen]

Comment on attachment 320309 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:3196
> +    redirectChain.append(URL(URL(), url));

We should make url a URL instead of a String.

> Source/WebKit/UIProcess/WebPageProxy.h:2008
> +    HashMap<uint64_t /* frameID */, Vector<WebCore::URL>> m_redirectChains;

This is just the redirect chains per frame navigation, right?  I think it could be named better.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm:521
> +    [TestProtocol registerWithScheme:@"http"];

This uses the WKBrowsingContextController which we are trying to remove.  Please write the test with WKURLSchemeHandler and a non-http scheme.

[Alex Christensen]

Comment on attachment 320309 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.h:2008
>> +    HashMap<uint64_t /* frameID */, Vector<WebCore::URL>> m_redirectChains;
> 
> This is just the redirect chains per frame navigation, right?  I think it could be named better.

I think this doesn't belong on the WebPageProxy.  I think it belongs on the navigation itself.

[Alex Christensen]

Comment on attachment 320309 [details]
Patch

We should also test a navigation of a subframe.

[Alex Christensen]

I have changed my mind and I now think we need to expose the redirect chain because of navigations of subframes becoming downloads that could've been redirected without those redirects having been exposed to the client, and we certainly don't want to expose those redirects just for this.  I think this patch needs better testing and organization, but then I'll approve of it.

[Chris Dumez]

(In reply to Alex Christensen from comment #13)
> Comment on attachment 320309 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320309&action=review
> 
> >> Source/WebKit/UIProcess/WebPageProxy.h:2008
> >> +    HashMap<uint64_t /* frameID */, Vector<WebCore::URL>> m_redirectChains;
> > 
> > This is just the redirect chains per frame navigation, right?  I think it could be named better.
> 
> I think this doesn't belong on the WebPageProxy.  I think it belongs on the
> navigation itself.

What navigation? My understanding is that navigations are top-frame only. My redirect chains are for all frames (including subframes).

[Chris Dumez]

(In reply to Chris Dumez from comment #16)
> (In reply to Alex Christensen from comment #13)
> > Comment on attachment 320309 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=320309&action=review
> > 
> > >> Source/WebKit/UIProcess/WebPageProxy.h:2008
> > >> +    HashMap<uint64_t /* frameID */, Vector<WebCore::URL>> m_redirectChains;
> > > 
> > > This is just the redirect chains per frame navigation, right?  I think it could be named better.
> > 
> > I think this doesn't belong on the WebPageProxy.  I think it belongs on the
> > navigation itself.
> 
> What navigation? My understanding is that navigations are top-frame only. My
> redirect chains are for all frames (including subframes).

Oh, I see you already commented on this. Never mind my comment then, still catching up!

[Chris Dumez]

(In reply to Alex Christensen from comment #12)
> Comment on attachment 320309 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320309&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:3196
> > +    redirectChain.append(URL(URL(), url));
> 
> We should make url a URL instead of a String.
> 
> > Source/WebKit/UIProcess/WebPageProxy.h:2008
> > +    HashMap<uint64_t /* frameID */, Vector<WebCore::URL>> m_redirectChains;
> 
> This is just the redirect chains per frame navigation, right?  I think it
> could be named better.
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm:521
> > +    [TestProtocol registerWithScheme:@"http"];
> 
> This uses the WKBrowsingContextController which we are trying to remove. 
> Please write the test with WKURLSchemeHandler and a non-http scheme.

Unfortunately, WKURLSchemeHandler does not seem to work with downloads.

[Chris Dumez]

Created attachment 321241 [details]
Patch

[Chris Dumez]

Constructing a Navigation object for subframe navigations seems to be fairly significant refactoring so I did not do it in this patch. Instead, I moved to redirect chain to WebFrame level.

[Build Bot]

Attachment 321241 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/API/Cocoa/_WKDownload.mm:64:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Alex, what do you think of this latest iteration?

[Alex Christensen]

Comment on attachment 321241 [details]
Patch

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

> Source/WebKit/UIProcess/Downloads/DownloadProxy.cpp:148
> +    m_processPool->downloadClient().willSendRequest(m_processPool.get(), this, proposedRequest, redirectResponse, [this, protectedThis](const ResourceRequest& newRequest) {

protectedThis should be a Ref and we should either WTFMove it here so we don't do unnecessary ref churn, or we should just create it in the lambda.

> Source/WebKit/UIProcess/Downloads/DownloadProxy.cpp:149
> +        m_redirectChain.append(newRequest.url());

Do we want the first URL to be in the redirect chain?

> Source/WebKit/UIProcess/WebFrameProxy.h:93
> +    const Vector<WebCore::URL>& redirectChain() const { return m_redirectChain; }

Vector<WebCore::URL>&& takeProvisionalLoadRedirectChain()

> Source/WebKit/UIProcess/WebFrameProxy.h:146
> +    Vector<WebCore::URL> m_redirectChain;

A frame isn't redirected.
I think a better name for this variable would be m_provisionalLoadRedirectChain.

> Source/WebKit/UIProcess/WebPageProxy.cpp:2281
> +        download->setRedirectChain(frame.redirectChain());

If it's not a download, won't we still need to clear the redirect chain?  Could you add a test that does more than one redirected load in the same frame?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm:482
> +    if (redirectChain.count > 0)

We already made sure count was 2.

[Chris Dumez]

Comment on attachment 321241 [details]
Patch

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

>> Source/WebKit/UIProcess/Downloads/DownloadProxy.cpp:148
>> +    m_processPool->downloadClient().willSendRequest(m_processPool.get(), this, proposedRequest, redirectResponse, [this, protectedThis](const ResourceRequest& newRequest) {
> 
> protectedThis should be a Ref and we should either WTFMove it here so we don't do unnecessary ref churn, or we should just create it in the lambda.

Will fix.

>> Source/WebKit/UIProcess/Downloads/DownloadProxy.cpp:149
>> +        m_redirectChain.append(newRequest.url());
> 
> Do we want the first URL to be in the redirect chain?

I personally do not think so. I feel there if there were no redirect then the redirect chain should be empty.

>> Source/WebKit/UIProcess/WebFrameProxy.h:93
>> +    const Vector<WebCore::URL>& redirectChain() const { return m_redirectChain; }
> 
> Vector<WebCore::URL>&& takeProvisionalLoadRedirectChain()

Ok.

>> Source/WebKit/UIProcess/WebFrameProxy.h:146
>> +    Vector<WebCore::URL> m_redirectChain;
> 
> A frame isn't redirected.
> I think a better name for this variable would be m_provisionalLoadRedirectChain.

Ok.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:2281
>> +        download->setRedirectChain(frame.redirectChain());
> 
> If it's not a download, won't we still need to clear the redirect chain?  Could you add a test that does more than one redirected load in the same frame?

Will check. There is an assertion protected against this in WebFrame so I would hit it.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm:482
>> +    if (redirectChain.count > 0)
> 
> We already made sure count was 2.

Yes, but the test does not stop if the previous check fails so the test ends up crashing instead of failing without this check.

[Geoffrey Garen]

> >> Source/WebKit/UIProcess/Downloads/DownloadProxy.cpp:149
> >> +        m_redirectChain.append(newRequest.url());
> > 
> > Do we want the first URL to be in the redirect chain?
> 
> I personally do not think so. I feel there if there were no redirect then
> the redirect chain should be empty.

If the load targets A, and A redirects to B, then the redirect chain in this implementation will be [ B ]. Is that right?

Mechanically, how is the client supposed to reconstruct the full [ A, B ] chain?

[Chris Dumez]

(In reply to Geoffrey Garen from comment #25)
> > >> Source/WebKit/UIProcess/Downloads/DownloadProxy.cpp:149
> > >> +        m_redirectChain.append(newRequest.url());
> > > 
> > > Do we want the first URL to be in the redirect chain?
> > 
> > I personally do not think so. I feel there if there were no redirect then
> > the redirect chain should be empty.
> 
> If the load targets A, and A redirects to B, then the redirect chain in this
> implementation will be [ B ]. Is that right?
> 
> Mechanically, how is the client supposed to reconstruct the full [ A, B ]
> chain?

You can always get A from the request's URL: _WKDownload.request.URL

[Chris Dumez]

(In reply to Chris Dumez from comment #26)
> (In reply to Geoffrey Garen from comment #25)
> > > >> Source/WebKit/UIProcess/Downloads/DownloadProxy.cpp:149
> > > >> +        m_redirectChain.append(newRequest.url());
> > > > 
> > > > Do we want the first URL to be in the redirect chain?
> > > 
> > > I personally do not think so. I feel there if there were no redirect then
> > > the redirect chain should be empty.
> > 
> > If the load targets A, and A redirects to B, then the redirect chain in this
> > implementation will be [ B ]. Is that right?
> > 
> > Mechanically, how is the client supposed to reconstruct the full [ A, B ]
> > chain?
> 
> You can always get A from the request's URL: _WKDownload.request.URL

Hmm, I have to check but this may not work in the case where is load is redirected before getting converted to a download. We may have to include A in the redirect chain.

[Chris Dumez]

Created attachment 321347 [details]
Patch

[Build Bot]

Attachment 321347 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/API/Cocoa/_WKDownload.mm:64:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 321347 [details]
Patch

Clearing flags on attachment: 321347

Committed r222308: <http://trac.webkit.org/changeset/222308>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Reopen to fix assertion being hit on debug wk2 bots.

[Chris Dumez]

Assertion temporarily disabled in r222320. Will upload a patch to re-enable it shortly.

[Chris Dumez]

Created attachment 321441 [details]
Follow-up

[WebKit Commit Bot]

Comment on attachment 321441 [details]
Follow-up

Rejecting attachment 321441 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 321441, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebKit/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/4616777

[Chris Dumez]

Created attachment 321446 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 321446 [details]
Patch

Clearing flags on attachment: 321446

Committed r222334: <http://trac.webkit.org/changeset/222334>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.