Atomic UserScript ID generation

chrome/common/extensions/manifest_handlers/content_scripts_handler.cc

Index: chrome/common/extensions/manifest_handlers/content_scripts_handler.cc
diff --git a/chrome/common/extensions/manifest_handlers/content_scripts_handler.cc b/chrome/common/extensions/manifest_handlers/content_scripts_handler.cc
index ff5eb90e3eeec0422079e95d951948c721360675..30953307b406776cc59402b5446c3749b63e1270 100644
--- a/chrome/common/extensions/manifest_handlers/content_scripts_handler.cc
+++ b/chrome/common/extensions/manifest_handlers/content_scripts_handler.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/common/extensions/manifest_handlers/content_scripts_handler.h"
 
+#include "base/atomic_sequence_num.h"
 #include "base/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/memory/scoped_ptr.h"
@@ -33,7 +34,7 @@ namespace errors = manifest_errors;
 namespace {
 
 // The globally-unique id for a user script.
-int64 g_next_user_script_id = 0;
+base::StaticAtomicSequenceNumber g_user_script_id_generator;

[Devlin, 2014/08/21 15:27:47]

This seems... overarchitected.  Any advantage over the old?

[Mark Dittmer, 2014/08/21 19:45:41]

In early testing of https://codereview.chromium.org/493633003/ and
https://codereview.chromium.org/492133002/ I discovered that UserScripts are
instantiated from different threads. This ensures that we don't have duplicate
IDs.

 
 // Helper method that loads either the include_globs or exclude_globs list
 // from an entry in the content_script lists of the manifest.
@@ -396,6 +397,11 @@ const std::vector<std::string> ContentScriptsHandler::Keys() const {
   return std::vector<std::string>(keys, keys + arraysize(keys));
 }
 
+// static
+int64 ContentScriptsHandler::GetNextUserScriptID() {

[Devlin, 2014/08/21 15:27:47]

GetNext() returns an int, not an int64.  Probably never a problem (unlikely that
we'll overflow 4b), but something to consider.

[Mark Dittmer, 2014/08/21 19:45:41]

This is done because UserScript IDs are int64. Should I change that to int to
avoid deceptive overflow bugs?

[Devlin, 2014/08/21 20:10:20]

Yes.  We should be consistent.

[Mark Dittmer, 2014/08/21 21:12:29]

Done.

[Devlin, 2014/08/21 21:24:23]

You're missing some.  What about all the renderer places?

+  return g_user_script_id_generator.GetNext();
+}
+
 bool ContentScriptsHandler::Parse(Extension* extension, base::string16* error) {
   scoped_ptr<ContentScriptsInfo> content_scripts_info(new ContentScriptsInfo);
   const base::ListValue* scripts_list = NULL;
@@ -428,7 +434,7 @@ bool ContentScriptsHandler::Parse(Extension* extension, base::string16* error) {
       // Greasemonkey matches all frames.
       user_script.set_match_all_frames(true);
     }
-    user_script.set_id(g_next_user_script_id++);
+    user_script.set_id(GetNextUserScriptID());
     content_scripts_info->content_scripts.push_back(user_script);
   }
   extension->SetManifestData(keys::kContentScripts,