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

Switch to cordova-plugin-advanced-http for update downloads #513

Merged
merged 22 commits into from
Jul 27, 2020

Conversation

ermik
Copy link
Contributor

@ermik ermik commented Mar 22, 2019

WIP

  • Needs tester
  • Needs review

Upd: July 15th, 2019 — this is back on the table for me; apologies for staleness.
Upd: November 21st, 2019 — resolved conflicts and fixed some mistakes.
Upd: April 16th, 2020 — @Krasavinigor jumped on to push this along.
Upd: June 24th, 2020 — I found the issue that was highlighted by Igor back in April, but haven't found a solution. I expect, now that's cordova-ios@6.0 is out this will become a P1 on my radar, if somebody else doesn't resolve it sooner.
Upd: July 27th, 2020 — #632 is tracking fixes to test suite which should be implemented before this major dependency change is merged in.

This PR follows through on the #384 by removing dependency on FileTransfer plugin which has been deprecated (apache/cordova-plugin-file-transfer@265b63f). It also implements fixes to the type checker errors highlighted during development.

  • Find a suitable replacement for FileTransfer — originally we went with XHRv2 which is cited as the migration strategy following FileTransfer deprecation, but it has raised issues with CORS (especially in a strict WKWebView networking environment on iOS). Native alternative is cordova-plugin-http which has been superseded by -advanced-http
  • (partially) Maintain type-safety — unfortunately cordova-plugin-advanced-http is written using common js and lacks typings. Additionally it writes to a non-standard cordova global cordova.plugin.http (plugin-singular) and we have to amend the definition coming from @types/cordova.
  • Verify test suite passes — currently tracked in Update tests #632
  • Verify in production environment

This PR now also replaces regular request utility, since plain XHR in iOS 13 may be failing.

bytenik and others added 3 commits February 26, 2018 22:00
- add build.json to pass additional flags to cordova
- use build.json to pass `UseModernBuildSystem=0` avoiding build failure
@msftclas
Copy link

msftclas commented Mar 22, 2019

CLA assistant check
All CLA requirements met.

@ermik ermik closed this Mar 22, 2019
Use XHR to pull remote file into sandbox
@ermik ermik reopened this Mar 22, 2019
@ermik ermik changed the title Feat XHR remote Use XHR to pull remote file into sandbox Mar 22, 2019
@ermik
Copy link
Contributor Author

ermik commented Mar 22, 2019

This is designed to follow #384, with deprecation ofFileTransfer still posing an issue.

There's a bit of a mess with getting the branch back on track, please let me know if you'd like me to rebase or something of that sort.

@ermik
Copy link
Contributor Author

ermik commented Mar 26, 2019

@microsoft @dlebu @alexandergoncharov is downloadUrl set on RemotePackage sufficient for retrieving the update or would this approach also have to set headers on XHR akin ones seen in HttpRequester implementation?

@ermik ermik changed the title Use XHR to pull remote file into sandbox Switch to cordova-plugin-advanced-http for update downloads Mar 30, 2019
@ermik
Copy link
Contributor Author

ermik commented Mar 30, 2019

I've updated the first comment to reflect the current state of the PR.

@jacobg
Copy link

jacobg commented May 3, 2019

Thanks for this @ermik!

Is this work ready to be merged into master?

@alexandergoncharov

@ermik
Copy link
Contributor Author

ermik commented May 3, 2019

@jacobg I wish, but not yet. We're still waiting to know what exactly are the requirements for initiating the download — there are no documentation of CodePush API that I could find, particularly how the downloadUrl is created and any headers that need to exist on the request when we are attempting to download the archive. Move to advanced-http has proven to be the right way forward, as described in the top comment, but I'd like to double check everything before we merge this.

Is there anyone on CodePush team that can speak to how download is/was set up and executed so we can make sure nothing will go wrong with new code?

this.currentFileTransfer.abort();

if (this.isDownloading) {
this.isDownloading = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is questionable, but cordova-plugin-advanced-http does not implement Canecelable / has no abort triggers I could track down.

@alexandergoncharov-zz
Copy link
Contributor

Hi @ermik
Sorry for delay! Thank you for contributing. You've done good and useful job =)

On the your question of the request headers: there are no requirements for explicit specifying headers. downloadUrl should be sufficient for downloading.
I tested your PR. Downloading and updating work fine for me in IOS.
But in Android it was thrown exception java.io.FileNotFoundException (No such file or directory). Did you face this?

Could you please clarify your status on this PR and what can I do to help you?

@ermik
Copy link
Contributor Author

ermik commented Jul 18, 2020

@ludufre I think you would help by checking out this branch running gulp test on your machine, but if you want to use it in your code, you can simply lower the version of the app you use locally below you last CodePush release and then run the app on the simulator for iOS and Android. If you see CodePush succeed — then it's good news.

@ermik
Copy link
Contributor Author

ermik commented Jul 18, 2020

@alexandergoncharov I can't seem to be able to make gulp orchestrate the testing on my machine, I spent a bunch of time trying to make it see Cordova binary, but no luck. Opening this up for review and hopefully there will be one or two testers to verify this works with Cordova 5 & 6 alike.

@ermik ermik marked this pull request as ready for review July 18, 2020 17:16
@ludufre
Copy link

ludufre commented Jul 18, 2020

@ludufre I think you would help by checking out this branch running gulp test on your machine, but if you want to use it in your code, you can simply lower the version of the app you use locally below you last CodePush release and then run the app on the simulator for iOS and Android. If you see CodePush succeed — then it's good news.

gulp test or gulp test-ios on main or this branch outputs: Failed to restore plugin "cordova-plugin-whitelist" from config.xml. You might need to try adding it again. Error: Error: Cannot find module './src/superspawn'
I strongly believe that I am doing something wrong.

@ludufre
Copy link

ludufre commented Jul 18, 2020

@ermik Just to be clear: with cordova-ios@5.1.1 it works perfectly. Just with cordova-ios@6.1.0 + this branch I have an error to compile, complains about UserAgent.

But regardless of that, I'm trying to help with the gulp test but it gives the error mentioned in the previous post.

@alexandergoncharov-zz
Copy link
Contributor

@ludufre, @ermik, Yeah, currently Cordova tests are broken(and before this PR). I also can run gulp command but can't run gulp test command. For now, we can test this changes only manually, unfortunately.
I think that this broken tests will be a good item for the next work.

@ermik
Copy link
Contributor Author

ermik commented Jul 18, 2020

Do what you think is best, but I wouldn't merge any PR's until tests can be run. /inv @Krasavinigor @matthiaswenz

@alexandergoncharov-zz
Copy link
Contributor

Yeah, your thoughts are really good!
But I am just afraid that fixing tests can take really long time as it was broken for a really long time and this PR can unblock users that use new version of cordova-ios. So, it has big priority.
So, my suggestion here: let's try to fix tests for 2 days(until Tuesday) and if on that day we will not have running tests, let's merge it for unblock users and fix test in the future.

@ludufre
Copy link

ludufre commented Jul 19, 2020

Working iOS and Android with defaults platform versions.

Ionic:

   Ionic CLI                     : 6.10.1 (/Users/ludufre/.npm-packages/lib/node_modules/@ionic/cli)
   Ionic Framework               : @ionic/angular 5.2.3
   @angular-devkit/build-angular : 0.901.11
   @angular-devkit/schematics    : 9.1.11
   @angular/cli                  : 9.1.11
   @ionic/angular-toolkit        : 2.2.0

Cordova:

   Cordova CLI       : 9.0.0 (cordova-lib@9.0.1)
   Cordova Platforms : android 8.1.0, ios 5.1.1
   Cordova Plugins   : cordova-plugin-ionic-keyboard 2.2.0, cordova-plugin-ionic-webview 4.2.1, (and 9 other plugins)

Utility:

   cordova-res : 0.15.1
   native-run  : 1.0.0

System:

   Android SDK Tools : 26.1.1 (/Users/ludufre/Library/Android/sdk)
   ios-deploy        : 1.10.0
   ios-sim           : 8.0.2
   NodeJS            : v12.18.1 (/usr/local/bin/node)
   npm               : 6.14.5
   OS                : macOS
   Xcode             : Xcode 11.6 Build version 11E708

With cordova-ios@6.1.0 get error with deprecated CDVUserAgentUtil:

image

I didn't find any code that [CDVUserAgentUtil ...acquireLock] so I commented that line and compiled and worked perfectly in 6.1.1.

I made that small change, here it is: https://github.com/ludufre/cordova-plugin-code-push/commit/159e18cfa9e2aa0c5c6d47ca7b3f0e1d2b1c9f1e (I new on this Github thing, so I don't know how to send this change to this Pull Request / Or @ermik can do this change himself)

@ludufre
Copy link

ludufre commented Jul 19, 2020

Another thing that I observed, perhaps it's default plugin behavior:

If the Server Trust Mode is changed do 'pinned' (this.http.setServerTrustMode('pinned')) on the cordova-plugin-advanced-http, the CodePush stop working because SSL not trusted.

Because this settings is global and not per HTTP request maybe can't be circumvented.

We can add to README.md this behavior. What do you think?

(In my case I have to add the appcenter.ms cert to www/certificates because I use SSL Pinning)

@ermik
Copy link
Contributor Author

ermik commented Jul 19, 2020

@ludufre this is quite interesting, thank you for taking the time to poke around.

@alexandergoncharov I'm afraid I know nothing about gulp. This doesn't seem like a gulp-related issue, but I've had trouble navigating the test suites from the very beginning of this PR. 😭 I'm gonna try but please don't get your hopes up.

@andreidubov
Copy link
Contributor

@alexandergoncharov @ermik @ludufre

I started working on fixing tests here.

@alexandergoncharov-zz
Copy link
Contributor

Hi @ludufre ,
Thanks for debugging this PR :)

Unfortunately, I can't reproduce your issue with cordova-ios@6.1.1. Also, as far as I know, the same for @andreidubov.
Could you please provide some demo app with the reproduced issue and reprosteps?

About Server Trust Mode: I think it is a good idea to add this info to docs. Could you please provide some code suggestions for this?

@szh
Copy link

szh commented Jul 23, 2020

I'm upgrading an app to use cordova-ios 6.1.0 and I'm having a compatibility issue with the file-transfer plugin. I'm using this fork until this PR is merged.

@szh
Copy link

szh commented Jul 23, 2020

I'm having the same issue as this user on iOS with this fork.
#611

@ludufre
Copy link

ludufre commented Jul 24, 2020

Hi @ludufre ,
Thanks for debugging this PR :)

Unfortunately, I can't reproduce your issue with cordova-ios@6.1.1. Also, as far as I know, the same for @andreidubov.
Could you please provide some demo app with the reproduced issue and reprosteps?

About Server Trust Mode: I think it is a good idea to add this info to docs. Could you please provide some code suggestions for this?

Nevermind. I had cloned wrong PR. Fully working now cordova-ios@6 and cordova-android@9.

I'm having the same issue as this user on iOS with this fork.
#611

The issue #611 have FileTransfer in the logs. Are you sure it's related? This fork don't use it.

@szh
Copy link

szh commented Jul 24, 2020

The issue #611 have FileTransfer in the logs. Are you sure it's related? This fork don't use it.

Whether or not it's related - what's happening is that it updates successfully, and then it freezes on a white screen, and rolls back when I relaunch the application. I get the same critical log message:

[CodePush] Install succeeded.
=================================================================
Main Thread Checker: UI API called on a background thread: -[WKWebView loadRequest:]
...

EDIT: the last part of the stack trace is loadRequest, not loadFileURL:allowingReadAccessToURL as it is in #611, but the rest of the stack looks the same

@ermik
Copy link
Contributor Author

ermik commented Jul 26, 2020

@szh, The unsafe UIKit call, in 90% of cases, would not cause a freeze or crash (some notable exceptions, no pun intended, are mutating calls to collections). It may also come from another Cordova dependency.

FWIW UIKit accessors, while a big no-no in the native world (and should be fixed, undoubtedly), have been unsafely called for a while, and many Cordova plugins are guilty of this.

@szh
Copy link

szh commented Jul 27, 2020

@ermik here's the full trace:

=================================================================
Main Thread Checker: UI API called on a background thread: -[WKWebView loadRequest:]
PID: 89240, TID: 1813967, Thread name: (none), Queue name: com.apple.root.default-qos, QoS: 0
Backtrace:
4   <my app name>                 0x000000010c704a3a -[CDVWebViewEngine loadRequest:] + 2154
5   <my app name>                 0x000000010c545ce2 -[CodePush loadURL:] + 130
6   <my app name>                 0x000000010c545c0e -[CodePush loadPackage:] + 110
7   <my app name>                 0x000000010c543df7 __20-[CodePush install:]_block_invoke + 551
8   libdispatch.dylib                   0x000000011284e5d1 _dispatch_call_block_and_release + 12
9   libdispatch.dylib                   0x000000011284f63e _dispatch_client_callout + 8
10  libdispatch.dylib                   0x00000001128520a0 _dispatch_queue_override_invoke + 1028
11  libdispatch.dylib                   0x0000000112860028 _dispatch_root_queue_drain + 351
12  libdispatch.dylib                   0x00000001128609cd _dispatch_worker_thread2 + 130
13  libsystem_pthread.dylib             0x0000000112c479f7 _pthread_wqthread + 220
14  libsystem_pthread.dylib             0x0000000112c46b77 start_wqthread + 15
2020-07-27 10:03:26.125974-0500 <my app name>[89240:1813967] [reports] Main Thread Checker: UI API called on a background thread: -[WKWebView loadRequest:]
PID: 89240, TID: 1813967, Thread name: (none), Queue name: com.apple.root.default-qos, QoS: 0
Backtrace:
4   <my app name>                 0x000000010c704a3a -[CDVWebViewEngine loadRequest:] + 2154
5   <my app name>                 0x000000010c545ce2 -[CodePush loadURL:] + 130
6   <my app name>                 0x000000010c545c0e -[CodePush loadPackage:] + 110
7   <my app name>                 0x000000010c543df7 __20-[CodePush install:]_block_invoke + 551
8   libdispatch.dylib                   0x000000011284e5d1 _dispatch_call_block_and_release + 12
9   libdispatch.dylib                   0x000000011284f63e _dispatch_client_callout + 8
10  libdispatch.dylib                   0x00000001128520a0 _dispatch_queue_override_invoke + 1028
11  libdispatch.dylib                   0x0000000112860028 _dispatch_root_queue_drain + 351
12  libdispatch.dylib                   0x00000001128609cd _dispatch_worker_thread2 + 130
13  libsystem_pthread.dylib             0x0000000112c479f7 _pthread_wqthread + 220
14  libsystem_pthread.dylib             0x0000000112c46b77 start_wqthread + 15
2020-07-27 10:03:26.254783-0500 <my app name>[89240:1813807] IAB.close() called but it was already closed.

I don't think file transfer is related to this error. It looks like the same as #611 to me... see my comment there too

@ermik
Copy link
Contributor Author

ermik commented Jul 27, 2020

@szh Thanks!

Please file your findings as an issue in this repo. It should be an easy fix whenever someone with decent Objective-C knowledge becomes available.

@alexandergoncharov-zz
Copy link
Contributor

Hi all,

We merged #632 test fixing and verified this PR - all work correctly.
So, I'm going to merge this PR and make a new release.

@ermik, thanks a lot for your big work and contributing! It was valuable work! 👍

@alexandergoncharov-zz alexandergoncharov-zz merged commit 1c661bc into microsoft:master Jul 27, 2020
@jacobg
Copy link

jacobg commented Jul 27, 2020

Thank you very much!

Why does plugin.xml still have a dev dependency on @types/cordova-plugin-file-transfer?

@alexandergoncharov-zz
Copy link
Contributor

@jacobg Thanks! Good catch!

I'm removing it in this PR: #633

@alexandergoncharov-zz
Copy link
Contributor

This PR already published in new version: https://github.com/microsoft/cordova-plugin-code-push/releases/tag/v1.13.0

@NickChowDev
Copy link

This PR already published in new version: https://github.com/microsoft/cordova-plugin-code-push/releases/tag/v1.13.0

image

Should the version number be changed to 1.13.0?

Anyway...it still freezes on a blank screen after [CodePush] Install succeeded.
and rolls back when I relaunch the application

codePushSyncStatus 7
[CodePush] Downloading update
[CodePush] Package download success: {"deploymentKey":"xxx","label":"v1","appVersion":"1.0.0","isMandatory":false,"packageHash":"xxx","isFirstRun":false,"failedInstall":false,"localPath":"cdvfile://localhost/library-nosync/codepush/download/update.zip"}
codePushSyncStatus 8
[CodePush] Installing update
[CodePush] Applying full update
[CodePush] Install succeeded.
codePushSyncStatus 1

Main Thread Checker: UI API called on a background thread: -[WKWebView loadRequest:]
PID: 3311, TID: 1439918, Thread name: (none), Queue name: com.apple.root.default-qos, QoS: 0
Backtrace:
[CDVWebViewEngine loadRequest:] + 2080
[CodePush loadURL:] + 156
[CodePush loadPackage:] + 120
__20-[CodePush install:]_block_invoke + 652
libdispatch.dylib _dispatch_call_block_and_release + 24
libdispatch.dylib _dispatch_client_callout + 16
libdispatch.dylib _dispatch_queue_override_invoke + 872
ibdispatch.dylib _dispatch_root_queue_drain + 376
libdispatch.dylib _dispatch_worker_thread2 + 152
libsystem_pthread.dylib _pthread_wqthread + 212
libsystem_pthread.dylib start_wqthread + 8

[reports] Main Thread Checker: UI API called on a background thread: -[WKWebView loadRequest:]
PID: 3311, TID: 1439918, Thread name: (none), Queue name: com.apple.root.default-qos, QoS: 0
Backtrace:
-[CDVWebViewEngine loadRequest:] + 2080
-[CodePush loadURL:] + 156
-[CodePush loadPackage:] + 120
__20-[CodePush install:]_block_invoke + 652
libdispatch.dylib _dispatch_call_block_and_release + 24
libdispatch.dylib _dispatch_client_callout + 16
libdispatch.dylib _dispatch_queue_override_invoke + 872
libdispatch.dylib _dispatch_root_queue_drain + 376
libdispatch.dylib _dispatch_worker_thread2 + 152
libsystem_pthread.dylib _pthread_wqthread + 212
libsystem_pthread.dylib start_wqthread + 8

tcp_input [C4.1:3] flags=[R] seq=846688115, ack=0, win=0 state=LAST_ACK rcv_nxt=846688115, snd_una=2978486252
tcp_input [C4.1:3] flags=[R] seq=846688115, ack=0, win=0 state=CLOSED rcv_nxt=846688115, snd_una=2978486252
tcp_input [C4.1:3] flags=[R] seq=846688115, ack=0, win=0 state=CLOSED rcv_nxt=846688115, snd_una=2978486252
tcp_input [C5.1:3] flags=[R] seq=267997014, ack=0, win=0 state=CLOSE_WAIT rcv_nxt=267997014, snd_una=3094212520
tcp_input [C6.1:3] flags=[R] seq=3760256249, ack=0, win=0 state=LAST_ACK rcv_nxt=3760256249, snd_una=1398964983
tcp_input [C6.1:3] flags=[R] seq=3760256249, ack=0, win=0 state=CLOSED rcv_nxt=3760256249, snd_una=1398964983
tcp_input [C6.1:3] flags=[R] seq=3760256249, ack=0, win=0 state=CLOSED rcv_nxt=3760256249, snd_una=1398964983

@ermik
Copy link
Contributor Author

ermik commented Jul 28, 2020

Sorry about missing plugin.xml but happy I can start my next PR. See you in 2 years!

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.