Make root-zone handleUncaughtError rethrow with the correct stack.

Make root-zone handleUncaughtError rethrow with the correct stack.

Adds _rethrow function which reifies the stack trace of an async
error on the VM. There is no corresponding JS functionality, it still
just throws an AsyncError wrapper.

Fixes issue #24163
BUG= http://dartbug.com/24163
R=iposva@google.com, turnidge@google.com

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

[Lasse Reichstein Nielsen, 2015-10-02 07:52:48]

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

[nweiz, 2015-10-02 20:35:23]

👍

[turnidge, 2015-10-02 21:00:53]

lgtm w/ 2 comments.

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (left):

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/async/zone.dart#old...
sdk/lib/async/zone.dart:1030: }
What's all this about?

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:894: if (error == null) error = new NullThrownError();
Use {}'s here and below.

[Lasse Reichstein Nielsen, 2015-10-02 22:50:59]

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (left):

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/async/zone.dart#old...
sdk/lib/async/zone.dart:1030: }
My editor told me the class was unused, so I removed it. Pure clean-up
while-I-was-here.

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:894: if (error == null) error = new NullThrownError();
We don't generally do that (the surrounding code is full of other examples).

[turnidge, 2015-10-02 22:54:35]

lgtm

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:894: if (error == null) error = new NullThrownError();
On 2015/10/02 22:50:59, Lasse Reichstein Nielsen wrote:
> We don't generally do that (the surrounding code is full of other examples).

Acknowledged.

[Ivan Posva, 2015-10-02 23:40:39]

iposva@google.com changed reviewers:
+ iposva@google.com

[Ivan Posva, 2015-10-02 23:40:39]

LGTM with comments.

-Ivan

https://codereview.chromium.org/1383983002/diff/1/runtime/lib/async_patch.dart
File runtime/lib/async_patch.dart (right):

https://codereview.chromium.org/1383983002/diff/1/runtime/lib/async_patch.dar...
runtime/lib/async_patch.dart:186: patch void _rethrow(Object error, StackTrace
stackTrace) native "Zone_rethrow";
Strictly speaking this native should be named "Async_rethrow" since it is a
top-level method in the async library and not a method in the Zone class.

Any particular reason why it has to be a top-level method?

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

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/async_patch.dart:512: throw new
AsyncError(error, stackTrace);
This seems to differ significantly from the VM implementation. How come? Are we
really willing to have this entirely different behaviour between the two
implementations?

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:900: external void _rethrow(Object error, StackTrace
stackTrace);
Nit: I think it would be better to match the signature of the calling function
which does not have Object as the error being thrown which is more correct in my
opinion.

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

Committed patchset #1 (id:1) manually as
da8baacae46e5f1f00d90ada1fa85dc0a9e0286c (presubmit successful).

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

https://codereview.chromium.org/1383983002/diff/1/runtime/lib/async_patch.dart
File runtime/lib/async_patch.dart (right):

https://codereview.chromium.org/1383983002/diff/1/runtime/lib/async_patch.dar...
runtime/lib/async_patch.dart:186: patch void _rethrow(Object error, StackTrace
stackTrace) native "Zone_rethrow";
Renamed to Async_rethrow.
It's top-level because it's called from another top-level function. There isn't
any class that it would obviously belong to.
I considered Error, but it's in a different library, so it would have to be
public, and then we would have to implement it in JavaScript too, which isn't
obvious to me how to do (even though having the functionality public would be
awesome!)

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

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/async_patch.dart:512: throw new
AsyncError(error, stackTrace);
I haven't found a way to throw an object *and* a stack-trace in JS.
Maybe there is a way, in which case we can add it here.

The behavior is (so far) only used for re-throwing an uncaught async error, so
the only thing that happens with the thrown error is that it's printed in the
console, or its string representations are captured by an isolate. This error
object has the same string representation as the original.

The VM version just got better, and that's great. Even if we can't make the JS
version better, it's still an improvement.

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/1383983002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:900: external void _rethrow(Object error, StackTrace
stackTrace);
I think Object is the right type here - it's accepting any object, and we won't
use it dynamically, so Object is the precise type. 

I also think that the Zone's error object should have been typed as Object, but
I guess that's too late to fix.