Atomic UserScript ID generation

chrome/common/extensions/manifest_handlers/content_scripts_handler.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/grit/generated_resources.h"
 #include "content/public/common/url_constants.h"
 #include "extensions/common/error_utils.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_resource.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/manifest_handlers/permissions_parser.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "extensions/common/url_pattern.h"
 #include "extensions/common/url_pattern_set.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "url/gurl.h"
 
 namespace extensions {
 
 namespace keys = extensions::manifest_keys;
 namespace values = manifest_values;
 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.
 bool LoadGlobsHelper(const base::DictionaryValue* content_script,
                      int content_script_index,
                      const char* globs_property_name,
                      base::string16* error,
                      void(UserScript::*add_method)(const std::string& glob),
                      UserScript* instance) {
   if (!content_script->HasKey(globs_property_name))
@@ skipping 342 matching lines @@
 ContentScriptsHandler::~ContentScriptsHandler() {
 }
 
 const std::vector<std::string> ContentScriptsHandler::Keys() const {
   static const char* keys[] = {
     keys::kContentScripts
   };
   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;
   if (!extension->manifest()->GetList(keys::kContentScripts, &scripts_list)) {
     *error = base::ASCIIToUTF16(errors::kInvalidContentScriptsList);
     return false;
   }
 
   for (size_t i = 0; i < scripts_list->GetSize(); ++i) {
     const base::DictionaryValue* script_dict = NULL;
@@ skipping 12 matching lines @@
                                       &user_script)) {
       return false;  // Failed to parse script context definition.
     }
 
     user_script.set_extension_id(extension->id());
     if (extension->converted_from_user_script()) {
       user_script.set_emulate_greasemonkey(true);
       // 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,
                              content_scripts_info.release());
   PermissionsParser::SetScriptableHosts(
       extension, ContentScriptsInfo::GetScriptableHosts(extension));
   return true;
 }
 
 bool ContentScriptsHandler::Validate(
@@ skipping 33 matching lines @@
       if (!IsScriptValid(path, css_script.relative_path(),
                          IDS_EXTENSION_LOAD_CSS_FAILED, error))
         return false;
     }
   }
 
   return true;
 }
 
 }  // namespace extensions