diff options
author | mpcomplete@chromium.org <mpcomplete@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-04-03 18:16:32 +0000 |
---|---|---|
committer | mpcomplete@chromium.org <mpcomplete@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-04-03 18:16:32 +0000 |
commit | da9a01cfbe84f32c73988f8edb8bf2134d17f10c (patch) | |
tree | 26f74fc05001e5f98912c779e0376453c10c324f | |
parent | 15877542c33babc237586513096f3077c313449c (diff) | |
download | chromium_src-da9a01cfbe84f32c73988f8edb8bf2134d17f10c.zip chromium_src-da9a01cfbe84f32c73988f8edb8bf2134d17f10c.tar.gz chromium_src-da9a01cfbe84f32c73988f8edb8bf2134d17f10c.tar.bz2 |
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
-rw-r--r-- | content/renderer/web_ui_mojo_context_state.cc | 3 | ||||
-rw-r--r-- | gin/gin.gyp | 2 | ||||
-rw-r--r-- | gin/test/file_runner.cc | 2 | ||||
-rw-r--r-- | gin/test/gc.cc | 39 | ||||
-rw-r--r-- | gin/test/gc.h | 21 | ||||
-rw-r--r-- | mojo/apps/js/bindings/connection_unittests.js | 5 | ||||
-rw-r--r-- | mojo/apps/js/mojo_runner_delegate.cc | 4 | ||||
-rw-r--r-- | mojo/bindings/js/core.cc | 86 | ||||
-rw-r--r-- | mojo/bindings/js/core_unittests.js | 4 | ||||
-rw-r--r-- | mojo/bindings/js/handle.cc | 18 | ||||
-rw-r--r-- | mojo/bindings/js/handle.h | 27 |
11 files changed, 180 insertions, 31 deletions
diff --git a/content/renderer/web_ui_mojo_context_state.cc b/content/renderer/web_ui_mojo_context_state.cc index 18c61fa..f1ff5f4 100644 --- a/content/renderer/web_ui_mojo_context_state.cc +++ b/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); } diff --git a/gin/gin.gyp b/gin/gin.gyp index 1b15dd1..a7b359f1 100644 --- a/gin/gin.gyp +++ b/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', diff --git a/gin/test/file_runner.cc b/gin/test/file_runner.cc index 86cb2f3..858fb39 100644 --- a/gin/test/file_runner.cc +++ b/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() { diff --git a/gin/test/gc.cc b/gin/test/gc.cc new file mode 100644 index 0000000..1ad6cf2 --- /dev/null +++ b/gin/test/gc.cc @@ -0,0 +1,39 @@ +// 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 diff --git a/gin/test/gc.h b/gin/test/gc.h new file mode 100644 index 0000000..25917ef --- /dev/null +++ b/gin/test/gc.h @@ -0,0 +1,21 @@ +// 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_ diff --git a/mojo/apps/js/bindings/connection_unittests.js b/mojo/apps/js/bindings/connection_unittests.js index 3a57820..60b867d 100644 --- a/mojo/apps/js/bindings/connection_unittests.js +++ b/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; diff --git a/mojo/apps/js/mojo_runner_delegate.cc b/mojo/apps/js/mojo_runner_delegate.cc index 2a6c1bd..13081e4 100644 --- a/mojo/apps/js/mojo_runner_delegate.cc +++ b/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); } diff --git a/mojo/bindings/js/core.cc b/mojo/bindings/js/core.cc index 3aaf629..de53cfc 100644 --- a/mojo/bindings/js/core.cc +++ b/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) diff --git a/mojo/bindings/js/core_unittests.js b/mojo/bindings/js/core_unittests.js index a05a5b20..62b20d1 100644 --- a/mojo/bindings/js/core_unittests.js +++ b/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) { diff --git a/mojo/bindings/js/handle.cc b/mojo/bindings/js/handle.cc index a6c229b..36a4988 100644 --- a/mojo/bindings/js/handle.cc +++ b/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 diff --git a/mojo/bindings/js/handle.h b/mojo/bindings/js/handle.h index 3cd4cf5..ad03f87 100644 --- a/mojo/bindings/js/handle.h +++ b/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, |