Add SPI to WKURLSchemeTask for redirection
Created attachment 313649 [details] Patch
Comment on attachment 313649 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313649&action=review > Source/WebKit2/ChangeLog:17 > + Right now just responding with an HTTP 301/302/307/308 response code > + doesn't work because there is nothing that synthesizes an NSURLRequest > + from the Location header, and that would require using an NSHTTPURLResponse > + for non-HTTP responses, which is conceptually wrong. Also, adding such NSURLResponse > + synthesizing to WebURLSchemeTaskProxy::didReceiveResponse like we do in > + SynchronousResourceHandleCFURLConnectionDelegate::willSendRequest et al. for HSTS > + would not work because we wouldn't properly do anything with the completion handler > + of ResourceLoader::willSendRequest. I disagree about the conceptual wrongness. Everything else here lists problems that can be solved with bug fixing in current code, without expanding the API/SPI surface. "for HSTS would not work because we wouldn't properly do anything with the completion handler of ResourceLoader::willSendRequest." I don't think there's actually a technical reason preventing this from working.
(In reply to Brady Eidson from comment #2) > I disagree about the conceptual wrongness. Let me rephrase. The "conceptual wrongness" is by no means a deal breaker for the approach of making this work within the footprint of the existing API. Plus, the existing API needs to work *for HTTP responses* if a URLSchemeHandler is being handled by performing an HTTP load in the UI process. So fixing the bugs makes something we intend to support work, allows the test that relies on redirects to work, and doesn't expand the API surface. win-win-win.
I assert that there is a technical problem of nothing to do with the completion handler if we synthesize a request from an HTTP response. We should not do that, as it would cause conceptual problems with future loading plans, such as implementing WebCoreNSURLSession's receivedRedirect. If we just fed it a series of responses and synthesized requests and did nothing with the completion handler, it would all be wrong. I think we are supporting NSHTTPURLResponses, but if they have a redirect status code they should go through this new SPI, *not* through didReceiveResponse.
If someone is feeding HTTP responses into WKURLSchemeTask, they will get their 302 responses through their NSURLSessionDataDelegate's willPerformHTTPRedirection which requires an NSURLRequest in the completion handler, not through didReceiveResponse, which does not.
We had an in-person, pow-wow and came up with a great modification to this approach that Alex will be working on shortly.
Created attachment 313669 [details] Patch
Comment on attachment 313669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313669&action=review > Source/WebKit2/UIProcess/WebURLSchemeTask.cpp:65 > + m_request = request; This is a little weird - Kinda wish we called the property "initialRequest" in API. Before we upgrade this SPI to API we'll need to think it through carefully - I'd wager that some code written with .request expects it to never change. Maybe don't change m_request but instead add a new property "currentRequest" to keep updated? > Source/WebKit2/UIProcess/WebURLSchemeTask.cpp:89 > -WebURLSchemeTask::ExceptionType WebURLSchemeTask::didReceiveData(Ref<SharedBuffer> buffer) > +auto WebURLSchemeTask::didReceiveData(Ref<SharedBuffer> buffer) -> ExceptionType Are these really necessary? We can use auto return types, but are they *preferred*...? > Source/WebKit2/UIProcess/API/Cocoa/WKURLSchemeTask.mm:54 > - [NSException raise:NSInternalInconsistencyException format:@"This task has already been stopped"]; > + [NSException raise:NSInternalInconsistencyException format:@"This task has already been stopped."]; > break; > case WebKit::WebURLSchemeTask::ExceptionType::CompleteAlreadyCalled: > - [NSException raise:NSInternalInconsistencyException format:@"[WKURLSchemeTask taskDidCompleteWithError:] has already been called for this task"]; > + [NSException raise:NSInternalInconsistencyException format:@"[WKURLSchemeTask taskDidCompleteWithError:] has already been called for this task."]; > break; > case WebKit::WebURLSchemeTask::ExceptionType::DataAlreadySent: > - [NSException raise:NSInternalInconsistencyException format:@"[WKURLSchemeTask taskDidReceiveData:] has already been called for this task"]; > + [NSException raise:NSInternalInconsistencyException format:@"[WKURLSchemeTask taskDidReceiveData:] has already been called for this task."]; > break; > case WebKit::WebURLSchemeTask::ExceptionType::NoResponseSent: > - [NSException raise:NSInternalInconsistencyException format:@"No response has been sent for this task"]; > + [NSException raise:NSInternalInconsistencyException format:@"No response has been sent for this task."]; I personally don't like the trailing period style. Analyzing with actual data: We currently raise 68 Objective-C exceptions in API. Exactly 3 of those have a period at the end of the message. I don't think we should change these - They're fine without the period. > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKURLSchemeHandler-1.mm:263 > + // FIXME: Add tests that exercise all the invalid state exceptions a WKURLSchemeHandler can raise. Plz to actually do this.
(In reply to Brady Eidson from comment #8) > Before we upgrade this SPI to API we'll need to think it through carefully - > I'd wager that some code written with .request expects it to never change. Definitely. If you redirect, you have changed the request. If you adopt the new functions, your code changes functionality. We should consider adding initialRequest. > > Source/WebKit2/UIProcess/WebURLSchemeTask.cpp:89 > > -WebURLSchemeTask::ExceptionType WebURLSchemeTask::didReceiveData(Ref<SharedBuffer> buffer) > > +auto WebURLSchemeTask::didReceiveData(Ref<SharedBuffer> buffer) -> ExceptionType > > Are these really necessary? We can use auto return types, but are they > *preferred*...? Trailing return types are in the name scope of the function. It removes a bunch of unnecessary WebURLSchemeTask::. I like it here. > I personally don't like the trailing period style. I thought it was cool. I'll change it back.
Created attachment 313678 [details] Patch
(In reply to Alex Christensen from comment #9) > (In reply to Brady Eidson from comment #8) > > Before we upgrade this SPI to API we'll need to think it through carefully - > > I'd wager that some code written with .request expects it to never change. > Definitely. If you redirect, you have changed the request. If you adopt > the new functions, your code changes functionality. We should consider > adding initialRequest. I think if this became API, and we added a non-changing initialRequest, then "request" changing as you redirected would be fine (as pointed out, only users of the new API would notice the change in behavior.) So I'm no longer held up on this. The red bots, though...
Created attachment 313692 [details] Patch
http://trac.webkit.org/r218750
Comment on attachment 313692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313692&action=review > Source/WebKit2/UIProcess/WebURLSchemeTask.cpp:66 > + m_page->send(Messages::WebPage::URLSchemeTaskDidPerformRedirection(m_urlSchemeHandler->identifier(), m_identifier, response, request)); We are not taking advantage of the fact that the arguments are rvalue references here; no WTFMove on response and request. Maybe that’s because tasks can’t do that? If so, then I suggest we consider making the arguments const& instead of &&. Or we could use WTFMove when setting m_request and use m_request here. > Source/WebKit2/UIProcess/WebURLSchemeTask.h:60 > + ExceptionType didPerformRedirection(WebCore::ResourceResponse&&, WebCore::ResourceRequest&&); I don’t understand why these arguments are rvalue references; didReceiveResponse doesn’t do that. > Source/WebKit2/UIProcess/WebURLSchemeTask.h:62 > ExceptionType didReceiveData(Ref<WebCore::SharedBuffer>); The type should be SharedBuffer&, not Ref<SharedBuffer>. > Source/WebKit2/UIProcess/API/Cocoa/WKURLSchemeTask.mm:96 > + auto result = _urlSchemeTask->task().didPerformRedirection(response, request); > + raiseExceptionIfNecessary(result); I think this reads better as a one-liner.
Can I clarify if this is expected behaviour for this bug: on iOS w/ Xcode 10.1, this is not performing a redirection (nothing happens) func webView(_ webView: WKWebView, start urlSchemeTask: WKURLSchemeTask) { let resp = HTTPURLResponse(url: urlSchemeTask.request.url!, statusCode: 301, httpVersion: "HTTP/1.1", headerFields: ["Location": "https://www.google.com"])! urlSchemeTask.didReceive(resp) urlSchemeTask.didFinish() }
(In reply to Garvan Keeley from comment #15) > Can I clarify if this is expected behaviour for this bug: on iOS w/ Xcode > 10.1, this is not performing a redirection (nothing happens) > > func webView(_ webView: WKWebView, start urlSchemeTask: WKURLSchemeTask) { > let resp = HTTPURLResponse(url: urlSchemeTask.request.url!, statusCode: > 301, httpVersion: "HTTP/1.1", headerFields: ["Location": > "https://www.google.com"])! > urlSchemeTask.didReceive(resp) > urlSchemeTask.didFinish() > } Based on your comment, I think you expect WebKit to start handling the load once you tell it about the redirect. But that's not right. This API is for you - the application UI process - to tell WebKit about loading progress. So in the above code you're telling WebKit: - "There was a redirect to this HTTP URL" - "The load finished" What I think you mean to do is tell WebKit: - "There was a redirect to this HTTP URL" - "Here is the data for the resource as I retrieved it from this new URL" - "The load finished"
(In reply to Brady Eidson from comment #16) > Based on your comment, I think you expect WebKit to start handling the load > once you tell it about the redirect. > > But that's not right. This API is for you - the application UI process - to > tell WebKit about loading progress. Indeed I was hoping to use this as redirection mechanism, we currently rely on a local webserver to that which is a clunky approach (i.e. a "http://localhost:9999/url?http://google.com" will forward to 'http://google.com' -excuse lack of escaping). This method is the only way we have to restore the history state in WKWebView, by `history.pushState('http://localhost:9999/url?<the url>')` for each URL in the history. Thanks for the info!
Can you confirm that this means currently without this new API, it is impossible to perform an HTTP redirect using a custom URL scheme? In my case, I'm using a custom url scheme to act as a proxy for a http API to support caching in my app since Service Workers aren't available. When using the fetch API from WKWebView to my custom url scheme, if that that upstream server returns a 302 response, and I pass that back through the custom URL scheme, the fetch API fails to detect that as redirect. I would appreciate confirmation if this is something I'm doing wrong, or if it's expected behavior that it will fail and will need to wait for this API to be made public. Thanks!
Zach, you are correct. It is impossible to perform an HTTP redirect using a custom URL scheme without this SPI.
Hi @Alex Christense, i also have the proble too. For example, I open a webView which url is `http://www.example.com`. Then, the server response 301 redirect to `http://www.foo.com`, if i use the code below, the redirect response will dismiss, so do you hava any solutions? I will be very grateful if you help me - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task willPerformHTTPRedirection:(NSHTTPURLResponse *)response newRequest:(NSURLRequest *)request completionHandler:(void (^)(NSURLRequest * _Nullable))completionHandler { // I will save `urlSchemeTask` in advance [urlSchemeTask didReceiveResponse:response]; completionHandler(request); } - (void)URLSession:(NSURLSession *)session dataTask:(NSURLSessionDataTask *)dataTask didReceiveResponse:(NSURLResponse *)response completionHandler:(void (^)(NSURLSessionResponseDisposition))completionHandler { [urlSchemeTask didReceiveResponse:response]; completionHandler(NSURLSessionResponseAllow); } - (void)URLSession:(NSURLSession *)session dataTask:(nonnull NSURLSessionDataTask *)dataTask didReceiveData:(nonnull NSData *)data { [urlSchemeTask didReceiveData:data]; } - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didCompleteWithError:(NSError *)error { if (error) { if (error.code != NSURLErrorCancelled) { [urlSchemeTask didFailWithError:error]; } } else { [urlSchemeTask didFinish]; } }
Apparently this is still not fixed, nearly five years after the patch was submitted? I have just run into exactly this problem in macOS 12.3. This is a pretty bad problem for any app that is trying to run a web-app in a WKWebView without having to bind its HTTP server to a real TCP socket. It's straightforward to implement a WKURLSchemeHandler that proxies the NSURLRequest to the embedded server's C/C++ API (I'm using Yuji Hirose's httplib, FWIW) and the server's responses back to an NSHTTPURLResponse. It works great ... until the server tries to redirect. - If I return the 30x response back to WebKit, it thinks it's an error and fails. - If I run the redirect loop myself, sending the updated location back to the server until I get a non-redirect response, WebKit does not recognize that the URL of the response is different from its initial request, even though I put the correct redirected URL in the NSURLResponse object I return. Again, this _almost_ works, but in my case JS running in the page is breaking because document.location contains the wrong URL. If it's too problematic to make this SPI public, would it be easier to have WebKit recognize the response URL when it's different from the request, and update its state to reflect the new redirected URL?
Not sure all the answers to what Jens said above. But if we need this for apps, then we need API, not "SPI".
FYI, in case it helps anyone else, I have a skanky workaround using the ancient client-side-redirect technique: Instead of giving WebKit a response with a 30x redirect, change the response to a 200 with the following HTML body: <!doctype html><html lang="en"><head> <meta http-equiv="refresh" content="0;URL='URL_TO_REDIRECT_TO'"> </head><body><p>Redirecting</p></body></html> where "URL_TO_REDIRECT_TO" is the URL from the real response's Location header. This works reasonably well in most cases; sometimes the redirect page shows up for a moment. The limitation is that it only works with GET requests. If you have a POST/PUT/whatever that redirects, you're out of luck.