Mac: Proof-of-concept task manager

chrome/browser/child_process_launcher.cc

Index: chrome/browser/child_process_launcher.cc
diff --git a/chrome/browser/child_process_launcher.cc b/chrome/browser/child_process_launcher.cc
index 1ab66c63dec021b27bfd78b589bf2de0866caf37..2573c4f8a6087cd8c068a196a7ac92fbeefd3cee 100644
--- a/chrome/browser/child_process_launcher.cc
+++ b/chrome/browser/child_process_launcher.cc
@@ -27,6 +27,93 @@
 #include "base/global_descriptors_posix.h"
 #endif
 
+#if defined(OS_MACOSX)
+#include "ipc/ipc_switches.h"
+#include "chrome/browser/mach_broker_mac.h"
+#include "chrome/common/mach_ipc_mac.h"
+#endif
+
+#if defined(OS_MACOSX)
+class MachTask : public Task {
+ public:
+  MachTask(std::string channel_name, mach_port_t* task)
+      : task_(task) {
+    // TODO(thakis): Move some place central

[Mark Mentovai, 2010/01/11 20:22:46]

Wanna do this now?

+    const std::string kMachChannelPrefix = "com.Google.Chrome";
+    std::string channel = kMachChannelPrefix + channel_name;

[Mark Mentovai, 2010/01/11 20:22:46]

Unless channel_name begins with a dot, this will give us names like
"com.Google.ChromeMyNewAwesomeChannelName".

+
+    MachBroker::instance();  // make sure registrations are registered on the right thread.

[Mark Mentovai, 2010/01/11 20:22:46]

80

+
+    // This creates our named server port -- needs to happen on the current
+    // thread.
+printf("Creating receive port %s\n", channel.c_str());

[Mark Mentovai, 2010/01/11 20:22:46]

Debugging turdlet?

(I leave my debugging turdlets flush with the left margin too, to make 'em
easier to spot when I'm done debugging.)

+    port_.reset(new ReceivePort(channel.c_str()));
+  }
+
+  virtual void Run() {
+    // TODO(thakis): Move some place central
+    const int kMachPortMessageID = 57;

[Mark Mentovai, 2010/01/11 20:22:46]

Magic?

+
+    const int kMachPortMessageReceiveWaitMs = 1000;
+
+    // TODO(thakis): time histogram between creation and port reception?
+    MachReceiveMessage message;
+    kern_return_t result = port_->WaitForMessage(
+        &message, kMachPortMessageReceiveWaitMs);
+    if (result == KERN_SUCCESS) {
+      CHECK(kMachPortMessageID == message.GetMessageID());

[Mark Mentovai, 2010/01/11 20:22:46]

CHECK_EQ?

+      CHECK(1 == message.GetDescriptorCount());

[Mark Mentovai, 2010/01/11 20:22:46]

CHECK_EQ?

+      *task_ = message.GetTranslatedPort(0);
+      printf("yay\n");

[Mark Mentovai, 2010/01/11 20:22:46]

Debugging turdlet?

+    } else {
+      // TODO(thakis): Log somewhere?
+      *task_ = 0;
+      printf("nay\n");

[Mark Mentovai, 2010/01/11 20:22:46]

LOG?

+    }
+  }
+
+ private:
+  scoped_ptr<ReceivePort> port_;
+  mach_port_t* task_;
+};
+
+class MachTask2 : public Task {

[Mark Mentovai, 2010/01/11 20:22:46]

Class-level comments would be nice for this and for MachTask, so that a casual
reader can tell the difference between the two classes.

+ public:
+  MachTask2(const mach_port_t& task, base::ProcessHandle pid)  // must be refs, might be 0 at call time and only be set later on the other thread

[Mark Mentovai, 2010/01/11 20:22:46]

80

+      : task_(task), pid_(pid) {}
+
+  virtual void Run() {
+    if (task_)

[Mark Mentovai, 2010/01/11 20:22:46]

You need {} here because this thing spills over a bunch of lines.

+      MachBroker::instance()->RegisterPid(
+          pid_,
+          MachBroker::MachInfo().SetTask(task_));
+  }
+ private:
+  const mach_port_t& task_;
+  base::ProcessHandle pid_;
+};
+
+class MachThread : public base::Thread {
+ public:
+  MachThread() : base::Thread("MachThread"), task_(0) {}

[Mark Mentovai, 2010/01/11 20:22:46]

task_(MACH_PORT_NULL) ?

+
+  void DoIt(const std::string& channel_name) {
+    DCHECK(message_loop());
+    message_loop()->PostTask(FROM_HERE,
+                             new MachTask(channel_name, &task_));
+  }
+
+  void DoIt2(base::ProcessHandle pid) {

[Mark Mentovai, 2010/01/11 20:22:46]

Oh, so is this just to avoid the ban on overloading?

DoItChannelName and DoItPid would be better names, then.

"Do it.  Do it.  DO IT."

+    DCHECK(message_loop());
+    message_loop()->PostTask(FROM_HERE,
+                             new MachTask2(task_, pid));
+  }
+
+ private:
+  mach_port_t task_;
+};
+#endif
+
 // Having the functionality of ChildProcessLauncher be in an internal
 // ref counted object allows us to automatically terminate the process when the
 // parent class destructs, while still holding on to state that we need.
@@ -160,9 +247,37 @@ class ChildProcessLauncher::Context
       }
 #endif  // defined(OS_LINUX)
 
+#if defined(OS_MACOSX)
+      // TODO(thakis): Possibly somewhere else?
+      // (then again, the fds duping stuff is here too, so maybe it's ok)
+      bool is_renderer = cmd_line->GetSwitchValueASCII(switches::kProcessType)
+          == switches::kRendererProcess;
+    bool is_extension = cmd_line->GetSwitchValueASCII(switches::kProcessType) ==

[Mark Mentovai, 2010/01/11 20:22:46]

Fix indent?

This whole section (253-256) should be broken up somehow to make it a little
more readable.  For example, factor out the GetSwitchValueASCII call?

+        switches::kExtensionProcess;
+
+for (size_t ii = 0; ii < cmd_line->argv().size(); ++ii)

[Mark Mentovai, 2010/01/11 20:22:46]

Debugging turdlets?

+  fprintf(stderr, "%s ", cmd_line->argv()[ii].c_str());
+fprintf(stderr, "\n");
+fprintf(stderr, "child type: %s\n", cmd_line->GetSwitchValueASCII(switches::kProcessType).c_str());
+      MachThread mach_thread;

[Mark Mentovai, 2010/01/11 20:22:46]

Fix the indent in here too?

+      if (true || is_renderer || is_extension) {

[Mark Mentovai, 2010/01/11 20:22:46]

Remove "true" - debugging turdlet?

+        CHECK(mach_thread.Start());
+        mach_thread.DoIt(
+            cmd_line->GetSwitchValueASCII(switches::kProcessChannelID));
+      }
+#endif
+
+
       // Actually launch the app.
       if (!base::LaunchApp(cmd_line->argv(), env, fds_to_map, false, &handle))
         handle = base::kNullProcessHandle;
+
+#if defined(OS_MACOSX)
+      if (true || is_renderer || is_extension) {

[Mark Mentovai, 2010/01/11 20:22:46]

true

+        // TODO(thakis): Check |handle| first.
+        mach_thread.DoIt2(handle);
+      }
+#endif
     }
 #endif
 
@@ -277,7 +392,7 @@ bool ChildProcessLauncher::IsStarting() {
 }
 
 base::ProcessHandle ChildProcessLauncher::GetHandle() {
-  DCHECK(!context_->starting_);
+//  DCHECK(!context_->starting_);

[Mark Mentovai, 2010/01/11 20:22:46]

?

   return context_->process_.handle();
 }