[TIMOB-26007] iOS: Refactor crash dialog #10022
Conversation
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
Just a few changes for now. Really really great work @garymathews!
iphone/Classes/TiUtils.m
Outdated
| { | ||
| TiObjectRef exceptionObj = TiValueToObject(context, exception, NULL); | ||
| TiStringRef stackProperty = TiStringCreateWithUTF8CString("stack"); | ||
| TiValueRef stackValue = JSObjectGetProperty(context, exceptionObj, stackProperty, NULL); |
There was a problem hiding this comment.
Use TiObjectGetProperty to make it usable with TiCore.
iphone/Classes/TiUtils.h
Outdated
| @@ -730,4 +730,6 @@ typedef enum { | |||
| */ | |||
| + (UIImage *)imageWithColor:(UIColor *)color; | |||
|
|
|||
| + (NSString*)stackFromException:(TiContextRef)context exception:(TiValueRef)exception; | |||
There was a problem hiding this comment.
Please format the source with clang-format -style=file -i Classes/TiUtils.* and for other related files. Only necessary at the end :-).
| /** | ||
| * Returns line column where error happened. | ||
| */ | ||
| @property (nonatomic, readonly) NSInteger column; |
There was a problem hiding this comment.
If the column cannot be < 0, you can use NSUInteger to be more precise.
iphone/Classes/TiErrorController.m
Outdated
|
|
||
| [dismissButton addTarget:self action:@selector(dismiss:) forControlEvents:UIControlEventTouchUpInside]; | ||
|
|
||
| UIScrollView* view = [[[NSBundle mainBundle] loadNibNamed:@"ErrorScreen" owner:self options:nil] objectAtIndex:0]; |
There was a problem hiding this comment.
Are we using an xib for convenience? Usually we try to add the AutoLayout code programmatically to have more control over it, but I understand that this may be critical for more complex layouts.
|
I assume this is sort of a parity ticket for iOS like we did in #10003 for Android? I'd love to be able to also add |
iphone/Classes/TiExceptionHandler.m
Outdated
| [[TiApp app] showModalError:[error description]]; | ||
| } else { | ||
| [[TiApp app] showModalError:[NSString stringWithFormat:@"%@\n\n%@", [error description], [nativeStackTrace componentsJoinedByString:@"\n"]]]; | ||
| } |
There was a problem hiding this comment.
@garymathews I have pushed one more commit that includes an alternative way of receiving the stack-symbols without throwing a "computed" error by using [NSThread callStackSymbols]. We may also need to change the start-index to 3, since the order is:
- showScriptError
- handleScriptError
- reportScriptError
- (ANY CLASS)
|
@garymathews I think we are good to merge here. Thanks again for your work on the iOS side! |
|
Forgive me if I'm missing something here, but it looks like we show the native stack in the modal error dialog, but don't set it on I think we should be generating the native stack lower-level where ever it is we surface the Specifically, I'd like to see this unit test suite tweaked slightly to support accessing nativeStack on both iOS and Android: https://github.com/appcelerator/titanium_mobile/pull/10003/files#diff-3dc400bf7f83b2691685b65f5a5066d1R13 There we added tests to verify 3 typical scenarios:
I think e should be able to enable this test on both platforms and have them each pass. Clearly the nativeStack will differ on Android versus iOS, and maybe the triggering code for causing a native error. But the general type/property checks should apply. |
JS ErrorsFor JS errors, but they are named differently. Native ErrorsFor native errors, we include Should we remove those for iOS? EDIT: We could add it pretty easily in the NSArray<NSString *> *nativeStack = [(NSException *)obj callStackSymbols];
if (nativeStack != nil) {
TiStringRef propertyName = TiStringCreateWithUTF8CString("nativeStack");
TiStringRef valueString = TiStringCreateWithCFString((CFStringRef)[nativeStack componentsJoinedByString:@"\n"]);
TiObjectSetProperty(jsContext, excObject, propertyName, TiValueMakeString(jsContext, valueString), kTiPropertyAttributeReadOnly, NULL);
TiStringRelease(propertyName);
TiStringRelease(valueString);
}it would show as the following: We should trim the last 2 stacks again to not include the exception handling like we do for our error-controller, but yes, that could work out. String Literal ErrorsQuick test that passes already, using the current state: try {
throw ('this is my error string');
} catch (ex) {
Ti.API.error(ex);
Ti.API.error(ex === 'this is my error string');
}I have added a PR for the above in garymathews#2 |
|
I'm satisfied with @hansemannn 's additional commits on his fork here, so if they get included - and we update the test to actually run on iOS, we should be good. |
|
@sgtcoolguy @garymathews Unit-test added as well. I have removed the "contains string" checks to be less error prone, especially because Windows (cc @infosia) will come as well. |
| NSUInteger exceptionStackTraceLength = [exceptionStackTrace count]; | ||
|
|
||
| // re-size stack trace and format results. Starting at index = 4 to not include the script-error API's | ||
| for (NSInteger i = 4; i <= (exceptionStackTraceLength >= 20 ? 20 : exceptionStackTraceLength); i++) { |
There was a problem hiding this comment.
@garymathews This is causing an array out of bound exception:
var win = Ti.UI.createWindow({
backgroundColor: '#fff',
homeIndicatorAutoHidden : true
});
var scrollable = Ti.UI.createScrollableView({
});
win.add(scrollable);
win.open();
setTimeout(function () {
scrollable.setViews([-1]);
}, 4000);We should probably do i < xxxxx?
TEST CASE
#1JIRA Ticket