From 9bae50bf47ad22ba9c6e2096fffd7fa4a2633ebb Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Tue, 31 Jan 2017 10:30:15 +0100 Subject: [PATCH] VM: [Kernel] Revert changes to the native ports that introduced peers. The problem with these changes was that closed ports still continue to receive messages and as a result we start using dead peer objects. Fixing that would require more intrusive changes into message handler implementation - so instead we are reverting the changes and restoring manual PORT -> PEER mapping. BUG= R=erikcorry@google.com Review-Url: https://codereview.chromium.org/2666063002 . --- runtime/bin/io_service.cc | 6 +- runtime/bin/io_service_no_ssl.cc | 6 +- runtime/bin/loader.cc | 12 ++-- runtime/bin/loader.h | 3 +- runtime/include/dart_native_api.h | 8 +-- runtime/vm/dart_api_impl_test.cc | 52 +++++------------ runtime/vm/kernel_isolate.cc | 60 ++++++++++++++++++-- runtime/vm/native_api_impl.cc | 5 +- runtime/vm/native_message_handler.cc | 7 +-- runtime/vm/native_message_handler.h | 6 +- samples/sample_extension/sample_extension.cc | 5 +- 11 files changed, 91 insertions(+), 79 deletions(-) diff --git a/runtime/bin/io_service.cc b/runtime/bin/io_service.cc index 121c978c5ea..2a685f1d6ad 100644 --- a/runtime/bin/io_service.cc +++ b/runtime/bin/io_service.cc @@ -27,9 +27,7 @@ namespace bin { response = type::method##Request(data); \ break; -void IOServiceCallback(Dart_Port dest_port_id, - Dart_CObject* message, - void* peer) { +void IOServiceCallback(Dart_Port dest_port_id, Dart_CObject* message) { Dart_Port reply_port_id = ILLEGAL_PORT; CObject* response = CObject::IllegalArgumentError(); CObjectArray request(message); @@ -57,7 +55,7 @@ void IOServiceCallback(Dart_Port dest_port_id, Dart_Port IOService::GetServicePort() { - return Dart_NewNativePort("IOService", IOServiceCallback, true, NULL); + return Dart_NewNativePort("IOService", IOServiceCallback, true); } diff --git a/runtime/bin/io_service_no_ssl.cc b/runtime/bin/io_service_no_ssl.cc index af9c01fd1f3..189eb0369fe 100644 --- a/runtime/bin/io_service_no_ssl.cc +++ b/runtime/bin/io_service_no_ssl.cc @@ -26,9 +26,7 @@ namespace bin { response = type::method##Request(data); \ break; -void IOServiceCallback(Dart_Port dest_port_id, - Dart_CObject* message, - void* peer) { +void IOServiceCallback(Dart_Port dest_port_id, Dart_CObject* message) { Dart_Port reply_port_id = ILLEGAL_PORT; CObject* response = CObject::IllegalArgumentError(); CObjectArray request(message); @@ -56,7 +54,7 @@ void IOServiceCallback(Dart_Port dest_port_id, Dart_Port IOService::GetServicePort() { - return Dart_NewNativePort("IOService", IOServiceCallback, true, NULL); + return Dart_NewNativePort("IOService", IOServiceCallback, true); } diff --git a/runtime/bin/loader.cc b/runtime/bin/loader.cc index f92d13dfd88..70ee44471d6 100644 --- a/runtime/bin/loader.cc +++ b/runtime/bin/loader.cc @@ -34,8 +34,7 @@ Loader::Loader(IsolateData* isolate_data) payload_length_(0) { monitor_ = new Monitor(); ASSERT(isolate_data_ != NULL); - port_ = - Dart_NewNativePort("Loader", Loader::NativeMessageHandler, false, this); + port_ = Dart_NewNativePort("Loader", Loader::NativeMessageHandler, false); isolate_data_->set_loader(this); AddLoader(port_, isolate_data_); } @@ -818,10 +817,13 @@ Loader* Loader::LoaderFor(Dart_Port port) { void Loader::NativeMessageHandler(Dart_Port dest_port_id, - Dart_CObject* message, - void* peer) { + Dart_CObject* message) { MutexLocker ml(loader_infos_lock_); - static_cast(peer)->QueueMessage(message); + Loader* loader = LoaderForLocked(dest_port_id); + if (loader == NULL) { + return; + } + loader->QueueMessage(message); } } // namespace bin diff --git a/runtime/bin/loader.h b/runtime/bin/loader.h index b5697244a61..cc4912dda70 100644 --- a/runtime/bin/loader.h +++ b/runtime/bin/loader.h @@ -142,8 +142,7 @@ class Loader { // This is the global callback for the native message handlers. static void NativeMessageHandler(Dart_Port dest_port_id, - Dart_CObject* message, - void* peer); + Dart_CObject* message); }; } // namespace bin diff --git a/runtime/include/dart_native_api.h b/runtime/include/dart_native_api.h index 12b1b1e05ae..fd0e0fb4dc1 100644 --- a/runtime/include/dart_native_api.h +++ b/runtime/include/dart_native_api.h @@ -124,8 +124,7 @@ DART_EXPORT bool Dart_PostInteger(Dart_Port port_id, int64_t message); */ typedef void (*Dart_NativeMessageHandler)(Dart_Port dest_port_id, - Dart_CObject* message, - void* peer); + Dart_CObject* message); /** * Creates a new native port. When messages are received on this @@ -136,16 +135,13 @@ typedef void (*Dart_NativeMessageHandler)(Dart_Port dest_port_id, * \param handler The C handler to run when messages arrive on the port. * \param handle_concurrently Is it okay to process requests on this * native port concurrently? - * \param peer Peer associated with this port. It will be passed down to the - handler when new message arrives. * * \return If successful, returns the port id for the native port. In * case of error, returns ILLEGAL_PORT. */ DART_EXPORT Dart_Port Dart_NewNativePort(const char* name, Dart_NativeMessageHandler handler, - bool handle_concurrently, - void* peer); + bool handle_concurrently); /* TODO(turnidge): Currently handle_concurrently is ignored. */ /** diff --git a/runtime/vm/dart_api_impl_test.cc b/runtime/vm/dart_api_impl_test.cc index fb8c433d118..eb8464594dc 100644 --- a/runtime/vm/dart_api_impl_test.cc +++ b/runtime/vm/dart_api_impl_test.cc @@ -7150,19 +7150,12 @@ TEST_CASE(ImportLibrary5) { } -void NewNativePort_send123(Dart_Port dest_port_id, - Dart_CObject* message, - void* peer) { +void NewNativePort_send123(Dart_Port dest_port_id, Dart_CObject* message) { // Gets a send port message. EXPECT_NOTNULL(message); EXPECT_EQ(Dart_CObject_kArray, message->type); EXPECT_EQ(Dart_CObject_kSendPort, message->value.as_array.values[0]->type); - // Check peer validity. - EXPECT_NOTNULL(peer); - EXPECT_EQ(static_cast(0xDEADBEEF), *static_cast(peer)); - *static_cast(peer) = 123; - // Post integer value. Dart_CObject* response = reinterpret_cast(Dart_ScopeAllocate(sizeof(Dart_CObject))); @@ -7173,19 +7166,12 @@ void NewNativePort_send123(Dart_Port dest_port_id, } -void NewNativePort_send321(Dart_Port dest_port_id, - Dart_CObject* message, - void* peer) { +void NewNativePort_send321(Dart_Port dest_port_id, Dart_CObject* message) { // Gets a null message. EXPECT_NOTNULL(message); EXPECT_EQ(Dart_CObject_kArray, message->type); EXPECT_EQ(Dart_CObject_kSendPort, message->value.as_array.values[0]->type); - // Check peer validity. - EXPECT_NOTNULL(peer); - EXPECT_EQ(static_cast(0xDEADBEEF), *static_cast(peer)); - *static_cast(peer) = 321; - // Post integer value. Dart_CObject* response = reinterpret_cast(Dart_ScopeAllocate(sizeof(Dart_CObject))); @@ -7212,13 +7198,12 @@ TEST_CASE(IllegalPost) { UNIT_TEST_CASE(NewNativePort) { // Create a port with a bogus handler. - Dart_Port error_port = Dart_NewNativePort("Foo", NULL, true, NULL); + Dart_Port error_port = Dart_NewNativePort("Foo", NULL, true); EXPECT_EQ(ILLEGAL_PORT, error_port); - intptr_t peer1 = 0xDEADBEEF; // Create the port w/o a current isolate, just to make sure that works. Dart_Port port_id1 = - Dart_NewNativePort("Port123", NewNativePort_send123, true, &peer1); + Dart_NewNativePort("Port123", NewNativePort_send123, true); TestIsolateScope __test_isolate__; const char* kScriptChars = @@ -7236,9 +7221,8 @@ UNIT_TEST_CASE(NewNativePort) { Dart_EnterScope(); // Create a port w/ a current isolate, to make sure that works too. - intptr_t peer2 = 0xDEADBEEF; Dart_Port port_id2 = - Dart_NewNativePort("Port321", NewNativePort_send321, true, &peer2); + Dart_NewNativePort("Port321", NewNativePort_send321, true); Dart_Handle send_port1 = Dart_NewSendPort(port_id1); EXPECT_VALID(send_port1); @@ -7254,7 +7238,6 @@ UNIT_TEST_CASE(NewNativePort) { EXPECT(Dart_IsError(result)); EXPECT(Dart_ErrorHasException(result)); EXPECT_SUBSTRING("Exception: 123\n", Dart_GetError(result)); - EXPECT_EQ(123, peer1); // result second port. dart_args[0] = send_port2; @@ -7264,7 +7247,6 @@ UNIT_TEST_CASE(NewNativePort) { EXPECT(Dart_IsError(result)); EXPECT(Dart_ErrorHasException(result)); EXPECT_SUBSTRING("Exception: 321\n", Dart_GetError(result)); - EXPECT_EQ(321, peer2); Dart_ExitScope(); @@ -7275,8 +7257,7 @@ UNIT_TEST_CASE(NewNativePort) { void NewNativePort_sendInteger123(Dart_Port dest_port_id, - Dart_CObject* message, - void* peer) { + Dart_CObject* message) { // Gets a send port message. EXPECT_NOTNULL(message); EXPECT_EQ(Dart_CObject_kArray, message->type); @@ -7289,8 +7270,7 @@ void NewNativePort_sendInteger123(Dart_Port dest_port_id, void NewNativePort_sendInteger321(Dart_Port dest_port_id, - Dart_CObject* message, - void* peer) { + Dart_CObject* message) { // Gets a null message. EXPECT_NOTNULL(message); EXPECT_EQ(Dart_CObject_kArray, message->type); @@ -7318,9 +7298,9 @@ TEST_CASE(NativePortPostInteger) { Dart_EnterScope(); Dart_Port port_id1 = - Dart_NewNativePort("Port123", NewNativePort_sendInteger123, true, NULL); + Dart_NewNativePort("Port123", NewNativePort_sendInteger123, true); Dart_Port port_id2 = - Dart_NewNativePort("Port321", NewNativePort_sendInteger321, true, NULL); + Dart_NewNativePort("Port321", NewNativePort_sendInteger321, true); Dart_Handle send_port1 = Dart_NewSendPort(port_id1); EXPECT_VALID(send_port1); @@ -7355,8 +7335,7 @@ TEST_CASE(NativePortPostInteger) { void NewNativePort_nativeReceiveNull(Dart_Port dest_port_id, - Dart_CObject* message, - void* peer) { + Dart_CObject* message) { EXPECT_NOTNULL(message); if ((message->type == Dart_CObject_kArray) && @@ -7386,8 +7365,8 @@ TEST_CASE(NativePortReceiveNull) { Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, NULL); Dart_EnterScope(); - Dart_Port port_id1 = Dart_NewNativePort( - "PortNull", NewNativePort_nativeReceiveNull, true, NULL); + Dart_Port port_id1 = + Dart_NewNativePort("PortNull", NewNativePort_nativeReceiveNull, true); Dart_Handle send_port1 = Dart_NewSendPort(port_id1); EXPECT_VALID(send_port1); @@ -7409,8 +7388,7 @@ TEST_CASE(NativePortReceiveNull) { void NewNativePort_nativeReceiveInteger(Dart_Port dest_port_id, - Dart_CObject* message, - void* peer) { + Dart_CObject* message) { EXPECT_NOTNULL(message); if ((message->type == Dart_CObject_kArray) && @@ -7441,8 +7419,8 @@ TEST_CASE(NativePortReceiveInteger) { Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, NULL); Dart_EnterScope(); - Dart_Port port_id1 = Dart_NewNativePort( - "PortNull", NewNativePort_nativeReceiveInteger, true, NULL); + Dart_Port port_id1 = + Dart_NewNativePort("PortNull", NewNativePort_nativeReceiveInteger, true); Dart_Handle send_port1 = Dart_NewSendPort(port_id1); EXPECT_VALID(send_port1); diff --git a/runtime/vm/kernel_isolate.cc b/runtime/vm/kernel_isolate.cc index a4af37fa4bd..d8f0a42b7a1 100644 --- a/runtime/vm/kernel_isolate.cc +++ b/runtime/vm/kernel_isolate.cc @@ -277,8 +277,11 @@ class KernelCompilationRequest : public ValueObject { : monitor_(new Monitor()), port_(Dart_NewNativePort("kernel-compilation-port", &HandleResponse, - false, - this)) { + false)), + next_(NULL), + prev_(NULL) { + ASSERT(port_ != ILLEGAL_PORT); + RegisterRequest(this); result_.status = Dart_KernelCompilationStatus_Unknown; result_.error = NULL; result_.kernel = NULL; @@ -286,6 +289,7 @@ class KernelCompilationRequest : public ValueObject { } ~KernelCompilationRequest() { + UnregisterRequest(this); Dart_CloseNativePort(port_); delete monitor_; } @@ -364,18 +368,62 @@ class KernelCompilationRequest : public ValueObject { ml.Notify(); } - static void HandleResponse(Dart_Port dest_port_id, - Dart_CObject* message, - void* peer) { - static_cast(peer)->HandleResponseImpl(message); + static void HandleResponse(Dart_Port port, Dart_CObject* message) { + MonitorLocker locker(requests_monitor_); + KernelCompilationRequest* rq = FindRequestLocked(port); + if (rq == NULL) { + return; + } + rq->HandleResponseImpl(message); } + static void RegisterRequest(KernelCompilationRequest* rq) { + MonitorLocker locker(requests_monitor_); + rq->next_ = requests_; + requests_ = rq; + } + + static void UnregisterRequest(KernelCompilationRequest* rq) { + MonitorLocker locker(requests_monitor_); + if (rq->next_ != NULL) { + rq->next_->prev_ = rq->prev_; + } + if (rq->prev_ != NULL) { + rq->prev_->next_ = rq->next_; + } else { + requests_ = rq->next_; + } + } + + // Note: Caller must hold requests_monitor_. + static KernelCompilationRequest* FindRequestLocked(Dart_Port port) { + for (KernelCompilationRequest* rq = requests_; rq != NULL; rq = rq->next_) { + if (rq->port_ == port) { + return rq; + } + } + return NULL; + } + + // This monitor must be held whenever linked list of requests is accessed. + static Monitor* requests_monitor_; + + // Linked list of all active requests. Used to find a request by port number. + // Guarded by requests_monitor_ lock. + static KernelCompilationRequest* requests_; + Monitor* monitor_; Dart_Port port_; + // Linked list of active requests. Guarded by requests_monitor_ lock. + KernelCompilationRequest* next_; + KernelCompilationRequest* prev_; + Dart_KernelCompilationResult result_; }; +Monitor* KernelCompilationRequest::requests_monitor_ = new Monitor(); +KernelCompilationRequest* KernelCompilationRequest::requests_ = NULL; Dart_KernelCompilationResult KernelIsolate::CompileToKernel( const char* script_uri) { diff --git a/runtime/vm/native_api_impl.cc b/runtime/vm/native_api_impl.cc index 13bab865499..4729375f4d5 100644 --- a/runtime/vm/native_api_impl.cc +++ b/runtime/vm/native_api_impl.cc @@ -82,8 +82,7 @@ DART_EXPORT bool Dart_PostInteger(Dart_Port port_id, int64_t message) { DART_EXPORT Dart_Port Dart_NewNativePort(const char* name, Dart_NativeMessageHandler handler, - bool handle_concurrently, - void* peer) { + bool handle_concurrently) { if (name == NULL) { name = ""; } @@ -95,7 +94,7 @@ DART_EXPORT Dart_Port Dart_NewNativePort(const char* name, // Start the native port without a current isolate. IsolateSaver saver(Isolate::Current()); - NativeMessageHandler* nmh = new NativeMessageHandler(name, handler, peer); + NativeMessageHandler* nmh = new NativeMessageHandler(name, handler); Dart_Port port_id = PortMap::CreatePort(nmh); PortMap::SetPortState(port_id, PortMap::kLivePort); nmh->Run(Dart::thread_pool(), NULL, NULL, 0); diff --git a/runtime/vm/native_message_handler.cc b/runtime/vm/native_message_handler.cc index baa073d69a3..41fbd2cabc0 100644 --- a/runtime/vm/native_message_handler.cc +++ b/runtime/vm/native_message_handler.cc @@ -12,9 +12,8 @@ namespace dart { NativeMessageHandler::NativeMessageHandler(const char* name, - Dart_NativeMessageHandler func, - void* peer) - : name_(strdup(name)), func_(func), peer_(peer) {} + Dart_NativeMessageHandler func) + : name_(strdup(name)), func_(func) {} NativeMessageHandler::~NativeMessageHandler() { @@ -42,7 +41,7 @@ MessageHandler::MessageStatus NativeMessageHandler::HandleMessage( Dart_CObject* object; ApiMessageReader reader(message); object = reader.ReadMessage(); - (*func())(message->dest_port(), object, peer()); + (*func())(message->dest_port(), object); delete message; return kOK; } diff --git a/runtime/vm/native_message_handler.h b/runtime/vm/native_message_handler.h index 270064e125e..713366261f6 100644 --- a/runtime/vm/native_message_handler.h +++ b/runtime/vm/native_message_handler.h @@ -15,14 +15,11 @@ namespace dart { // native C handlers. class NativeMessageHandler : public MessageHandler { public: - NativeMessageHandler(const char* name, - Dart_NativeMessageHandler func, - void* peer); + NativeMessageHandler(const char* name, Dart_NativeMessageHandler func); ~NativeMessageHandler(); const char* name() const { return name_; } Dart_NativeMessageHandler func() const { return func_; } - void* peer() const { return peer_; } MessageStatus HandleMessage(Message* message); @@ -37,7 +34,6 @@ class NativeMessageHandler : public MessageHandler { private: char* name_; Dart_NativeMessageHandler func_; - void* peer_; }; } // namespace dart diff --git a/samples/sample_extension/sample_extension.cc b/samples/sample_extension/sample_extension.cc index b869b800463..5af3f74ca53 100644 --- a/samples/sample_extension/sample_extension.cc +++ b/samples/sample_extension/sample_extension.cc @@ -80,8 +80,7 @@ uint8_t* randomArray(int seed, int length) { void wrappedRandomArray(Dart_Port dest_port_id, - Dart_CObject* message, - void* peer) { + Dart_CObject* message) { Dart_Port reply_port_id = ILLEGAL_PORT; if (message->type == Dart_CObject_kArray && 3 == message->value.as_array.length) { @@ -121,7 +120,7 @@ void randomArrayServicePort(Dart_NativeArguments arguments) { Dart_EnterScope(); Dart_SetReturnValue(arguments, Dart_Null()); Dart_Port service_port = - Dart_NewNativePort("RandomArrayService", wrappedRandomArray, true, NULL); + Dart_NewNativePort("RandomArrayService", wrappedRandomArray, true); if (service_port != ILLEGAL_PORT) { Dart_Handle send_port = HandleError(Dart_NewSendPort(service_port)); Dart_SetReturnValue(arguments, send_port);