fix(animations): change throw -> trace to avoid unnecessary app crash #5475

NathanWalker · 2018-02-27T18:39:22Z

Fixes cause of crashes/bugs in production apps using animation.

PR Checklist

The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md#commit-messages.
There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
You have signed the CLA.
All existing tests are passing: https://github.com/NativeScript/NativeScript/blob/master/DevelopmentWorkflow.md#running-unit-tests.
Tests for the changes are included - https://github.com/NativeScript/NativeScript/blob/master/WritingUnitTests.md.

What is the current behavior?

Developers create apps using animation library and under normal circumstances and well coded scenarios the app will throw when navigating or other normal app usage causing bugginess.

What is the new behavior?

Developers can now see the warning in the console when developing if tracing is enabled to let them know but keep their app stable.

Cause of instability in production Portable North Pole

See screenshot of Crashlytics reports on this.

/cc @atanasovg @vakrilov @sis0k0 @DickSmith

ns-bot · 2018-02-27T18:39:24Z

Can one of the admins verify this patch?

ns-bot · 2018-02-27T18:39:24Z

Can one of the admins verify this patch?

ns-bot · 2018-02-27T18:39:24Z

Can one of the admins verify this patch?

vakrilov · 2018-02-28T07:57:22Z

Hey Nathan,
There is an isPlaying property of animations that you can check before calling play() or cancel().

Trying to cancel() an animation that is not currently playing is probably caused by an error in the application code - thus we decided to throw in such cases. If it is intentional - you can always do the check with isPlaying?

NathanWalker · 2018-02-28T18:38:19Z

Thanks @vakrilov - we always check isPlaying before attempting to call cancel. It's a side effect likely as a result of CSS processing keyframe animations in unforeseen ways (views getting destroyed when navigating or some other reasons, etc) - that neither you nor I can anticipate and really should not be considered exceptional.

A brief inspection of tns-core-modules reveals 198 matches across 67 files where athrow is raised.
This breaks down to an average ~3 errors for every source file.
This looks over 538 individual files accounting for 2.5 Mb of source code in tns-core-modules.

https://stackoverflow.com/a/77164

beware, throwing [...] exception can kill a senior user who is just a slow typer.

+1 excellent answer. I am so frustrated by developers working on API's that I have to consume, and throw Exceptions for every little thing. VERY few cases really require exceptions.

Exceptions should be reserved for what's truly exceptional.

@vakrilov Is this animation condition truly exceptional?

Reasons not to use exceptions:
Sometimes it's overkill if the error handling is simple.
Throwing and catching exceptions is generally significantly more expensive...

The framework core currently makes developing bug free apps more difficult due to the extraneous and voluminous usage of throw.

What do you recommend?

/cc @jlooper @tjvantoll @sipacate @sebawita @rdlauer

vakrilov · 2018-02-28T22:16:11Z

Hey @NathanWalker
In this particular example - indeed trying to play() an animation that is already playing might not be a case for exception, although it might still be an indicator of faulty logic. If the general opinion is that this will make a better and more easy to use API - sure, let's change it.

Looking at the crash analysis report you've attached - it is the exception thrown in keyframe-animation.ts file that is causing trouble, so I would suggest applying this approach in this file also.

As for throwing exceptions in general - I absolutely agree that exceptions should not be used as a control flow mechanism. Non of that APIs in the core framework are designed in a way that requires you to catch exceptions in order achieve your goal.

What we do use as a guiding principle when writing code is fail early/fail early. Most of the throw statements in the code are there to warn you about a problem on the first possible place when it is visible. It is better to get a crash with a stack-trace than debug a program that is just not working and you don't know why. The exceptions are meant to point you to the exact place of a potential bug while you are developing and testing, not to crash your app in production.

That said, I don't agree that judging the quality and ergonomics of a framework, based on the number of throws in the code is fair.

NathanWalker · 2018-02-28T23:57:30Z

Thank you for the understanding @vakrilov and pointing out keyframe-animation.ts file, likely the worst offender since CSS animation issues have been reported by others in several other outlets (forums,issues,etc.). I have updated the PR to include an adequate fix for that file as well.

Big believer in fail fast/fail early however around animation handling doing the correct thing to help a sequence continue/succeed vs. fail is a far better route to take. Consider the comment in the SO post you mentioned:

Consider this invalid HTML: <div><b>Bold Text!</div> It's missing the </b>, but web browsers choose to do the opposite of "fail early" by trying to recover from the problem and hoping it doesn't make things worse. In their case, it's a sensible route to take.

The browser vendors do the right thing. Here we want to help an animation sequence/condition continue while still allowing the developer to dive deeper by turning tracing on to gain deeper insight if they need to. An appreciated priority would be on ensuring dev's can create animations via CSS keyframes or otherwise without coming to find out some 1% of users find a path that was impossible to find during testing and worse not even fixable by the developer themselves as is the case here since it is coming from the core framework in use.

1% of a 1.5 million user base is 15,000 users that were adversely affected by the throw that this PR effectively solves.

Please let me know if there are other concerns to these changes and I can adjust per your advice.

vakrilov · 2018-03-01T16:38:40Z

tns-core-modules/ui/animation/animation-common.ts

+                traceWrite("Animation is already playing.", traceCategories.Animation, 2);
+            }
+            return <AnimationPromiseDefinition>new Promise<void>((resolve, reject) => {
+                reject();


Can you add Animation is already playing. as a reason for rejecting.

vakrilov · 2018-03-01T16:39:20Z

tns-core-modules/ui/animation/keyframe-animation.ts

+                traceWrite("Keyframe animation is already playing.", traceCategories.Animation, 2);
+            }
+            return new Promise<void>((resolve, reject) => {
+                reject();


Can you add Keyframe animation is already playing as a reason for rejecting

vakrilov · 2018-03-01T16:41:21Z

Hey thanks for your explanations.
Indeed you are right that for animations a more forgiving API is perhaps more useful.

I have run the CI tests. Can I ask you to write a couple of unit tests (probably in animation-tests.ts assuring this behavior:

Calling play() second time does not trow but just rejects.
Calling cancel() does nothing if animation is not playing).

Fixes major cause of crashes/bugs in production apps using animation.

NathanWalker · 2018-03-01T23:31:11Z

test

@vakrilov Changes made and tests added 👍

DickSmith · 2018-03-02T02:57:43Z

Any way we could see this in a hotfix release? This appears to have been a regression in 3.4.x. I had none of these before 3.4, but 3.4 seems to have surfaced this and I'm seeing it at around a ~2% user rate in production. I'd be happy to take an rc tagged build for this to roll out to production for confirmation.
https://pasteboard.co/H9YhUDp.png

vakrilov · 2018-03-02T08:25:34Z

@NathanWalker the unit tests you have written seem to fail now on Android (that's what test do).

Indeed that was helpful because seems there is something we've missed. There is a platform-specific implementation for both play() and cancel() methods for android and ios. The code there will still execute even though the super.play()/super.cancel() calls do not throw.

vakrilov

You can slip these // >> comments in the tests. They are not related to testing, we use them to generate code snippets for the docs from the wrapped sections.

NathanWalker · 2018-03-02T08:32:51Z

Ok excellent I’ll fix that up tomorrow since I’m going to bed now. If you knew the fix and wanted to pull my branch to finish it up would be fine, otherwise I’ll get in morning.

vakrilov · 2018-03-02T12:17:06Z

NathanWalker#1 ;)

* chore(tests): Cleanup code snippets comments * refactor(animations): Plat-specific cancel and play methods refactored

NathanWalker · 2018-03-02T14:45:49Z

Perfection thank you 👍

vakrilov · 2018-03-03T09:42:04Z

test

This addition adds more insight into iOS crashes, especially for failures in JS that result in the crash. Using this logging we were able to identify this issue: NativeScript/NativeScript#5475 which otherwise would have been impossible to trace, as it had a 5% repro among users but never occurred internally.

NathanWalker · 2018-03-06T21:09:30Z

@vakrilov I'm unable to see the jenkins test detail - just lemme know if any other modifications are needed to get those to clear 👍 - thanks again.

SvetoslavTsenov · 2018-03-07T19:20:47Z

test branch_animations#master

SvetoslavTsenov · 2018-03-08T12:21:54Z

Hey @NathanWalker we are currently investigating the failed tests from the CI and I need some time. I just want to let you know that we are working on it. Most probably the issue is not related to this PR but I need to be sure.

lock · 2019-08-26T13:54:35Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

ghost added the ♥ community PR label Feb 27, 2018

ns-bot added the cla: yes label Feb 27, 2018

NathanWalker force-pushed the fix-animation-throw branch 2 times, most recently from 14b2a83 to 2ef8cf2 Compare February 28, 2018 23:39

NathanWalker force-pushed the fix-animation-throw branch from 2ef8cf2 to 9c3b041 Compare March 1, 2018 00:00

vakrilov approved these changes Mar 1, 2018

View reviewed changes

NathanWalker force-pushed the fix-animation-throw branch from 9c3b041 to 1d7fac2 Compare March 1, 2018 22:39

fix(animations): change throw -> trace to avoid unnecessary app crash

61937a2

Fixes major cause of crashes/bugs in production apps using animation.

NathanWalker force-pushed the fix-animation-throw branch from 1d7fac2 to 61937a2 Compare March 1, 2018 23:28

vakrilov reviewed Mar 2, 2018

View reviewed changes

Fix fix animation throw (#1)

60ff125

* chore(tests): Cleanup code snippets comments * refactor(animations): Plat-specific cancel and play methods refactored

DickSmith mentioned this pull request Mar 5, 2018

Basic integration of Crashlytics for iOS... EddyVerbruggen/nativescript-plugin-firebase#549

Merged

sis0k0 approved these changes Mar 8, 2018

View reviewed changes

SvetoslavTsenov merged commit fa80355 into NativeScript:master Mar 8, 2018

ghost removed the ♥ community PR label Mar 8, 2018

lock bot locked and limited conversation to collaborators Aug 26, 2019

Uh oh!

fix(animations): change throw -> trace to avoid unnecessary app crash #5475

fix(animations): change throw -> trace to avoid unnecessary app crash #5475

Uh oh!

Conversation

NathanWalker commented Feb 27, 2018 • edited by vakrilov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

What is the current behavior?

What is the new behavior?

Cause of instability in production Portable North Pole

Uh oh!

ns-bot commented Feb 27, 2018

Uh oh!

ns-bot commented Feb 27, 2018

Uh oh!

ns-bot commented Feb 27, 2018

Uh oh!

vakrilov commented Feb 28, 2018

Uh oh!

NathanWalker commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vakrilov commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NathanWalker commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vakrilov Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

vakrilov Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

vakrilov commented Mar 1, 2018

Uh oh!

NathanWalker commented Mar 1, 2018

Uh oh!

DickSmith commented Mar 2, 2018

Uh oh!

vakrilov commented Mar 2, 2018

Uh oh!

vakrilov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NathanWalker commented Mar 2, 2018

Uh oh!

vakrilov commented Mar 2, 2018

Uh oh!

NathanWalker commented Mar 2, 2018

Uh oh!

vakrilov commented Mar 3, 2018

Uh oh!

NathanWalker commented Mar 6, 2018

Uh oh!

SvetoslavTsenov commented Mar 7, 2018

Uh oh!

SvetoslavTsenov commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lock bot commented Aug 26, 2019

Uh oh!

Uh oh!

NathanWalker commented Feb 27, 2018 •

edited by vakrilov

Loading

NathanWalker commented Feb 28, 2018 •

edited

Loading

vakrilov commented Feb 28, 2018 •

edited

Loading

NathanWalker commented Feb 28, 2018 •

edited

Loading

vakrilov left a comment •

edited

Loading

SvetoslavTsenov commented Mar 8, 2018 •

edited

Loading