Allow plugins to hook into the WKURLSchemeHandler #1030
Conversation
NiklasMerz
changed the title
This comment has been minimized.
This comment has been minimized.
|
Just noticed it was added to 6.1.2 milestone. I think this is a feature, so should be added as 6.2.0 |
👍 |
| BOOL anyPluginsResponded = NO; | ||
| BOOL handledRequest = NO; | ||
|
|
||
| for (NSString* pluginName in vc.pluginObjects) { |
There was a problem hiding this comment.
This for loop pattern, which we also see in CDVWebViewEngine.m, occasionally causes crashes in our production apps because vc.pluginObjects can be mutated on a different thread while you're looping. Here's an example:
0 CoreFoundation 0x1ac96a5ac __exceptionPreprocess + 220 (NSException.m:199)
1 libobjc.A.dylib 0x1c0a5842c objc_exception_throw + 60 (objc-exception.mm:565)
2 CoreFoundation 0x1ac969f04 __NSFastEnumerationMutationHandler + 124 (NSEnumerator.m:129)
3 Our App 0x1022e13ec -[CDVWebViewEngine webView:decidePolicyForNavigationAction:decisionHandler:] + 336 (CDVWebViewEngine.m:543)
There are several right ways to deal with this:
- The best would be to use a protocol, like my similar request here: CDVWebViewEngine needs a way to set the websiteDataStore of its configuration #900
- You could alternatively use DispatchQueue to safely read and write pluginObjects on separate threads
- You could simply make a copy of pluginObjects and enumerate the copy instead
Since #3 is the easiest to write in a PR comment, that would look like the below, but I'd be happy to help you do #1, which would be a lot better.
/*
* Give plugins the chance to handle the url
*/
__block BOOL anyPluginsResponded = NO;
__block BOOL shouldAllowRequest = NO;
NSDictionary *pluginObjects = [[vc pluginObjects] copy];
SEL selector = NSSelectorFromString(@"shouldOverrideLoadWithRequest:navigationType:");
[pluginObjects enumerateKeysAndObjectsUsingBlock:^(id _Nonnull key, id _Nonnull plugin, BOOL * _Nonnull stop) {
if ([plugin respondsToSelector:selector]) {
anyPluginsResponded = YES;
// https://issues.apache.org/jira/browse/CB-12497
int navType = (int)navigationAction.navigationType;
shouldAllowRequest = (((BOOL (*)(id, SEL, id, int))objc_msgSend)(plugin, selector, navigationAction.request, navType));
if (!shouldAllowRequest) {
*stop = YES;
}
}
}];
There was a problem hiding this comment.
I updated #900 with an example of a protocol that you could use here.
This comment has been minimized.
This comment has been minimized.
|
@adamdport Thanks for you tests and comments. Most of your comments apply to the "proxy" I built in #1004. This PR is a generic way for plugins to intercept web requests. I built the first plugin to use this API from the proxy code of the previous PR here: https://github.com/GEDYSIntraWare/cordova-plugin-webview-proxy So this PR has nothing to do with CORS, cookies etc. I just happen to use for these things by building a plugin that depends on this PR. |
|
@NiklasMerz my mistake. I'll open issues on that repo instead, thanks |
Addresses review comment: apache#1030 (comment)
This comment has been minimized.
This comment has been minimized.
|
@girish2711 Thanks for you feedback. Seems like you hit some edge cases with the cookie quirks in WebKit. We should move this discussion to the proxy plugin and not continue in this PR. This PR just adds the ability for plugins to hook into the code in the platform. Again please don't discuss issues around the "proxy" here. |
Codecov Report
@@ Coverage Diff @@
## master #1030 +/- ##
=======================================
Coverage 74.82% 74.82%
=======================================
Files 13 13
Lines 1720 1720
=======================================
Hits 1287 1287
Misses 433 433 Continue to review full report at Codecov.
|
|
If no one objects I will merge this in 24 hours. |
This newly merged PR copied the crashing pattern that I've addressed in #1050 and #1051. You might want to open a new issue to follow the new pattern once those two PRs get merged. |
|
@msmtamburro Yes we should address that in the next version with #1050, #1051 and changes to this feature together. Feel free to open an issue. |
|
@NiklasMerz Thank you (and everyone else) for putting this together! I'm excited to put this to use. It's not clear to me how this could be integrated into a Cordova app, for folks looking to solve the original issue of bypassing CORS errors in their app. Is updating to |
|
@mattdsteele As far as I know my plugin https://github.com/GEDYSIntraWare/cordova-plugin-webview-proxy is only one to use this feature so far. I built it to solve my CORS and cookie issues. Maybe others will use this hook to implementen different solutions. Give the plugin a try and let me know in the plugins repo if it works for you or issues you find. |
|
@NiklasMerz I have tried using the cordova ios 6.2.0 but unfortunately still getting the "error: Build input file cannot be found: '../mobile/platforms/ios/CordovaLib/Classes/Private/Plugins/CDVWebViewEngine/CDVWebViewProcessPoolFactory.m' (in target 'CordovaLib' from project 'CordovaLib')" and if you check the code https://github.com/apache/cordova-ios/tree/master/CordovaLib/Classes/Private/Plugins/CDVWebViewEngine the folder indeed doesn't seem to be containing CDVWebViewProcessPoolFactory.m |
* set CDVWebViewProcessPoolFactory to public * Move CDVWebViewProcessPoolFactory.m to Classes/Public
|
@tudordumitriu This is not the right PR for this. Please open a new issue or contine in the commit. |
|
@NiklasMerz |
We don't rely on the protocol itself being implemented by the plugins (we continue to check with `-respondsToSelector:`) but this allows us to avoid `objc_msgSend` and provides a way to document some of this plugin behaviour that is not otherwise explained. This should also resolve the unsafe plugin iteration issue that was mentioned in apacheGH-1272 and apacheGH-1030 by always iterating over an array of plugin objects that is a copy (due to calling `-allValues`).
We don't rely on the protocol itself being implemented by the plugins (we continue to check with `-respondsToSelector:`) but this allows us to avoid `objc_msgSend` and provides a way to document some of this plugin behaviour that is not otherwise explained. This should also resolve the unsafe plugin iteration issue that was mentioned in GH-1272 and GH-1030 by always iterating over an array of plugin objects that is a copy (due to calling `-allValues`).
Platforms affected
iOS
Motivation and Context
This offers plugin authors the possibility to plug into the WKURLSchemeHandler. Some options where discussed on the list
I did this "hook" to implement a plugin which acts as a proxy to bypass CORS and cookie issues: https://github.com/GEDYSIntraWare/cordova-plugin-webview-proxy.
Supersedes #1004
Description
The implementation was based on the discussions on the list and similarly implemented like
shouldOverrideLoadWithRequest. Thanks to @erisu for the hint in the right direction.TODO
Testing
window.WebviewProxy.convertProxyUrl(url)Checklist
(platform)if this change only applies to one platform (e.g.(android))