Dart: Removes C++ set for closing handles on an unhandled exception. Replaces it with a call back into Dart from the embedder's unhandled exception callback. This has the beneficial side-effect of decoupling closing handles on an unhandled exception from registration of finalizers on handles for leak detection. fixes #478 R=johnmccutchan@google.com Review URL: https://codereview.chromium.org/1411843005 .
diff --git a/mojo/dart/embedder/dart_controller.cc b/mojo/dart/embedder/dart_controller.cc index 98020b1..a8b41f3 100644 --- a/mojo/dart/embedder/dart_controller.cc +++ b/mojo/dart/embedder/dart_controller.cc
@@ -259,8 +259,12 @@ DART_CHECK_VALID(result); result = Dart_RunLoop(); + + // Here we log the error, but we don't do DART_CHECK_VALID because we don't + // want to bring the whole process down due to an error in application code, + // whereas above we do want to bring the whole process down for a bug in + // library or generated code. tonic::LogIfError(result); - DART_CHECK_VALID(result); } Dart_Handle DartController::LibraryTagHandler(Dart_LibraryTag tag, @@ -445,19 +449,26 @@ } void DartController::UnhandledExceptionCallback(Dart_Handle error) { + Dart_Handle mojo_core_lib = + Builtin::GetLibrary(Builtin::kMojoInternalLibrary); + DART_CHECK_VALID(mojo_core_lib); + Dart_Handle handle_natives_type = + Dart_GetType(mojo_core_lib, + Dart_NewStringFromCString("MojoHandleNatives"), 0, nullptr); + DART_CHECK_VALID(handle_natives_type); + Dart_Handle method_name = Dart_NewStringFromCString("_closeUnclosedHandles"); + CHECK(!Dart_IsError(method_name)); + Dart_Handle result = + Dart_Invoke(handle_natives_type, method_name, 0, nullptr); + DART_CHECK_VALID(result); + auto isolate_data = MojoDartState::Current(); if (!isolate_data->callbacks().exception.is_null()) { - // TODO(zra): Instead of passing an error handle, it may make life easier - // for clients if we pass any error string here instead. - isolate_data->callbacks().exception.Run(error); + int64_t handles_closed = 0; + Dart_Handle int_result = Dart_IntegerToInt64(result, &handles_closed); + DART_CHECK_VALID(int_result); + isolate_data->callbacks().exception.Run(error, handles_closed); } - - // Close handles generated by the isolate. - std::set<MojoHandle>& handles = isolate_data->unclosed_handles(); - for (auto it = handles.begin(); it != handles.end(); ++it) { - MojoClose((*it)); - } - handles.clear(); }
diff --git a/mojo/dart/embedder/mojo_dart_state.h b/mojo/dart/embedder/mojo_dart_state.h index adaa1d0..773945a 100644 --- a/mojo/dart/embedder/mojo_dart_state.h +++ b/mojo/dart/embedder/mojo_dart_state.h
@@ -19,7 +19,7 @@ namespace dart { struct IsolateCallbacks { - base::Callback<void(Dart_Handle)> exception; + base::Callback<void(Dart_Handle, int64_t)> exception; }; // State associated with an isolate (retrieved via |Dart_CurrentIsolateData|). @@ -43,14 +43,6 @@ const IsolateCallbacks& callbacks() const { return callbacks_; } const std::string& script_uri() const { return script_uri_; } const std::string& package_root() const { return package_root_; } - std::set<MojoHandle>& unclosed_handles() { - return unclosed_handles_; - } - - const std::set<MojoHandle>& unclosed_handles() const { - return unclosed_handles_; - } - void set_library_provider(tonic::DartLibraryProvider* library_provider) { library_provider_.reset(library_provider); @@ -99,7 +91,6 @@ IsolateCallbacks callbacks_; std::string script_uri_; std::string package_root_; - std::set<MojoHandle> unclosed_handles_; std::unique_ptr<tonic::DartLibraryProvider> library_provider_; mojo::NetworkServicePtr network_service_; };
diff --git a/mojo/dart/embedder/mojo_natives.cc b/mojo/dart/embedder/mojo_natives.cc index 812ad27..f193837 100644 --- a/mojo/dart/embedder/mojo_natives.cc +++ b/mojo/dart/embedder/mojo_natives.cc
@@ -6,12 +6,10 @@ #include <string.h> #include <memory> -#include <set> #include <vector> #include "dart/runtime/include/dart_api.h" #include "mojo/dart/embedder/builtin.h" -#include "mojo/dart/embedder/mojo_dart_state.h" #include "mojo/public/c/system/core.h" #include "mojo/public/cpp/environment/logging.h" #include "mojo/public/cpp/system/core.h" @@ -38,7 +36,7 @@ V(Mojo_GetTimeTicksNow, 0) \ V(MojoHandle_Close, 1) \ V(MojoHandle_Wait, 3) \ - V(MojoHandle_Register, 2) \ + V(MojoHandle_RegisterFinalizer, 2) \ V(MojoHandle_WaitMany, 3) \ V(MojoHandleWatcher_GrowStateArrays, 1) \ V(MojoHandleWatcher_WaitMany, 2) \ @@ -137,7 +135,7 @@ // Setup a weak persistent handle for a MojoHandle that calls MojoClose on the // handle when the MojoHandle is GC'd or the VM is going down. -void MojoHandle_Register(Dart_NativeArguments arguments) { +void MojoHandle_RegisterFinalizer(Dart_NativeArguments arguments) { Dart_Handle mojo_handle_instance = Dart_GetNativeArgument(arguments, 0); if (!Dart_IsInstance(mojo_handle_instance)) { SetInvalidArgumentReturn(arguments); @@ -153,9 +151,6 @@ // Add the handle to this isolate's set. MojoHandle handle = static_cast<MojoHandle>(raw_handle); - auto isolate_data = MojoDartState::Current(); - assert(isolate_data != nullptr); - isolate_data->unclosed_handles().insert(handle); // Set up a finalizer. CloserCallbackPeer* callback_peer = new CloserCallbackPeer(); @@ -178,10 +173,6 @@ // Remove the handle from this isolate's set. MojoHandle handle = static_cast<MojoHandle>(raw_handle); - auto isolate_data = MojoDartState::Current(); - assert(isolate_data != nullptr); - isolate_data->unclosed_handles().erase(handle); - MojoResult res = MojoClose(handle); Dart_SetIntegerReturnValue(arguments, static_cast<int64_t>(res)); @@ -734,7 +725,7 @@ static MojoWaitManyState* GetInstance(); private: - DISALLOW_COPY_AND_ASSIGN(MojoWaitManyState); + MOJO_DISALLOW_COPY_AND_ASSIGN(MojoWaitManyState); }; // This global is safe because it is only accessed by the single handle watcher
diff --git a/mojo/dart/embedder/test/dart_to_cpp_tests.cc b/mojo/dart/embedder/test/dart_to_cpp_tests.cc index 90dd1f7..4f1a88d 100644 --- a/mojo/dart/embedder/test/dart_to_cpp_tests.cc +++ b/mojo/dart/embedder/test/dart_to_cpp_tests.cc
@@ -270,8 +270,12 @@ base::MessageLoop loop; base::RunLoop run_loop_; - static void UnhandledExceptionCallback(bool* exception, Dart_Handle error) { + static void UnhandledExceptionCallback(bool* exception, + int64_t* closed_handles, + Dart_Handle error, + int64_t count) { *exception = true; + *closed_handles = count; } static bool GenerateEntropy(uint8_t* buffer, intptr_t length) { @@ -285,6 +289,7 @@ const char** arguments, int arguments_count, bool* unhandled_exception, + int64_t* closed_handles, char** error) { base::FilePath path; PathService::Get(base::DIR_SOURCE_ROOT, &path); @@ -305,8 +310,8 @@ config->strict_compilation = true; config->script_uri = path.AsUTF8Unsafe(); config->package_root = package_root.AsUTF8Unsafe(); - config->callbacks.exception = - base::Bind(&UnhandledExceptionCallback, unhandled_exception); + config->callbacks.exception = base::Bind( + &UnhandledExceptionCallback, unhandled_exception, closed_handles); config->entropy = GenerateEntropy; config->handle = handle; config->SetVmFlags(arguments, arguments_count); @@ -334,14 +339,10 @@ DartControllerConfig config; char* error; bool unhandled_exception = false; + int64_t closed_handles; InitializeDartConfig( - &config, - test, - dart_side_request.PassMessagePipe().release().value(), - nullptr, - 0, - &unhandled_exception, - &error); + &config, test, dart_side_request.PassMessagePipe().release().value(), + nullptr, 0, &unhandled_exception, &closed_handles, &error); dart_thread->Start(); dart_thread->message_loop()->PostTask(FROM_HERE,
diff --git a/mojo/dart/embedder/test/run_dart_tests.cc b/mojo/dart/embedder/test/run_dart_tests.cc index e903e88..d58f942 100644 --- a/mojo/dart/embedder/test/run_dart_tests.cc +++ b/mojo/dart/embedder/test/run_dart_tests.cc
@@ -22,13 +22,19 @@ return true; } -static void exceptionCallback(bool* exception, Dart_Handle error) { +static void exceptionCallback(bool* exception, + int64_t* closed_handles, + Dart_Handle error, + int64_t count) { *exception = true; + *closed_handles = count; } static void RunTest(const std::string& test, - const char** extra_args, - int num_extra_args) { + const char** extra_args = nullptr, + int num_extra_args = 0, + bool expect_unhandled_exception = false, + int expected_unclosed_handles = 0) { base::FilePath path; PathService::Get(base::DIR_SOURCE_ROOT, &path); path = path.AppendASCII("mojo") @@ -45,6 +51,7 @@ char* error = NULL; bool unhandled_exception = false; + int64_t closed_handles = 0; DartControllerConfig config; // Run with strict compilation even in Release mode so that ASAN testing gets // coverage of Dart asserts, type-checking, etc. @@ -52,69 +59,70 @@ config.script_uri = path.AsUTF8Unsafe(); config.package_root = package_root.AsUTF8Unsafe(); config.callbacks.exception = - base::Bind(&exceptionCallback, &unhandled_exception); + base::Bind(&exceptionCallback, &unhandled_exception, &closed_handles); config.entropy = generateEntropy; config.SetVmFlags(extra_args, num_extra_args); config.error = &error; bool success = DartController::RunSingleDartScript(config); EXPECT_TRUE(success) << error; - EXPECT_FALSE(unhandled_exception); + EXPECT_EQ(expect_unhandled_exception, unhandled_exception); + EXPECT_EQ(expected_unclosed_handles, closed_handles); } // TODO(zra): instead of listing all these tests, search //mojo/dart/test for // _test.dart files. TEST(DartTest, hello_mojo) { - RunTest("hello_mojo.dart", nullptr, 0); + RunTest("hello_mojo.dart"); } TEST(DartTest, core_types_test) { - RunTest("core_types_test.dart", nullptr, 0); + RunTest("core_types_test.dart"); } TEST(DartTest, async_test) { - RunTest("async_test.dart", nullptr, 0); + RunTest("async_test.dart"); } TEST(DartTest, isolate_test) { - RunTest("isolate_test.dart", nullptr, 0); + RunTest("isolate_test.dart"); } TEST(DartTest, import_mojo) { - RunTest("import_mojo.dart", nullptr, 0); + RunTest("import_mojo.dart"); } TEST(DartTest, simple_handle_watcher_test) { - RunTest("simple_handle_watcher_test.dart", nullptr, 0); + RunTest("simple_handle_watcher_test.dart"); } TEST(DartTest, ping_pong_test) { - RunTest("ping_pong_test.dart", nullptr, 0); + RunTest("ping_pong_test.dart"); } TEST(DartTest, timer_test) { - RunTest("timer_test.dart", nullptr, 0); + RunTest("timer_test.dart"); } TEST(DartTest, async_await_test) { - RunTest("async_await_test.dart", nullptr, 0); + RunTest("async_await_test.dart"); } TEST(DartTest, core_test) { - RunTest("core_test.dart", nullptr, 0); + RunTest("core_test.dart"); } TEST(DartTest, codec_test) { - RunTest("codec_test.dart", nullptr, 0); + RunTest("codec_test.dart"); } TEST(DartTest, handle_watcher_test) { - RunTest("handle_watcher_test.dart", nullptr, 0); + RunTest("handle_watcher_test.dart"); } TEST(DartTest, bindings_generation_test) { - RunTest("bindings_generation_test.dart", nullptr, 0); + RunTest("bindings_generation_test.dart"); } TEST(DartTest, compile_all_interfaces_test) { @@ -123,15 +131,15 @@ } TEST(DartTest, uri_base_test) { - RunTest("uri_base_test.dart", nullptr, 0); + RunTest("uri_base_test.dart"); } TEST(DartTest, exception_test) { - RunTest("exception_test.dart", nullptr, 0); + RunTest("exception_test.dart"); } TEST(DartTest, control_messages_test) { - RunTest("control_messages_test.dart", nullptr, 0); + RunTest("control_messages_test.dart"); } TEST(DartTest, handle_finalizer_test) { @@ -142,6 +150,10 @@ RunTest("handle_finalizer_test.dart", args, kNumArgs); } +TEST(DartTest, unhandled_exception_test) { + RunTest("unhandled_exception_test.dart", nullptr, 0, true, 2); +} + } // namespace } // namespace dart } // namespace mojo
diff --git a/mojo/dart/embedder/test/validation_unittest.cc b/mojo/dart/embedder/test/validation_unittest.cc index 1fde4d6..3fdae14 100644 --- a/mojo/dart/embedder/test/validation_unittest.cc +++ b/mojo/dart/embedder/test/validation_unittest.cc
@@ -40,8 +40,12 @@ return true; } -void ExceptionCallback(bool* exception, Dart_Handle error) { +void ExceptionCallback(bool* exception, + int64_t* closed_handles, + Dart_Handle error, + int64_t count) { *exception = true; + *closed_handles = count; } // Enumerates files inside |path| and collects all data needed to run @@ -118,6 +122,7 @@ char* error = NULL; bool unhandled_exception = false; + int64_t closed_handles = 0; DartControllerConfig config; // Run with strict compilation even in Release mode so that ASAN testing gets // coverage of Dart asserts, type-checking, etc. @@ -125,7 +130,7 @@ config.script_uri = path.value(); config.package_root = package_root.AsUTF8Unsafe(); config.callbacks.exception = - base::Bind(&ExceptionCallback, &unhandled_exception); + base::Bind(&ExceptionCallback, &unhandled_exception, &closed_handles); config.entropy = GenerateEntropy; config.SetVmFlags(nullptr, 0); config.error = &error; @@ -134,6 +139,7 @@ bool success = DartController::RunSingleDartScript(config); EXPECT_TRUE(success) << error; EXPECT_FALSE(unhandled_exception); + EXPECT_EQ(closed_handles, 0); } TEST(DartTest, validation) {
diff --git a/mojo/dart/test/unhandled_exception_test.dart b/mojo/dart/test/unhandled_exception_test.dart new file mode 100644 index 0000000..7579c93 --- /dev/null +++ b/mojo/dart/test/unhandled_exception_test.dart
@@ -0,0 +1,10 @@ +// Copyright 2015 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. + +import 'package:mojo/core.dart'; + +void main() { + var pipe = new MojoMessagePipe(); + throw "Unhandled Exception"; +}
diff --git a/mojo/public/dart/mojo/lib/src/event_stream.dart b/mojo/public/dart/mojo/lib/src/event_stream.dart index 095564a..c56e1ad 100644 --- a/mojo/public/dart/mojo/lib/src/event_stream.dart +++ b/mojo/public/dart/mojo/lib/src/event_stream.dart
@@ -31,7 +31,7 @@ : _handle = handle, _signals = signals, _isListening = false { - MojoResult result = MojoHandle.register(this); + MojoResult result = MojoHandle.registerFinalizer(this); if (!result.isOk) { throw "Failed to register the MojoHandle: $result."; } @@ -98,6 +98,7 @@ Future _handleWatcherClose({bool immediate: false}) { assert(_handle != null); assert(MojoHandle._removeUnclosedHandle(_handle)); + MojoHandleNatives.removeUnclosed(_handle.h); return MojoHandleWatcher.close(_handle.h, wait: !immediate).then((r) { if (_receivePort != null) { _receivePort.close();
diff --git a/mojo/public/dart/mojo/lib/src/handle.dart b/mojo/public/dart/mojo/lib/src/handle.dart index 391fa84..bfc912e 100644 --- a/mojo/public/dart/mojo/lib/src/handle.dart +++ b/mojo/public/dart/mojo/lib/src/handle.dart
@@ -19,7 +19,9 @@ int get h => _h; MojoHandle(this._h, {String description}) { + // TODO(zra): Merge unclosed handle tracking. assert(_addUnclosedHandle(this, description: description)); + MojoHandleNatives.addUnclosed(_h); } MojoHandle._internal(this._h); @@ -28,6 +30,7 @@ MojoResult close() { assert(_removeUnclosedHandle(this)); + MojoHandleNatives.removeUnclosed(_h); int result = MojoHandleNatives.close(_h); _h = INVALID; return new MojoResult(result); @@ -35,6 +38,7 @@ MojoHandle pass() { assert(_removeUnclosedHandle(this)); + MojoHandleNatives.removeUnclosed(_h); return this; } @@ -87,9 +91,9 @@ return new MojoWaitManyResult(new MojoResult(result[0]), result[1], states); } - static MojoResult register(MojoEventStream eventStream) { - return new MojoResult( - MojoHandleNatives.register(eventStream, eventStream._handle.h)); + static MojoResult registerFinalizer(MojoEventStream eventStream) { + return new MojoResult(MojoHandleNatives.registerFinalizer( + eventStream, eventStream._handle.h)); } static HashMap<int, _HandleCreationRecord> _unclosedHandles = new HashMap(); @@ -104,8 +108,8 @@ stack = s; } - var handleCreate = new _HandleCreationRecord( - handle, stack, description: description); + var handleCreate = + new _HandleCreationRecord(handle, stack, description: description); _unclosedHandles[handle.h] = handleCreate; return true; }
diff --git a/mojo/public/dart/mojo/sdk_ext/src/natives.dart b/mojo/public/dart/mojo/sdk_ext/src/natives.dart index 236069e..a9ec169 100644 --- a/mojo/public/dart/mojo/sdk_ext/src/natives.dart +++ b/mojo/public/dart/mojo/sdk_ext/src/natives.dart
@@ -9,13 +9,39 @@ } class MojoHandleNatives { - static int register(Object eventStream, int handle) - native "MojoHandle_Register"; + static Set<int> _unclosedHandles = new Set<int>(); + + static void addUnclosed(int handle) { + _unclosedHandles.add(handle); + } + + static void removeUnclosed(int handle) { + _unclosedHandles.remove(handle); + } + + static int registerFinalizer(Object eventStream, int handle) + native "MojoHandle_RegisterFinalizer"; + static int close(int handle) native "MojoHandle_Close"; + static List wait(int handle, int signals, int deadline) native "MojoHandle_Wait"; + static List waitMany(List<int> handles, List<int> signals, int deadline) native "MojoHandle_WaitMany"; + + // Called from the embedder's unhandled exception callback. + // Returns the number of successfully closed handles. + static int _closeUnclosedHandles() { + int count = 0; + _unclosedHandles.forEach((h) { + if (MojoHandleNatives.close(h) == 0) { + count++; + } + }); + _unclosedHandles.clear(); + return count; + } } class MojoHandleWatcherNatives {