flutter / samples

A collection of Flutter examples and demos
https://flutter.github.io/samples/
Other
17.2k stars 7.51k forks source link

guardWithCrashlytics in game_template is causing issues after upgrading to 3.10 #1818

Closed githubmonkey closed 1 year ago

githubmonkey commented 1 year ago

Flutter 3.10 includes a breaking change related to zone initialization. https://github.com/flutter/flutter/pull/122836

The game_template sample is using guardWithCrashlytics() to run call runApp() inside a new zone. When calling guardWithCrashlytics in main.dart, a previously instantiated crashlytics object is passed in. To obtain that object, WidgetsFlutterBinding.ensureInitialized(); is called in a different zone . This is no longer allowed.

Fatal Exception: io.flutter.plugins.firebase.crashlytics.FlutterError: Zone mismatch.
The Flutter bindings were initialized in a different zone than is now being used. This will likely cause confusion and bugs as any zone-specific configuration will inconsistently use the configuration of the original binding initialization zone or this zone based on hard-to-predict factors such as which zone was active when a particular callback was set.
It is important to use the same zone when calling `ensureInitialized` on the binding as when calling `runApp` later.
To make this warning fatal, set BindingBase.debugZoneErrorsAreFatal to true before the bindings are initialized (i.e. as the first statement in `void main() { }`).. Error thrown during runApp.
       at BindingBase.debugCheckZone.<fn>(binding.dart:497)
       at BindingBase.debugCheckZone(binding.dart:502)
       at .runApp(binding.dart:1080)
       at .guardedMain(main.dart:76)
filiph commented 1 year ago

Ok, I've been thinking about this and I think I will just remove the guardWithCrashlytics concept. It doesn't seem to be mentioned in the Crashlytics documentation, and when I asked (https://github.com/flutter/flutter/pull/122836#issuecomment-1575510530) about how to do it, Ian suggests to "have the zone error handler call a configurable callback that you can set up from inside the zone". That seems super convoluted and I wouldn't be surprised if it leads to more runtime errors than what it prevents.

One last chance is to ask the folks who are behind the new Firebase Crashlytics documentation and/or the migration from the old one. They probably had to deal with the same conundrum. After all, they had to see the Zoned errors section in the old documentation and had to make the conscious decision to remove it.

cc @domesticmouse -- Would you please ask? You will have better access to these folks than I do. This question (how to catch errors that happen in callbacks, and how to send them to Firebase Crashlytics / Sentry) seems much larger than the game_template sample.

domesticmouse commented 1 year ago

FYI @puf and @nohe427

nohe427 commented 1 year ago

I believe that the current guidance is to use:

// Pass all uncaught asynchronous errors that aren't handled by the Flutter framework to Crashlytics
    PlatformDispatcher.instance.onError = (error, stack) {
      FirebaseCrashlytics.instance.recordError(error, stack, fatal: true);
      return true;
    };

for asynchronous errors.

This is current guidance based on this documentation and is documented here.

The old documentation is no longer current and the firebase.google.com site should be referenced for new docs.

We should likely update the game template to use PlatformDispatcher instead.

Let me know how else I can help

azazadev commented 1 year ago

Ok, I've been thinking about this and I think I will just remove the guardWithCrashlytics concept. It doesn't seem to be mentioned in the Crashlytics documentation, and when I asked (flutter/flutter#122836 (comment)) about how to do it, Ian suggests to "have the zone error handler call a configurable callback that you can set up from inside the zone". That seems super convoluted and I wouldn't be surprised if it leads to more runtime errors than what it prevents.

One last chance is to ask the folks who are behind the new Firebase Crashlytics documentation and/or the migration from the old one. They probably had to deal with the same conundrum. After all, they had to see the Zoned errors section in the old documentation and had to make the conscious decision to remove it.

cc @domesticmouse -- Would you please ask? You will have better access to these folks than I do. This question (how to catch errors that happen in callbacks, and how to send them to Firebase Crashlytics / Sentry) seems much larger than the game_template sample.

@filiph should we remove guardWithCrashlytics from game_template ? ca you please update the git repo

domesticmouse commented 1 year ago

PTAL @filiph

filiph commented 1 year ago

Sorry for the delay. I'm in discussion to make this whole template significantly simpler (and instead document the complexities of adding things like Crashlytics etc.) so please allow for a few more weeks. It's quite possible this whole thing with guardWithCrashlytics will go away soon.

I'll keep the issue open and assigned to myself, of course.