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

[TIMOB-26391] Protect js objects for proxies #10352

Merged
merged 3 commits into from Sep 27, 2018

Conversation

janvennemann
Copy link
Contributor

@janvennemann janvennemann commented Sep 26, 2018

JIRA: https://jira.appcelerator.org/browse/TIMOB-26391

Optional Description:
This will protect JS objects temporarily until they are passed back into the js context so they don't get CG'd during the small timeframe of their creation and until they are actually referenced in the JS object graph.

This will protect js objects temporarily until they are passed back into the js context so they don't get CG'd during the small timeframe of their creation and actual usage.
@build
Copy link
Contributor

build commented Sep 26, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

Copy link
Collaborator

@hansemannn hansemannn left a comment

Choose a reason for hiding this comment

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

Working good on my site! Approved if @sgtcoolguy agrees!

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

Generally LGTM.

I'm not 100% sure these are the exact right positions to be protecting/unprotecting the values. I was looking at HAL to see where it was done on Windows, and it's basically done right in the constructor for objects created on the native side, and unprotected in the destructor (Of the JSObject c++ type which wraps the JSObjectRef c type).

I think if we were to translate that sort of policy over, then it'd probably be done in the KrollObject initWithTarget:, and unprotected in dealloc? (Though there appear to be other places we call TiObjectMake to generate js objects, but I'm not sure if any of those are kept around long-term as fields - they look to be returned as values from a method or set as properties on some other object)

@sgtcoolguy
Copy link
Contributor

Just for reference, since we're dealing with the exact same JS engine in both cases, but in pretty different ways - but the concepts remain applicable to both.

Generally speaking, the flow for Windows/HAL is:

  • We decide to new up a proxy, which calls...
  • JSObjectCallAsConstructorCallback - This news up a JSObject that conforms to the proxy's JS type we've defined, which we do in...
  • JSContext::CreateObject - Here's where we new up the object with the class for a given context, which calls the constructor...
  • JSObject::JSObject - the constructor of creating a new JS object, at the end we "register" it with the context it was created in for doing lookups...
  • JSObject::RegisterJSContext - here's where we keep a map of contexts to objects. If this is a new object, we then protect the js object until the destructor is called on the C++ side.

@sgtcoolguy sgtcoolguy added this to the 7.4.1 milestone Sep 27, 2018
@sgtcoolguy sgtcoolguy merged commit b0d4c71 into tidev:7_4_X Sep 27, 2018
sgtcoolguy pushed a commit to sgtcoolguy/titanium_mobile that referenced this pull request Sep 27, 2018
* [TIMOB-26391] Protect js objects for proxies

This will protect js objects temporarily until they are passed back into the js context so they don't get CG'd during the small timeframe of their creation and actual usage.

* [TIMOB-26391] Only guard against GC in JSCore
sgtcoolguy pushed a commit that referenced this pull request Sep 27, 2018
* [TIMOB-26391] Protect js objects for proxies

This will protect js objects temporarily until they are passed back into the js context so they don't get CG'd during the small timeframe of their creation and actual usage.

* [TIMOB-26391] Only guard against GC in JSCore
miniman42 pushed a commit to miniman42/titanium_mobile that referenced this pull request Sep 27, 2018
* 7_4_X:
  [TIMOB-26399] (7_4_X) iOS: Fix notification update to trigger correct callback for default notification action (tidev#10355)
  [TIMOB-26391] Protect js objects for proxies (tidev#10352)

# Conflicts:
#	iphone/Classes/TiApp.m
@alitele
Copy link

alitele commented Dec 3, 2018

Please give permission to me to download SDK .

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

6 participants