apache / cordova-ios

Apache Cordova iOS
https://cordova.apache.org/
Apache License 2.0
2.15k stars 987 forks source link

convenience init behavior within `CDVPlugin.m` for conformance to Swift init and `let` #1288

Closed smallscript closed 1 year ago

smallscript commented 1 year ago

Bug Report

Currently CDVPlugin.m uses a self = [super init] pattern within the initWithWebViewEngine: convenience initializer.

Problem

The designated initializer pattern is used in place of a convenience initializer pattern. 🧶👷 convenience initializers should invoke required designated initializers using self, as in self = [self init] 「not self = [super init]」. Specifically, when building a Swift based CDVPlugin subclass its init never gets invoked.

Additionally, on iOS, this has a negative impact on plugin design because pluginInitialize is NOT an init method and it is only invoked (later) once the plugin has been initialized and registered.

Which leads to ⟨confusion in design patterns⟩ many temporal configuration times within the plugin architecture. 1) CDVPlugin subtype inst self being constructed and initialized. 「designated init」 which DOES NOT happen now, but needs to for proper Swift init designs. 2) pluginInitialize when plugin is registered-as-ready 3) JavaScript plugins module/source has been loaded and configured the JS wrapper 4) deviceReady when JavaScript layer has been notified that Cordova itself is all configured.

What is expected to happen?

The Objective-C/Swift convenience initializer pattern for initWithWebViewEngine invoking self = [self init] 「not self = [super init]」.

What does actually happen?

Currently the convenience-init initWithWebViewEngine is written as if it were a designated-init calling a convenience-init.

It is using self = [super init], which results in a Swift subclass never having its init member called and thus it never gets properly initialized if it has an override init or any memberwise initializers. Which, in turn, means it may not compile if it used let consts members or it may simply crash due to being uninitialized.

Information

explained in other parts of this submission...

Command or Code

Try the following changes with the swift plugin example below. self = [super init] vs self = [self init] within CDVPlugin.m method initWithWebViewEngine:.

class example : CDVPlugin {
  let a: String = "never actually set";
  let b: String; // `let` can only be set in an `init` func
  override init() {self.b = "never actually set either";}
}

Environment, Platform, Device

Version information

Checklist

dpogue commented 1 year ago

It seems like maybe the right thing to do is to mark initWithWebViewEngine with NS_DESIGNATED_INITIALIZER (to make it the designated initializer) and to mark init as NS_UNAVAILABLE so that it cannot be called directly.

That should allow member initializers in Swift plugins to work, but not allow overriding the init method. Plugins should always use pluginInitialize.

smallscript commented 1 year ago

Much as I would applaud that idea for unified behavior, doing what you suggest does not fix the problem. Put another way, pluginInitialize is NOT an init method in the Objective-C/Swift object constructor semantic sense. It is an onRegistered config method.

A solution requires properly adhering to Objective-C and Swift object construction patterns. Which means that init must be invoked on the Swift classes instances; which currently is not happening. The initWithWebViewEngine: is a correctly named and factored to be a convenience-init method and is calling the designated init it just isn't quite calling it correctly. It needs to call self init not super init.

That's why I provided the correct change to self = [self init] that does it the right way and conforms to the iOS required model.

To unify with pluginInitialize pattern would require that the pluginInitialize method invocation and signature be refactored to become the NS_DESIGNATED_INITIALIZER; which is quite a bit more refactoring than simply changing super to self in the initWithWebViewEngine:.

However, there is nothing wrong with having the ObjC/Swift object construction init and a segregated onRegistered config method via pluginInitialize. I was more suggesting that the (4) phases of startup lifecycle be explicitly spelled out in documentation and design advice.

Whichever refactoring change is made, it still must ensure that init is properly invoked on Swift class construction.

Without that fix Swift based plugin classes are unsafe, broken, lead to very unexpected behavior. Which only someone who understands the internals of Swift and Cordova's plugin model would be able to figure out and once they did so they, in the end, would need to make the same basic 「super changed to self」 patch I tested and subsequently proposed. 🌈

p.s., to clarify something that may not be obvious to all reviewers, in ObjC you can explicitly invoke init functions etc. However, in Swift it is FIRMLY disabled and not allowed as part of the Swift language safety design guarantees. Currently, the ObjC implementation within Cordova for CDVPlugin, it is breaking the 「init is always and only invoked by the Swift new-inst construction pattern」 init invocation rule.

dpogue commented 1 year ago

The intention is that initWithWebViewEngine: is the only initializer. It's not a convenience initializer, it is the initializer. It is not safe to call init because that will leave the WebViewEngine with a null value.

Plugin subclasses should never override init. Any work that needs to be done by a plugin on initialization of that plugin must happen in pluginInitialize (which is deliberately not an initializer).

smallscript commented 1 year ago

Well. Unless Cordova uses init properly, it is a broken model. Try any Swift based CDVPlugin example and you will clearly see that the Swift class is NEVER initialized properly and leaves illegal null pointers instead. Which is 100% broken.

Fundamentally you're asking a plugin designer for Cordova on an iOS platform to subclass from CDVPlugin. The primary language of choice from Apple for iOS is Swift. Right now, the Cordova iOS design of CDVPlugin is violating the Swift init model for subclassing. That worked for Objective-C because it operates as effectively unsafe C/C++ and allowed calling an init function at any time from any place which safe Swift does not.

The statement that WebViewEngine will be left with a null value is 100% wrong. Please review and try the code change I provided and you will see that it DOES NOT leave it null, and it will NOT leave it null. That code is tested and being used in products we have now.

The only way in which it would become null is IF and ONLY IF the plugin developer wrote their plugins designated init improperly by throwing an ERROR. It is unaffected by whether they [super init] aka super.init() which would be a worse Swift code usage problem in general.

What a Swift plugin developer should NOT do is override the initWithWebViewEngine: convenience func.

I suspect there is some confusion going on here in that whomever is reading this is not distinguishing the various code-func actors at play in the init and config dance [initWithWebViewEngine:, init, pluginInitialize, the-plugin.js「load」, ondeviceready].

Those actors (players) are temporally sequenced in the startup as follows: 1) initWithWebViewEngine: <WkWebViewEngine>

Note: I am leaving for a different discussion the basic FFI/Marshaling patterns that are used, vs what should be prescribed for modern JavaScript and efficiency across the call-boundary for async-promise management integration and proper CDVPlugin patterns for retention of the callbackId for pushing async notifications back up to the Js layer.

What is CORRECT is that a Plugin developer SHOULD NOT be overriding initWithWebViewEngine:. There is NO REASON to use that override. If one did, they would be REQUIRED to call super.initWithWebViewEngine(wkWebView) to ensure it was properly invoked. Which, because the initWithWebViewEngine is effectively PRIVATE, a Swift subclass cannot see this ObjC-initWithWebViewEngine: func so attempting override and then calling super.initWithWebViewEngine(wkWebView) causes compiler-errors making THIS a NON-STARTER option. (tested and verified statement).

Because pluginInitialize is NOT an init func is is NOT possible to perform/invoke Swift init construction actions on self. By the point at which pluginInitialize is invoked, it is "already too late" as the Swift self has already been constructed from the language model point of view. And because of the way Cordova is invoking init (within initWithWebViewEngine) via self = [super init] instead of self = [self init], the Swift classes init NEVER gets called leaving a CORRUPT/BROKEN Swift self (CDVPlugin subclass) instance. (tested and verified in current Cordova code). FULL STOP

oli06 commented 1 year ago

I completely agree with @smallscript here. I have just stumbled upon another null pointer error due to the incorrect design of swift plugins.

Even if it's not planned to change the underlying design, you should consider mentioning this in a fundamental and prominent way.