Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ios): maintain native module proxies #10855

Merged
merged 6 commits into from May 8, 2019

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Apr 18, 2019

  • Prevent native module proxies from being released when cached
TEST CASE
  • Create Alloy project
  • Use ACA 1.0.1
alloy.js
Alloy.Globals.aca =  = require('com.appcelerator.aca');
// should not crash

JIRA Ticket

@garymathews garymathews added this to the 8.1.0 milestone Apr 18, 2019
@build build requested a review from a team April 18, 2019 18:18
@build
Copy link
Contributor

build commented Apr 18, 2019

Warnings
⚠️

🔍 Can't find junit reports at ./junit.*.xml, skipping generating JUnit Report.

Generated by 🚫 dangerJS against c8aad99

@vijaysingh-axway
Copy link
Contributor

@garymathews I tried to run an app using sdk created with this code. It is giving following error -
`
[ERROR] Script Error {
[ERROR] column = 168;
[ERROR] line = 70;
[ERROR] message = "Can't find variable: regeneratorRuntime";
[ERROR] sourceURL = "file:///var/containers/Bundle/Application/222CF9EF-165B-402B-BE0F-5A67B4E55197/Lambus.app/login-manager.js";
[ERROR] stack = " at (/login-manager.js:70:168)\n at (/login-manager.js:150:231)\n at (/login-manager.js:382:421)\n at global code(/login-manager.js:383:70)\n at require@[native code]\n at require(/ti.internal/extensions/binding.js:24:118)\n at (/app.js:1:346)\n at global code(/app.js:224:70)\n at require@[native code]\n at require(/ti.internal/extensions/binding.js:24:118)\n at (/ti.main.js:40:8)\n at (/ti.internal/bootstrap.loader.js:157:9)\n at doLoad(/ti.internal/bootstrap.loader.js:139:9)\n at loadBootstrapScripts(/ti.internal/bootstrap.loader.js:151:7)\n at loadAsync(/ti.internal/bootstrap.loader.js:154:21)\n at global code(/ti.main.js:37:52)";
[ERROR] toJSON = "<KrollCallback: 0x281b379c0>";
[ERROR] }
[ERROR] Script Error Module "login-manager.js" failed to leave a valid exports object
[ERROR] Script Error Module "app.js" failed to leave a valid exports object

`

@garymathews
Copy link
Contributor Author

@vijaysingh-axway Looks like that's to do with Babel

@vijaysingh-axway
Copy link
Contributor

@garymathews But without this change it is working fine.

@garymathews
Copy link
Contributor Author

@vijaysingh-axway Try it with the latest master build, because it looks like an issue with transpilation.

Copy link
Contributor

@vijaysingh-axway vijaysingh-axway left a comment

Choose a reason for hiding this comment

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

CR passed.

@hansemannn
Copy link
Collaborator

Is this save to be cherry picked? I'm not sure if this may cause a memory leak at some point. Protecting and unprotecting JSCore references is a pretty sensitive topic.

@vijaysingh-axway
Copy link
Contributor

Backported it to 8_0_X. PR

@@ -21,6 +21,9 @@ - (void)unregisterForNotifications

- (void)dealloc
{
// Allow JavascriptCore to release module proxy.
[self forgetSelf];
Copy link
Contributor

Choose a reason for hiding this comment

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

This will never get called. A module/proxy only get's deallocated when JSCore will finalize a JSValueRef (i.e. when it's getting GC'd) and you actively prevent that by protecting the JSValueRef via rememberSelf. Basically the dealloc chain is: KrollFinalizer (triggered by JSCore GC) -> KrollObject dealloc -> TiProxy dealloc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since all native modules will be cached until the KrollBridge shuts down anyway it should be ok to remove the above lines and just stick with the rememberSelf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to keep the forgetSelf in dealloc since it could be possible to call dealloc on the KrollObject and there's no harm in keeping it it

Copy link
Contributor

Choose a reason for hiding this comment

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

You never send a dealloc manually, it will be done automatically though retain/release. And since the JS object retains the KrollObject there is no way that it will be called in a way that makes sense.

Sure there is no harm in keeping it, but i would prefer not having code around that has no effect ;)

@garymathews garymathews dismissed janvennemann’s stale review April 24, 2019 20:50

Keeping forgetSelf in dealloc

@lokeshchdhry
Copy link
Contributor

lokeshchdhry commented Apr 29, 2019

Not able to test the master PR due to https://jira.appcelerator.org/browse/TIMOB-27040. Will check it once its fixed.

@lokeshchdhry
Copy link
Contributor

FR Passed.

Studio Ver: 5.1.2.201903111843
SDK Ver: 8.1.0 local build
OS Ver: 10.14
Xcode Ver: Xcode 10.1
Appc NPM: 4.2.13
Appc CLI: 7.0.10
Daemon Ver: 1.1.3
Ti CLI Ver: 5.1.1
Alloy Ver: 1.13.9
Node Ver: 8.15.1
NPM Ver: 6.4.1
Java Ver: 10.0.2
IOS Simulator: ⇨ iPhone XS (iOS 12.1)

@sgtcoolguy sgtcoolguy merged commit fcba4e4 into tidev:master May 8, 2019
hansemannn pushed a commit to hansemannn/titanium_mobile that referenced this pull request May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants