Make dart2js code retain the original stack trace for uncaught async errors.

Make dart2js code retain the original stack trace for uncaught async errors.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/5363e5c9453b096900d3a9bbc37012a4ea65f754

[Lasse Reichstein Nielsen, 2015-10-05 08:08:05]

lrn@google.com changed reviewers:
+ sigmund@google.com

[Lasse Reichstein Nielsen, 2015-10-05 08:08:06]

This tries to match the VM's new behavior, where an uncaught async error retains
its original stack trace when rethrown at top level.

[Siggi Cherem (dart-lang), 2015-10-05 17:09:33]

Thanks Lasse!

could you add a test to ensure we get the expected behavior across different
browsers?

https://codereview.chromium.org/1383213002/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/async_patch.dart (right):

https://codereview.chromium.org/1383213002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/async_patch.dart:514: JS("void", "#.stack = #",
error, stackTrace.toString());
I'm not super familiar with this logic yet, but I wonder if we need to do this
assignment of the stack, or if we can only set the cachedTrace property like we
did with the sync rethrow?
(https://github.com/dart-lang/sdk/blob/8697392367be57f9f595abca0cd106c2f3e56f3...)

One of my concerns on setting stack is that I recall IE sets the stack after the
throw, so I'm not sure if it will override our stack in that case?

[Lasse Reichstein Nielsen, 2015-10-06 07:57:26]

I will try to add a test (in the browser it might actually be possible to
capture an "uncaught" error).

Hmm, maybe I can do the same thing with isolates in the VM.

https://codereview.chromium.org/1383213002/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/async_patch.dart (right):

https://codereview.chromium.org/1383213002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/async_patch.dart:514: JS("void", "#.stack = #",
error, stackTrace.toString());
As I understand it, the "generally accepted" behavior is that if you actively
assign to the stack property, it's not automatically overwritten after that.

This seems to work the same in Chrome, Firefox and IE 11 (I can't test in Safari
or earlier IEs).

Test:
  function test() {
    var e = new Error();
    e.stack = "Arglebargle glop glyf";
    throw e;
  }
  test();
Then check the stack of the throw error.

[Lasse Reichstein Nielsen, 2015-10-14 14:54:50]

I am having difficulties creating a test that tests the contents of an uncaught
error.
My attempts at using js-integration to override window.onerror are not
successful (maybe because the testing framework for browsers also override that
one).
Suggestions are welcome!

[Siggi Cherem (dart-lang), 2015-10-14 20:18:39]

Good point, window.onerror is used by the test_controller as well.

Some random ideas:

a) we could change the test_controller.js
(https://github.com/dart-lang/sdk/blob/master/tools/testing/dart/test_controll...)
to call the previous callback is one exists:
  var old = window.onerror;
  window.onerror = function (...) { old(...); ... }

b) we could add a "js-zone" on the test. If our implementation of zones relies
on using certain JS APIs that can be monkey-patched, then we can replace the JS
definition to include some extra logic to wrap the callback and invoke it around
a try-catch block. This is sort of achieving a js-zone, but it would be quite
hacky given that we need to know internals of how our zones are mapped to JS. At
least the hack would be contained to just this test though.

[Lasse Reichstein Nielsen, 2015-11-16 13:40:55]

Finally made a test that runs. PTAL!

[Siggi Cherem (dart-lang), 2015-11-16 17:15:50]

lgtm!

[Lasse Reichstein Nielsen, 2015-11-17 14:17:15]

Committed patchset #2 (id:20001) manually as
5363e5c9453b096900d3a9bbc37012a4ea65f754 (presubmit successful).