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

dart2js errors have stack traces that don't remain identical over time. #15171

Closed
nex3 opened this issue Nov 19, 2013 · 17 comments
Closed

dart2js errors have stack traces that don't remain identical over time. #15171

nex3 opened this issue Nov 19, 2013 · 17 comments
Assignees

Comments

@nex3
Copy link
Member

nex3 commented Nov 19, 2013

Consider the following code:

    var st1;
    try {
      try {
        throw 'bad';
      } catch (e, st) {
        st1 = st;
        rethrow;
      }
    } catch (e, st2) {
      print(st1 == st2);
      print(identical(st1, st2));
    }

On the VM, this prints "true" and "true", but on dart2js it prints "false" and "false". This makes it difficult to compare stack traces for equality and impossible to use them in expandos, which is blocking my work on issue #7040 on dart2js.

@peter-ahe-google
Copy link
Contributor

Nathan,

Try this patch:

diff --git a/dart/sdk/lib/_internal/lib/js_helper.dart b/dart/sdk/lib/_internal/lib/js_helper.dart
index 2e350b7..7cba4fd 100644

--- a/dart/sdk/lib/_internal/lib/js_helper.dart
+++ b/dart/sdk/lib/_internal/lib/js_helper.dart
@@ -1419,7 +1419,12 @­@ unwrapException(ex) {
  * Called by generated code to fetch the stack trace from an
  * exception. Should never return null.
  */
-StackTrace getTraceFromException(exception) => new _StackTrace(exception);
+StackTrace getTraceFromException(exception) {

  • _StackTrace trace = JS('_StackTrace|Null', r'#.$cachedTrace', exception);
  • if (trace != null) return trace;
  • trace = new _StackTrace(exception);
  • return JS('_StackTrace', r'#.$cachedTrace = #', exception, trace);
    +}
     
     class _StackTrace implements StackTrace {
       var _exception;

I'm at home without VPN right now, but I'll try to get this submitted tomorrow.

Cheers,
Peter


Set owner to @peter-ahe-google.
Added Started label.

@nex3
Copy link
Member Author

nex3 commented Nov 19, 2013

That works great, thanks!

@peter-ahe-google
Copy link
Contributor

@efortuna
Copy link
Contributor

I'd like to cast a vote in support of getting this CL in!

@sethladd
Copy link
Contributor

What's the priority?

@nex3
Copy link
Member Author

nex3 commented Apr 16, 2014

I'd certainly like to see this patch land, but it's not blocking me personally since I don't do any debugging in the browser.

@peter-ahe-google
Copy link
Contributor

The CL https://codereview.chromium.org/78343002 has met surprising resistance from the VM team. So I'm stuck.

@efortuna
Copy link
Contributor

Peter, you mention on the code review that the language team would discuss to come to a decision. Do we know if they did?

@nex3
Copy link
Member Author

nex3 commented Apr 17, 2014

The objection to CL 78343002 seems to be that this isn't a guarantee we want to offer, but this guarantee is crucial to making stack chain tracking work. I contend that breaking stack chains is an unacceptable usability regression, so we should commit to guaranteeing identity equality of stack traces.

@peter-ahe-google
Copy link
Contributor

I agree with Nathan.

Emily: I not sure those meetings exist anymore.

@efortuna
Copy link
Contributor

@peter, yes, but I'm curious if it was actually discussed back when they did.

@peter-ahe-google
Copy link
Contributor

I don't think it was ever discussed. I don't recall seeing anything about it in Bob's minutes.

@kasperl
Copy link

kasperl commented Apr 23, 2014

Marked this as being blocked by #18394.

@sigmundch
Copy link
Member

I hope that we will agree on guaranteeing that the stacktraces are identical (I just pinged issue #18394 to follow up on that).

Regardless of that discussion, I'd like to push forward the change Peter proposed earlier. Doing so will make it easier for people to debug async stack traces, and it will make the dart2js consistent with the current VM behavior.

If and when we decide that we don't want those guarantees and the VM needs to change the behavior in the future, we can revisit this at that point.


cc @peter-ahe-google.
Set owner to @sigmundch.

@sigmundch
Copy link
Member

just sent out https://codereview.chromium.org/1152023004/

@sigmundch
Copy link
Member

Looks like the new language in the spec actually now requires that rethrow gives you the same stack trace object (see discussion in issue #18394), so we should be good to go.

@sigmundch
Copy link
Member

Forgot to close this - this was fixed in 1763aa3

nex3 added a commit to dart-lang/stack_trace that referenced this issue Jun 29, 2015
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

7 participants