Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Fix cordova-ios 6 issue with file:// requests being rejected #655

Merged
merged 7 commits into from
Oct 12, 2020

Conversation

ruslan-bikkinin
Copy link
Contributor

@ruslan-bikkinin ruslan-bikkinin commented Oct 7, 2020

Related issue #624.

This PR is similar to #635 but makes a fix more clear way.

Problem

Starting with cordova-ios 6 UIWebView is replaced with WKWebView and also WKURLSchemeHandler is introduced.
If add these parameters to config.xml

<preference name="scheme" value="app" />
<preference name="hostname" value="localhost" />

then file:// requests are rejected by WKURLSchemeHandler which causes app to become blank after applying CodePush update.

There was already a fix for that in CDVWKWebViewEngine+CodePush.m but it was applied only for the case when

  • cordova-ios 4, 5 was used
  • Additional cordova-plugin-wkwebview-engine package was installed and configured within application (it was supposed to be used as a temporary solution for cordova-ios 4, 5 to let developers use WKWebView unless official support is added)

This PR applies the fix for cordova-ios 6 as well.

Implementation specifics

  • loadRequest and loadPluginRequest methods was extracted from CDVWKWebViewEngine+CodePush.m into new WebViewShared class. Now both CodePush.m and CDVWKWebViewEngine+CodePush.m shares the same fix logic written in one place.
  • This class works as a thread-safe singleton to avoid it's possible re-initialization during the CodePush plugin life cycle.
  • To avoid showing blank screen after applying the update and restarting application a workaround was implemented (please see comment here for the explanation)

@sithwarrior
Copy link

A couple of comments, as stated in the issues, UIWebView, is not only deprecated, its persona non grata.

So the backwards comp in CodePush.m

        // In order to make use of the "modern" Cordova platform, while still
        // maintaining back-compat with Cordova iOS 3.9.0, we need to conditionally
        // use the WebViewEngine for performing navigations only if the host app
        // is running 4.0.0+, and fallback to directly using the WebView otherwise.
        #if (WK_WEB_VIEW_ONLY && defined(__CORDOVA_4_0_0)) || defined(__CORDOVA_4_0_0)
            [self.webViewEngine loadRequest:[NSURLRequest requestWithURL:url]];
        #else
            [(UIWebView*)self.webView loadRequest:[NSURLRequest requestWithURL:url]];
        #endif

that mentions UIWebView, is actualy a showstopper. New Apps with this code will be rejected by the App store.
App updates with this code, will be rejected by December.

Doing a search, shows thats the CodePushReportingManager.m also has UIWebView code

Both should be easy, to just cut?
No one needs backwards comp with old Cordova, if you need apps on the App store.
However, as this is a breaking change, Major version change is maybe warranted?

Here is the post from Apple, about not having UIWebView code: https://developer.apple.com/news/?id=12232019b

@ruslan-bikkinin
Copy link
Contributor Author

Hi @sithwarrior yeah, we are aware about that. The changes in this PR are only to fix broken cordova 6 issues for customers. Removing of UIWebView is in our TODO list for the next release (after the upcoming v1.13.2).

@Wouter33
Copy link

Hi @ruslan-bikkinin, thanks for this amazing PR. So you are aware that this PR does not allow us to submit new apps right? Only update existing apps for now.

In my opinion, priority should be to support new (and by definition also existing) cordova-ios 6 apps with a new major version which removes all UIWebView mentions. People using cordova-ios 5 and below can still use older versions of this library. Unless this is significant more work for now and you just want to make the PR available for existing apps.

src/ios/Utilities.m Outdated Show resolved Hide resolved
Copy link
Contributor

@Krasavinigor Krasavinigor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 😄

@ruslan-bikkinin ruslan-bikkinin merged commit 94a18fa into master Oct 12, 2020
@ruslan-bikkinin ruslan-bikkinin deleted the cordova-ios6-fix branch October 12, 2020 12:26
@ruslan-bikkinin
Copy link
Contributor Author

Hi @Wouter33 good point. I'm going to start working on removing UIWebView remnants today. There will be one new version 2.0.0 that accumulates all cordova-ios changes.

@sithwarrior
Copy link

@ruslan-bikkinin I just tried installing this directly from the branch

https://github.com/microsoft/cordova-plugin-code-push#cordova-ios6-fix

It dosent work for me, it goes straight to the blank page.

I've tried my poor objective C debugging skills, and it seems there is no good case for a Cordov-IOS 6 without the ionic WKWebView? (Cordova ios 6 has builtin WKwebview)

The app is now loaded from file:// instead of the custom url scheme.
This causes a bunch of CORS errors when the app loads, and breaks the app.

This is actually even worse than before.

@ruslan-bikkinin ruslan-bikkinin restored the cordova-ios6-fix branch October 12, 2020 13:43
@ruslan-bikkinin
Copy link
Contributor Author

@sithwarrior what's Cordova/Ionic version you are using? Could you please also share your config.xml and package.json (you can strip all sensitive data)?

@ruslan-bikkinin
Copy link
Contributor Author

@sithwarrior If you also could attach demo app on which you are experiencing the issue that would be very helpful!

@sithwarrior
Copy link

@sithwarrior If you also could attach demo app on which you are experiencing the issue that would be very helpful!

I just reproduced this on the codepush basic app, included with this project.

Steps to reproduce:
use Cordova-iOS 6.1

Setup custom URL scheme as recommended in config.xml on iOS
<preference name="scheme" value="app" /> <preference name="hostname" value="localhost" />

remove the codepush plugin
cordova plugin rm cordova-plugin-code-push

Install codepush from master, as the PR is now merged
cordova plugin add https://github.com/microsoft/cordova-plugin-code-push

Build and run.

The app is now served from file://
This is a big no no on ios, as CORS on iOS blocks everything served from file://

@ruslan-bikkinin
Copy link
Contributor Author

@sithwarrior I just tried to reproduce issue using the steps you provided and faced no problems:
image

May be iOS version is involved. Do you use simulator or real device? What iOS version of device/simulator did you use?

@sithwarrior
Copy link

sithwarrior commented Oct 12, 2020

@ruslan-bikkinin Yes the app loads, but if you use Safari remote debugger, you can see that with the new changes in the PR, the app is now served from file://

That should not happen, as CORS will not work, in any way.
It should be hosted from the custom url scheme, defined in the config.xml ie app://localhost

edit:
yes im using the simulator, and latest Xcode

@ruslan-bikkinin
Copy link
Contributor Author

@sithwarrior could you please clarify what should I do in the basic app to start running into CORS issue you're describing?

@sithwarrior
Copy link

@ruslan-bikkinin
The core of the problem is that the custom url scheme is not beeing used in this update. (it is in the current released version)

hosting from file:// creates a lot of problems on ios, thats why the ionic wkwebview uses a built-in http server or custom url schemes, and thats why the new cordova-ios 6, also uses this feature.

Additionaly, WKURLSchemeHandler support has been introduced with this release. Using a custom scheme to serve your app content through fixes CORS issues that exist because of the strict security policies that WKWebView has applied to the file scheme. You can easily configure your Cordova project to use a custom scheme by setting the preference options scheme and hostname in the config.xml file.
https://cordova.apache.org/announcements/2020/06/01/cordova-ios-release-6.0.0.html

The problem is most apparent, when you need to load additional files, such as json config files, local media etc.
We in our production app, use angular translate, with .json files for the translations, and multiple .html files for our templates.

serving the app from file:// results in this:
Screenshot 2020-10-12 at 18 03 07

@sithwarrior
Copy link

Im not a an objective C developer, so I dont no if it helps, but you can see here, how cordova-ios 6 loads the url scheme

https://github.com/apache/cordova-ios/blob/master/CordovaLib/Classes/Private/Plugins/CDVWebViewEngine/CDVWebViewEngine.m

    // viewController would be available now. we attempt to set all possible delegates to it, by default
    CDVViewController* vc = (CDVViewController*)self.viewController;
    NSDictionary* settings = self.commandDelegate.settings;

    NSString *scheme = [settings cordovaSettingForKey:@"scheme"];

    // If scheme is file or nil, then default to file scheme
    self.cdvIsFileScheme = [scheme isEqualToString: @"file"] || scheme == nil;

    NSString *hostname = @"";
    if(!self.cdvIsFileScheme) {
        if(scheme == nil || [WKWebView handlesURLScheme:scheme]){
            scheme = @"app";
        }
        vc.appScheme = scheme;

        hostname = [settings cordovaSettingForKey:@"hostname"];
        if(hostname == nil){
            hostname = @"localhost";
        }

        self.CDV_ASSETS_URL = [NSString stringWithFormat:@"%@://%@", scheme, hostname];
    }

@ruslan-bikkinin
Copy link
Contributor Author

Thanks a lot @sithwarrior, now I see the problem. I'm going to revisit this and fix as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants