Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(8)

Side by Side Diff: mojo/edk/system/message_pipe_dispatcher.cc

Issue 1534013003: Fix memory leak in unittests with new Mojo EDK. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: fix Created 5 years ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View unified diff | Download patch
OLDNEW
1 // Copyright 2015 The Chromium Authors. All rights reserved. 1 // Copyright 2015 The Chromium Authors. All rights reserved.
2 // Use of this source code is governed by a BSD-style license that can be 2 // Use of this source code is governed by a BSD-style license that can be
3 // found in the LICENSE file. 3 // found in the LICENSE file.
4 4
5 #include "mojo/edk/system/message_pipe_dispatcher.h" 5 #include "mojo/edk/system/message_pipe_dispatcher.h"
6 6
7 #include "base/bind.h" 7 #include "base/bind.h"
8 #include "base/debug/stack_trace.h" 8 #include "base/debug/stack_trace.h"
9 #include "base/logging.h" 9 #include "base/logging.h"
10 #include "base/message_loop/message_loop.h" 10 #include "base/message_loop/message_loop.h"
(...skipping 136 matching lines...) Expand 10 before | Expand all | Expand 10 after
147 base::AutoLock locker(lock()); 147 base::AutoLock locker(lock());
148 CHECK(transferable_); 148 CHECK(transferable_);
149 calling_init_ = true; 149 calling_init_ = true;
150 if (channel_) 150 if (channel_)
151 channel_->Init(this); 151 channel_->Init(this);
152 calling_init_ = false; 152 calling_init_ = false;
153 } 153 }
154 154
155 void MessagePipeDispatcher::CloseOnIO() { 155 void MessagePipeDispatcher::CloseOnIO() {
156 base::AutoLock locker(lock()); 156 base::AutoLock locker(lock());
157 Release(); // To match CloseImplNoLock. 157 if (release_reference_in_close_)
158 Release(); // To match CloseImplNoLock.
158 if (transferable_) { 159 if (transferable_) {
159 if (channel_) { 160 if (channel_) {
160 channel_->Shutdown(); 161 channel_->Shutdown();
161 channel_ = nullptr; 162 channel_ = nullptr;
162 } 163 }
163 } else { 164 } else {
164 if (non_transferable_state_ == CONNECT_CALLED || 165 if (non_transferable_state_ == CONNECT_CALLED ||
165 non_transferable_state_ == WAITING_FOR_READ_OR_WRITE) { 166 non_transferable_state_ == WAITING_FOR_READ_OR_WRITE) {
166 if (non_transferable_state_ == WAITING_FOR_READ_OR_WRITE) 167 if (non_transferable_state_ == WAITING_FOR_READ_OR_WRITE)
167 RequestNontransferableChannel(); 168 RequestNontransferableChannel();
(...skipping 231 matching lines...) Expand 10 before | Expand all | Expand 10 after
399 MessagePipeDispatcher::MessagePipeDispatcher(bool transferable) 400 MessagePipeDispatcher::MessagePipeDispatcher(bool transferable)
400 : channel_(nullptr), 401 : channel_(nullptr),
401 serialized_read_fds_length_(0u), 402 serialized_read_fds_length_(0u),
402 serialized_write_fds_length_(0u), 403 serialized_write_fds_length_(0u),
403 serialized_message_fds_length_(0u), 404 serialized_message_fds_length_(0u),
404 pipe_id_(0), 405 pipe_id_(0),
405 non_transferable_state_(WAITING_FOR_READ_OR_WRITE), 406 non_transferable_state_(WAITING_FOR_READ_OR_WRITE),
406 serialized_(false), 407 serialized_(false),
407 calling_init_(false), 408 calling_init_(false),
408 write_error_(false), 409 write_error_(false),
409 transferable_(transferable) { 410 transferable_(transferable),
411 release_reference_in_close_(false) {
410 } 412 }
411 413
412 MessagePipeDispatcher::~MessagePipeDispatcher() { 414 MessagePipeDispatcher::~MessagePipeDispatcher() {
413 // |Close()|/|CloseImplNoLock()| should have taken care of the channel. The 415 // |Close()|/|CloseImplNoLock()| should have taken care of the channel. The
414 // exception is if they posted a task to run CloseOnIO but the IO thread shut 416 // exception is if they posted a task to run CloseOnIO but the IO thread shut
415 // down and so when it was deleting pending tasks it caused the last reference 417 // down and so when it was deleting pending tasks it caused the last reference
416 // to destruct this object. In that case, safe to destroy the channel. 418 // to destruct this object. In that case, safe to destroy the channel.
417 if (channel_ && 419 if (channel_ &&
418 internal::g_io_thread_task_runner->RunsTasksOnCurrentThread()) { 420 internal::g_io_thread_task_runner->RunsTasksOnCurrentThread()) {
419 if (transferable_) { 421 if (transferable_) {
(...skipping 15 matching lines...) Expand all
435 } 437 }
436 438
437 void MessagePipeDispatcher::CloseImplNoLock() { 439 void MessagePipeDispatcher::CloseImplNoLock() {
438 lock().AssertAcquired(); 440 lock().AssertAcquired();
439 // We take a manual refcount because at shutdown, the task below might not get 441 // We take a manual refcount because at shutdown, the task below might not get
440 // a chance to execute. If that happens, the RawChannel's will still call our 442 // a chance to execute. If that happens, the RawChannel's will still call our
441 // OnError method because it always runs (since it watches thread 443 // OnError method because it always runs (since it watches thread
442 // destruction). So to avoid UAF, manually add a reference and only release it 444 // destruction). So to avoid UAF, manually add a reference and only release it
443 // if the task runs. 445 // if the task runs.
444 AddRef(); 446 AddRef();
447 release_reference_in_close_ = true;
445 internal::g_io_thread_task_runner->PostTask( 448 internal::g_io_thread_task_runner->PostTask(
446 FROM_HERE, base::Bind(&MessagePipeDispatcher::CloseOnIO, this)); 449 FROM_HERE, base::Bind(&MessagePipeDispatcher::CloseOnIO, this));
447 } 450 }
448 451
449 void MessagePipeDispatcher::SerializeInternal() { 452 void MessagePipeDispatcher::SerializeInternal() {
450 serialized_ = true; 453 serialized_ = true;
451 if (!transferable_) { 454 if (!transferable_) {
452 CHECK(non_transferable_state_ == WAITING_FOR_READ_OR_WRITE) 455 CHECK(non_transferable_state_ == WAITING_FOR_READ_OR_WRITE)
453 << "Non transferable message pipe being sent after read/write/waited. " 456 << "Non transferable message pipe being sent after read/write/waited. "
454 << "MOJO_CREATE_MESSAGE_PIPE_OPTIONS_FLAG_TRANSFERABLE must be used if " 457 << "MOJO_CREATE_MESSAGE_PIPE_OPTIONS_FLAG_TRANSFERABLE must be used if "
(...skipping 453 matching lines...) Expand 10 before | Expand all | Expand 10 after
908 break; 911 break;
909 case ERROR_WRITE: 912 case ERROR_WRITE:
910 // Write errors are slightly notable: they probably shouldn't happen under 913 // Write errors are slightly notable: they probably shouldn't happen under
911 // normal operation (but maybe the other side crashed). 914 // normal operation (but maybe the other side crashed).
912 LOG(WARNING) << "MessagePipeDispatcher write error"; 915 LOG(WARNING) << "MessagePipeDispatcher write error";
913 DCHECK_EQ(write_error_, false) << "Should only get one write error."; 916 DCHECK_EQ(write_error_, false) << "Should only get one write error.";
914 write_error_ = true; 917 write_error_ = true;
915 break; 918 break;
916 } 919 }
917 920
921 bool call_release = false;
918 if (started_transport_.Try()) { 922 if (started_transport_.Try()) {
919 base::AutoLock locker(lock()); 923 base::AutoLock locker(lock());
920 // We can get two OnError callbacks before the post task below completes. 924 // We can get two OnError callbacks before the post task below completes.
921 // Although RawChannel still has a pointer to this object until Shutdown is 925 // Although RawChannel still has a pointer to this object until Shutdown is
922 // called, that is safe since this class always does a PostTask to the IO 926 // called, that is safe since this class always does a PostTask to the IO
923 // thread to self destruct. 927 // thread to self destruct.
924 if (channel_ && error != ERROR_WRITE) { 928 if (channel_ && error != ERROR_WRITE) {
925 if (transferable_) { 929 if (transferable_) {
926 channel_->Shutdown(); 930 channel_->Shutdown();
927 } else { 931 } else {
928 CHECK_NE(non_transferable_state_, CLOSED); 932 CHECK_NE(non_transferable_state_, CLOSED);
929 internal::g_broker->CloseMessagePipe(pipe_id_, this); 933 internal::g_broker->CloseMessagePipe(pipe_id_, this);
930 non_transferable_state_ = CLOSED; 934 non_transferable_state_ = CLOSED;
931 } 935 }
932 channel_ = nullptr; 936 channel_ = nullptr;
937 if (release_reference_in_close_) {
938 // CloseOnIO might never get called on process shutdown. Release the
939 // extra reference here to avoid leaks in tests.
940 call_release = true;
msw 2015/12/18 01:54:26 nit: could you actually just call Release() here a
jam 2015/12/18 02:03:47 that's what I did in patchset 1 but that failed si
941 release_reference_in_close_ = false;
942 }
933 } 943 }
934 awakable_list_.AwakeForStateChange(GetHandleSignalsStateImplNoLock()); 944 awakable_list_.AwakeForStateChange(GetHandleSignalsStateImplNoLock());
935 started_transport_.Release(); 945 started_transport_.Release();
936 } else { 946 } else {
937 // We must be waiting to call ReleaseHandle. It will call Shutdown. 947 // We must be waiting to call ReleaseHandle. It will call Shutdown.
938 } 948 }
949
950 if (call_release)
951 Release();
939 } 952 }
940 953
941 MojoResult MessagePipeDispatcher::AttachTransportsNoLock( 954 MojoResult MessagePipeDispatcher::AttachTransportsNoLock(
942 MessageInTransit* message, 955 MessageInTransit* message,
943 std::vector<DispatcherTransport>* transports) { 956 std::vector<DispatcherTransport>* transports) {
944 DCHECK(!message->has_dispatchers()); 957 DCHECK(!message->has_dispatchers());
945 958
946 // You're not allowed to send either handle to a message pipe over the message 959 // You're not allowed to send either handle to a message pipe over the message
947 // pipe, so check for this. (The case of trying to write a handle to itself is 960 // pipe, so check for this. (The case of trying to write a handle to itself is
948 // taken care of by |Core|. That case kind of makes sense, but leads to 961 // taken care of by |Core|. That case kind of makes sense, but leads to
(...skipping 48 matching lines...) Expand 10 before | Expand all | Expand 10 after
997 // PostTask since the broker can call us back synchronously. 1010 // PostTask since the broker can call us back synchronously.
998 internal::g_io_thread_task_runner->PostTask( 1011 internal::g_io_thread_task_runner->PostTask(
999 FROM_HERE, 1012 FROM_HERE,
1000 base::Bind(&Broker::ConnectMessagePipe, 1013 base::Bind(&Broker::ConnectMessagePipe,
1001 base::Unretained(internal::g_broker), pipe_id_, 1014 base::Unretained(internal::g_broker), pipe_id_,
1002 base::Unretained(this))); 1015 base::Unretained(this)));
1003 } 1016 }
1004 1017
1005 } // namespace edk 1018 } // namespace edk
1006 } // namespace mojo 1019 } // namespace mojo
OLDNEW
« content/app/mojo/mojo_init.cc ('K') | « mojo/edk/system/message_pipe_dispatcher.h ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698