[firebase_crashlytics] runtime exceptions #1803

hffmnn · 2019-07-05T11:56:52Z

Description

This PR adds support to send non-FlutterErrorDetails errors to Crashlytics that have not been caught by the Flutter.onError handler.

When e.g. a exception is thrown in a Future, then the error handler does not work and the crash will never be reported. By using runZoned and using its onError handler it is possible to catch those, but the current plugin only supports sending FlutterErrorDetails errors. I updated the example to make this point more clear.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

googlebot · 2019-07-05T11:57:07Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

hffmnn · 2019-07-05T12:05:58Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+      final dynamic result = await channel
+          .invokeMethod<dynamic>('Crashlytics#onError', <String, dynamic>{
+        'exception': "${exception.toString()}",
+        'context': 'onRuntimeException',


I am not sure what to send here, because we do not have ErrorDescription.

I think it's fine to just leave it out if there's nothing to say

Ok, I double checked: The native method signatures the context gets send to are:
- (void)recordCustomExceptionName:(NSString *)name reason:(nullable NSString *)reason frameArray:(CLS_GENERIC_NSARRAY(CLSStackFrame *) *)frameArray; on iOS. In the end the context gets set on the reason parameter, which is nullable, so ✅.

On Android it looks like it does no get used at all, so ✅.

Will remove then.

hffmnn · 2019-07-05T12:18:44Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

    }
  }

+  Future<void> onRuntimeException(dynamic exception, dynamic stack) async {


Naming is hard. I guess there are better names for this method?

collinjackson

Thanks for the PR! We should land #1799 before this one. (Edit: Done!)

I think I'd like to rename onError to recordError to match other platforms, which will be a breaking change. This is a good opportunity to revisit the method signature as well.

It sounds like we need to support either a FlutterErrorDetails or an arbitrary error and stack, I'm debating whether to try to shoehorn them onto one method, or split it into two methods (recordFlutterError and recordError).

There's a bit of code duplication here, I think we should try to get more code re-use

collinjackson · 2019-07-08T13:27:45Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

+      final dynamic result = await channel
+          .invokeMethod<dynamic>('Crashlytics#onError', <String, dynamic>{
+        'exception': "${exception.toString()}",
+        'context': 'onRuntimeException',


I think it's fine to just leave it out if there's nothing to say

hffmnn · 2019-07-08T19:01:51Z

I signed it!

googlebot · 2019-07-08T19:01:54Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2019-07-09T09:59:16Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-07-09T10:01:24Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

hffmnn · 2019-07-09T10:04:50Z

There's a bit of code duplication here, I think we should try to get more code re-use

Sure. Let me know if you would like to keep the 2 methods or funnel all through one.

I think I'd like to rename onError to recordError to match other platforms, which will be a breaking change. This is a good opportunity to revisit the method signature as well.

I would start working on that, as soon as the API (one method vs two methods) decision is done.

collinjackson · 2019-07-22T21:08:13Z

We've made a (tentative) decision to go with two methods:

Future<void> recordFlutterError(FlutterErrorDetails details);
Future<void> recordError(dynamic error, StackTrace stackTrace, { dynamic context });

Example of usage:

void main() {
 FlutterError.onError = Crashlytics.instance.recordFlutterError;
 runZoned<Future<void>>(() async {
   runApp(MyApp());
 }, onError: Crashlytics.instance.recordError);
}

Let me know if you have any feedback, and if it looks good, I'd appreciate your help updating the PR. This is a breaking change, and we'll be making some other breaking changes at the same time, but I can handle that part.

hffmnn · 2019-07-24T16:06:38Z

@collinjackson I updated the PR with the new API and refactored the duplicated code. Let me know what you think.

Before I started I rebased to the latest master so I have no idea why some checks are failing with Execution failed for task ':app:buildPluginDebugFirebaseAuth'..

hffmnn · 2019-07-24T16:02:38Z

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

-        'keys': _prepareKeys(),
-      });
-    }
+  /// Submits non-fatal crash report to Firebase Crashlytics.


I think we might work on this doc comment.

packages/firebase_crashlytics/pubspec.yaml

collinjackson · 2019-07-24T17:11:48Z

@collinjackson I updated the PR with the new API and refactored the duplicated code. Let me know what you think.

Before I started I rebased to the latest master so I have no idea why some checks are failing with Execution failed for task ':app:buildPluginDebugFirebaseAuth'..

Don't worry about these failures, they're affecting master as well https://cirrus-ci.com/task/4801326419017728

collinjackson · 2019-07-24T17:12:38Z

packages/firebase_crashlytics/example/lib/main.dart

  Crashlytics.instance.enableInDevMode = true;

  // Pass all uncaught errors to Crashlytics.
  FlutterError.onError = (FlutterErrorDetails details) {


Might be simpler here to just say FlutterError.onError = Crashlytics.instance.recordFlutterError;

collinjackson · 2019-07-24T17:13:09Z

packages/firebase_crashlytics/example/lib/main.dart

+
+  runZoned<Future<void>>(() async {
+    runApp(MyApp());
+  }, onError: (dynamic error, dynamic stack) async {


Might be simpler here to just say onError: Crashlytics.instance.recordError

packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart

packages/firebase_crashlytics/CHANGELOG.md

packages/firebase_crashlytics/example/lib/main.dart

googlebot · 2019-07-25T08:38:15Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

Co-Authored-By: Collin Jackson <[email protected]>

collinjackson · 2019-07-25T15:29:21Z

Looks good. Thanks for the contribution.

* update AUTHORS file * add new method recordError * rename onError to recordFlutterError Co-Authored-By: Collin Jackson <[email protected]>

hffmnn requested review from collinjackson and kroikie as code owners July 5, 2019 11:56

googlebot added the cla: no label Jul 5, 2019

hffmnn force-pushed the feature/runtime-exceptions branch from cd78021 to c6a5aa0 Compare July 5, 2019 12:00

hffmnn commented Jul 5, 2019

View reviewed changes

collinjackson reviewed Jul 8, 2019

View reviewed changes

googlebot added cla: yes and removed cla: no labels Jul 8, 2019

hffmnn force-pushed the feature/runtime-exceptions branch from 41ec823 to a778d89 Compare July 8, 2019 20:01

googlebot added cla: no and removed cla: yes labels Jul 9, 2019

hffmnn force-pushed the feature/runtime-exceptions branch from bb496fb to 4180e59 Compare July 9, 2019 10:01

googlebot added cla: yes and removed cla: no labels Jul 9, 2019

collinjackson added the in review label Jul 11, 2019

collinjackson self-assigned this Jul 15, 2019

collinjackson added the flutterfire label Jul 16, 2019

hffmnn and others added 6 commits July 24, 2019 15:56

update AUTHORS file

c7c09eb

bump pubspec version and update CHANGELOG

7cfec23

add new method onRuntimeException

7f84fa4

update the example

3fcfb79

fix CHANGELOG

b54212c

Update AUTHORS

87b51ce

hffmnn added 4 commits July 24, 2019 15:58

make tests green again

ede8676

remove context

9e25d82

update to new API

0d3b3f7

funnel all public methods through one internal method

845d000

hffmnn force-pushed the feature/runtime-exceptions branch from 4180e59 to 845d000 Compare July 24, 2019 14:54

fix example

beda9fd

hffmnn commented Jul 24, 2019

View reviewed changes

collinjackson reviewed Jul 24, 2019

View reviewed changes

googlebot added cla: no and removed cla: yes labels Jul 25, 2019

apply suggested changes

20480ba

Co-Authored-By: Collin Jackson <[email protected]>

hffmnn force-pushed the feature/runtime-exceptions branch from 8118c88 to 20480ba Compare July 25, 2019 08:45

update version in pubspec to 0.1.0

2f97c7d

collinjackson merged commit 589f4e1 into flutter:master Jul 25, 2019

[firebase_crashlytics] runtime exceptions #1803

[firebase_crashlytics] runtime exceptions #1803

Uh oh!

Conversation

hffmnn commented Jul 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Breaking Change

Uh oh!

googlebot commented Jul 5, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

collinjackson 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!

Choose a reason for hiding this comment

Uh oh!

hffmnn commented Jul 8, 2019

Uh oh!

googlebot commented Jul 8, 2019

Uh oh!

googlebot commented Jul 9, 2019

Uh oh!

googlebot commented Jul 9, 2019

Uh oh!

hffmnn commented Jul 9, 2019

Uh oh!

collinjackson commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hffmnn commented Jul 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

collinjackson commented Jul 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

googlebot commented Jul 25, 2019

Uh oh!

collinjackson commented Jul 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hffmnn commented Jul 5, 2019 •

edited

Loading

collinjackson left a comment •

edited

Loading

collinjackson commented Jul 22, 2019 •

edited

Loading