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): use weak instead of assign in properties for Objects #12706

Merged
merged 5 commits into from Apr 29, 2021

Conversation

vijaysingh-axway
Copy link
Contributor

@vijaysingh-axway vijaysingh-axway commented Apr 8, 2021

https://jira.appcelerator.org/browse/TIMOB-28413
https://jira.appcelerator.org/browse/TIMOB-28344
In general 'assign' is used for primitive data types and 'weak' is used for objects. If object is deallocated from anywhere, any 'weak' reference to that object is set nil. In assign, if object is deallocated you will still have a pointer to it's memory (which is garbage now)

@build build added this to the 10.1.0 milestone Apr 8, 2021
@build build requested a review from a team April 8, 2021 19:00
@build build added the ios label Apr 8, 2021
@build
Copy link
Contributor

build commented Apr 8, 2021

Fails
🚫 Tests have failed, see below for more information.
Messages
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖 ❌ 2 tests have failed There are 2 tests failing and 961 skipped out of 16041 total tests.
📖

💾 Here's the generated SDK zipfile.

📖 👍 Hey!, You deleted more code than you added. That's awesome!

Tests:

ClassnameNameTimeError
ios.ipad.Titanium.UI.TableViewAll text should show if TableView.style is .INSET_GROUPED (14.4.0)0.497
Error: expected 'Ti.UI.View' to match image ('snapshots/tableview_style_inset_grouped@2x.png')
    mismatched pixels. allowed: 14840, actual: 53921
value@file:///node_modules/should/cjs/should.js:356:23
file:///ti.ui.tableview.test.js:1811:28
run@file:///ti.main.js:8796:22
processImmediateQueue@file:///ti.main.js:8859:18
drainQueues@file:///ti.main.js:8836:52
ios.iphone.Titanium.UI.TableViewAll text should show if TableView.style is .INSET_GROUPED (14.4.0)0.869
Error: expected 'Ti.UI.View' to match image ('snapshots/tableview_style_inset_grouped@3x.png')
    mismatched pixels. allowed: 14840, actual: 200203
value@file:///node_modules/should/cjs/should.js:356:23
file:///ti.ui.tableview.test.js:1811:28
run@file:///ti.main.js:8796:22
processImmediateQueue@file:///ti.main.js:8859:18
drainQueues@file:///ti.main.js:8836:52

Generated by 🚫 dangerJS against 263732e

@sgtcoolguy sgtcoolguy added the bug label Apr 8, 2021
@sgtcoolguy
Copy link
Contributor

Could this be the cause of https://jira.appcelerator.org/browse/TIMOB-28296 ? There we have pretty consistent crashes with Ti.UI.Picker on macOS for dates - where some underlying object is GC'd and we're trying to send it messages.
mocha_2021-04-09-082446_macos-walle.crash.txt

Do we need to make the TiUIPicker's UIControl *picker field weak?

@vijaysingh-axway
Copy link
Contributor Author

Could this be the cause of https://jira.appcelerator.org/browse/TIMOB-28296 ? There we have pretty consistent crashes with Ti.UI.Picker on macOS for dates - where some underlying object is GC'd and we're trying to send it messages.
mocha_2021-04-09-082446_macos-walle.crash.txt

Do we need to make the TiUIPicker's UIControl *picker field weak?

It doesn't look we have to make UIControl *picker field weak . I'll look in this crash.

@sgtcoolguy sgtcoolguy added backport 10_2_X when applied, PRs with this label will get an auto-generated backport to 10_2_X branch on merge in-qe-testing 🕵 labels Apr 15, 2021
@sgtcoolguy sgtcoolguy merged commit c6e20f7 into tidev:master Apr 29, 2021
@build build removed the backport 10_2_X when applied, PRs with this label will get an auto-generated backport to 10_2_X branch on merge label Apr 29, 2021
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

3 participants