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

Comments

Projects
None yet
7 participants
@nex3
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

This comment has been minimized.

Show comment
Hide comment
@peter-ahe-google

peter-ahe-google Nov 19, 2013

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.

Contributor

peter-ahe-google commented Nov 19, 2013

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

This comment has been minimized.

Show comment
Hide comment
@nex3

nex3 Nov 19, 2013

Member

That works great, thanks!

Member

nex3 commented Nov 19, 2013

That works great, thanks!

@peter-ahe-google

This comment has been minimized.

Show comment
Hide comment
@efortuna

This comment has been minimized.

Show comment
Hide comment
@efortuna

efortuna Apr 11, 2014

Contributor

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

Contributor

efortuna commented Apr 11, 2014

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

@sethladd

This comment has been minimized.

Show comment
Hide comment
@sethladd

sethladd Apr 16, 2014

Member

What's the priority?

Member

sethladd commented Apr 16, 2014

What's the priority?

@nex3

This comment has been minimized.

Show comment
Hide comment
@nex3

nex3 Apr 16, 2014

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@peter-ahe-google

peter-ahe-google Apr 17, 2014

Contributor

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

Contributor

peter-ahe-google commented Apr 17, 2014

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

@efortuna

This comment has been minimized.

Show comment
Hide comment
@efortuna

efortuna Apr 17, 2014

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?

Contributor

efortuna commented Apr 17, 2014

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

This comment has been minimized.

Show comment
Hide comment
@nex3

nex3 Apr 17, 2014

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@peter-ahe-google

peter-ahe-google Apr 17, 2014

Contributor

I agree with Nathan.

Emily: I not sure those meetings exist anymore.

Contributor

peter-ahe-google commented Apr 17, 2014

I agree with Nathan.

Emily: I not sure those meetings exist anymore.

@efortuna

This comment has been minimized.

Show comment
Hide comment
@efortuna

efortuna Apr 17, 2014

Contributor

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

Contributor

efortuna commented Apr 17, 2014

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

@peter-ahe-google

This comment has been minimized.

Show comment
Hide comment
@peter-ahe-google

peter-ahe-google Apr 17, 2014

Contributor

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

Contributor

peter-ahe-google commented Apr 17, 2014

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

@kasperl

This comment has been minimized.

Show comment
Hide comment
@kasperl

kasperl Apr 23, 2014

Contributor

Marked this as being blocked by #18394.

Contributor

kasperl commented Apr 23, 2014

Marked this as being blocked by #18394.

@sigmundch

This comment has been minimized.

Show comment
Hide comment
@sigmundch

sigmundch Jun 2, 2015

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.

Member

sigmundch commented Jun 2, 2015

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

This comment has been minimized.

Show comment
Hide comment
Member

sigmundch commented Jun 2, 2015

@sigmundch

This comment has been minimized.

Show comment
Hide comment
@sigmundch

sigmundch Jun 2, 2015

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.

Member

sigmundch commented Jun 2, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sigmundch

sigmundch Jun 29, 2015

Member

Forgot to close this - this was fixed in 1763aa3

Member

sigmundch commented Jun 29, 2015

Forgot to close this - this was fixed in 1763aa3

@sigmundch sigmundch closed this Jun 29, 2015

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