Dart: Better handle leak checks. close() is async.

Dart: Better handle leak checks. close() is async.

This change adds tracking of MojoHandle creation and closing in Debug
mode, and makes it possible to emit a log of leaked handles on Mojo app
shutdown. If any handles are leaked, causing the finalizer to need to
close a handle, an error message is emitted with LOG(ERROR). All of the
handles created by an isolate are also closed on an unhandled exception.
This should prevent hangs.

To aid tracking handles, close() now returns a Future in most cases.
This is because a call to close() will generally send a command to the
handle watcher, and so the actual close() of the underlying handle
happens asynchronously. In particular when the future for a call to
close() on an Application resolves, we should be able to do:

assert(MojoHandle.reportLeakedHandles());

This change also fixes analyzer warnings and removes the special case
to ignore warnings from the analyzer's wrapper script.

BUG=
R=erg@chromium.org, sky@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/b7959afdc4fa4acb65c67cafa6985394633138c8

[zra, 2015-03-10 20:59:44]

zra@google.com changed reviewers:
+ erg@chromium.org, sky@chromium.org

[zra, 2015-03-10 20:59:45]

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

https://codereview.chromium.org/996923003/diff/20001/mojo/dart/embedder/test/...
mojo/dart/embedder/test/run_dart_tests.cc:124: TEST(DartTest, interface_test) {
This test doesn't make sense anymore now that bindings code generation is more
stable.

[zra, 2015-03-10 21:00:15]

+johnmccutchan

[Elliot Glaysher, 2015-03-10 21:11:02]

lgtm

(I was JUST about to send out https://codereview.chromium.org/998693004/ , which
is a subset of this patch with fixes the warnings.)

https://codereview.chromium.org/996923003/diff/20001/mojo/public/tools/dart_a...
File mojo/public/tools/dart_analyze.py (right):

https://codereview.chromium.org/996923003/diff/20001/mojo/public/tools/dart_a...
mojo/public/tools/dart_analyze.py:24: r'^[0-9]+ errors( and [0-9]+ warnings)*
found.')
You probably also want to add ? to the two 's' characters.

[zra, 2015-03-10 21:28:38]

Thanks!

https://codereview.chromium.org/996923003/diff/20001/mojo/public/tools/dart_a...
File mojo/public/tools/dart_analyze.py (right):

https://codereview.chromium.org/996923003/diff/20001/mojo/public/tools/dart_a...
mojo/public/tools/dart_analyze.py:24: r'^[0-9]+ errors( and [0-9]+ warnings)*
found.')
On 2015/03/10 21:11:02, Elliot Glaysher wrote:
> You probably also want to add ? to the two 's' characters.

I just took the regexes from your CL =)

[sky, 2015-03-10 21:44:56]

Is there a reason close needs to be async? It's sync on the C++ side to avoid
races.

[zra, 2015-03-10 23:12:16]

On 2015/03/10 21:44:56, sky wrote:
> Is there a reason close needs to be async? It's sync on the C++ side to avoid
> races.

Dart's handle watcher may have ownership of the handle in which case we close by
sending a command to the handle watcher isolate. close() can't block while the
handle watcher receives the command and does the close. The "Dart" way to do
things in this situation is to return a Future.

Even if the application isolate took ownership of the handle from the handle
watcher and synchronously called close, we'd still have to deal with the
deferred close in Stub, which finishes sending response messages after close()
is called before doing the actual close() of the underlying handle. Rather than
doing this, it's probably easier to keep things uniform and have all the close()
calls be asynchronous.

With the Dart bindings, there is only the possibility of a race when doing the
close() of the Application before exiting the app. In this case we want to make
sure the underlying handles are closed before asserting that no handles have
leaked.

[sky, 2015-03-11 16:04:46]

On 2015/03/10 23:12:16, zra wrote:
> On 2015/03/10 21:44:56, sky wrote:
> > Is there a reason close needs to be async? It's sync on the C++ side to
avoid
> > races.
> 
> Dart's handle watcher may have ownership of the handle in which case we close
by
> sending a command to the handle watcher isolate. close() can't block while the
> handle watcher receives the command and does the close. The "Dart" way to do
> things in this situation is to return a Future.
> 
> Even if the application isolate took ownership of the handle from the handle
> watcher and synchronously called close, we'd still have to deal with the
> deferred close in Stub, which finishes sending response messages after close()
> is called before doing the actual close() of the underlying handle. Rather
than
> doing this, it's probably easier to keep things uniform and have all the
close()
> calls be asynchronous.

What do you mean by response messages?

It seems to me all this should be sync to avoid races that we've had in the C++
side before making it all sync. For example, what happens know if I want to take
a handle and pass in some where else? If it's async, what happens to any
inflight messages that are received before the async take is processed?

> 
> With the Dart bindings, there is only the possibility of a race when doing the
> close() of the Application before exiting the app. In this case we want to
make
> sure the underlying handles are closed before asserting that no handles have
> leaked.

[zra, 2015-03-11 16:13:58]

On 2015/03/11 16:04:46, sky wrote:
> What do you mean by response messages?

The stuff on the right-hand side of a => in a mojom interface.

> It seems to me all this should be sync to avoid races that we've had in the
C++
> side before making it all sync. For example, what happens know if I want to
take
> a handle and pass in some where else? If it's async, what happens to any
> inflight messages that are received before the async take is processed?

In the Dart bindings, when a handle is passed, messages will be read from the
endpoint until PEER_CLOSED is set as a result of passing the handle. I think
might not be understanding the concern. I'm headed over for the meeting now.
Let's chat in person before/after.

[sky, 2015-03-11 17:17:57]

https://codereview.chromium.org/996923003/diff/40001/mojo/dart/embedder/isola...
File mojo/dart/embedder/isolate_data.h (left):

https://codereview.chromium.org/996923003/diff/40001/mojo/dart/embedder/isola...
mojo/dart/embedder/isolate_data.h:9: #include <string.h>
AFAIK if you use std::string then you should include <string>

https://codereview.chromium.org/996923003/diff/40001/mojo/dart/embedder/isola...
File mojo/dart/embedder/isolate_data.h (right):

https://codereview.chromium.org/996923003/diff/40001/mojo/dart/embedder/isola...
mojo/dart/embedder/isolate_data.h:45: std::set<int64_t> unclosed_handles;
Is there a reason you don't make this MojoHandle?

https://codereview.chromium.org/996923003/diff/40001/mojo/dart/embedder/mojo_...
File mojo/dart/embedder/mojo_natives.cc (right):

https://codereview.chromium.org/996923003/diff/40001/mojo/dart/embedder/mojo_...
mojo/dart/embedder/mojo_natives.cc:129: LOG(ERROR) << "Handle Finalizer closing
handle:\n\tisolate: "
Why the LOG(ERROR) here? Doesn't MOJO_RESULT_OK mean the close was successful?

https://codereview.chromium.org/996923003/diff/40001/mojo/public/dart/src/han...
File mojo/public/dart/src/handle.dart (right):

https://codereview.chromium.org/996923003/diff/40001/mojo/public/dart/src/han...
mojo/public/dart/src/handle.dart:108: assert(false);
Ick.

https://codereview.chromium.org/996923003/diff/40001/mojo/public/dart/src/han...
mojo/public/dart/src/handle.dart:115: return true;
Is there a reason you have a return value here since it always seems to be true?

https://codereview.chromium.org/996923003/diff/40001/mojo/public/dart/src/han...
mojo/public/dart/src/handle.dart:120: return true;
Same comment here about always true return value.

[zra, 2015-03-11 18:58:20]

https://codereview.chromium.org/996923003/diff/40001/mojo/dart/embedder/isola...
File mojo/dart/embedder/isolate_data.h (left):

https://codereview.chromium.org/996923003/diff/40001/mojo/dart/embedder/isola...
mojo/dart/embedder/isolate_data.h:9: #include <string.h>
On 2015/03/11 17:17:57, sky wrote:
> AFAIK if you use std::string then you should include <string>

Added.

https://codereview.chromium.org/996923003/diff/40001/mojo/dart/embedder/isola...
File mojo/dart/embedder/isolate_data.h (right):

https://codereview.chromium.org/996923003/diff/40001/mojo/dart/embedder/isola...
mojo/dart/embedder/isolate_data.h:45: std::set<int64_t> unclosed_handles;
On 2015/03/11 17:17:57, sky wrote:
> Is there a reason you don't make this MojoHandle?

Ah. I was apparently trying to avoid some extra casting code at use sites, but
on second look that doesn't make sense. Changed to MojoHandle.

https://codereview.chromium.org/996923003/diff/40001/mojo/dart/embedder/mojo_...
File mojo/dart/embedder/mojo_natives.cc (right):

https://codereview.chromium.org/996923003/diff/40001/mojo/dart/embedder/mojo_...
mojo/dart/embedder/mojo_natives.cc:129: LOG(ERROR) << "Handle Finalizer closing
handle:\n\tisolate: "
On 2015/03/11 17:17:57, sky wrote:
> Why the LOG(ERROR) here? Doesn't MOJO_RESULT_OK mean the close was successful?

If the finalizer callback successfully closes a handle, it means that the handle
was leaked from the Dart code. I'll add a comment.

https://codereview.chromium.org/996923003/diff/40001/mojo/public/dart/src/han...
File mojo/public/dart/src/handle.dart (right):

https://codereview.chromium.org/996923003/diff/40001/mojo/public/dart/src/han...
mojo/public/dart/src/handle.dart:108: assert(false);
On 2015/03/11 17:17:57, sky wrote:
> Ick.

In case someone accidentally uses one of these calls outside of an assert() in
Release mode this assert() will be ignored, therefore not triggering an
exception and the expense of generating a stack trace for the catch {}. If that
doesn't seem like a big concern, I can change this to just "throw 'whatever'".

https://codereview.chromium.org/996923003/diff/40001/mojo/public/dart/src/han...
mojo/public/dart/src/handle.dart:115: return true;
On 2015/03/11 17:17:57, sky wrote:
> Is there a reason you have a return value here since it always seems to be
true?

Making these calls only inside of assert() statements means that they will only
run in a Debug build. Since they go inside of an assert() they return true so
that the assert() doesn't trigger.

https://codereview.chromium.org/996923003/diff/40001/mojo/public/dart/src/han...
mojo/public/dart/src/handle.dart:120: return true;
On 2015/03/11 17:17:57, sky wrote:
> Same comment here about always true return value.

Same reason.

[sky, 2015-03-11 19:17:24]

LGTM

https://codereview.chromium.org/996923003/diff/40001/mojo/public/dart/src/han...
File mojo/public/dart/src/handle.dart (right):

https://codereview.chromium.org/996923003/diff/40001/mojo/public/dart/src/han...
mojo/public/dart/src/handle.dart:115: return true;
On 2015/03/11 18:58:20, zra wrote:
> On 2015/03/11 17:17:57, sky wrote:
> > Is there a reason you have a return value here since it always seems to be
> true?
> 
> Making these calls only inside of assert() statements means that they will
only
> run in a Debug build. Since they go inside of an assert() they return true so
> that the assert() doesn't trigger.

Ah, tricky. Got it.

[zra, 2015-03-11 20:55:54]

zra@google.com changed reviewers:
+ jamesr@chromium.org

[zra, 2015-03-11 20:55:54]

+jamesr

The ASAN try bot is failing on dart unit tests. I have been unable to reproduce
locally. It looks like maybe the bot is just low on memory? Is that something
that happens? How should I proceed?

Thanks!

[jamesr, 2015-03-11 21:28:52]

How much memory does this test consume on your machine?

I'm not sure off the top of my head what memory requirements asan has. It's not
trying to allocate all that much when it fails but it looks like it's trying to
use a pretty narrow band of virtual address space.

[zra, 2015-03-11 23:04:38]

https://codereview.chromium.org/996923003/diff/100001/mojo/public/dart/src/ha...
File mojo/public/dart/src/handle.dart (right):

https://codereview.chromium.org/996923003/diff/100001/mojo/public/dart/src/ha...
mojo/public/dart/src/handle.dart:108: assert(false);
I narrowed the problem down to here. Dart exception throwing and ASAN apparently
don't agree with each other under Mojo's ASAN flags. Memory usage explodes.
Sometimes tests pass, and sometimes my machine goes deep into swap and grinds to
a halt or crashes on OOM. koda@ is out today, but I'll have to debug with him
when he gets back.

After turning off assert()s in Dart unittests, as I've now added to this CL,
there are no new bugs introduced wrt ASAN by this change.

[zra, 2015-03-12 16:18:26]

Committed patchset #7 (id:120001) manually as
b7959afdc4fa4acb65c67cafa6985394633138c8 (presubmit successful).