Dart: Removes C++ set for closing handles on an unhandled exception.

Dart: Removes C++ set for closing handles on an unhandled exception.

Replaces it with a call back into Dart from the embedder's
unhandled exception callback. This has the beneficial side-effect
of decoupling closing handles on an unhandled exception from
registration of finalizers on handles for leak detection.

fixes #478

R=johnmccutchan@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/241ff8a72dad01717aceac49af8f62bedf1218a2

[zra, 2015-10-24 02:22:46]

zra@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2015-10-26 17:10:03]

https://codereview.chromium.org/1411843005/diff/20001/mojo/dart/embedder/dart...
File mojo/dart/embedder/dart_controller.cc (right):

https://codereview.chromium.org/1411843005/diff/20001/mojo/dart/embedder/dart...
mojo/dart/embedder/dart_controller.cc:470: // TODO(zra): Instead of passing an
error handle, it may make life easier
Is this TODO still relevant?

https://codereview.chromium.org/1411843005/diff/20001/mojo/dart/embedder/test...
File mojo/dart/embedder/test/run_dart_tests.cc (right):

https://codereview.chromium.org/1411843005/diff/20001/mojo/dart/embedder/test...
mojo/dart/embedder/test/run_dart_tests.cc:72: EXPECT_GT(closed_handles, 0);
Do we want to pass in the exact number of closed handles we expect or is it not
deterministic?

https://codereview.chromium.org/1411843005/diff/20001/mojo/public/dart/mojo/s...
File mojo/public/dart/mojo/sdk_ext/src/natives.dart (right):

https://codereview.chromium.org/1411843005/diff/20001/mojo/public/dart/mojo/s...
mojo/public/dart/mojo/sdk_ext/src/natives.dart:12: static Set<int>
_unclosedHandles = new Set<int>();
This is redundant with the code in handle.dart. We are now keeping two sets of
unclosed handles. One under assert the other (being added in this CL) is
unconditional.

This set should become the  map from handle  to _HandleCreationRecord.
_HandleCreationRecord can still only grab the stack trace under certain
conditions (assert for now).

Then you can kill the code in handle.dart (or vice versa).

[zra, 2015-10-26 17:24:55]

Description was changed from

==========
Dart: Removes C++ set for closing handles on an unhandled exception.

Replaces it with a call back into Dart from the embedder's
unhandled exception callback. This has the beneficial side-effect
of decoupling closing handles on an unhandled exception from
registration of finalizers on handles for leak detection.

BUG=
==========

to

==========
Dart: Removes C++ set for closing handles on an unhandled exception.

Replaces it with a call back into Dart from the embedder's
unhandled exception callback. This has the beneficial side-effect
of decoupling closing handles on an unhandled exception from
registration of finalizers on handles for leak detection.

fixes #478
==========

[zra, 2015-10-26 17:53:54]

https://codereview.chromium.org/1411843005/diff/20001/mojo/dart/embedder/dart...
File mojo/dart/embedder/dart_controller.cc (right):

https://codereview.chromium.org/1411843005/diff/20001/mojo/dart/embedder/dart...
mojo/dart/embedder/dart_controller.cc:470: // TODO(zra): Instead of passing an
error handle, it may make life easier
On 2015/10/26 17:10:03, Cutch wrote:
> Is this TODO still relevant? 

No. Removed.

https://codereview.chromium.org/1411843005/diff/20001/mojo/dart/embedder/test...
File mojo/dart/embedder/test/run_dart_tests.cc (right):

https://codereview.chromium.org/1411843005/diff/20001/mojo/dart/embedder/test...
mojo/dart/embedder/test/run_dart_tests.cc:72: EXPECT_GT(closed_handles, 0);
On 2015/10/26 17:10:03, Cutch wrote:
> Do we want to pass in the exact number of closed handles we expect or is it
not
> deterministic? 

Done.

https://codereview.chromium.org/1411843005/diff/20001/mojo/public/dart/mojo/s...
File mojo/public/dart/mojo/sdk_ext/src/natives.dart (right):

https://codereview.chromium.org/1411843005/diff/20001/mojo/public/dart/mojo/s...
mojo/public/dart/mojo/sdk_ext/src/natives.dart:12: static Set<int>
_unclosedHandles = new Set<int>();
On 2015/10/26 17:10:03, Cutch wrote:
> This is redundant with the code in handle.dart. We are now keeping two sets of
> unclosed handles. One under assert the other (being added in this CL) is
> unconditional.
> 
> This set should become the  map from handle  to _HandleCreationRecord.
> _HandleCreationRecord can still only grab the stack trace under certain
> conditions (assert for now).
> 
> Then you can kill the code in handle.dart (or vice versa).

Yah. This CL was getting big, so I'm going to do this in two CLs. Added TODO in
handle.dart.

[zra, 2015-10-27 18:30:48]

ping

[Cutch, 2015-10-27 20:48:54]

lgtm with one comment

https://codereview.chromium.org/1411843005/diff/40001/mojo/dart/embedder/test...
File mojo/dart/embedder/test/run_dart_tests.cc (right):

https://codereview.chromium.org/1411843005/diff/40001/mojo/dart/embedder/test...
mojo/dart/embedder/test/run_dart_tests.cc:73: EXPECT_EQ(closed_handles,
expected_unclosed_handles);
Lines 71-76 can be replaced with:

EXPECT_EQ(expected_unhandled_exception, unhandled_exception);
EXPECT_EQ(expected_unclosed_handles, closed_handles);

[zra, 2015-10-27 22:05:58]

Thanks!

https://codereview.chromium.org/1411843005/diff/40001/mojo/dart/embedder/test...
File mojo/dart/embedder/test/run_dart_tests.cc (right):

https://codereview.chromium.org/1411843005/diff/40001/mojo/dart/embedder/test...
mojo/dart/embedder/test/run_dart_tests.cc:73: EXPECT_EQ(closed_handles,
expected_unclosed_handles);
On 2015/10/27 20:48:54, Cutch wrote:
> Lines 71-76 can be replaced with:
> 
> EXPECT_EQ(expected_unhandled_exception, unhandled_exception);
> EXPECT_EQ(expected_unclosed_handles, closed_handles);

Done.

[zra, 2015-10-27 22:06:16]

Committed patchset #4 (id:60001) manually as
241ff8a72dad01717aceac49af8f62bedf1218a2 (presubmit successful).