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

Behaviour of promiseToFuture has changed from Dart SDK => 3.0.0 (Flutter >=3.10.0) #52572

Open
russellwheatley opened this issue May 31, 2023 · 12 comments
Assignees
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. P1 A high priority bug; for example, a single project is unusable or has many test failures web-libraries Issues impacting dart:html, etc., libraries

Comments

@russellwheatley
Copy link

This tracker is for issues related to:

  • Dart core library
    dart:js_util.dart

  • Dart SDK Version (dart --version)
    Dart 3.0.0

  • Whether you are using Windows, MacOSX, or Linux (if applicable)
    MacOSX version 12.5.1

  • Whether you are using Chrome, Safari, Firefox, Edge (if applicable)
    Chrome version 113.0.5672.126

We have an issue on the FlutterFire repository that you can read about here.

We use the dart:js_util promiseToFuture API to make async calls to the Firebase web JS SDK.

In previous versions of Flutter (e.g. version 3.7.3 which uses the Dart SDK 2.19.2), we were able to access the underlying JS exception. You can see our implementation here. When the exception would come through, it would be an instance of LegacyJavaScriptObject. This allowed us to access the properties of the Firebase Auth exception.

In the latest Flutter stable version 3.10.0 which uses Dart SDK 3.0.0, we now see NativeError instance come through as the exception. This doesn't allow us to access the properties we need for the Firebase Auth exception. It also doesn't allow us to do a type check on the exception instance as the NativeError appears to be hidden as an internal implementation on the Dart SDK.

Is there any way around this or a solution you propose?

@stevemessick stevemessick added the area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). label May 31, 2023
@lrhn lrhn added area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. and removed area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). labels Jun 1, 2023
@sigmundch sigmundch added the web-libraries Issues impacting dart:html, etc., libraries label Jun 1, 2023
@sigmundch
Copy link
Member

@russellwheatley - Thanks for reaching out and sharing all the details!

From the issue in the flutterfire repo, it appears to me that this discrepancy is visible only during development. Do you know if it is also visible in production/release code?

I ask because we have 2 different compilers at play, when using flutter run you are indirectly using DDC, our dev compiler, and when using flutter build -m release you are instead using dart2js, our production compiler. If the issue only shows up in one of the environment, it may indicate an issue on the compiler pipeline instead.

Also any help creating a smaller repro we can try locally would be super useful. I tried coping the sample from the flutterfire issue tracker, but the code is missing a few imports so I haven't yet been able to reproducable locally. If it's at all possible to create a simpler Dart program that doesn't use flutter to reproduce this, that would be ideal.

Thanks in advance!

FYI @srujzs in case this kind of behavior change seems related to any of our recent changes in this area. To be honest, I am not sure yet whether this is a web-libraries issue, or a ddc issue.

@sigmundch sigmundch added the P1 A high priority bug; for example, a single project is unusable or has many test failures label Jun 1, 2023
@srujzs
Copy link
Contributor

srujzs commented Jun 1, 2023

I think this is unrelated to promiseToFuture, but rather related to a change in DDC that makes NativeError a subtype of JavaScriptObject so that custom errors can be interoperable with @staticInterop types. #50899 is the related issue there.

I am a little confused on how you were seeing LegacyJavaScriptObject before instead of NativeError. I'd have expected the error to be intercepted as a NativeError regardless. You can try casting the exception to a @staticInterop type e.g.

@JS('Error')
@staticInterop
abstract class AuthError {}

extension AuthErrorExtension on AuthError {
  external String get code;
  external set code(String s);
  external String get message;
  external set message(String s);
  external String get email;
  external set email(String s);
  external AuthCredential get credential;
  external set credential(AuthCredential c);
  external String get tenantId;
  external set tenantId(String s);
  external String get phoneNumber;
  external set phoneNumber(String s);
}

to see if that allows you to interact with it? I'm also a little confused on what type checks you were doing before that are now failing.

A repro and/or a log of the exception and related code would help here if possible, thanks!

@sigmundch
Copy link
Member

Ah, I see, thanks for pointing that out @srujzs.

@russellwheatley - can you point us closer to where you see a change in behavior in the firebase package? For example, I found this conversion between JS errors to Dart errors here: https://github.com/firebase/flutterfire/blob/81f1b80c445cfaab3e7752b8c58f35c92d234d8d/packages/firebase_auth/firebase_auth_web/lib/src/utils/web_utils.dart#LL25-L30

Is that where you see a change in behavior (the true branch is taken in 3.0.0 but the false branch was taken in the previous SDK)?

Looking at that code, I wonder if something wrong is surfacing with that is check. It is worth highlighting that is checks on JSInterop types may be misleading. In particular, all jsobjects match is X where X is a JSInterop type, so is checks can't be used to distinguish between JSinterop types, only to distinguish between types that are known to Dart and objects in JavaScript unknown to us.

NativeError is in a strange position because it was always known to Dart, it changed where it is in the type hierarchy, but it didn't change how we interpret it. While I can imagine that a change to the NativeError hierarchy could affect the result of is checks here, so far I'm not seeing it on small local examples. Instead, I see that any JavaScript object that extends from window.Error is mapped to NativeError both in 2.19.2 and 3.0.0, and as a result, I would expect the firebase_auth code to take the same branch with both SDK versions.

@russellwheatley
Copy link
Author

russellwheatley commented Jun 5, 2023

Thanks for the quick response!

@srujzs - I tried making it a @staticInterop but it was the same result.

I thought the easiest way to demonstrate is to update the auth example on the FlutterFire repo. Steps to reproduce:

This is the branch you need to use: https://github.com/firebase/flutterfire/tree/auth-10966
This shows changes to repo for purpose of demonstration: https://github.com/firebase/flutterfire/compare/auth-10966?expand=1

  1. Clone FlutterFire repository and checkout auth-10966 branch.
  2. Install melos:
dart pub global activate melos
  1. Bootstrap melos within FlutterFire repo:
melos bootstrap
  1. Use Flutter version 3.7.3 and navigate to packages/firebase_auth/firebase_auth/example in your terminal
  2. Run flutter run -d chrome. Note that customData is an object which contains these properties. You will see the log:
CUSTOM DATA: [object Object]

Which I am printing here: https://github.com/firebase/flutterfire/compare/auth-10966?expand=1#diff-a522fde29aeb319e6149a657c49c90dcce5c910e0e1166a1b37a0f00b4360885R28

  1. Use Flutter version 3.10 and follow step 5. You should see this in the console:
Error: NoSuchMethodError: 'customData'
method not found
Receiver: Instance of 'NativeError'
Arguments: []

If you want to see the instance type for yourself, you may add a checkpoint here. Check out the instance type. You will see LegacyJavaScriptObject for Flutter version 3.7.3, and NativeError for Flutter >= 3.10.

@sigmundch
Copy link
Member

Thanks @russellwheatley for all the details.

I'm still curious about the behavior in release mode. Based on the fact that Error is not intercepted by our production compiler, it's quite possible that it works in both 3.7.3 and 3.10.

What's really strange here is that the dev-mode behavior in 3.10 is more consistent with how the compiler's runtime was written (even how it was written in 3.7). One theory we haven't explored is that potentially flutter changed how the .html loads the firebase and Dart code. It is quite possible that our output is sensitive to the order in which the code loaded.

While we still need to investigate why the behavior changed, what I notice here is that firebase was accidentally working because of some internal compiler representation choices. I'd recommend making a couple changes to firebase to make it more robust and ensure it doesn't indirectly depend on implementation details.

The pointers you share match exactly my suspicion that the issue is surfaced at getFirebaseAuthException in web_utils.dart. I'd recommend that we change the logic so it work with either JavaScriptObject or NativeError as follows:

  • Use static interop: as @srujzs suggested, but with not just AuthError, also the parent types like core_interop.FirebaseError
  • No longer use is checks to identify these kind of errors.

Some rationale for why:

  • static interop: any type that happens to also be supported by the Dart SDK internally cannot be modeled using non-static package:js types. Often users notice this with native types exposed in dart:html like HtmlElement, but it applies to any browser type that could be represented in the SDK. Currently NativeError in the dev-mode compiler is one of those. Note that the future of JS-interop is to move towards a style similr to static-interop.

  • is-checks: With either the traditional-interop or static-interop, is checks are problematic. We don't reify JSInterop types like we do Dart types, as a result is checks on JSInterop types are likely to give you an unexpected answer or an answer that depends on implementation details. Today exception is! core_interop.FirebaseError is compiled to exception is! LegacyJavaScriptObject.

To replace the is check, my recommendation is to use JavaScript's instanceof instead. For example:

import 'dart:js_util' as js_util;
import 'package:js/js.dart';

@JS('firebase_core.FirebaseError')
Object get firebaseErrorType;

...
FirebaseAuthException getFirebaseAuthException(
  Object exception, [
  auth_interop.Auth? auth,
]) {
  if (!js_util.instanceof(exception, firebaseErrorType)) { ...

@sigmundch
Copy link
Member

Seems I made a mistake in my local testing last Friday, but I actually have a minimal repro we can use here:

This program:

import 'package:js/js.dart';

@JS()
@anonymous
class L1 {}

@JS()
external Object get o1;

@JS()
external void eval(String s);

const String jsCode = r'''
self.FirebaseError = class FirebaseError extends Error {}
self.o1 = new FirebaseError("boo");
''';

main() async {
  eval(jsCode);
  final o = o1;
  print('${o.runtimeType}: ${o is L1}');
}

Prints something different on each runtime and version:

  • dart2js prints: "JSObject: true"
  • DDC 2.19.3: "LegacyJavaScriptObject: true"
  • DDC 3.0.0: "NativeError: false"

It appears that the change to the NativeError extends Interceptor is not what caused this change to happen, but we can now easily bisect where the error comes from.

@sigmundch sigmundch self-assigned this Jun 5, 2023
@sigmundch
Copy link
Member

I found the cause: the @JsPeerInterface for Error was introduced in c010e4f (part of Dart 3). Removing the annotation reverts to the old behavior. This is also what caused #50899 (in 2.19.3 it was possible to use static interop with Error).

@nshahan - seems the use of NativeError in the SDK is very limited and only to check whether we have a TypeError (that we map to a JSNoSuchMethodError). It appears that in the dart2js runtime we direclty use instanceof TypeError for a similar purpose, so maybe a solution to keep things simpler here going forward may be to use that instead.

Next steps:

  • @russellwheatley - I'd still recommend refactoring the firebase code to avoid the is check and use static interop. Let us know how that goes!
  • @srujzs, @nshahan, @annagrin - it may still be worth simplifying our solution here. This may mean removing the NativeError type from DDC's runtime, which would also elide the issue initially filed in [ddc] Cannot staticInterop with subclass of Error #50899, but may require a follow up change in dwds since it may also depend on this indirectly. I may take a look later this week.

@srujzs
Copy link
Contributor

srujzs commented Jun 5, 2023

This may mean removing the NativeError type from DDC's runtime, which would also elide the issue initially filed

Right, if it no longer gets intercepted as a special type, it can be interoperable for our purposes.

It does look like dwds depends on some of this behavior: https://github.com/dart-lang/webdev/blob/b10d62b83288f92560bf9c05a06c4dc0a3cf84e8/dwds/lib/src/debugging/metadata/class.dart#L223, but that looks refactorable to instanceof. There's also JSNoSuchMethodError which is also intercepted and extends NativeError.

@russellwheatley
Copy link
Author

I've created an issue to track on the FF repository to track the refactor needed for removing is checks and using @staticInterop. I tried the above mentioned in debug mode but I could not get it to work I'm afraid. My sense is to wait for this issue to be resolved first before proceeding with the refactor.

@sigmundch
Copy link
Member

Thanks @russellwheatley - happy to look at your PR, in case we can help figure out why the fix didn't work on your end.

@russellwheatley
Copy link
Author

Hey @sigmundch, I have created a demonstration PR which shows how we are not able to capture a FirebaseError with the current version of Flutter: firebase/flutterfire#11258

This isn't just a problem with Auth but also Firestore and presumably all plugins on the web platform. I've shown what is happening in the PR description.

This is fairly critical for FlutterFire so if you have any other suggestions, I'd be very grateful. Thanks.

@sigmundch
Copy link
Member

Hi @russellwheatley, thank you so much for creating that demonstration PR showing the issue! I was able to run it locally and see the issue first hand. It was extremely helpful!

I better understand why the suggestion didn't work as expected. There are actually a lot of issues at play at once. I created a new branch that, I hope, addresses all of them here: sigmundch/flutterfire@master...additional_fixes

I hope this unblocks you!

Just so you know. We do intend to make changes in DDC to revert the NativeError behavior, that said it will take us time to land it properly. I still believe the fixes above would be a better approach: your package will be more robust as a result.

Here is a summary of the issues:

  • issue 1: is FirebaseError is brittle and too permissive.
  • issue 2: two versions of FirebaseError.
  • issue 3: as AuthError is brittle.

In particular:

issue 1 is what we discussed earlier. The is only checks that the underlying value is a JavaScript object not known to the Dart runtime (DDC or dart2js). This is more permissive than you want, because it accepts any JavaScript object that is not a firebase errors as well. This broke because JavaScript's Error was not known to DDC before, but now it is.

If we switched FirebaseError to use @staticInterop, the is check will behave again like it did before the DDC change. That's because with @staticInterop the is check doesn't exclude types known to the Dart runtime. This is still more permissive than what you want.

The instanceof workaround didn't work because of the second issue above. There are two different definitions of FirebaseError in the program, the exception was created from the definition in firebase-auth.js, which is minified and encapsulated, while the instanceof check used the type from firebase-app.js.

Possible fixes here are to either:

  • (a) Change how the firebase-auth.js and firebase-app.js are packaged. This can be either to:

    • expose the FirebaseError class from firebase-auth.js and use that in the instanceof check instead of the one from firebase-app.js (via firebase_core).
    • remove the definition from firebase-auth.js and export and replace it with the one in firebase-app.js.
  • (b) use something else to discern whether exception is a FirebaseError. For example, instead of checking whether it is an instance of core.FirebaseError, we can check that it is an instance of the regular Error and that also has a code property.

I prefer (a), but I cannot demonstrate that, so I implemented (b) in my changes. (b) is more permissive than (a), but much more restricted than the original is check.

The third issue is similar to the first. The cast to AuthError fails because a cast on a package:js type only works for types not-known to the DDC runtime. My only advice is to switch to @staticInterop as well. I also reflected this in my sample above.

With the proposed changes, I can run the example a bit further, but then I get a null safety error (the email getter returns null). I expect that is just due to the nature of the sample, not because of issues related to interop anymore. If that's not the case, let me know and I'm happy to look further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. P1 A high priority bug; for example, a single project is unusable or has many test failures web-libraries Issues impacting dart:html, etc., libraries
Projects
None yet
Development

No branches or pull requests

5 participants