dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.21k stars 1.57k forks source link

js-interop: Dart exceptions and errors thrown through js-interop are not un-jsified. #36363

Open kealjones-wk opened 5 years ago

kealjones-wk commented 5 years ago

Setup:

Does it happen in Dartium or when compiled to JavaScript?: Both Describe the issue you're seeing: When a dart exception is thrown and is caught by javascript if the error is then passed back into a dart callback the error comes through as a JsObject in dart2js and as a NativeJavaScriptObject in DDC. Is this an issue or is this expected behavior?

Workarounds: We have found ways around this, that i wanted ask about and see if they are safe ways to go about re-dartifying thrown exceptions or if these solutions were potentially problematic: In these examples _JoesException is a dart class that extends from Exception And just adds some additional fields.

in dart2js:

_JoesException joesError = js_util.getProperty(error, 'dartException');

in ddc:

_JoesException joesError = _getSymbolProperty(error, 'Symbol(_thrownValue)');

The js side function accessed in dart using package:js/js.dart in example two is:

function _getSymbolProperty(obj, key){
  var sym = Object.getOwnPropertySymbols(obj).find(function(s) {
    return String(s) === key;
  });
  return obj[sym];
}

Failing code: It is not really failing, this is where the issue was observed. We are seeing it in componentDidCatch and getDerivedStateFromError. PR: https://github.com/cleandart/react-dart/pull/175 You can test using this file: https://github.com/cleandart/react-dart/blob/785f74ca68c8c4ecf3c051459d6357d29813f636/example/test/react_test_components.dart Run pub run build_runner serve example [-r] and navigate to http://localhost:8080/test/react_test.html In _Component2ErrorTestComponent you can update the componentDidCatch method to use the info argument or getDerivedStateFromError method to use the error argument.

Any insight or better more correct solutions are totally welcome! Also if you need more detail about anything let me know I am happy to share.

aaronlademann-wf commented 5 years ago

@kevmoo @robbecker-wf @johnpryan

kealjones-wk commented 2 years ago

Just hit this issue again for https://github.com/Workiva/react_testing_library/

We were using the window.onError from dart:html to catch errors from event handlers. Test Case: onClick: (_) => throw StateError('EXPLOSION')

final errorStream = window.onError.cast<ErrorEvent>().listen((event) {
  errors.add(event.error);
});

event.error is typed as Object, fair enough considering this is a catchall for unhandled errors, including non-dart errors, but even trying to do is type checks on known dart errors fail and it ultimately is always a NativeJavaScriptObject.

We worked around this by rethrowing event.error from JS, Dart for whatever reason will catch this throw from JS and dartify that error...

js:

function throwErrorFromJS(error) {
  throw error;
}

dart:

@JS()
external void throwErrorFromJS(error);

then just call the interop throwErrorFromJS and boom correctly typed Dart error! if (errors.isNotEmpty) throwErrorFromJS(errors.first);

kevmoo commented 2 years ago

@srujzs - is this on your radar?

srujzs commented 2 years ago

Nope, but now it is. :) The difference in type you see between the compilers is just a difference in the way the two compilers reify unknown JS objects. It might be worth unifying the runtime type so it's less confusing, but that's tangential.

It's interesting that the exception is caught when it's directly thrown from JS but not in the dart:html case. I'll have to repro the different examples locally and report back.

KealJones commented 2 years ago

@srujzs I hope this helps, I wrote a dartpad based example that demonstrates getting the error in dart then throwing in js to dartify: Gist: https://gist.github.com/kealjones-wk/0e955521d53848aef580ebc837642b47 Dartpad: https://dartpad.dev/?id=0e955521d53848aef580ebc837642b47

srujzs commented 2 years ago

I think the behavior you see is a consequence of the way we handle exceptions in the web compilers. We wrap values that are thrown and then they are unwrapped when caught in a catch block. In the Dart world, this is fine, but it presents issues with interop. Since we never catch the error, the error never gets unwrapped, which is what you've noticed.

It's also the same reason why throwing it from JS and catching it in Dart ends up unwrapping the error. The compilers don't try to unwrap something that's already unwrapped, so even if you called throwFromJS('hello world') and catch it in Dart, it'd work as expected. The struggle though is that if we throw in Dart, we don't know whether or not it's caught in Dart or JS, so we don't know concretely whether it should be wrapped or not.

Fixing this might be complicated, but I suppose there are two options:

1) Ironically enough, rethrowing through JS and catching it in Dart is a solution. We could make this simpler by adding similar functionality to a JS library (or have it just unwrap if possible directly). 2) A potentially better solution might be to make some changes to the RTI to handle the wrapped errors. is and as would handle it such that the underlying error also gets checked, with the as check doing the unwrapping. Right now, the wrapped errors are JavaScriptObjects so that'll need to be changed. This would at least allow you to treat the error as the expected Dart value, even though it requires a cast (although maybe we can modify the error streams in dart:html to do these casts beforehand). I'm not sure what complications might arise from this however. For example, are there instances where we don't want wrapped errors to be treated as the underlying error?

@sigmundch, any thoughts on this?

sigmundch commented 2 years ago

/cc @rakudrama

I too feel that maybe this is should have been recognized by our error streams in dart:html. Today it exposes the errors as they are, but we know they could have originated from Dart, in which case, it seems reasonable that the API would provide them transparently as Dart errors.

That said, generally we have too much magic in our dart:html APIs. Yet another option to consider here is whether we could expose the unwrapping somehow. For example, by allowing Error to be a static interop type, and expose a getter with the underlying Dart error. Maybe this is not an option for dart:html that intends to be batteries included, but that may be the option for how one using JS-interop to access browser event streams could manually unwrap when needed.

kealjones-wk commented 2 years ago

I do like the idea of something clean like the RTI is check and as cast but id also be fine with a function in a dart JS library. The original issue I posted doesn't use any dart:html streams, the error is caught in JS and then handed to an allowInteropd function by react. So any magic to make dart:html handle their streams or whatnot better is not something I am all that interested in because this issue originated outside of that use case.

If it was a function in the dart:js_util library, maybe something similar to jsify, but in reverse, or promiseToFuture, so that we could pass a js error in and get a dartified version back without having to try/catch or throwFromJs ourselves, that would be ideal.