VM: Fix asan memory leak in some standalone tests

VM: Fix asan memory leak in some standalone tests

Fixes #28350

R=vegorov@google.com

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

[kustermann, 2017-01-23 10:33:05]

kustermann@google.com changed reviewers:
+ vegorov@google.com

[kustermann, 2017-01-23 10:33:05]

/slava Should we cc someone else on these changes?

[kustermann, 2017-01-23 10:33:55]

Description was changed from

==========
VM: Fix asan memory leak in some standalone tests
==========

to

==========
VM: Fix asan memory leak in some standalone tests

Fixes 28350
==========

[kustermann, 2017-01-23 10:34:04]

Description was changed from

==========
VM: Fix asan memory leak in some standalone tests

Fixes 28350
==========

to

==========
VM: Fix asan memory leak in some standalone tests

Fixes #28350
==========

[Vyacheslav Egorov (Google), 2017-01-23 11:09:51]

I think it's better to refactor this code to avoid doing Dart_PropagateError
deep down the call-stack.

Alternative would be to introduce StackResource like API.

[kustermann, 2017-01-23 12:10:45]

PTAL

[Vyacheslav Egorov (Google), 2017-01-23 14:56:32]

LGTM

[zra, 2017-01-23 17:04:51]

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

[zra, 2017-01-23 17:04:52]

https://codereview.chromium.org/2653583002/diff/20001/runtime/bin/directory.cc
File runtime/bin/directory.cc (right):

https://codereview.chromium.org/2653583002/diff/20001/runtime/bin/directory.c...
runtime/bin/directory.cc:136: Directory::List(&sync_listing);
dart_error = sync_listing.error();

https://codereview.chromium.org/2653583002/diff/20001/runtime/bin/directory.h
File runtime/bin/directory.h (right):

https://codereview.chromium.org/2653583002/diff/20001/runtime/bin/directory.h...
runtime/bin/directory.h:220: Dart_Handle* dart_error_;
DBC:

Why is this a pointer? Why can't it be a regular field with setter/getter?

[kustermann, 2017-01-23 18:45:32]

https://codereview.chromium.org/2653583002/diff/20001/runtime/bin/directory.cc
File runtime/bin/directory.cc (right):

https://codereview.chromium.org/2653583002/diff/20001/runtime/bin/directory.c...
runtime/bin/directory.cc:136: Directory::List(&sync_listing);
On 2017/01/23 17:04:51, zra wrote:
> dart_error = sync_listing.error();

sync_listing.error() returns a bool:

  class DirectoryListing { 
    ...
    bool error() const { return error_; }
    ...
  }

https://codereview.chromium.org/2653583002/diff/20001/runtime/bin/directory.h
File runtime/bin/directory.h (right):

https://codereview.chromium.org/2653583002/diff/20001/runtime/bin/directory.h...
runtime/bin/directory.h:220: Dart_Handle* dart_error_;
On 2017/01/23 17:04:51, zra wrote:
> DBC:
> 
> Why is this a pointer? Why can't it be a regular field with setter/getter?

I made it a pointer to avoid an uninitialized variable on the call site.

But if you have a different preference, I change it of course!

[kustermann, 2017-01-23 18:45:36]

Description was changed from

==========
VM: Fix asan memory leak in some standalone tests

Fixes #28350
==========

to

==========
VM: Fix asan memory leak in some standalone tests

Fixes #28350

R=vegorov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/3f0ea0f50ee06b395d2c02f592ae8c13ba1b461d
==========

[kustermann, 2017-01-23 18:45:38]

Committed patchset #3 (id:40001) manually as
3f0ea0f50ee06b395d2c02f592ae8c13ba1b461d (presubmit successful).

[zra, 2017-01-23 18:53:15]

https://codereview.chromium.org/2653583002/diff/20001/runtime/bin/directory.cc
File runtime/bin/directory.cc (right):

https://codereview.chromium.org/2653583002/diff/20001/runtime/bin/directory.c...
runtime/bin/directory.cc:136: Directory::List(&sync_listing);
On 2017/01/23 18:45:32, kustermann wrote:
> On 2017/01/23 17:04:51, zra wrote:
> > dart_error = sync_listing.error();
> 
> sync_listing.error() returns a bool:
> 
>   class DirectoryListing { 
>     ...
>     bool error() const { return error_; }
>     ...
>   }

By sync_listing.error() I meant the field added to SyncDirectoryListing in lieu
of dart_error. That is I mean that here you would invoke the getter for the new
field.

https://codereview.chromium.org/2653583002/diff/20001/runtime/bin/directory.h
File runtime/bin/directory.h (right):

https://codereview.chromium.org/2653583002/diff/20001/runtime/bin/directory.h...
runtime/bin/directory.h:220: Dart_Handle* dart_error_;
On 2017/01/23 18:45:32, kustermann wrote:
> On 2017/01/23 17:04:51, zra wrote:
> > DBC:
> > 
> > Why is this a pointer? Why can't it be a regular field with setter/getter?
> 
> I made it a pointer to avoid an uninitialized variable on the call site.
> 
> But if you have a different preference, I change it of course!

Not sure what you mean by this. What call site?

[kustermann, 2017-01-23 18:55:44]

https://codereview.chromium.org/2653583002/diff/20001/runtime/bin/directory.h
File runtime/bin/directory.h (right):

https://codereview.chromium.org/2653583002/diff/20001/runtime/bin/directory.h...
runtime/bin/directory.h:220: Dart_Handle* dart_error_;
> What call site?

Line 128 here 
https://codereview.chromium.org/2653583002/diff/40001/runtime/bin/directory.cc