Fixes leak of native File objects.

Fixes leak of native File objects.

Also employs the same reference counting technique as secure sockets to
avoid the IO Service touching dangling pointers.

R=iposva@google.com

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

[zra, 2016-04-14 20:59:58]

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

[zra, 2016-04-19 21:41:21]

Fixed dart2js patch file. ptal.

[Ivan Posva, 2016-04-20 05:31:07]

LGTM with comments.

-Ivan

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

https://codereview.chromium.org/1892623002/diff/20001/runtime/bin/file.cc#new...
runtime/bin/file.cc:120: Dart_Handle dart_this = Dart_GetNativeArgument(args,
0);
Can you rely on the Dart code to do the disassociation with the native object
instead?

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

https://codereview.chromium.org/1892623002/diff/20001/runtime/bin/file.h#newc...
runtime/bin/file.h:227: Dart_WeakPersistentHandle weak_handle_;
Please add a comment why we store the weak_handle_ here. This is a bit unusual
because in other uses customer of the API will just wait for the GC to release
the object before the weak_handle_ is being deleted.

https://codereview.chromium.org/1892623002/diff/20001/sdk/lib/io/file_impl.dart
File sdk/lib/io/file_impl.dart (right):

https://codereview.chromium.org/1892623002/diff/20001/sdk/lib/io/file_impl.da...
sdk/lib/io/file_impl.dart:568: external factory _RandomAccessFileOps(int
pointer);
Needs a patch in sdk/lib/_internal/js_runtime/lib/io_patch.dart

https://codereview.chromium.org/1892623002/diff/20001/sdk/lib/io/file_impl.da...
sdk/lib/io/file_impl.dart:985: if (start == null) {
This test is a bit confusing to say the least. It looks as if it is a left over
from a non-default value being set for start.

Similarly the test above is a bit strange that it is checking for null as if
null was a legal parameter value. Which I believe it is not...

Same applies to end which should probably get a -1 as default value, no?

https://codereview.chromium.org/1892623002/diff/20001/sdk/lib/io/file_impl.da...
sdk/lib/io/file_impl.dart:1007: if (start == null) {
ditto

[zra, 2016-04-20 17:07:52]

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

https://codereview.chromium.org/1892623002/diff/20001/runtime/bin/file.cc#new...
runtime/bin/file.cc:120: Dart_Handle dart_this = Dart_GetNativeArgument(args,
0);
On 2016/04/20 05:31:06, Ivan Posva wrote:
> Can you rely on the Dart code to do the disassociation with the native object
> instead?

The native object is associated by extending NativeFieldWrapperClass1, so AFAIU
it isn't possible to touch the field in Dart code.

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

https://codereview.chromium.org/1892623002/diff/20001/runtime/bin/file.h#newc...
runtime/bin/file.h:227: Dart_WeakPersistentHandle weak_handle_;
On 2016/04/20 05:31:06, Ivan Posva wrote:
> Please add a comment why we store the weak_handle_ here. This is a bit unusual
> because in other uses customer of the API will just wait for the GC to release
> the object before the weak_handle_ is being deleted.

Done.

https://codereview.chromium.org/1892623002/diff/20001/sdk/lib/io/file_impl.dart
File sdk/lib/io/file_impl.dart (right):

https://codereview.chromium.org/1892623002/diff/20001/sdk/lib/io/file_impl.da...
sdk/lib/io/file_impl.dart:568: external factory _RandomAccessFileOps(int
pointer);
On 2016/04/20 05:31:06, Ivan Posva wrote:
> Needs a patch in sdk/lib/_internal/js_runtime/lib/io_patch.dart

Done.

https://codereview.chromium.org/1892623002/diff/20001/sdk/lib/io/file_impl.da...
sdk/lib/io/file_impl.dart:985: if (start == null) {
On 2016/04/20 05:31:06, Ivan Posva wrote:
> This test is a bit confusing to say the least. It looks as if it is a left
over
> from a non-default value being set for start.
> 
> Similarly the test above is a bit strange that it is checking for null as if
> null was a legal parameter value. Which I believe it is not...
> 
> Same applies to end which should probably get a -1 as default value, no?

Done.

https://codereview.chromium.org/1892623002/diff/20001/sdk/lib/io/file_impl.da...
sdk/lib/io/file_impl.dart:1007: if (start == null) {
On 2016/04/20 05:31:06, Ivan Posva wrote:
> ditto

Done.

[zra, 2016-04-20 17:08:40]

Description was changed from

==========
Fixes leak of native File objects.

Also employs the same reference counting technique as secure sockets to
avoid the IO Service touching dangling pointers.
==========

to

==========
Fixes leak of native File objects.

Also employs the same reference counting technique as secure sockets to
avoid the IO Service touching dangling pointers.

R=iposva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/218545ba10465624413a2f50f89247ae77b13a3d
==========

[zra, 2016-04-20 17:08:41]

Committed patchset #4 (id:60001) manually as
218545ba10465624413a2f50f89247ae77b13a3d (presubmit successful).