diff options
| -rw-r--r-- | base/mach_ipc_mac.h | 33 | ||||
| -rw-r--r-- | base/mach_ipc_mac.mm | 13 | ||||
| -rw-r--r-- | base/process_util.h | 12 | ||||
| -rw-r--r-- | base/process_util_posix.cc | 101 | ||||
| -rw-r--r-- | chrome/app/chrome_dll_main.cc | 28 | ||||
| -rw-r--r-- | chrome/browser/child_process_launcher.cc | 36 | ||||
| -rw-r--r-- | chrome/browser/mach_broker_mac.cc | 158 | ||||
| -rw-r--r-- | chrome/browser/mach_broker_mac.h | 37 | ||||
| -rw-r--r-- | chrome/browser/mach_broker_mac_unittest.cc | 46 |
9 files changed, 296 insertions, 168 deletions
diff --git a/base/mach_ipc_mac.h b/base/mach_ipc_mac.h index 5836c3a..91b9729 100644 --- a/base/mach_ipc_mac.h +++ b/base/mach_ipc_mac.h @@ -20,7 +20,7 @@ // // The three main classes of interest are // -// MachMessage: a wrapper for a mach message of the following form +// MachMessage: a wrapper for a Mach message of the following form // mach_msg_header_t // mach_msg_body_t // optional descriptors @@ -30,10 +30,10 @@ // and are used instead of MachMessage which is an abstract base class // // ReceivePort: -// Represents a mach port for which we have receive rights +// Represents a Mach port for which we have receive rights // // MachPortSender: -// Represents a mach port for which we have send rights +// Represents a Mach port for which we have send rights // // Here's an example to receive a message on a server port: // @@ -127,7 +127,7 @@ class MachMsgPortDescriptor : public mach_msg_port_descriptor_t { }; //============================================================================== -// MachMessage: a wrapper for a mach message +// MachMessage: a wrapper for a Mach message // (mach_msg_header_t, mach_msg_body_t, extra data) // // This considerably simplifies the construction of a message for sending @@ -165,7 +165,7 @@ class MachMessage { int32_t GetMessageID() { return EndianU32_LtoN(GetDataPacket()->id); } - // Adds a descriptor (typically a mach port) to be translated + // Adds a descriptor (typically a Mach port) to be translated // returns true if successful, otherwise not enough space bool AddDescriptor(const MachMsgPortDescriptor &desc); @@ -175,7 +175,7 @@ class MachMessage { MachMsgPortDescriptor *GetDescriptor(int n); - // Convenience method which gets the mach port described by the descriptor + // Convenience method which gets the Mach port described by the descriptor mach_port_t GetTranslatedPort(int n); // A simple message is one with no descriptors @@ -212,7 +212,7 @@ class MachMessage { int CalculateSize(); // Returns total storage size that this object can grow to, this is inclusive - // of the mach header. + // of the Mach header. size_t MaxSize() const { return storage_length_bytes_; } protected: @@ -242,7 +242,7 @@ class MachMessage { //============================================================================== // MachReceiveMessage and MachSendMessage are useful to separate the idea -// of a mach message being sent and being received, and adds increased type +// of a Mach message being sent and being received, and adds increased type // safety: // ReceivePort::WaitForMessage() only accepts a MachReceiveMessage // MachPortSender::SendMessage() only accepts a MachSendMessage @@ -271,26 +271,27 @@ class MachSendMessage : public MachMessage { }; //============================================================================== -// Represents a mach port for which we have receive rights +// Represents a Mach port for which we have receive rights class ReceivePort { public: - // Creates a new mach port for receiving messages and registers a name for it + // Creates a new Mach port for receiving messages and registers a name for it explicit ReceivePort(const char *receive_port_name); - // Given an already existing mach port, use it. We take ownership of the + // Given an already existing Mach port, use it. We take ownership of the // port and deallocate it in our destructor. explicit ReceivePort(mach_port_t receive_port); - // Create a new mach port for receiving messages + // Create a new Mach port for receiving messages ReceivePort(); ~ReceivePort(); - // Waits on the mach port until message received or timeout + // Waits on the Mach port until message received or timeout. If |timeout| is + // MACH_MSG_TIMEOUT_NONE, this method waits forever. kern_return_t WaitForMessage(MachReceiveMessage *out_message, mach_msg_timeout_t timeout); - // The underlying mach port that we wrap + // The underlying Mach port that we wrap mach_port_t GetPort() const { return port_; } private: @@ -301,14 +302,14 @@ class ReceivePort { }; //============================================================================== -// Represents a mach port for which we have send rights +// Represents a Mach port for which we have send rights class MachPortSender { public: // get a port with send rights corresponding to a named registered service explicit MachPortSender(const char *receive_port_name); - // Given an already existing mach port, use it. Does not take ownership of + // Given an already existing Mach port, use it. Does not take ownership of // |send_port|. explicit MachPortSender(mach_port_t send_port); diff --git a/base/mach_ipc_mac.mm b/base/mach_ipc_mac.mm index 973b66a..a0bfdc8 100644 --- a/base/mach_ipc_mac.mm +++ b/base/mach_ipc_mac.mm @@ -198,7 +198,12 @@ ReceivePort::ReceivePort(const char *receive_port_name) { if (init_result_ != KERN_SUCCESS) return; - NSPort *ns_port = [NSMachPort portWithMachPort:port_]; + // Without |NSMachPortDeallocateNone|, the NSMachPort seems to deallocate + // receive rights on port when it is eventually released. It is not necessary + // to deallocate any rights here as |port_| is fully deallocated in the + // ReceivePort destructor. + NSPort *ns_port = [NSMachPort portWithMachPort:port_ + options:NSMachPortDeallocateNone]; NSString *port_name = [NSString stringWithUTF8String:receive_port_name]; [[NSMachBootstrapServer sharedInstance] registerPort:ns_port name:port_name]; } @@ -252,8 +257,12 @@ kern_return_t ReceivePort::WaitForMessage(MachReceiveMessage *out_message, out_message->Head()->msgh_reserved = 0; out_message->Head()->msgh_id = 0; + mach_msg_option_t rcv_options = MACH_RCV_MSG; + if (timeout != MACH_MSG_TIMEOUT_NONE) + rcv_options |= MACH_RCV_TIMEOUT; + kern_return_t result = mach_msg(out_message->Head(), - MACH_RCV_MSG | MACH_RCV_TIMEOUT, + rcv_options, 0, out_message->MaxSize(), port_, diff --git a/base/process_util.h b/base/process_util.h index 4699c9e2..e64d0a7 100644 --- a/base/process_util.h +++ b/base/process_util.h @@ -235,18 +235,6 @@ bool LaunchApp(const std::vector<std::string>& argv, // The returned array is allocated using new[] and must be freed by the caller. char** AlterEnvironment(const environment_vector& changes, const char* const* const env); - -#if defined(OS_MACOSX) -// Similar to the above, but also returns the new process's task_t if -// |task_handle| is not NULL. If |task_handle| is not NULL, the caller is -// responsible for calling |mach_port_deallocate()| on the returned handle. -bool LaunchAppAndGetTask(const std::vector<std::string>& argv, - const environment_vector& environ, - const file_handle_mapping_vector& fds_to_remap, - bool wait, - task_t* task_handle, - ProcessHandle* process_handle); -#endif // defined(OS_MACOSX) #endif // defined(OS_POSIX) // Executes the application specified by cl. This function delegates to one diff --git a/base/process_util_posix.cc b/base/process_util_posix.cc index 8fe64d0..d6171b1 100644 --- a/base/process_util_posix.cc +++ b/base/process_util_posix.cc @@ -32,8 +32,6 @@ #if defined(OS_MACOSX) #include <crt_externs.h> #define environ (*_NSGetEnviron()) -#include "base/mach_ipc_mac.h" -#include "base/rand_util.h" #else extern char** environ; #endif @@ -301,82 +299,6 @@ void CloseSuperfluousFds(const base::InjectiveMultimap& saved_mapping) { } } -#if defined(OS_MACOSX) -static std::string MachErrorCode(kern_return_t err) { - return StringPrintf("0x%x %s", err, mach_error_string(err)); -} - -// Forks the current process and returns the child's |task_t| in the parent -// process. -static pid_t fork_and_get_task(task_t* child_task) { - const int kTimeoutMs = 100; - kern_return_t err; - - // Put a random number into the channel name, so that a compromised renderer - // can't pretend being the child that's forked off. - std::string mach_connection_name = StringPrintf( - "com.google.Chrome.samplingfork.%p.%d", - child_task, base::RandInt(0, std::numeric_limits<int>::max())); - - // Create the mach receive port before forking to ensure that it exists when - // the child tries to connect. Mach ports are not duped into the child, so - // this is safe to set up here. - ReceivePort parent_recv_port(mach_connection_name.c_str()); - - // Error handling philosophy: If Mach IPC fails, don't touch |child_task| but - // return a valid pid. If IPC fails in the child, the parent will have to wait - // until kTimeoutMs is over. This is not optimal, but I've never seen it - // happen, and stuff should still mostly work. - pid_t pid = fork(); - switch (pid) { - case -1: - return pid; - case 0: { // child - // Must reset signal handlers before doing any mach IPC, as the mach IPC - // calls can potentially hang forever. - ResetChildSignalHandlersToDefaults(); - MachSendMessage child_message(/* id= */0); - if (!child_message.AddDescriptor(mach_task_self())) { - LOG(ERROR) << "child AddDescriptor(mach_task_self()) failed."; - return pid; - } - - MachPortSender child_sender(mach_connection_name.c_str()); - err = child_sender.SendMessage(child_message, kTimeoutMs); - if (err != KERN_SUCCESS) { - LOG(ERROR) << "child SendMessage() failed: " << MachErrorCode(err); - return pid; - } - break; - } - default: { // parent - MachReceiveMessage child_message; - err = parent_recv_port.WaitForMessage(&child_message, kTimeoutMs); - if (err != KERN_SUCCESS) { - LOG(ERROR) << "parent WaitForMessage() failed: " << MachErrorCode(err); - return pid; - } - - if (child_message.GetTranslatedPort(0) == MACH_PORT_NULL) { - LOG(ERROR) << "parent GetTranslatedPort(0) failed."; - return pid; - } - *child_task = child_message.GetTranslatedPort(0); - break; - } - } - return pid; -} - -bool LaunchApp(const std::vector<std::string>& argv, - const environment_vector& env_changes, - const file_handle_mapping_vector& fds_to_remap, - bool wait, ProcessHandle* process_handle) { - return LaunchAppAndGetTask( - argv, env_changes, fds_to_remap, wait, NULL, process_handle); -} -#endif // defined(OS_MACOSX) - char** AlterEnvironment(const environment_vector& changes, const char* const* const env) { unsigned count = 0; @@ -498,18 +420,11 @@ char** AlterEnvironment(const environment_vector& changes, return ret; } -#if defined(OS_MACOSX) -bool LaunchAppAndGetTask( -#else bool LaunchApp( -#endif const std::vector<std::string>& argv, const environment_vector& env_changes, const file_handle_mapping_vector& fds_to_remap, bool wait, -#if defined(OS_MACOSX) - task_t* task_handle, -#endif ProcessHandle* process_handle) { pid_t pid; InjectiveMultimap fd_shuffle1, fd_shuffle2; @@ -518,20 +433,7 @@ bool LaunchApp( scoped_array<char*> argv_cstr(new char*[argv.size() + 1]); scoped_array<char*> new_environ(AlterEnvironment(env_changes, environ)); -#if defined(OS_MACOSX) - if (task_handle == NULL) { - pid = fork(); - } else { - // On OS X, the task_t for a process is needed for several reasons. Sadly, - // the function task_for_pid() requires privileges a normal user doesn't - // have. Instead, a short-lived Mach IPC connection is opened between parent - // and child, and the child sends its task_t to the parent at fork time. - *task_handle = MACH_PORT_NULL; - pid = fork_and_get_task(task_handle); - } -#else pid = fork(); -#endif if (pid < 0) return false; @@ -541,10 +443,7 @@ bool LaunchApp( RestoreDefaultExceptionHandler(); #endif - // On mac, the signal handlers are reset in |fork_and_get_task()|. -#if !defined(OS_MACOSX) ResetChildSignalHandlersToDefaults(); -#endif #if 0 // When debugging it can be helpful to check that we really aren't making diff --git a/chrome/app/chrome_dll_main.cc b/chrome/app/chrome_dll_main.cc index 68e8048..70dacf7 100644 --- a/chrome/app/chrome_dll_main.cc +++ b/chrome/app/chrome_dll_main.cc @@ -80,8 +80,10 @@ #if defined(OS_MACOSX) #include "app/l10n_util_mac.h" #include "base/mac_util.h" -#include "chrome/common/chrome_paths_internal.h" +#include "base/mach_ipc_mac.h" #include "chrome/app/breakpad_mac.h" +#include "chrome/browser/mach_broker_mac.h" +#include "chrome/common/chrome_paths_internal.h" #include "grit/chromium_strings.h" #include "third_party/WebKit/WebKit/mac/WebCoreSupport/WebSystemInterface.h" #endif @@ -409,6 +411,29 @@ void SetMacProcessName(const std::string& process_type) { mac_util::SetProcessName(reinterpret_cast<CFStringRef>(app_name)); } } + +// Completes the Mach IPC handshake by sending this process' task port to the +// parent process. The parent is listening on the Mach port given by +// |GetMachPortName()|. The task port is used by the parent to get CPU/memory +// stats to display in the task manager. +void SendTaskPortToParentProcess() { + const mach_msg_timeout_t kTimeoutMs = 100; + const int32_t kMessageId = 0; + std::string mach_port_name = MachBroker::GetMachPortName(); + + base::MachSendMessage child_message(kMessageId); + if (!child_message.AddDescriptor(mach_task_self())) { + LOG(ERROR) << "child AddDescriptor(mach_task_self()) failed."; + return; + } + + base::MachPortSender child_sender(mach_port_name.c_str()); + kern_return_t err = child_sender.SendMessage(child_message, kTimeoutMs); + if (err != KERN_SUCCESS) { + LOG(ERROR) << StringPrintf("child SendMessage() failed: 0x%x %s", err, + mach_error_string(err)); + } +} #endif // defined(OS_MACOSX) void InitializeStatsTable(base::ProcessId browser_pid, @@ -588,6 +613,7 @@ int ChromeMain(int argc, char** argv) { DCHECK_NE(browser_pid, 0u); #elif defined(OS_MACOSX) browser_pid = base::GetCurrentProcId(); + SendTaskPortToParentProcess(); #elif defined(OS_POSIX) // On linux, we're in the zygote here; so we need the parent process' id. browser_pid = base::GetParentProcessId(base::GetCurrentProcId()); diff --git a/chrome/browser/child_process_launcher.cc b/chrome/browser/child_process_launcher.cc index f24244f..ad464c1 100644 --- a/chrome/browser/child_process_launcher.cc +++ b/chrome/browser/child_process_launcher.cc @@ -7,6 +7,7 @@ #include <utility> // For std::pair. #include "base/command_line.h" +#include "base/lock.h" #include "base/logging.h" #include "base/scoped_ptr.h" #include "base/thread.h" @@ -161,20 +162,29 @@ class ChildProcessLauncher::Context } #endif // defined(OS_LINUX) - // Actually launch the app. - bool launched; + bool launched = false; #if defined(OS_MACOSX) - task_t child_task; - launched = base::LaunchAppAndGetTask( - cmd_line->argv(), env, fds_to_map, false, &child_task, &handle); - if (launched && child_task != MACH_PORT_NULL) { - MachBroker::instance()->RegisterPid( - handle, - MachBroker::MachInfo().SetTask(child_task)); - } -#else - launched = base::LaunchApp(cmd_line->argv(), env, fds_to_map, - /* wait= */false, &handle); + // It is possible for the child process to die immediately after + // launching. To prevent leaking MachBroker map entries in this case, + // lock around all of LaunchApp(). If the child dies, the death + // notification will be processed by the MachBroker after the call to + // AddPlaceholderForPid(), enabling proper cleanup. + { // begin scope for AutoLock + MachBroker* broker = MachBroker::instance(); + AutoLock lock(broker->GetLock()); + + // This call to |PrepareForFork()| will start the MachBroker listener + // thread, if it is not already running. Therefore the browser process + // will be listening for Mach IPC before LaunchApp() is called. + broker->PrepareForFork(); +#endif + // Actually launch the app. + launched = base::LaunchApp(cmd_line->argv(), env, fds_to_map, + /* wait= */false, &handle); +#if defined(OS_MACOSX) + if (launched) + broker->AddPlaceholderForPid(handle); + } // end scope for AutoLock #endif if (!launched) handle = base::kNullProcessHandle; diff --git a/chrome/browser/mach_broker_mac.cc b/chrome/browser/mach_broker_mac.cc index 39f3fce..6c24541 100644 --- a/chrome/browser/mach_broker_mac.cc +++ b/chrome/browser/mach_broker_mac.cc @@ -4,13 +4,26 @@ #include "chrome/browser/mach_broker_mac.h" +#include "base/command_line.h" #include "base/logging.h" +#include "base/mach_ipc_mac.h" +#include "base/platform_thread.h" +#include "base/string_util.h" +#include "base/sys_string_conversions.h" #include "chrome/browser/chrome_thread.h" #include "chrome/browser/extensions/extension_host.h" #include "chrome/browser/renderer_host/render_process_host.h" #include "chrome/common/child_process_info.h" +#include "chrome/common/chrome_switches.h" #include "chrome/common/notification_service.h" +namespace { +// Prints a string representation of a Mach error code. +std::string MachErrorCode(kern_return_t err) { + return StringPrintf("0x%x %s", err, mach_error_string(err)); +} +} // namespace + // Required because notifications happen on the UI thread. class RegisterNotificationTask : public Task { public: @@ -38,28 +51,124 @@ class RegisterNotificationTask : public Task { private: MachBroker* broker_; + DISALLOW_COPY_AND_ASSIGN(RegisterNotificationTask); }; -MachBroker::MachBroker() { - ChromeThread::PostTask( - ChromeThread::UI, FROM_HERE, new RegisterNotificationTask(this)); -} +class MachListenerThreadDelegate : public PlatformThread::Delegate { + public: + MachListenerThreadDelegate(MachBroker* broker) : broker_(broker) { + DCHECK(broker_); + std::string port_name = MachBroker::GetMachPortName(); + + // Create the receive port in the constructor, not in ThreadMain(). It is + // important to create and register the receive port before starting the + // thread so that child processes will always have someone who's listening. + receive_port_.reset(new base::ReceivePort(port_name.c_str())); + } + + // Implement |PlatformThread::Delegate|. + void ThreadMain() { + base::MachReceiveMessage message; + kern_return_t err; + while ((err = receive_port_->WaitForMessage(&message, + MACH_MSG_TIMEOUT_NONE)) == + KERN_SUCCESS) { + // 0 was the secret message id. Reject any messages that don't have it. + if (message.GetMessageID() != 0) { + LOG(ERROR) << "Received message with incorrect id: " + << message.GetMessageID(); + continue; + } + + const task_t child_task = message.GetTranslatedPort(0); + if (child_task == MACH_PORT_NULL) { + LOG(ERROR) << "parent GetTranslatedPort(0) failed."; + continue; + } + + // It is possible for the child process to die after the call to + // |pid_for_task()| but before the call to |FinalizePid()|. To prevent + // leaking MachBroker map entries in this case, lock around both these + // calls. If the child dies, the death notification will be processed + // after the call to FinalizePid(), ensuring proper cleanup. + AutoLock lock(broker_->GetLock()); + + int pid; + err = pid_for_task(child_task, &pid); + if (err == KERN_SUCCESS) { + broker_->FinalizePid(pid, + MachBroker::MachInfo().SetTask(child_task)); + } else { + LOG(ERROR) << "Error getting pid for task " << child_task + << ": " << MachErrorCode(err); + } + } + + LOG(ERROR) << "Mach listener thread exiting; " + << "parent WaitForMessage() likely failed: " + << MachErrorCode(err); + } + + private: + // The Mach port to listen on. Created on thread startup. + scoped_ptr<base::ReceivePort> receive_port_; + + // The MachBroker to use when new child task rights are received. Can be + // NULL. + MachBroker* broker_; // weak + + DISALLOW_COPY_AND_ASSIGN(MachListenerThreadDelegate); +}; // Returns the global MachBroker. MachBroker* MachBroker::instance() { - return Singleton<MachBroker>::get(); + return Singleton<MachBroker, LeakySingletonTraits<MachBroker> >::get(); } -// Adds mach info for a given pid. -void MachBroker::RegisterPid( - base::ProcessHandle pid, const MachInfo& mach_info) { - AutoLock lock(lock_); +MachBroker::MachBroker() : listener_thread_started_(false) { +} + +void MachBroker::PrepareForFork() { + if (!listener_thread_started_) { + listener_thread_started_ = true; + + ChromeThread::PostTask( + ChromeThread::UI, FROM_HERE, new RegisterNotificationTask(this)); + + // Intentional leak. This thread is never joined or reaped. + PlatformThread::CreateNonJoinable(0, new MachListenerThreadDelegate(this)); + } +} + +// Adds a placeholder to the map for the given pid with MACH_PORT_NULL. +void MachBroker::AddPlaceholderForPid(base::ProcessHandle pid) { + lock_.AssertAcquired(); + + MachInfo mach_info; DCHECK_EQ(0u, mach_map_.count(pid)); mach_map_[pid] = mach_info; } +// Updates the mapping for |pid| to include the given |mach_info|. +void MachBroker::FinalizePid(base::ProcessHandle pid, + const MachInfo& mach_info) { + lock_.AssertAcquired(); + + const int count = mach_map_.count(pid); + if (count == 0) { + // Do nothing for unknown pids. + LOG(ERROR) << "Unknown process " << pid << " is sending Mach IPC messages!"; + return; + } + + DCHECK_EQ(1, count); + DCHECK(mach_map_[pid].mach_task_ == MACH_PORT_NULL); + if (mach_map_[pid].mach_task_ == MACH_PORT_NULL) + mach_map_[pid] = mach_info; +} + // Removes all mappings belonging to |pid| from the broker. -void MachBroker::Invalidate(base::ProcessHandle pid) { +void MachBroker::InvalidatePid(base::ProcessHandle pid) { AutoLock lock(lock_); MachBroker::MachMap::iterator it = mach_map_.find(pid); if (it == mach_map_.end()) @@ -69,10 +178,14 @@ void MachBroker::Invalidate(base::ProcessHandle pid) { it->second.mach_task_); LOG_IF(WARNING, kr != KERN_SUCCESS) << "Failed to mach_port_deallocate mach task " << it->second.mach_task_ - << ", error " << kr; + << ", error " << MachErrorCode(kr); mach_map_.erase(it); } +Lock& MachBroker::GetLock() { + return lock_; +} + // Returns the mach task belonging to |pid|. mach_port_t MachBroker::TaskForPid(base::ProcessHandle pid) const { AutoLock lock(lock_); @@ -85,6 +198,9 @@ mach_port_t MachBroker::TaskForPid(base::ProcessHandle pid) const { void MachBroker::Observe(NotificationType type, const NotificationSource& source, const NotificationDetails& details) { + // TODO(rohitrao): These notifications do not always carry the proper PIDs, + // especially when the renderer is already gone or has crashed. Find a better + // way to listen for child process deaths. http://crbug.com/55734 base::ProcessHandle handle = 0; switch (type.value) { case NotificationType::RENDERER_PROCESS_CLOSED: @@ -103,5 +219,23 @@ void MachBroker::Observe(NotificationType type, NOTREACHED() << "Unexpected notification"; break; } - Invalidate(handle); + InvalidatePid(handle); +} + +// static +std::string MachBroker::GetMachPortName() { + static const char kFormatString[] = +#if defined(GOOGLE_CHROME_BUILD) + "com.google.Chrome" +#else + "org.chromium.Chromium" +#endif + ".rohitfork.%d"; + + const CommandLine& command_line = *CommandLine::ForCurrentProcess(); + const bool is_child = command_line.HasSwitch(switches::kProcessType); + + // In non-browser (child) processes, use the parent's pid. + const pid_t pid = is_child ? getppid() : getpid(); + return StringPrintf(kFormatString, pid); } diff --git a/chrome/browser/mach_broker_mac.h b/chrome/browser/mach_broker_mac.h index 58ef3e1..7b0257f 100644 --- a/chrome/browser/mach_broker_mac.h +++ b/chrome/browser/mach_broker_mac.h @@ -7,6 +7,7 @@ #pragma once #include <map> +#include <string> #include <mach/mach.h> @@ -37,6 +38,10 @@ class MachBroker : public base::ProcessMetrics::PortProvider, // Returns the global MachBroker. static MachBroker* instance(); + // Performs any necessary setup that cannot happen in the constructor. + // Clients MUST call this method before fork()ing any children. + void PrepareForFork(); + struct MachInfo { MachInfo() : mach_task_(MACH_PORT_NULL) {} @@ -48,11 +53,28 @@ class MachBroker : public base::ProcessMetrics::PortProvider, mach_port_t mach_task_; }; - // Adds mach info for a given pid. - void RegisterPid(base::ProcessHandle pid, const MachInfo& mach_info); + // Adds a placeholder to the map for the given pid with MACH_PORT_NULL. + // Callers are expected to later update the port with FinalizePid(). Callers + // MUST acquire the lock given by GetLock() before calling this method (and + // release the lock afterwards). + void AddPlaceholderForPid(base::ProcessHandle pid); + + // Updates the mapping for |pid| to include the given |mach_info|. Does + // nothing if PlaceholderForPid() has not already been called for the given + // |pid|. Callers MUST acquire the lock given by GetLock() before calling + // this method (and release the lock afterwards). + void FinalizePid(base::ProcessHandle pid, const MachInfo& mach_info); // Removes all mappings belonging to |pid| from the broker. - void Invalidate(base::ProcessHandle pid); + void InvalidatePid(base::ProcessHandle pid); + + // The lock that protects this MachBroker object. Clients MUST acquire and + // release this lock around calls to PlaceholderForPid() and FinalizePid(). + Lock& GetLock(); + + // Returns the Mach port name to use when sending or receiving messages. + // Does the Right Thing in the browser and in child processes. + static std::string GetMachPortName(); // Implement |ProcessMetrics::PortProvider|. virtual mach_port_t TaskForPid(base::ProcessHandle process) const; @@ -65,13 +87,13 @@ class MachBroker : public base::ProcessMetrics::PortProvider, // Private constructor. MachBroker(); + // True if the listener thread has been started. + bool listener_thread_started_; + // Used to register for notifications received by NotificationObserver. // Accessed only on the UI thread. NotificationRegistrar registrar_; - friend struct DefaultSingletonTraits<MachBroker>; - friend class MachBrokerTest; - // Stores mach info for every process in the broker. typedef std::map<base::ProcessHandle, MachInfo> MachMap; MachMap mach_map_; @@ -79,7 +101,10 @@ class MachBroker : public base::ProcessMetrics::PortProvider, // Mutex that guards |mach_map_|. mutable Lock lock_; + friend class MachBrokerTest; friend class RegisterNotificationTask; + // Needed in order to make the constructor private. + friend struct DefaultSingletonTraits<MachBroker>; DISALLOW_COPY_AND_ASSIGN(MachBroker); }; diff --git a/chrome/browser/mach_broker_mac_unittest.cc b/chrome/browser/mach_broker_mac_unittest.cc index 56cc2ae2..b0e5b05 100644 --- a/chrome/browser/mach_broker_mac_unittest.cc +++ b/chrome/browser/mach_broker_mac_unittest.cc @@ -4,21 +4,57 @@ #include "chrome/browser/mach_broker_mac.h" +#include "base/lock.h" #include "testing/gtest/include/gtest/gtest.h" class MachBrokerTest : public testing::Test { public: + // Helper function to acquire/release locks and call |PlaceholderForPid()|. + void AddPlaceholderForPid(base::ProcessHandle pid) { + AutoLock lock(broker_.GetLock()); + broker_.AddPlaceholderForPid(pid); + } + + // Helper function to acquire/release locks and call |FinalizePid()|. + void FinalizePid(base::ProcessHandle pid, + const MachBroker::MachInfo& mach_info) { + AutoLock lock(broker_.GetLock()); + broker_.FinalizePid(pid, mach_info); + } + + protected: MachBroker broker_; }; -TEST_F(MachBrokerTest, Setter) { - broker_.RegisterPid(1u, MachBroker::MachInfo().SetTask(2u)); - EXPECT_EQ(2u, broker_.TaskForPid(1)); +TEST_F(MachBrokerTest, Locks) { + // Acquire and release the locks. Nothing bad should happen. + AutoLock lock(broker_.GetLock()); +} + +TEST_F(MachBrokerTest, AddPlaceholderAndFinalize) { + // Add a placeholder for PID 1. + AddPlaceholderForPid(1); + EXPECT_EQ(0u, broker_.TaskForPid(1)); + + // Finalize PID 1. + FinalizePid(1, MachBroker::MachInfo().SetTask(100u)); + EXPECT_EQ(100u, broker_.TaskForPid(1)); + + // Should be no entry for PID 2. EXPECT_EQ(0u, broker_.TaskForPid(2)); } TEST_F(MachBrokerTest, Invalidate) { - broker_.RegisterPid(1u, MachBroker::MachInfo().SetTask(2)); - broker_.Invalidate(1u); + AddPlaceholderForPid(1); + FinalizePid(1, MachBroker::MachInfo().SetTask(100u)); + + EXPECT_EQ(100u, broker_.TaskForPid(1)); + broker_.InvalidatePid(1u); EXPECT_EQ(0u, broker_.TaskForPid(1)); } + +TEST_F(MachBrokerTest, FinalizeUnknownPid) { + // Finalizing an entry for an unknown pid should not add it to the map. + FinalizePid(1u, MachBroker::MachInfo().SetTask(100u)); + EXPECT_EQ(0u, broker_.TaskForPid(1u)); +} |