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): manually evaluate all error properties #11576

Merged
merged 8 commits into from Apr 8, 2020

Conversation

janvennemann
Copy link
Contributor

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

Starting with iOS 13.4 JavaScriptCore follows the spec and makes all Error properties non-enumerable. This breaks our existing conversion from JS error object to native since we relied on property enumeration. To address the issue our runtime will now manually check for available properties on the error object and populate the values to the native TiScriptError.

@build
Copy link
Contributor

build commented Mar 30, 2020

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.
📖

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

Generated by 🚫 dangerJS against 3b9039e

@sgtcoolguy
Copy link
Contributor

I suspect it's not as simple as this as I assume this affects our crash reporting, cc @garymathews

evaluationError = YES;
[[TiExceptionHandler defaultExceptionHandler] reportScriptError:[TiUtils scriptErrorValue:excm]];
[TiExceptionHandler.defaultExceptionHandler reportScriptError:exception inKrollContext:context];
Copy link
Contributor

Choose a reason for hiding this comment

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

@janvennemann defaultExceptionHandler is a method not property. I think it should be used as [TiExceptionHandler defaultExceptionHandler]. Same applies for other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, a property will only create a variable and getter/setters for you and the compiler will automatically call the getter method when you access the property. By using TiExceptionHandler.defaultExceptionHandler here I just instruct the compiler to automatically call the method defaultExceptionHandler, so it shouldn't make a difference.

@@ -692,8 +691,7 @@ - (KrollWrapper *)loadCommonJSModule:(NSString *)code withSourceURL:(NSURL *)sou
[eval release];

if (exception != NULL) {
id excm = [KrollObject toID:context value:exception];
[[TiExceptionHandler defaultExceptionHandler] reportScriptError:[TiUtils scriptErrorValue:excm]];
[TiExceptionHandler.defaultExceptionHandler reportScriptError:exception inKrollContext:context];
Copy link
Contributor

Choose a reason for hiding this comment

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

@janvennemann I think two methods you introduced in TiExceptionHandler are not required. You can get TiScriptError by using the new method in TiUtils. Something like below -

[TiExceptionHandler.defaultExceptionHandler reportScriptError:[TiUtils scriptErrorFromValueRef: exception inContext: context.context]];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, they are not required. However, i added them purely for convenience reasons since we have different source types that we need to create TiScriptError from. I didn't like repeatedly having to use [TiUtils scriptErrorFromValueRef: exception inContext: context.context]] all over the place so i moved that into the two new methods.

IMHO [TiExceptionHandler.defaultExceptionHandler reportScriptError:exception inKrollContext:context]; looks nicer and is easier to read than having a nested method. Another benefit is that we only need to adjust the TiUtils usage in one place should we ever need to change this again.

@vijaysingh-axway
Copy link
Contributor

In aca module, I can't see if anything will get affected. ACA is receiver of TiScriptError and NSException from TiExceptionHandler via delegate.

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.

See the comments please.

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.

Looks good.

@ssekhri
Copy link

ssekhri commented Apr 3, 2020

@janvennemann An error report is shown when we explicitly throw an error from the code. However there is a difference in behavior when I compare between iOS 13.4 and older iOS version. The app closes from the code block which throws an error on iOS 13.4 whereas on older version (I tested 12.2) the error is thrown but the app continues.
To reproduce:

  1. Create a default alloy app
  2. In index.js changed the doClick() as follows
    function doClick(e) { alert($.label.text); try { throw new Error('example exception'); } catch (e) { aca.logHandledException(e); } }
  3. Run on iOS sim
    On iOS 13.4, as soon as we click on "Hello World" the app closes. An error report is shown in logs.
    On iOS 12.2 on click of Hello World the alert message is shown and error report is also shown in logs.

@ssekhri
Copy link

ssekhri commented Apr 7, 2020

Tested on iOS 13.4 device and got the crash logs.

CrashLog.zip

@ssekhri
Copy link

ssekhri commented Apr 8, 2020

FR Passed.
Thrown error reports shown with all properties and no crash observed.
Verified on:
Mac OS: 10.15.4
SDK: 9.1.0.v20200408132629
Appc CLI: 8.0.0
JDK: 11.0.4
Node: 10.17.0
Studio: 6.0.0.202003181504
Xcode: 11.4
Device: iPhone X(v13.4), iOS simulator 13.4, iOS simulator 12.2

@ssekhri ssekhri merged commit 4111298 into tidev:master Apr 8, 2020
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