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): avoid leaking objects in hasProperty callback (7_5_X) #10525

Merged
merged 7 commits into from
Dec 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions iphone/Classes/KrollBridge.m
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,24 @@ - (void)gc
{
}

- (BOOL)hasProperty:(NSString *)propertyName
{
if (dynprops != nil && dynprops[propertyName] != nil) {
return YES;
}

id module = modules[propertyName];
if (module != nil) {
return YES;
}
module = [host moduleNamed:propertyName context:pageContext];
if (module != nil) {
return YES;
}

return [super hasProperty:propertyName];
}

- (id)valueForKey:(NSString *)key
{
// allow dynprops to override built-in modules
Expand Down
83 changes: 77 additions & 6 deletions iphone/Classes/KrollObject.m
Original file line number Diff line number Diff line change
Expand Up @@ -182,12 +182,7 @@ bool KrollHasProperty(TiContextRef jsContext, TiObjectRef object, TiStringRef pr

NSString *name = (NSString *)TiStringCopyCFString(kCFAllocatorDefault, propertyName);
[name autorelease];
id result = [o valueForKey:name];
if (result != nil) {
return true;
}

return false;
return [o hasProperty:name];
}

//
Expand Down Expand Up @@ -833,6 +828,82 @@ - (id)_valueForKey:(NSString *)key
return nil;
}

/**
Checks if a property with the given name exists on our target.

Contains all the magic of valueForKey withouth trying to retrieve any actual
value.

The checks for property existance are done in the following order:
* The Kroll object's own statics and properties cache
* Dynamic getter and setter in the form of getSomeProperty or setSomeProperty
* Property on the actual target
* "toString" and "valueOf" are always available on all objects
* "className" has a special handling with valueForUndefinedKey, return true
for the sake of simplicity
* Method with the same name on the target and single parameter
* Method with the same name on the target and no parameter
* Create factory method

As soon as one of the above checks passes this method returns true, meaning
the property exists. If none of the checks passed the property does not exists
and the method returns false.

@param propertyName The property name to check for.
*/
- (BOOL)hasProperty:(NSString *)propertyName
{
if (statics != nil && statics[propertyName] != nil) {
return YES;
}

if (properties != nil && properties[propertyName] != nil) {
return YES;
}

if (([propertyName hasPrefix:@"get"] || [propertyName hasPrefix:@"set"]) && (propertyName.length >= 4) &&
[NSCharacterSet.uppercaseLetterCharacterSet characterIsMember:[propertyName characterAtIndex:3]]) {
return YES;
}

objc_property_t p = class_getProperty([target class], propertyName.UTF8String);
if (p != NULL) {
return YES;
}

if ([propertyName isEqualToString:@"toString"] || [propertyName isEqualToString:@"valueOf"]) {
return YES;
}

if ([propertyName isEqualToString:@"className"]) {
return YES;
}

SEL selector = NSSelectorFromString([NSString stringWithFormat:@"%@:", propertyName]);
if ([target respondsToSelector:selector]) {
return YES;
}

selector = NSSelectorFromString([NSString stringWithFormat:@"%@", propertyName]);
if ([target respondsToSelector:selector]) {
return YES;
}

id result = [target valueForKey:propertyName];
if (result != nil) {
return YES;
}

if ([propertyName hasPrefix:@"create"]) {
SEL selector = @selector(createProxy:forName:context:);
if ([target respondsToSelector:selector]) {
return YES;
}
}

return NO;
}

- (id)valueForKey:(NSString *)key
{
BOOL executionSet = NO;
Expand Down
122 changes: 122 additions & 0 deletions tests/Resources/core.runtime.addontest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
/*
* Appcelerator Titanium Mobile
* Copyright (c) 2011-Present by Appcelerator, Inc. All Rights Reserved.
* Licensed under the terms of the Apache Public License
* Please see the LICENSE included with this distribution for details.
*/
/* eslint-env mocha */
/* global Ti */
/* eslint no-unused-expressions: "off" */

'use strict';

var should = require('./utilities/assertions');

describe('Core', () => {
describe('Runtime', () => {
describe('hasProperty', () => {
describe('Top-Module', () => {
describe('Submodules', () => {
// @fixme the not.have.property check throws an error on android
it.androidBroken('should check for sub-module', () => {
Ti.should.have.property('UI');
Ti.should.not.have.property('Foo');
});
});

describe('Custom properties', () => {
// @fixme the not.have.property check throws an error on android
it.androidBroken('should check for custom properties', () => {
Ti.should.not.have.property('custom');
Ti.custom = {};
Ti.should.have.property('custom');
});
});
});

describe('Proxy', () => {
describe('Getter/Setter', () => {
it('should check for existing getter method', () => {
const tabGroup = Ti.UI.createTabGroup();
tabGroup.should.have.property('getTabs');
});

it.ios('should check for dynamic getter method', () => {
const view = Ti.UI.createView();
view.should.have.property('getFoo');
});

it('should check for existing setter method', () => {
const webView = Ti.UI.createWebView();
webView.should.have.property('setHtml');
});

it.ios('should check for dynamic setter method', () => {
const view = Ti.UI.createView();
view.should.have.property('setFoo');
});
});

describe('Properties', () => {
it('should check for properties on the object\'s target', () => {
const view = Ti.UI.createView({
backgroundColor: 'black'
});
view.should.not.have.property('bollocks');
view.should.have.property('backgroundColor');
});

it('should check for custom properties', () => {
const view = Ti.UI.createView();
view.should.not.have.property('bollocks');
view.should.not.have.property('foo');
view.foo = 'bar';
view.should.have.property('foo');
view.foo.should.be.equal('bar');
});

it('should properly handle properties with value of nil (TIMOB-26452)', () => {
should(Ti.Geolocation).have.property('lastGeolocation');
if (Ti.Platform.osname === 'android') {
should(Ti.Geolocation.lastGeolocation).be.equal('{}');
} else {
should.not.exist(Ti.Geolocation.lastGeolocation);
}
});
});

describe('toString/valueOf', () => {
it('should be available on any proxy', () => {
const view = Ti.UI.createView();
view.should.have.property('toString');
view.should.have.property('valueOf');
});
});

describe('className', () => {
it.ios('should be availably on any proxy', () => {
const view = Ti.UI.createView();
view.should.have.property('className');
});
});

describe('Methods', () => {
it('should check for methods on the object\'s target', () => {
const view = Ti.UI.createView();
view.should.not.have.property('addSomething');
view.should.have.property('add');
should(view.add).be.a.Function;
});
});

describe('Factory Methods', () => {
it('should check for dynamic create factory method', () => {
Ti.UI.should.have.property('createView');
const view = Ti.UI.createView();
view.should.not.have.property('createSomething');
});
});
});
});
});
});