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 over release of list item proxies #12104

Merged
merged 1 commit into from Sep 29, 2020

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Sep 18, 2020

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

Optional Description:
TiProxy is blindly releasing modelDelegate in its dealloc method. This is a problem for two reasons:

  • _destroy already handles releasing modelDelegate (and dealloc called _destroy on the very next line)
  • modelDelegate may be == self resulting in the recursive dealloc that prompts the warning from macOS.

This likely affected other proxies that assigned self to modelDelegate:

  • TiUIListItemProxy
  • TiUINavBarButton
  • TiUITableViewRowProxy
  • TiUITableViewSectionProxy

To reproduce, run this app on macOS target, click label and list view entries multiple times, then let app sit idle in bg for a while to kick in GC:

var win = Ti.UI.createWindow({
    backgroundColor: '#fff'
});
 
var btn = Ti.UI.createButton({
    title: 'Trigger'
});
 
btn.addEventListener('click', function() {
 
			var myTemplate = {
				childTemplates: [
					{
						type: 'Ti.UI.ImageView',
						bindId: 'pic',
						properties: {
							width: '50', height: '50', left: 0
						}
					},
					{
						type: 'Ti.UI.Label',
						bindId: 'info',
						properties: {
							color: 'black',
							font: { fontSize: '20', fontWeight: 'bold' },
							left: '60', top: 0,
						}
					},
					{
						type: 'Ti.UI.Label',
						bindId: 'es_info',
						properties: {
							color: 'gray',
							font: { fontSize: '14' },
							left: '60', top: '25',
						}
					}
				]
			},
			listView = Ti.UI.createListView({
				templates: { template: myTemplate },
				defaultItemTemplate: 'template'
			}),
			sections = [],
			fruitSection,
			fruitDataSet,
			vegSection,
			vegDataSet,
			grainSection,
			grainDataSet;
		    window = Ti.UI.createWindow({ backgroundColor: 'green' });
 
		fruitSection = Ti.UI.createListSection({ headerTitle: 'Fruits / Frutas' });
		fruitDataSet = [
			{ info: { text: 'Apple' }, es_info: { text: 'Manzana' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Apple' }, es_info: { text: 'Manzana' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Apple' }, es_info: { text: 'Manzana' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Apple' }, es_info: { text: 'Manzana' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Apple' }, es_info: { text: 'Manzana' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Apple' }, es_info: { text: 'Manzana' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Apple' }, es_info: { text: 'Manzana' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Banana' }, es_info: { text: 'Banana' }, pic: { image: 'Logo.png' } }
		];
		fruitSection.setItems(fruitDataSet);
		sections.push(fruitSection);
 
		vegSection = Ti.UI.createListSection({ headerTitle: 'Vegetables / Verduras' });
		vegDataSet = [
			{ info: { text: 'Carrot' }, es_info: { text: 'Zanahoria' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Carrot' }, es_info: { text: 'Zanahoria' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Carrot' }, es_info: { text: 'Zanahoria' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Carrot' }, es_info: { text: 'Zanahoria' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Carrot' }, es_info: { text: 'Zanahoria' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Carrot' }, es_info: { text: 'Zanahoria' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Carrot' }, es_info: { text: 'Zanahoria' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Carrot' }, es_info: { text: 'Zanahoria' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Carrot' }, es_info: { text: 'Zanahoria' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Carrot' }, es_info: { text: 'Zanahoria' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Carrot' }, es_info: { text: 'Zanahoria' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Potato' }, es_info: { text: 'Patata' }, pic: { image: 'Logo.png' } }
		];
		vegSection.setItems(vegDataSet);
		sections.push(vegSection);
 
		grainSection = Ti.UI.createListSection({ headerTitle: 'Grains / Granos' });
		grainDataSet = [
			{ info: { text: 'Corn' }, es_info: { text: 'Maiz' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Corn' }, es_info: { text: 'Maiz' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Corn' }, es_info: { text: 'Maiz' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Corn' }, es_info: { text: 'Maiz' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Corn' }, es_info: { text: 'Maiz' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Corn' }, es_info: { text: 'Maiz' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Corn' }, es_info: { text: 'Maiz' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Corn' }, es_info: { text: 'Maiz' }, pic: { image: 'Logo.png' } },
			{ info: { text: 'Rice' }, es_info: { text: 'Arroz' }, pic: { image: 'Logo.png' } }
		];
		grainSection.setItems(grainDataSet);
		sections.push(grainSection);
 
		listView.setSections(sections);
 
		window.addEventListener('focus', function () {
 
		});
		listView.addEventListener('itemclick',  function(e){
			window.close();
		});
 
		window.add(listView);
		window.open();
});
 
win.add(btn);
win.open();

I suspect it's how we're generating the sections/rows for the data property items. The code returns proxies tagged as autorelease, when I suspect they should not be. The general flow here seems shaky:
https://github.com/appcelerator/titanium_mobile/blob/master/iphone/Classes/TiUITableViewProxy.m#L694-L746

@build
Copy link
Contributor

build commented Sep 18, 2020

Fails
🚫

Test suite crashed on iOS simulator. Please see the crash log for more details.

Messages
📖 👍 Hey!, You deleted more code than you added. That's awesome!
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

✅ All tests are passing
Nice one! All 7450 tests are passing.
(There are 720 skipped tests not included in that total)

Generated by 🚫 dangerJS against 50e6327

@@ -379,7 +379,6 @@ - (void)setDictTemplates_:(id)args
[_measureProxies setObject:cell forKey:key];
[theProxy setIndexPath:[NSIndexPath indexPathForRow:-1 inSection:-1]];
[cell release];
[theProxy release];
Copy link
Contributor

Choose a reason for hiding this comment

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

@sgtcoolguy I had tried with same changes and it was leaking memory.

avoid explicitly releasing modelDelgate as _destroy handles that and
guards against releasing self

Fixes TIMOB-28127
@sgtcoolguy
Copy link
Contributor Author

The change that looks to be involved here dates back to: 46e83b2

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.

LGTM!

Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed: No longer able to reproduce crashes with the above code.

Test Environment

MacOS Big Sur: 11.0 Beta 7
Xcode: 12.0 
Java Version: 1.8.0_242
Android NDK: 21.3.6528147
Node.js: 12.18.1
""NPM":"5.0.0","CLI":"8.1.1""
Max OS
iphone 11

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

4 participants