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): check for selector availability #12128

Merged
merged 6 commits into from Oct 2, 2020

Conversation

vijaysingh-axway
Copy link
Contributor

@build build added this to the 9.3.0 milestone Sep 24, 2020
@build build requested a review from a team September 24, 2020 00:20
@build build added the ios label Sep 24, 2020
@build
Copy link
Contributor

build commented Sep 24, 2020

Fails
🚫 Tests have failed, see below for more information.
Warnings
⚠️

tests/Resources/ti.ui.tableview.test.js#L1347 - tests/Resources/ti.ui.tableview.test.js line 1347 – 'value' is assigned a value but never used. (no-unused-vars)

Messages
📖

💾 Here's the generated SDK zipfile.

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖 ❌ 1 tests have failed There are 1 tests failing and 984 skipped out of 10904 total tests.

Tests:

ClassnameNameTimeError
ios.iphone.Titanium.UI.iOS.CollisionBehavior.exampleworks (14.0)15
Error: timeout of 15000ms exceeded
file:///Users/build/Library/Developer/CoreSimulator/Devices/0BA89FD6-590B-4E1C-A92C-C7071914B3F8/data/Containers/Bundle/Application/12106AD2-A890-4995-A13B-4E54B7ECC13D/mocha.app/ti-mocha.js:4326:27

Generated by 🚫 dangerJS against 6d1ffc9

[[self view] updateClipping];
if ([[self view] respondsToSelector:@selector(updateClipping)]) {
[[self view] updateClipping];
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure this does not fix the real issue. Primarily, because this used to work before and the error suggests that a wrong instance is bubbled up in the event

Copy link
Contributor

Choose a reason for hiding this comment

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

@vijaysingh-axway I think Hans is right here. I did a quick debug session and it appears that self.view is holding a wrong value. Xcode debugger says it's nil so it may have been accidentally released and now holds an invalid pointer. Can you please double check what's going on?

Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

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

See comment above.

@sgtcoolguy
Copy link
Contributor

sgtcoolguy commented Sep 24, 2020

Looking at diffs between 9.0.3 and 9.1.0, this change related to TIMOB-27935 looks suspicious: b15d184

I think this stems because now a row proxy returns the TiUITableViewCell whereas before it returned nil.

The code generally assumes the view returned is a TiUIView, but in this case, it is not.

@vijaysingh-axway
Copy link
Contributor Author

To fix an issue (kind of parity) https://jira.appcelerator.org/browse/TIMOB-27935, as Chris has mentioned, I have returned the cell view associated with row proxy which is not TiUIView. This is causing the crash. Initially it was nil so there was not issue. So-
In current fix I have checked for availability of selector then only call it. I think there should not be any related issue after this fix.

@sgtcoolguy
Copy link
Contributor

So, this may be a quick and dirty fix for this issue, but it exposes the fact that the earlier fix broke the [TiViewProxy view] return type contract of returning a TiUIView*. It no longer returns a TiUIView*, it returns a subclass of UIView*. The better long-term "fix" here would be if we could separate callers that needed a TiUIView* from those needing a UIView*. i.e. the code around calculating the rect property actually only needs a UIView*, but the size property needs a TiUIView* (which shows that the earlier fix only fixed the rect property, not size!) - and then have different methods to return either?

Or maybe we can extract some common protocol used by callers and ensure TiUITableViewCell and TiUIView adhere to it?

@vijaysingh-axway
Copy link
Contributor Author

I am thinking to revert the changes of PR #11758 and reopen https://jira.appcelerator.org/browse/TIMOB-27935 to properly fix.

@vijaysingh-axway
Copy link
Contributor Author

Reopened https://jira.appcelerator.org/browse/TIMOB-27935 to look in this again.

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: Tested with the following test case:

var tableData = [];
   
var win = Ti.UI.createWindow({ backgroundColor: 'white' });
 
var table = Ti.UI.createTableView({ objName: 'table' });
 
for (var i = 0; i <= 20; i++) {
var row = Ti.UI.createTableViewRow({ title: 'click me' });
 
var view = Ti.UI.createView({
layout: 'vertical',
height: Ti.UI.SIZE,
touchEnabled: false,
width: "50%",
left: 0
});
row.add(view)
tableData.push(row);
}
 
table.setData(tableData);
 
table.addEventListener('click', function (e) {
if (e.source && e.source.objName !== 'table') {
e.source.add(Ti.UI.createLabel({
text: 'REQUIRED',
left: '60%',
color: 'blue',
horizontalWrap: true,
width: "40%",
height: Ti.UI.SIZE
}));
 
}
});
 
win.add(table);
win.open();

Test Environment

MacOS Big Sur: 11.0 Beta 9
Xcode: 12.2 beta 2
Java Version: 1.8.0_242
Android NDK: 21.3.6528147
Node.js: 12.18.1
""NPM":"5.0.0","CLI":"8.1.1""
iphone 11 (ios 14)

@sgtcoolguy sgtcoolguy self-requested a review October 2, 2020 15:13
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.

I get that the rollback off the other fix "fixes" this issue. But it also re-opens a different crash that affected a customer. We need to address that as well - we cannot merge this in and be done with it, because we're just trading one crash for another

@sgtcoolguy sgtcoolguy dismissed their stale review October 2, 2020 17:32

Spoke about what this re-opens on Teams

@sgtcoolguy
Copy link
Contributor

OK, I was mistaken. The fix for TIMOB-27935 caused this bug and TIMOB-28001 to occur - not that the fix for TIMOB-27935 also fixed TIMOB-28001
So, rolling back really only re-opens https://jira.appcelerator.org/browse/TIMOB-27935 which is an issue found internally and does not seem to be as severe as this bug or TIMOB-28001 (both crashes).

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