Skip to content

DDC doesn't offer a way to log formatted errors/traces #33331

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

Closed
2 tasks
matanlurey opened this issue Jun 5, 2018 · 27 comments
Closed
2 tasks

DDC doesn't offer a way to log formatted errors/traces #33331

matanlurey opened this issue Jun 5, 2018 · 27 comments

Comments

@matanlurey
Copy link
Contributor

This issue is blocking resolution of angulardart/angular#1367.

Almost everything can be viewed at angulardart/angular#1367, but basically:

  • throw should log a nice JavaScript error and stack trace by default.
  • Logging an exception/stack trace manually (AngularDart does this) should be formatted.

For example, I'd like the output in the browser to look like this when we are done:

screen shot 2018-06-04 at 6 53 46 pm

(Obviously not minified, this is just an example from Dart2JS that works fine)

Instead of this:

image

@matanlurey
Copy link
Contributor Author

As mentioned by @vsmenon - this is fixed if I enable "Custom Formatters" in devtools:

http://www.mattzeunert.com/2016/02/19/custom-chrome-devtools-object-formatters.html

@matanlurey
Copy link
Contributor Author

Actually will reopen because I'm confused how Dart2JS does this for free.

@matanlurey matanlurey reopened this Jun 5, 2018
@matanlurey
Copy link
Contributor Author

Ping. Is this a bug? Intentional? Something else?

@vsmenon
Copy link
Member

vsmenon commented Jun 5, 2018

@jmesserly

It's not exactly free. Dart2JS is wrapping all throws in a JS Error object and unwrapping on a catch. Chrome dev tools (but not node) formats the native Error in a specific way.

DDC could do similar, though it would increase code size on throw / catch.

@matanlurey
Copy link
Contributor Author

Do we know what the cost is?

Seems a bit suspect that it is OK for production but not the dev compiler.

@jmesserly
Copy link

It's a question of JS interop spec: when a Dart object is thrown, what should JS see (and vice versa)? Do we wrap all thrown Dart objects in a JS Error, or only some of them (e.g. only things that extend or implement dart:core Error/Exception)?

The DDC/Dart 2 approach is to use wrapperless interop when possible. There are many benefits to that in general.

I think we hit a problem here because most Dart Error/Exception types do not correspond to JS Error. I think there may be a mismatch in stack traces between JS and Dart, which is why we kept them distinct (I think JS Errors capture stack at creation, rather than when thrown).

Considering all of that, the simplest approach is probably to wrap all throws. I don't think it will be a noticeable perf issue because IIRC, we already create JS Error objects anyways for stack traces.

@matanlurey
Copy link
Contributor Author

matanlurey commented Jun 6, 2018

Thanks @jmesserly!

It's a question of JS interop spec:

Which I'd also love ;)

when a Dart object is thrown, what should JS see (and vice versa)? Do we wrap all thrown Dart objects in a JS Error, or only some of them (e.g. only things that extend or implement dart:core Error/Exception)?

Hard to answer that question definitively. Here are some questions:

  • How useful is it to throw something that doesn't implement Error or Exception
  • If we support other arbitrary objects, do users expect those to be valid JS errors

My guess is making all Dart errors JS errors feels "right", and where/when it isn't possible, we can just not do it. Personally this is part of the language that doesn't make a lot of sense to me - I can sort of see throw <string> as useful if its just sugar for throw new Error(string), similar to the callable classes de-sugaring.

The DDC/Dart 2 approach is to use wrapperless interop when possible. There are many benefits to that in general.

Could you explain in 1 or 2 sentences what that is and why that benefits users?

I think we hit a problem here because most Dart Error/Exception types do not correspond to JS Error.
I think there may be a mismatch in stack traces between JS and Dart, which is why we kept them distinct (I think JS Errors capture stack at creation, rather than when thrown).

Considering all of that, the simplest approach is probably to wrap all throws. I don't think it will be a noticeable perf issue because IIRC, we already create JS Error objects anyways for stack traces.

My read of this is you agree that Dart2JS and DDC should have similar semantics here?

@jmesserly
Copy link

The DDC/Dart 2 approach is to use wrapperless interop when possible. There are many benefits to that in general.

Could you explain in 1 or 2 sentences what that is and why that benefits users?

Generally: wrapperless interop performs better, allows passing complex structures (e.g. objects of user-defined classes), provides identity semantics, allows incremental porting and/or hybrid apps (because each language can express all of the necessary operations on the same shared data structures). It also makes writing the interop API signatures easier.

My read of this is you agree that Dart2JS and DDC should have similar semantics here?

Yes.

@matanlurey
Copy link
Contributor Author

Thanks!

@matanlurey
Copy link
Contributor Author

This fell off a bit, /cc @vsmenon for now. How can we act on this?

@vsmenon
Copy link
Member

vsmenon commented Aug 8, 2018

We should be encouraging folks to enable custom formatters immediately (per https://webdev.dartlang.org/guides/debugging).

Flagging for @jmesserly to update when she's back. I suspect we'll want to do this in conjunction with #34060.

@matanlurey
Copy link
Contributor Author

Any more action here?

@jmesserly
Copy link

Any more action here?

Sort of :). I started taking a look at this yesterday as I was reviewing bugs. Will update when I have a CL ready.

@matanlurey
Copy link
Contributor Author

Thanks! Just doing a bug scrub - there is an usptream google3 bug blocked on this one.

@jmesserly
Copy link

I'm juggling a lot of things right now :), but I did manage to finally upload my fix for this: https://dart-review.googlesource.com/c/sdk/+/84446

@zolotyh
Copy link

zolotyh commented Nov 21, 2018

Hi, I have a problem with the formatting of stack traces. I am using stagehand (stagehand web-simple) for creating a simple project and webdev serve for running it. I have unformatted traces.

example code:

void main(){
    throw '123123123';
 }

screenshot 2018-11-21 17 11 12

What I am doing worng? FYI @matanlurey

@matanlurey
Copy link
Contributor Author

Ping @jmesserly 😛

dart-bot pushed a commit that referenced this issue Dec 13, 2018
This improves the default JS display of exceptions/errors from DDC
compiled code. This gives a better "default" experience if JS code
(or a JS engine, like browers/Node.js) ends up catching Dart exceptions.

Change-Id: Ib2dda6eee710f8b536d5ed7223e0101310a137b3
Reviewed-on: https://dart-review.googlesource.com/c/84446
Commit-Queue: Jenny Messerly <[email protected]>
Reviewed-by: Vijay Menon <[email protected]>
@jmesserly
Copy link

jmesserly commented Dec 13, 2018

I landed a CL that changes DDC to throw JS errors--they'll be better understood by JS tools, etc.

Tracking of the Dart StackTrace object is improved for async and async* functions (there were some circumstances where the original trace could get dropped).

That sounds like it'll address your original issue, but I'm not 100% sure (were there other things you wanted?)

@jmesserly jmesserly removed their assignment Dec 13, 2018
@jmesserly jmesserly added the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Dec 13, 2018
@jmesserly
Copy link

Added "needs-info" ... LMK if there's anything else you need (or, should we open another issue for additional requests?)

@matanlurey
Copy link
Contributor Author

I landed a CL that changes DDC to throw JS errors--they'll be better understood by JS tools, etc.

Thanks! Is that expected to make it into google3 anytime soon? If so, we can close this!

@jmesserly
Copy link

SGTM! thank you

@jmesserly
Copy link

edit: yeah it'll make it in the next roll; or it may have already happened (I'm not sure what the holiday schedule was). But in any case, yeah, soon.

@jmesserly jmesserly removed the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Jan 10, 2019
@jmesserly jmesserly reopened this Jan 10, 2019
@jmesserly
Copy link

sadly need to revert this for now, there are some (very obscure) failures in a few tests.

I wasn't able to make too much sense of it, but it looks like code may be relying on DDC's former, incorrect behavior. When async code loses a stack trace, and passes in explicit "null" where a trace is expected, we used to keep the old stacktrace (the thrown object stored it). That's not technically correct behavior in DDC, but it's useful.

Perhaps I can back out of some parts of the CL, while keeping the "always throw JSError" part. Needs more investigation.

@matanlurey
Copy link
Contributor Author

😢

I wasn't able to make too much sense of it, but it looks like code may be relying on DDC's former, incorrect behavior.

What code? Clients in google3? The SDK itself?

If its the latter, I don't mind helping to debug.

@jmesserly
Copy link

It wasn't clear who was passing in the null, it was an async error so the origin got lost :\ ... and any breakpoints or "break on exception" was making the bug go away. It could be some code in DDC's SDK impl, I'm going through that to see if anything jumps out. Also I'm trying some "shot in the dart" type of fixes.

(That might not even be the root cause, but it is odd. A future then handler should never be passed "null" as the stack trace.)

@jmesserly
Copy link

wow, spoke too soon, turns out my "shot in the dark" may have worked. Either the "preserve stacktrace when null is passed" or the "preserve original stack trace, even when throw is used" fixed that test.

@jmesserly
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants