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(android): fix crash when using window.add([]) with a null object #14164

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

m1ga
Copy link
Contributor

@m1ga m1ga commented Jan 3, 2025

Issue
If you add a null object when using .add([]) it will crash with

(main) [178,178] <embedded>:5351
    _add.call(this, child);
         ^
Error: Attempt to invoke virtual method 'java.lang.Class java.lang.Object.getClass()' on a null object reference
    at Window.add (<embedded>:5351:10)
    at new Controller (/alloy/controllers/index.js:93:5)
    at Object.exports.createController (/alloy.js:427:8)
    at /app.js:32:7
    at Module._runScript (ti:/kroll.js:1196:15)
    at Module.load (ti:/kroll.js:734:13)
    at Module.loadJavascriptText (ti:/kroll.js:1062:15)
    at Module.loadAsFile (ti:/kroll.js:1108:22)
    at Module.loadAsFileOrDirectory (ti:/kroll.js:1039:26)
    at Module.require (ti:/kroll.js:865:30)

    org.appcelerator.titanium.proxy.TiViewProxy.add(TiViewProxy.java:590)

because it tries to get the class name for the warning. Since it is null it will crash instead of showing a warning.

This PR adds a null check and outputs a different warning.

Test

const win = Ti.UI.createWindow({layout:"vertical"});
const lbl1 = Ti.UI.createLabel({text:"lbl"});
const lbl2 = Ti.UI.createLabel({text:"lbl"});
win.add([lbl1, , lbl2]);
win.open();

@hansemannn
Copy link
Collaborator

Why would someone want to do that? It feels like the correct behavior. We shouldn't need to save-guard everything and educate devs to properly null-check their view hierarchies.

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.

I am not in favor of additional logic in this performance critical call

@m1ga
Copy link
Contributor Author

m1ga commented Jan 5, 2025

It won't print the warning what happened because it will crash. This way it will give the dev the warning. The check is already in the fail part so it shouldn't be called in a normal app.

But when a user is adding dynamic elements (I only used the blank in my example to quickly test it but it could also be an empty variable) using an array it might appear that one of those is null. Rather then crashing I thought it would be nice to give the proper warning.

@hansemannn
Copy link
Collaborator

Maybe someone else can vote for it? I would not add this, but if others want it, we can include it.

@m1ga
Copy link
Contributor Author

m1ga commented Jan 5, 2025

it's been like this for 9 years and no complaints, so it's not a big issue at all. So I'm fine with just closing it.
I just had it before when I was preparing another demo for a PR and was curious why it it was crashing 😄 But I accidentally added ,, in my array.

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.

Let's take this, as you've correctly mentioned that the extra if case is only processed if there is an error case already.

@hansemannn hansemannn merged commit 81dd7c9 into master Jan 29, 2025
5 checks passed
@hansemannn hansemannn deleted the androidFixArrayNull branch January 29, 2025 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants