Fixes leak of native File objects.

sdk/lib/io/file_impl.dart

Index: sdk/lib/io/file_impl.dart
diff --git a/sdk/lib/io/file_impl.dart b/sdk/lib/io/file_impl.dart
index 0d0bc9e3a1868bd04c0be55354828e62b2b978c0..5511ac34df19e2f762549bbe770c481b216d1123 100644
--- a/sdk/lib/io/file_impl.dart
+++ b/sdk/lib/io/file_impl.dart
@@ -564,19 +564,43 @@ class _File extends FileSystemEntity implements File {
   }
 }
 
+abstract class _RandomAccessFileOps {
+  external factory _RandomAccessFileOps(int pointer);

[Ivan Posva, 2016/04/20 05:31:06]

Needs a patch in sdk/lib/_internal/js_runtime/lib/io_patch.dart

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

Done.

+
+  int getPointer();
+  int close();
+  readByte();
+  read(int bytes);
+  readInto(List<int> buffer, int start, int end);
+  writeByte(int value);
+  writeFrom(List<int> buffer, int start, int end);
+  position();
+  setPosition(int position);
+  truncate(int length);
+  length();
+  flush();
+  lock(int lock, int start, int end);
+}
 
-class _RandomAccessFile
-    implements RandomAccessFile {
+class _RandomAccessFile implements RandomAccessFile {
   static bool _connectedResourceHandler = false;
 
   final String path;
-  int _id;
+
+  // Calling this function will increase the reference count on the native
+  // object that implements the file operations. It should only be called to
+  // pass the pointer to the IO Service, which will decrement the reference
+  // count when it is finished with it.
+  int _pointer() => _ops.getPointer();
+
   bool _asyncDispatched = false;
   SendPort _fileService;
 
   _FileResourceInfo _resourceInfo;
+  _RandomAccessFileOps _ops;
 
-  _RandomAccessFile(this._id, this.path) {
+  _RandomAccessFile(int pointer, this.path) {
+    _ops = new _RandomAccessFileOps(pointer);
     _resourceInfo = new _FileResourceInfo(this);
     _maybeConnectHandler();
   }
@@ -587,12 +611,10 @@ class _RandomAccessFile
     }
   }
 
-  external static int _getFD(int id);
-
   _maybeConnectHandler() {
     if (!_connectedResourceHandler) {
-      // TODO(ricow): we probably need set these in some initialization code.
-      // We need to make sure that these are always awailable from the
+      // TODO(ricow): We probably need to set these in some initialization code.
+      // We need to make sure that these are always available from the
       // observatory even if no files (or sockets for the socket ones) are
       // open.
       registerExtension('ext.dart.io.getOpenFiles',
@@ -604,9 +626,9 @@ class _RandomAccessFile
   }
 
   Future<RandomAccessFile> close() {
-    return _dispatch(_FILE_CLOSE, [_id], markClosed: true).then((result) {
+    return _dispatch(_FILE_CLOSE, [_pointer()], markClosed: true).then((result) {
       if (result != -1) {
-        _id = result;
+        closed = closed || (result == 0);
         _maybePerformCleanup();
         return this;
       } else {
@@ -615,20 +637,18 @@ class _RandomAccessFile
     });
   }
 
-  external static int _close(int id);
-
   void closeSync() {
     _checkAvailable();
-    var id = _close(_id);
+    var id = _ops.close();
     if (id == -1) {
       throw new FileSystemException("Cannot close file", path);
     }
-    _id = id;
+    closed = closed || (id == 0);
     _maybePerformCleanup();
   }
 
   Future<int> readByte() {
-    return _dispatch(_FILE_READ_BYTE, [_id]).then((response) {
+    return _dispatch(_FILE_READ_BYTE, [_pointer()]).then((response) {
       if (_isErrorResponse(response)) {
         throw _exceptionFromResponse(response, "readByte failed", path);
       }
@@ -637,11 +657,9 @@ class _RandomAccessFile
     });
   }
 
-  external static _readByte(int id);
-
   int readByteSync() {
     _checkAvailable();
-    var result = _readByte(_id);
+    var result = _ops.readByte();
     if (result is OSError) {
       throw new FileSystemException("readByte failed", path, result);
     }
@@ -653,7 +671,7 @@ class _RandomAccessFile
     if (bytes is !int) {
       throw new ArgumentError(bytes);
     }
-    return _dispatch(_FILE_READ, [_id, bytes]).then((response) {
+    return _dispatch(_FILE_READ, [_pointer(), bytes]).then((response) {
       if (_isErrorResponse(response)) {
         throw _exceptionFromResponse(response, "read failed", path);
       }
@@ -662,14 +680,12 @@ class _RandomAccessFile
     });
   }
 
-  external static _read(int id, int bytes);
-
   List<int> readSync(int bytes) {
     _checkAvailable();
     if (bytes is !int) {
       throw new ArgumentError(bytes);
     }
-    var result = _read(_id, bytes);
+    var result = _ops.read(bytes);
     if (result is OSError) {
       throw new FileSystemException("readSync failed", path, result);
     }
@@ -678,15 +694,17 @@ class _RandomAccessFile
   }
 
   Future<int> readInto(List<int> buffer, [int start = 0, int end]) {
-    if (buffer is !List ||
-        (start != null && start is !int) ||
-        (end != null && end is !int)) {
+    if ((buffer is !List) ||
+        ((start != null) && (start is !int)) ||
+        ((end != null) && (end is !int))) {
       throw new ArgumentError();
     }
     end = RangeError.checkValidRange(start, end, buffer.length);
-    if (end == start) return new Future.value(0);
+    if (end == start) {
+      return new Future.value(0);
+    }
     int length = end - start;
-    return _dispatch(_FILE_READ_INTO, [_id, length]).then((response) {
+    return _dispatch(_FILE_READ_INTO, [_pointer(), length]).then((response) {
       if (_isErrorResponse(response)) {
         throw _exceptionFromResponse(response, "readInto failed", path);
       }
@@ -698,18 +716,18 @@ class _RandomAccessFile
     });
   }
 
-  external static _readInto(int id, List<int> buffer, int start, int end);
-
   int readIntoSync(List<int> buffer, [int start = 0, int end]) {
     _checkAvailable();
-    if (buffer is !List ||
-        (start != null && start is !int) ||
-        (end != null && end is !int)) {
+    if ((buffer is !List) ||
+        ((start != null) && (start is !int)) ||
+        ((end != null) && (end is !int))) {
       throw new ArgumentError();
     }
     end = RangeError.checkValidRange(start, end, buffer.length);
-    if (end == start) return 0;
-    var result = _readInto(_id, buffer, start, end);
+    if (end == start) {
+      return 0;
+    }
+    var result = _ops.readInto(buffer, start, end);
     if (result is OSError) {
       throw new FileSystemException("readInto failed", path, result);
     }
@@ -721,7 +739,7 @@ class _RandomAccessFile
     if (value is !int) {
       throw new ArgumentError(value);
     }
-    return _dispatch(_FILE_WRITE_BYTE, [_id, value]).then((response) {
+    return _dispatch(_FILE_WRITE_BYTE, [_pointer(), value]).then((response) {
       if (_isErrorResponse(response)) {
         throw _exceptionFromResponse(response, "writeByte failed", path);
       }
@@ -730,14 +748,12 @@ class _RandomAccessFile
     });
   }
 
-  external static _writeByte(int id, int value);
-
   int writeByteSync(int value) {
     _checkAvailable();
     if (value is !int) {
       throw new ArgumentError(value);
     }
-    var result = _writeByte(_id, value);
+    var result = _ops.writeByte(value);
     if (result is OSError) {
       throw new FileSystemException("writeByte failed", path, result);
     }
@@ -748,12 +764,14 @@ class _RandomAccessFile
   Future<RandomAccessFile> writeFrom(
       List<int> buffer, [int start = 0, int end]) {
     if ((buffer is !List) ||
-        (start != null && start is !int) ||
-        (end != null && end is !int)) {
+        ((start != null) && (start is !int)) ||
+        ((end != null) && (end is !int))) {
       throw new ArgumentError("Invalid arguments to writeFrom");
     }
     end = RangeError.checkValidRange(start, end, buffer.length);
-    if (end == start) return new Future.value(this);
+    if (end == start) {
+      return new Future.value(this);
+    }
     _BufferAndStart result;
     try {
       result = _ensureFastAndSerializableByteData(buffer, start, end);
@@ -762,7 +780,7 @@ class _RandomAccessFile
     }
 
     List request = new List(4);
-    request[0] = _id;
+    request[0] = _pointer();
     request[1] = result.buffer;
     request[2] = result.start;
     request[3] = end - (start - result.start);
@@ -775,23 +793,22 @@ class _RandomAccessFile
     });
   }
 
-  external static _writeFrom(int id, List<int> buffer, int start, int end);
-
   void writeFromSync(List<int> buffer, [int start = 0, int end]) {
     _checkAvailable();
-    if (buffer is !List ||
-        (start != null && start is !int) ||
-        (end != null && end is !int)) {
+    if ((buffer is !List) ||
+        ((start != null) && (start is !int)) ||
+        ((end != null) && (end is !int))) {
       throw new ArgumentError("Invalid arguments to writeFromSync");
     }
     end = RangeError.checkValidRange(start, end, buffer.length);
-    if (end == start) return;
+    if (end == start) {
+      return;
+    }
     _BufferAndStart bufferAndStart =
         _ensureFastAndSerializableByteData(buffer, start, end);
-    var result = _writeFrom(_id,
-                            bufferAndStart.buffer,
-                            bufferAndStart.start,
-                            end - (start - bufferAndStart.start));
+    var result = _ops.writeFrom(bufferAndStart.buffer,
+                                  bufferAndStart.start,
+                                  end - (start - bufferAndStart.start));
     if (result is OSError) {
       throw new FileSystemException("writeFrom failed", path, result);
     }
@@ -816,7 +833,7 @@ class _RandomAccessFile
   }
 
   Future<int> position() {
-    return _dispatch(_FILE_POSITION, [_id]).then((response) {
+    return _dispatch(_FILE_POSITION, [_pointer()]).then((response) {
       if (_isErrorResponse(response)) {
         throw _exceptionFromResponse(response, "position failed", path);
       }
@@ -824,11 +841,9 @@ class _RandomAccessFile
     });
   }
 
-  external static _position(int id);
-
   int positionSync() {
     _checkAvailable();
-    var result = _position(_id);
+    var result = _ops.position();
     if (result is OSError) {
       throw new FileSystemException("position failed", path, result);
     }
@@ -836,7 +851,7 @@ class _RandomAccessFile
   }
 
   Future<RandomAccessFile> setPosition(int position) {
-    return _dispatch(_FILE_SET_POSITION, [_id, position])
+    return _dispatch(_FILE_SET_POSITION, [_pointer(), position])
         .then((response) {
           if (_isErrorResponse(response)) {
             throw _exceptionFromResponse(response, "setPosition failed", path);
@@ -845,18 +860,16 @@ class _RandomAccessFile
         });
   }
 
-  external static _setPosition(int id, int position);
-
   void setPositionSync(int position) {
     _checkAvailable();
-    var result = _setPosition(_id, position);
+    var result = _ops.setPosition(position);
     if (result is OSError) {
       throw new FileSystemException("setPosition failed", path, result);
     }
   }
 
   Future<RandomAccessFile> truncate(int length) {
-    return _dispatch(_FILE_TRUNCATE, [_id, length]).then((response) {
+    return _dispatch(_FILE_TRUNCATE, [_pointer(), length]).then((response) {
       if (_isErrorResponse(response)) {
         throw _exceptionFromResponse(response, "truncate failed", path);
       }
@@ -864,18 +877,16 @@ class _RandomAccessFile
     });
   }
 
-  external static _truncate(int id, int length);
-
   void truncateSync(int length) {
     _checkAvailable();
-    var result = _truncate(_id, length);
+    var result = _ops.truncate(length);
     if (result is OSError) {
       throw new FileSystemException("truncate failed", path, result);
     }
   }
 
   Future<int> length() {
-    return _dispatch(_FILE_LENGTH, [_id]).then((response) {
+    return _dispatch(_FILE_LENGTH, [_pointer()]).then((response) {
       if (_isErrorResponse(response)) {
         throw _exceptionFromResponse(response, "length failed", path);
       }
@@ -883,11 +894,9 @@ class _RandomAccessFile
     });
   }
 
-  external static _length(int id);
-
   int lengthSync() {
     _checkAvailable();
-    var result = _length(_id);
+    var result = _ops.length();
     if (result is OSError) {
       throw new FileSystemException("length failed", path, result);
     }
@@ -895,7 +904,7 @@ class _RandomAccessFile
   }
 
   Future<RandomAccessFile> flush() {
-    return _dispatch(_FILE_FLUSH, [_id]).then((response) {
+    return _dispatch(_FILE_FLUSH, [_pointer()]).then((response) {
       if (_isErrorResponse(response)) {
         throw _exceptionFromResponse(response,
                                      "flush failed",
@@ -905,11 +914,9 @@ class _RandomAccessFile
     });
   }
 
-  external static _flush(int id);
-
   void flushSync() {
     _checkAvailable();
-    var result = _flush(_id);
+    var result = _ops.flush();
     if (result is OSError) {
       throw new FileSystemException("flush failed", path, result);
     }
@@ -921,18 +928,22 @@ class _RandomAccessFile
 
   Future<RandomAccessFile> lock(
       [FileLock mode = FileLock.EXCLUSIVE, int start = 0, int end]) {
-    if ((start != null && start is !int) ||
-        (end != null && end is !int) ||
-         mode is !FileLock) {
+    if (((start != null) && (start is !int)) ||
+        ((end != null) && (end is !int)) ||
+         (mode is !FileLock)) {
       throw new ArgumentError();
     }
-    if (start == null) start = 0;
-    if (end == null) end = -1;
-    if (start < 0 || end < -1 || (end != -1 && start >= end)) {
+    if (start == null) {
+      start = 0;
+    }
+    if (end == null) {
+      end = -1;
+    }
+    if ((start < 0) || (end < -1) || ((end != -1) && (start >= end))) {
       throw new ArgumentError();
     }
-    int lock = mode == FileLock.EXCLUSIVE ? LOCK_EXCLUSIVE : LOCK_SHARED;
-    return _dispatch(_FILE_LOCK, [_id, lock, start, end])
+    int lock = (mode == FileLock.EXCLUSIVE) ? LOCK_EXCLUSIVE : LOCK_SHARED;
+    return _dispatch(_FILE_LOCK, [_pointer(), lock, start, end])
         .then((response) {
           if (_isErrorResponse(response)) {
             throw _exceptionFromResponse(response, 'lock failed', path);
@@ -942,14 +953,20 @@ class _RandomAccessFile
   }
 
   Future<RandomAccessFile> unlock([int start = 0, int end]) {
-    if ((start != null && start is !int) ||
-        (end != null && end is !int)) {
+    if (((start != null) && (start is !int)) ||
+        ((end != null) && (end is !int))) {
+      throw new ArgumentError();
+    }
+    if (start == null) {
+      start = 0;
+    }
+    if (end == null) {
+      end = -1;
+    }
+    if (start == end) {
       throw new ArgumentError();
     }
-    if (start == null) start = 0;
-    if (end == null) end = -1;
-    if (start == end) throw new ArgumentError();
-    return _dispatch(_FILE_LOCK, [_id, LOCK_UNLOCK, start, end])
+    return _dispatch(_FILE_LOCK, [_pointer(), LOCK_UNLOCK, start, end])
         .then((response) {
           if (_isErrorResponse(response)) {
             throw _exceptionFromResponse(response, 'unlock failed', path);
@@ -958,22 +975,24 @@ class _RandomAccessFile
         });
   }
 
-  external static _lock(int id, int lock, int start, int end);
-
   void lockSync([FileLock mode = FileLock.EXCLUSIVE, int start = 0, int end]) {
     _checkAvailable();
-    if ((start != null && start is !int) ||
-        (end != null && end is !int) ||
-         mode is !FileLock) {
+    if (((start != null) && (start is !int)) ||
+        ((end != null) && (end is !int)) ||
+         (mode is !FileLock)) {
       throw new ArgumentError();
     }
-    if (start == null) start = 0;
-    if (end == null) end = -1;
-    if (start < 0 || end < -1 || (end != -1 && start >= end)) {
+    if (start == null) {

[Ivan Posva, 2016/04/20 05:31:06]

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?

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

Done.

+      start = 0;
+    }
+    if (end == null) {
+      end = -1;
+    }
+    if ((start < 0) || (end < -1) || ((end != -1) && (start >= end))) {
       throw new ArgumentError();
     }
-    int lock = mode == FileLock.EXCLUSIVE ? LOCK_EXCLUSIVE : LOCK_SHARED;
-    var result = _lock(_id, lock, start, end);
+    int lock = (mode == FileLock.EXCLUSIVE) ? LOCK_EXCLUSIVE : LOCK_SHARED;
+    var result = _ops.lock(lock, start, end);
     if (result is OSError) {
       throw new FileSystemException('lock failed', path, result);
     }
@@ -981,20 +1000,26 @@ class _RandomAccessFile
 
   void unlockSync([int start = 0, int end]) {
     _checkAvailable();
-    if ((start != null && start is !int) ||
-        (end != null && end is !int)) {
+    if (((start != null) && (start is !int)) ||
+        ((end != null) && (end is !int))) {
+      throw new ArgumentError();
+    }
+    if (start == null) {

[Ivan Posva, 2016/04/20 05:31:06]

ditto

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

Done.

+      start = 0;
+    }
+    if (end == null) {
+      end = -1;
+    }
+    if (start == end) {
       throw new ArgumentError();
     }
-    if (start == null) start = 0;
-    if (end == null) end = -1;
-    if (start == end) throw new ArgumentError();
-    var result = _lock(_id, LOCK_UNLOCK, start, end);
+    var result = _ops.lock(LOCK_UNLOCK, start, end);
     if (result is OSError) {
       throw new FileSystemException('unlock failed', path, result);
     }
   }
 
-  bool get closed => _id == 0;
+  bool closed = false;
 
   Future _dispatch(int request, List data, { bool markClosed: false }) {
     if (closed) {
@@ -1005,9 +1030,9 @@ class _RandomAccessFile
       return new Future.error(new FileSystemException(msg, path));
     }
     if (markClosed) {
-      // Set the id_ to 0 (NULL) to ensure the no more async requests
-      // can be issued for this file.
-      _id = 0;
+      // Set closed to true to ensure that no more async requests can be issued
+      // for this file.
+      closed = true;
     }
     _asyncDispatched = true;
     return _IOService._dispatch(request, data)