Bug 137302 – location.href is not updated on a redirect with a custom protocol

• Component: WebKit API
• Status: NEW

[Daniel]

If there's a custom NSURLProtocol, which is redirecting some page, the web view URL (as reported by javascript location.href) is not updated.
If page p1 redirects to p2, then the final location.href will be p1 (while expected is p2).
This happens only if the protocol is registered using [WKBrowsingContextController registerSchemeForCustomProtocol:], because otherwise the protocol code is bypassed.

Happens on iOS 8.1 [12B401].
I'm providing a test case code for the master branch at 809db5f8edac632ec3332b43dc90795d96914440.

Test case:
https://gist.github.com/anonymous/7fd3352c9f51eea9c4ce
Run it with ./TestWebKitAPI --gtest_filter=WebKit2CustomProtocolsTest.RedirectChangesURL

Manual test case on iOS:
1. go to http://m.wikipedia.org/
2. click a link
Expected results: link loads a page like http://en.m.wikipedia.org/wiki/Deaths_in_2014
Actual results: 404 error, because the link host is "m.wikipedia.org", and was not updated to en.m.wikipedia.org

[Daniel]

Created attachment 239025 [details]
Automatic test case code

[Daniel]

If I understand it right the reason for this is that WKCustomProtocolLoader doesn't forward a redirect event back to the web process. When a redirect happens, "connection:willSendRequest:redirectResponse:" method is called, which should send a message about the redirect to the web process so that it propagates to an URL change.

See:
https://github.com/WebKit/webkit/blob/master/Source/WebKit2/UIProcess/Network/CustomProtocols/mac/CustomProtocolManagerProxyMac.mm#L112

I'm going to make a patch for this.

[Daniel]

Created attachment 239529 [details]
Patch

[Darin Adler]

Comment on attachment 239529 [details]
Patch

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

> Source/WebCore/platform/network/CredentialStorage.cpp:128
>      ASSERT(url.isValid());

Why is it OK to assert this? Can’t the redirect be an invalid URL as well as a valid non-HTTP URL?

[Daniel]

Darin, thanks for looking at the patch.
You were right, I've tried to do a test redirect to an URL like "http://[::1", i.e. an URL where a host has an invalid IPv6 format, and it triggered this second ASSERT.
Uploading a fix.

[Daniel]

Created attachment 239608 [details]
Patch

[Benjamin Poulain]

Comment on attachment 239608 [details]
Patch

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

Round1: Quick review, mostly style.

> Source/WebCore/ChangeLog:3
> +        Allow non-HTTP or invalid URLs to be passed to findDefaultProtectionSpaceForURL

You should keep the title in sync with the bug report on bugzilla.

This line of explanation should go under the "Reviewed by" line.

> Source/WebCore/ChangeLog:12
> +        No new tests, no behavior change.

You can only use this excuse for tests when you believe there is no observable difference after your change.

In this case, we would crash in Debug due to the assertion so it is observable and should have a test.

> Source/WebKit2/ChangeLog:3
> +        Add CustomProtocolManager::DidRedirect message

Keep the title "location.href is not updated on a redirect with a custom protocol" and have the explanation below.

> Source/WebKit2/ChangeLog:9
> +        The message is received after an HTTP redirect,
> +        and is forwarded to the NSURLProtocol client.

Here you should have an explanation of the bug, why it happens, and how you solved it.

> Source/WebKit2/UIProcess/Network/CustomProtocols/mac/CustomProtocolManagerProxyMac.mm:112
> +    // not a redirect, just continue

Comments should be complete sentences in WebKit, starting with uppercase for the first letter, and ending with a period.

> Tools/TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm:60
> +    NSMutableURLRequest *request = [self.request mutableCopy];
> +    RetainPtr<NSMutableURLRequest> requestPtr = adoptNS(request);

Let's merge those two lines.

> Tools/TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm:64
> +    NSURLResponse *response = [[NSHTTPURLResponse alloc] initWithURL:targetURL
> +        statusCode:302 HTTPVersion:@"HTTP/1.0" headerFields:@{ @"Location" : targetURL.absoluteString }];

Objective-C multiline alignment is a bit weird. You should split this with one line per argument and get XCode to align it for you.

[Daniel]

Created attachment 240478 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm:65:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 9 files


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

[Daniel]

> You can only use this excuse for tests when you believe there is no observable difference after your change.
> In this case, we would crash in Debug due to the assertion so it is observable and should have a test.

I couldn't find a place in code where I could put a simple unit test for CredentialStorage::findDefaultProtectionSpaceForURL, so I've just changed the log message.

The rest remarks are fixed.

> Weird number of spaces at line-start
What does it mean?
I did Xcode style indentation there that you've asked there.

[Daniel]

Created attachment 240670 [details]
Patch

[Daniel]

Fixed compiling on Linux.

[WebKit Commit Bot]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm:65:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 10 files


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

[Benjamin Poulain]

Comment on attachment 240670 [details]
Patch

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

> > Weird number of spaces at line-start
> What does it mean?
> I did Xcode style indentation there that you've asked there.

Don't worry, the style-checker does not know about Objective-C indentation. A code style "error" is a hint for reviewers.

> Source/WebCore/ChangeLog:13
> +        No new tests, just an ASSERT removal.

(In reply to comment #10)
> > You can only use this excuse for tests when you believe there is no observable difference after your change.
> > In this case, we would crash in Debug due to the assertion so it is observable and should have a test.
> 
> I couldn't find a place in code where I could put a simple unit test for
> CredentialStorage::findDefaultProtectionSpaceForURL, so I've just changed
> the log message.

Honestly I don't understand why you remove the assertion if you cannot find a case that hits it...

> Source/WebKit2/ChangeLog:10
> +        This was happening, because WKCustomProtocolLoader didn't forward

Extra coma "happening, because" -> happening because

> Source/WebKit2/UIProcess/Network/CustomProtocols/mac/CustomProtocolManagerProxyMac.mm:113
> +    if (redirectResponse == nil)

WebKit style is:
    if (!redirectResponse)

> Source/WebKit2/UIProcess/Network/CustomProtocols/mac/CustomProtocolManagerProxyMac.mm:121
> +    // Abandon the current connection, a new protocol connection for "request" will be initiated later.
> +    _customProtocolManagerProxy->stopLoading(_customProtocolID);

It is not clear to me why this is correct. NSURLConnection never calls connectionDidFinishLoading: for redirection? This interface should behave the same.

[Daniel]

Created attachment 240730 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm:65:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 10 files


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

[Daniel]

I've made the minor updates you've asked, and added some comments.

> Honestly I don't understand why you remove the assertion if you cannot find a case that hits it...

Of course, my new TC from TestWebKitAPI is affected. I've updated the comment to reflect that. I just thought that a change in WebCore must have a TC in WebCore.

> It is not clear to me why this is correct. NSURLConnection never calls connectionDidFinishLoading: for redirection? This interface should behave the same.

It's not the same. Although NSURLConnection stays on the same instance for doing redirects, it creates a new NSURLProtocol instance under the hood for each redirect. Each redirection gets a new NSURLProtocol in the web process, and makes a new startLoading call to the UI process. In respond to this in the UI process it creates a new instance of WKCustomProtocolLoader, and a new NSURLConnection. So the old NSURLConnection is not reused, which means that after a redirect it needs to be released. This is the purpose of this line.

Keep in mind that we are working with 2 NSURLConnection instances here. One is in the web process (somewhere in the networking code), and that's kept and reused. Another one is in the UI process (a part of WKCustomProtocolLoader), which is released after a redirect and not reused.

[Michael Catanzaro]

Comment on attachment 240730 [details]
Patch

Hi,

Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.

To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.