Clean up ChildProcessHost unique id generation.

BUG=none
TEST=none

Review URL: https://codereview.chromium.org/126033004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@243683 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/signin/signin_manager.cc

@@ -33,6 +33,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/common/child_process_host.h"
 #include "google_apis/gaia/gaia_auth_fetcher.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 #include "google_apis/gaia/gaia_constants.h"
@@ -44,14 +45,13 @@
 using namespace signin_internals_util;
 
 using content::BrowserThread;
+using content::ChildProcessHost;
 
 namespace {
 
 const char kGetInfoDisplayEmailKey[] = "displayEmail";
 const char kGetInfoEmailKey[] = "email";
 
-const int kInvalidProcessId = -1;
-
 const char kChromiumSyncService[] = "service=chromiumsync";
 
 }  // namespace
@@ -86,16 +86,17 @@ SigninManager::SigninManager(scoped_ptr<SigninManagerDelegate> delegate)
       had_two_factor_error_(false),
       type_(SIGNIN_TYPE_NONE),
       weak_pointer_factory_(this),
-      signin_process_id_(kInvalidProcessId),
+      signin_host_id_(ChildProcessHost::kInvalidUniqueID),
       delegate_(delegate.Pass()) {
 }
 
 void SigninManager::SetSigninProcess(int process_id) {
-  if (process_id == signin_process_id_)
+  if (process_id == signin_host_id_)
     return;
-  DLOG_IF(WARNING, signin_process_id_ != kInvalidProcessId) <<
-      "Replacing in-use signin process.";
-  signin_process_id_ = process_id;
+  DLOG_IF(WARNING,
+          signin_host_id_ != ChildProcessHost::kInvalidUniqueID)
+      << "Replacing in-use signin process.";
+  signin_host_id_ = process_id;
   const content::RenderProcessHost* process =
       content::RenderProcessHost::FromID(process_id);
   DCHECK(process);
@@ -105,15 +106,15 @@ void SigninManager::SetSigninProcess(int process_id) {
 }
 
 void SigninManager::ClearSigninProcess() {
-  signin_process_id_ = kInvalidProcessId;
+  signin_host_id_ = ChildProcessHost::kInvalidUniqueID;
 }
 
 bool SigninManager::IsSigninProcess(int process_id) const {
-  return process_id == signin_process_id_;
+  return process_id == signin_host_id_;
 }
 
 bool SigninManager::HasSigninProcess() const {
-  return signin_process_id_ != kInvalidProcessId;
+  return signin_host_id_ != ChildProcessHost::kInvalidUniqueID;
 }
 
 SigninManager::~SigninManager() {
@@ -606,14 +607,14 @@ void SigninManager::Observe(int type,
 
   // It's possible we're listening to a "stale" renderer because it was
   // replaced with a new process by process-per-site. In either case,
-  // stop listening to it, but only reset signin_process_id_ tracking
+  // stop listening to it, but only reset signin_host_id_ tracking
   // if this was from the current signin process.
   registrar_.Remove(this,
                     content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
                     source);
-  if (signin_process_id_ ==
+  if (signin_host_id_ ==
       content::Source<content::RenderProcessHost>(source)->GetID()) {
-    signin_process_id_ = kInvalidProcessId;
+    signin_host_id_ = ChildProcessHost::kInvalidUniqueID;
   }
 }
 

chrome/browser/signin/signin_manager.h

@@ -174,13 +174,15 @@ class SigninManager : public SigninManagerBase,
   static bool IsSigninAllowedOnIOThread(ProfileIOData* io_data);
 
   // Allows the SigninManager to track the privileged signin process
-  // identified by |process_id| so that we can later ask (via IsSigninProcess)
+  // identified by |host_id| so that we can later ask (via IsSigninProcess)
   // if it is safe to sign the user in from the current context (see
   // OneClickSigninHelper).  All of this tracking state is reset once the
   // renderer process terminates.
-  void SetSigninProcess(int process_id);
+  //
+  // N.B. This is the id returned by RenderProcessHost::GetID().
+  void SetSigninProcess(int host_id);
   void ClearSigninProcess();
-  bool IsSigninProcess(int process_id) const;
+  bool IsSigninProcess(int host_id) const;
   bool HasSigninProcess() const;
 
  protected:
@@ -274,7 +276,7 @@ class SigninManager : public SigninManagerBase,
 
   // See SetSigninProcess.  Tracks the currently active signin process
   // by ID, if there is one.
-  int signin_process_id_;
+  int signin_host_id_;
 
   // Callback invoked during signin after an OAuth token has been fetched
   // but before signin is complete.

content/browser/dom_storage/dom_storage_namespace.cc

@@ -16,8 +16,8 @@
 #include "content/browser/dom_storage/dom_storage_context_impl.h"
 #include "content/browser/dom_storage/dom_storage_task_runner.h"
 #include "content/browser/dom_storage/session_storage_database.h"
-#include "content/common/child_process_host_impl.h"
 #include "content/common/dom_storage/dom_storage_types.h"
+#include "content/public/common/child_process_host.h"
 
 namespace content {
 
@@ -247,14 +247,14 @@ DOMStorageNamespace::GetAreaHolder(const GURL& origin) {
 }
 
 void DOMStorageNamespace::AddTransactionLogProcessId(int process_id) {
-  DCHECK(process_id != ChildProcessHostImpl::kInvalidChildProcessId);
+  DCHECK(process_id != ChildProcessHost::kInvalidUniqueID);
   DCHECK(transactions_.count(process_id) == 0);
   TransactionData* transaction_data = new TransactionData;
   transactions_[process_id] = transaction_data;
 }
 
 void DOMStorageNamespace::RemoveTransactionLogProcessId(int process_id) {
-  DCHECK(process_id != ChildProcessHostImpl::kInvalidChildProcessId);
+  DCHECK(process_id != ChildProcessHost::kInvalidUniqueID);
   DCHECK(transactions_.count(process_id) == 1);
   delete transactions_[process_id];
   transactions_.erase(process_id);
@@ -353,7 +353,7 @@ SessionStorageNamespace::MergeResult DOMStorageNamespace::Merge(
 }
 
 bool DOMStorageNamespace::IsLoggingRenderer(int process_id) {
-  DCHECK(process_id != ChildProcessHostImpl::kInvalidChildProcessId);
+  DCHECK(process_id != ChildProcessHost::kInvalidUniqueID);
   if (transactions_.count(process_id) < 1)
     return false;
   return !transactions_[process_id]->max_log_size_exceeded;

content/common/child_process_host_impl.cc

@@ -6,7 +6,7 @@
 
 #include <limits>
 
-#include "base/atomicops.h"
+#include "base/atomic_sequence_num.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
@@ -30,9 +30,9 @@
 #include "content/common/font_cache_dispatcher_win.h"
 #endif  // OS_LINUX
 
-#if defined(OS_MACOSX)
 namespace {
 
+#if defined(OS_MACOSX)
 // Given |path| identifying a Mac-style child process executable path, adjusts
 // it to correspond to |feature|. For a child process path such as
 // ".../Chromium Helper.app/Contents/MacOS/Chromium Helper", the transformed
@@ -66,19 +66,22 @@ base::FilePath TransformPathForFeature(const base::FilePath& path,
   new_basename_app.append(kAppExtension);
 
   base::FilePath new_path = root_path.Append(new_basename_app)
-                               .Append(kContentsName)
-                               .Append(kMacOSName)
-                               .Append(new_basename);
+                                     .Append(kContentsName)
+                                     .Append(kMacOSName)
+                                     .Append(new_basename);
 
   return new_path;
 }
+#endif  // OS_MACOSX
+
+// Global atomic to generate child process unique IDs.
+base::StaticAtomicSequenceNumber g_unique_id;
 
 }  // namespace
-#endif  // OS_MACOSX
 
 namespace content {
 
-int ChildProcessHostImpl::kInvalidChildProcessId = -1;
+int ChildProcessHost::kInvalidUniqueID = -1;
 
 // static
 ChildProcessHost* ChildProcessHost::Create(ChildProcessHostDelegate* delegate) {
@@ -212,11 +215,13 @@ void ChildProcessHostImpl::AllocateSharedMemory(
 int ChildProcessHostImpl::GenerateChildProcessUniqueId() {
   // This function must be threadsafe.
   //
-  // TODO(ajwong): Why not StaticAtomicSequenceNumber?
-  static base::subtle::Atomic32 last_unique_child_id = 0;
-  int id = base::subtle::NoBarrier_AtomicIncrement(&last_unique_child_id, 1);
+  // Historically, this function returned ids started with 1, so in several
+  // places in the code a value of 0 (rather than kInvalidUniqueID) was used as
+  // an invalid value. So we retain those semantics.
+  int id = g_unique_id.GetNext() + 1;
 
-  CHECK_NE(kInvalidChildProcessId, id);
+  CHECK_NE(0, id);
+  CHECK_NE(kInvalidUniqueID, id);
 
   return id;
 }

content/common/child_process_host_impl.h

@@ -37,10 +37,6 @@ class CONTENT_EXPORT ChildProcessHostImpl : public ChildProcessHost,
  public:
   virtual ~ChildProcessHostImpl();
 
-  // This value is guaranteed to never be returned by
-  // GenerateChildProcessUniqueId() below.
-  static int kInvalidChildProcessId;
-
   // Public and static for reuse by RenderMessageFilter.
   static void AllocateSharedMemory(
       size_t buffer_size, base::ProcessHandle child_process,
@@ -53,6 +49,8 @@ class CONTENT_EXPORT ChildProcessHostImpl : public ChildProcessHost,
   //
   // This function is threadsafe since RenderProcessHost is on the UI thread,
   // but normally this will be used on the IO thread.
+  //
+  // This will never return ChildProcessHost::kInvalidUniqueID.
   static int GenerateChildProcessUniqueId();
 
   // ChildProcessHost implementation

content/public/browser/render_process_host.h

@@ -156,6 +156,8 @@ class CONTENT_EXPORT RenderProcessHost : public IPC::Sender,
   //
   // This ID will be unique across all child process hosts, including workers,
   // plugins, etc.
+  //
+  // This will never return ChildProcessHost::kInvalidUniqueID.
   virtual int GetID() const = 0;
 
   // Returns true iff channel_ has been set to non-NULL. Use this for checking

content/public/common/child_process_host.h

@@ -24,9 +24,12 @@ class CONTENT_EXPORT ChildProcessHost : public IPC::Sender {
  public:
   virtual ~ChildProcessHost() {}
 
+  // This is a value never returned as the unique id of any child processes of
+  // any kind, including the values returned by RenderProcessHost::GetID().
+  static int kInvalidUniqueID;
+
   // Used to create a child process host. The delegate must outlive this object.
-  static ChildProcessHost* Create(
-      ChildProcessHostDelegate* delegate);
+  static ChildProcessHost* Create(ChildProcessHostDelegate* delegate);
 
   // These flags may be passed to GetChildPath in order to alter its behavior,
   // causing it to return a child path more suited to a specific task.