Change mojo JS bindings to expose a handle object, which is Closed when garbage

collected, rather than a raw integer.

BUG=357785

Review URL: https://codereview.chromium.org/214183003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@261479 0039d316-1c4b-4281-b951-d872f2087c98

content/renderer/web_ui_mojo_context_state.cc

@@ -14,6 +14,7 @@
 #include "gin/public/context_holder.h"
 #include "gin/try_catch.h"
 #include "mojo/bindings/js/core.h"
+#include "mojo/bindings/js/handle.h"
 #include "mojo/bindings/js/support.h"
 #include "third_party/WebKit/public/platform/WebURLResponse.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
@@ -41,7 +42,7 @@ void RunMain(base::WeakPtr<gin::Runner> runner,
   v8::Handle<v8::Function> start;
   CHECK(gin::ConvertFromV8(isolate, module, &start));
   v8::Handle<v8::Value> args[] = {
-      gin::ConvertToV8(isolate, handle->release().value()) };
+      gin::ConvertToV8(isolate, mojo::Handle(handle->release().value())) };
   runner->Call(start, runner->global(), 1, args);
 }
 

gin/gin.gyp

@@ -103,6 +103,8 @@
       'sources': [
         'test/file_runner.cc',
         'test/file_runner.h',
+        'test/gc.cc',
+        'test/gc.h',
         'test/gtest.cc',
         'test/gtest.h',
         'test/v8_test.cc',

gin/test/file_runner.cc

@@ -12,6 +12,7 @@
 #include "gin/modules/module_registry.h"
 #include "gin/public/context_holder.h"
 #include "gin/public/isolate_holder.h"
+#include "gin/test/gc.h"
 #include "gin/test/gtest.h"
 #include "gin/try_catch.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -34,6 +35,7 @@ FileRunnerDelegate::FileRunnerDelegate()
     : ModuleRunnerDelegate(GetModuleSearchPaths()) {
   AddBuiltinModule(Console::kModuleName, Console::GetModule);
   AddBuiltinModule(GTest::kModuleName, GTest::GetModule);
+  AddBuiltinModule(GC::kModuleName, GC::GetModule);
 }
 
 FileRunnerDelegate::~FileRunnerDelegate() {

gin/test/gc.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "gin/test/gc.h"
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "gin/arguments.h"
+#include "gin/converter.h"
+#include "gin/function_template.h"
+#include "gin/object_template_builder.h"
+#include "gin/per_isolate_data.h"
+#include "gin/public/wrapper_info.h"
+#include "gin/wrappable.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace gin {
+
+namespace {
+WrapperInfo g_wrapper_info = { kEmbedderNativeGin };
+}  // namespace
+
+const char GC::kModuleName[] = "gc";
+
+v8::Local<v8::Value> GC::GetModule(v8::Isolate* isolate) {
+  PerIsolateData* data = PerIsolateData::From(isolate);
+  v8::Local<v8::ObjectTemplate> templ =
+      data->GetObjectTemplate(&g_wrapper_info);
+  if (templ.IsEmpty()) {
+    templ = ObjectTemplateBuilder(isolate)
+        .SetMethod("collectGarbage", v8::V8::LowMemoryNotification)
+        .Build();
+    data->SetObjectTemplate(&g_wrapper_info, templ);
+  }
+  return templ->NewInstance();
+}
+
+}  // namespace gin

gin/test/gc.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef GIN_TEST_GC_H_
+#define GIN_TEST_GC_H_
+
+#include "v8/include/v8.h"
+
+namespace gin {
+
+// This module provides bindings to the garbage collector.
+class GC {
+ public:
+  static const char kModuleName[];
+  static v8::Local<v8::Value> GetModule(v8::Isolate* isolate);
+};
+
+}  // namespace gin
+
+#endif  // GIN_TEST_GC_H_

mojo/apps/js/bindings/connection_unittests.js

@@ -53,16 +53,19 @@ define([
     "mojo/public/bindings/js/connection",
     "mojo/public/interfaces/bindings/tests/sample_interfaces.mojom",
     "mojo/public/interfaces/bindings/tests/sample_service.mojom",
+    "gc",
 ], function(expect,
             mockSupport,
             core,
             connection,
             sample_interfaces,
-            sample_service) {
+            sample_service,
+            gc) {
   testClientServer();
   testWriteToClosedPipe();
   testRequestResponse();
   this.result = "PASS";
+  gc.collectGarbage();  // should not crash
 
   function testClientServer() {
     var receivedFrobinate = false;

mojo/apps/js/mojo_runner_delegate.cc

@@ -15,6 +15,7 @@
 #include "mojo/apps/js/bindings/monotonic_clock.h"
 #include "mojo/apps/js/bindings/threading.h"
 #include "mojo/bindings/js/core.h"
+#include "mojo/bindings/js/handle.h"
 #include "mojo/bindings/js/support.h"
 
 namespace mojo {
@@ -39,7 +40,8 @@ void StartCallback(base::WeakPtr<gin::Runner> runner,
   v8::Handle<v8::Function> start;
   CHECK(gin::ConvertFromV8(isolate, module, &start));
 
-  v8::Handle<v8::Value> args[] = { gin::ConvertToV8(isolate, pipe) };
+  v8::Handle<v8::Value> args[] = {
+      gin::ConvertToV8(isolate, mojo::Handle(pipe)) };
   runner->Call(start, runner->global(), 1, args);
 }
 

mojo/bindings/js/core.cc

@@ -11,9 +11,11 @@
 #include "gin/converter.h"
 #include "gin/dictionary.h"
 #include "gin/function_template.h"
+#include "gin/handle.h"
 #include "gin/object_template_builder.h"
 #include "gin/per_isolate_data.h"
 #include "gin/public/wrapper_info.h"
+#include "gin/wrappable.h"
 #include "mojo/bindings/js/handle.h"
 
 namespace mojo {
@@ -21,6 +23,26 @@ namespace js {
 
 namespace {
 
+MojoResult CloseHandle(gin::Handle<gin::HandleWrapper> handle) {
+  if (!handle->get().is_valid())
+    return MOJO_RESULT_INVALID_ARGUMENT;
+  handle->Close();
+  return MOJO_RESULT_OK;
+}
+
+MojoResult WaitHandle(mojo::Handle handle,
+                      MojoWaitFlags flags,
+                      MojoDeadline deadline) {
+  return MojoWait(handle.value(), flags, deadline);
+}
+
+MojoResult WaitMany(
+    const std::vector<mojo::Handle>& handles,
+    const std::vector<MojoWaitFlags>& flags,
+    MojoDeadline deadline) {
+  return mojo::WaitMany(handles, flags, deadline);
+}
+
 gin::Dictionary CreateMessagePipe(const gin::Arguments& args) {
   MojoHandle handle0 = MOJO_HANDLE_INVALID;
   MojoHandle handle1 = MOJO_HANDLE_INVALID;
@@ -28,30 +50,41 @@ gin::Dictionary CreateMessagePipe(const gin::Arguments& args) {
   CHECK(result == MOJO_RESULT_OK);
 
   gin::Dictionary dictionary = gin::Dictionary::CreateEmpty(args.isolate());
-  dictionary.Set("handle0", handle0);
-  dictionary.Set("handle1", handle1);
+  dictionary.Set("handle0", mojo::Handle(handle0));
+  dictionary.Set("handle1", mojo::Handle(handle1));
   return dictionary;
 }
 
-MojoResult WriteMessage(MojoHandle handle,
-                        const gin::ArrayBufferView& buffer,
-                        const std::vector<MojoHandle>& handles,
-                        MojoWriteMessageFlags flags) {
-  return MojoWriteMessage(handle,
+MojoResult WriteMessage(
+    mojo::Handle handle,
+    const gin::ArrayBufferView& buffer,
+    const std::vector<gin::Handle<gin::HandleWrapper> >& handles,
+    MojoWriteMessageFlags flags) {
+  std::vector<MojoHandle> raw_handles(handles.size());
+  for (size_t i = 0; i < handles.size(); ++i)
+    raw_handles[i] = handles[i]->get().value();
+  MojoResult rv = MojoWriteMessage(handle.value(),
                           buffer.bytes(),
                           static_cast<uint32_t>(buffer.num_bytes()),
-                          handles.empty() ? NULL : &handles[0],
-                          static_cast<uint32_t>(handles.size()),
+                          raw_handles.empty() ? NULL : &raw_handles[0],
+                          static_cast<uint32_t>(raw_handles.size()),
                           flags);
+  // MojoWriteMessage takes ownership of the handles upon success, so
+  // release them here.
+  if (rv == MOJO_RESULT_OK) {
+    for (size_t i = 0; i < handles.size(); ++i)
+      mojo::Handle _ MOJO_ALLOW_UNUSED = handles[i]->release();
+  }
+  return rv;
 }
 
 gin::Dictionary ReadMessage(const gin::Arguments& args,
-                            MojoHandle handle,
+                            mojo::Handle handle,
                             MojoReadMessageFlags flags) {
   uint32_t num_bytes = 0;
   uint32_t num_handles = 0;
   MojoResult result = MojoReadMessage(
-      handle, NULL, &num_bytes, NULL, &num_handles, flags);
+      handle.value(), NULL, &num_bytes, NULL, &num_handles, flags);
   if (result != MOJO_RESULT_RESOURCE_EXHAUSTED) {
     gin::Dictionary dictionary = gin::Dictionary::CreateEmpty(args.isolate());
     dictionary.Set("result", result);
@@ -60,16 +93,17 @@ gin::Dictionary ReadMessage(const gin::Arguments& args,
 
   v8::Handle<v8::ArrayBuffer> array_buffer =
       v8::ArrayBuffer::New(args.isolate(), num_bytes);
-  std::vector<MojoHandle> handles(num_handles);
+  std::vector<mojo::Handle> handles(num_handles);
 
   gin::ArrayBuffer buffer;
   ConvertFromV8(args.isolate(), array_buffer, &buffer);
   CHECK(buffer.num_bytes() == num_bytes);
 
-  result = MojoReadMessage(handle,
+  result = MojoReadMessage(handle.value(),
                            buffer.bytes(),
                            &num_bytes,
-                           handles.empty() ? NULL : &handles[0],
+                           handles.empty() ? NULL :
+                               reinterpret_cast<MojoHandle*>(&handles[0]),
                            &num_handles,
                            flags);
 
@@ -116,17 +150,18 @@ gin::Dictionary CreateDataPipe(const gin::Arguments& args,
   CHECK_EQ(MOJO_RESULT_OK, result);
 
   dictionary.Set("result", result);
-  dictionary.Set("producerHandle", producer_handle);
-  dictionary.Set("consumerHandle", consumer_handle);
+  dictionary.Set("producerHandle", mojo::Handle(producer_handle));
+  dictionary.Set("consumerHandle", mojo::Handle(consumer_handle));
   return dictionary;
 }
 
 gin::Dictionary WriteData(const gin::Arguments& args,
-                          MojoHandle handle,
+                          mojo::Handle handle,
                           const gin::ArrayBufferView& buffer,
                           MojoWriteDataFlags flags) {
   uint32_t num_bytes = static_cast<uint32_t>(buffer.num_bytes());
-  MojoResult result = MojoWriteData(handle, buffer.bytes(), &num_bytes, flags);
+  MojoResult result =
+      MojoWriteData(handle.value(), buffer.bytes(), &num_bytes, flags);
   gin::Dictionary dictionary = gin::Dictionary::CreateEmpty(args.isolate());
   dictionary.Set("result", result);
   dictionary.Set("numBytes", num_bytes);
@@ -134,11 +169,11 @@ gin::Dictionary WriteData(const gin::Arguments& args,
 }
 
 gin::Dictionary ReadData(const gin::Arguments& args,
-                         MojoHandle handle,
+                         mojo::Handle handle,
                          MojoReadDataFlags flags) {
   uint32_t num_bytes = 0;
   MojoResult result = MojoReadData(
-      handle, NULL, &num_bytes, MOJO_READ_DATA_FLAG_QUERY);
+      handle.value(), NULL, &num_bytes, MOJO_READ_DATA_FLAG_QUERY);
   if (result != MOJO_RESULT_OK) {
     gin::Dictionary dictionary = gin::Dictionary::CreateEmpty(args.isolate());
     dictionary.Set("result", result);
@@ -151,7 +186,7 @@ gin::Dictionary ReadData(const gin::Arguments& args,
   ConvertFromV8(args.isolate(), array_buffer, &buffer);
   CHECK_EQ(num_bytes, buffer.num_bytes());
 
-  result = MojoReadData(handle, buffer.bytes(), &num_bytes, flags);
+  result = MojoReadData(handle.value(), buffer.bytes(), &num_bytes, flags);
   CHECK_EQ(num_bytes, buffer.num_bytes());
 
   gin::Dictionary dictionary = gin::Dictionary::CreateEmpty(args.isolate());
@@ -173,10 +208,11 @@ v8::Local<v8::Value> Core::GetModule(v8::Isolate* isolate) {
 
   if (templ.IsEmpty()) {
      templ = gin::ObjectTemplateBuilder(isolate)
-        .SetMethod("close", mojo::CloseRaw)
-        .SetMethod("wait", mojo::Wait)
-        .SetMethod("waitMany", mojo::WaitMany<std::vector<mojo::Handle>,
-                                              std::vector<MojoWaitFlags> >)
+        // TODO(mpcomplete): Should these just be methods on the JS Handle
+        // object?
+        .SetMethod("close", CloseHandle)
+        .SetMethod("wait", WaitHandle)
+        .SetMethod("waitMany", WaitMany)
         .SetMethod("createMessagePipe", CreateMessagePipe)
         .SetMethod("writeMessage", WriteMessage)
         .SetMethod("readMessage", ReadMessage)

mojo/bindings/js/core_unittests.js

@@ -5,11 +5,13 @@
 define([
     "gin/test/expect",
     "mojo/bindings/js/core",
-  ], function(expect, core) {
+    "gc",
+  ], function(expect, core, gc) {
   runWithMessagePipe(testNop);
   runWithMessagePipe(testReadAndWriteMessage);
   runWithDataPipe(testNop);
   runWithDataPipe(testReadAndWriteDataPipe);
+  gc.collectGarbage();  // should not crash
   this.result = "PASS";
 
   function runWithMessagePipe(test) {

mojo/bindings/js/handle.cc

@@ -6,15 +6,29 @@
 
 namespace gin {
 
+gin::WrapperInfo HandleWrapper::kWrapperInfo = { gin::kEmbedderNativeGin };
+
+HandleWrapper::HandleWrapper(MojoHandle handle)
+    : handle_(mojo::Handle(handle)) {
+}
+
+HandleWrapper::~HandleWrapper() {
+}
+
 v8::Handle<v8::Value> Converter<mojo::Handle>::ToV8(v8::Isolate* isolate,
                                                     const mojo::Handle& val) {
-  return Converter<MojoHandle>::ToV8(isolate, val.value());
+  return HandleWrapper::Create(isolate, val.value()).ToV8();
 }
 
 bool Converter<mojo::Handle>::FromV8(v8::Isolate* isolate,
                                      v8::Handle<v8::Value> val,
                                      mojo::Handle* out) {
-  return Converter<MojoHandle>::FromV8(isolate, val, out->mutable_value());
+  gin::Handle<HandleWrapper> handle;
+  if (!Converter<gin::Handle<HandleWrapper> >::FromV8(isolate, val, &handle))
+    return false;
+
+  *out = handle->get();
+  return true;
 }
 
 }  // namespace gin

mojo/bindings/js/handle.h

@@ -6,10 +6,37 @@
 #define MOJO_BINDINGS_JS_HANDLE_H_
 
 #include "gin/converter.h"
+#include "gin/handle.h"
+#include "gin/wrappable.h"
 #include "mojo/public/cpp/system/core.h"
 
 namespace gin {
 
+// Wrapper for mojo Handles exposed to JavaScript. This ensures the Handle
+// is Closed when its JS object is garbage collected.
+class HandleWrapper : public gin::Wrappable<HandleWrapper> {
+ public:
+  static gin::WrapperInfo kWrapperInfo;
+
+  static gin::Handle<HandleWrapper> Create(v8::Isolate* isolate,
+                                           MojoHandle handle) {
+    return gin::CreateHandle(isolate, new HandleWrapper(handle));
+  }
+
+  mojo::Handle get() const { return handle_.get(); }
+  mojo::Handle release() { return handle_.release(); }
+  void Close() { handle_.reset(); }
+
+ protected:
+  HandleWrapper(MojoHandle handle);
+  virtual ~HandleWrapper();
+  mojo::ScopedHandle handle_;
+};
+
+// Note: It's important to use this converter rather than the one for
+// MojoHandle, since that will do a simple int32 conversion. It's unfortunate
+// there's no way to prevent against accidental use.
+// TODO(mpcomplete): define converters for all Handle subtypes.
 template<>
 struct Converter<mojo::Handle> {
   static v8::Handle<v8::Value> ToV8(v8::Isolate* isolate,