Rework some UserScriptLoader logic in preparation to removing UserScript copy.

extensions/browser/user_script_loader.cc

 // Copyright 2014 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 "extensions/browser/user_script_loader.h"
 
 #include <stddef.h>
 
 #include <set>
 #include <string>
@@ skipping 10 matching lines @@
 #include "extensions/browser/notification_types.h"
 #include "extensions/common/extension_messages.h"
 
 using content::BrowserThread;
 using content::BrowserContext;
 
 namespace extensions {
 
 namespace {
 
+#if DCHECK_IS_ON()
+bool AreScriptsUnique(const UserScriptList& scripts) {
+  std::set<int> script_ids;
+  for (const UserScript& script : scripts) {
+    if (script_ids.count(script.id()))
+      return false;
+    script_ids.insert(script.id());
+  }
+  return true;
+}
+#endif  // DCHECK_IS_ON()
+
 // Helper function to parse greasesmonkey headers
 bool GetDeclarationValue(const base::StringPiece& line,
                          const base::StringPiece& prefix,
                          std::string* value) {
   base::StringPiece::size_type index = line.find(prefix);
   if (index == base::StringPiece::npos)
     return false;
 
   std::string temp(line.data() + index + prefix.length(),
                    line.length() - index - prefix.length());
@@ skipping 113 matching lines @@
       weak_factory_(this) {
   registrar_.Add(this,
                  content::NOTIFICATION_RENDERER_PROCESS_CREATED,
                  content::NotificationService::AllBrowserContextsAndSources());
 }
 
 UserScriptLoader::~UserScriptLoader() {
   FOR_EACH_OBSERVER(Observer, observers_, OnUserScriptLoaderDestroyed(this));
 }
 
-void UserScriptLoader::AddScripts(const std::set<UserScript>& scripts) {
-  for (std::set<UserScript>::const_iterator it = scripts.begin();
-       it != scripts.end();
-       ++it) {
-    removed_scripts_.erase(*it);
-    added_scripts_.insert(*it);
+void UserScriptLoader::AddScripts(const UserScriptList& scripts) {
+#if DCHECK_IS_ON()
+  // |scripts| with non-unique IDs will work, but that would indicate we are
+  // doing something wrong somewhere, so DCHECK that.
+  DCHECK(AreScriptsUnique(scripts))
+      << "AddScripts() expects scripts with unique IDs.";
+#endif  // DCHECK_IS_ON()
+  for (const UserScript& user_script : scripts) {
+    int id = user_script.id();
+    removed_script_hosts_map_.erase(id);
+    if (added_scripts_map_.count(id) == 0)

[Devlin, 2016/08/09 23:28:54]

When does this happen?  (This could be a behavior change, since previously we'd
take the new copy of the script rather than keep the old - but I think IDs are
meant to be globally unique.)

[lazyboy, 2016/08/10 00:03:41]

Not sure, I'm keeping the behavior as is.
(This could be a behavior change, 
The count == 0 check is to make sure we keep the same behavior.
Previously new scripts wouldn't be inserted because we called
added_scripts_(*it),
if it->id() existed in the set, it wouldn't insert "it" to the set, right?

+      added_scripts_map_[id] = user_script;
   }
   AttemptLoad();
 }
 
-void UserScriptLoader::AddScripts(const std::set<UserScript>& scripts,
+void UserScriptLoader::AddScripts(const UserScriptList& scripts,
                                   int render_process_id,
                                   int render_view_id) {
   AddScripts(scripts);
 }
 
-void UserScriptLoader::RemoveScripts(const std::set<UserScript>& scripts) {
-  for (std::set<UserScript>::const_iterator it = scripts.begin();
-       it != scripts.end();
-       ++it) {
-    added_scripts_.erase(*it);
-    removed_scripts_.insert(*it);
+void UserScriptLoader::RemoveScripts(
+    const std::set<UserScriptIDPair>& scripts) {
+  for (const UserScriptIDPair& id_pair : scripts) {
+    removed_script_hosts_map_[id_pair.id] = id_pair.host_id;

[Devlin, 2016/08/09 23:28:54]

If we don't actually remove any scripts, do we still need to add the id?

[lazyboy, 2016/08/10 00:03:41]

Can you elaborate this one, not sure I understand?
Do you mean to check if |added_scripts_map_| had that script?

[Devlin, 2016/08/10 16:22:40]

I think it would involve checking added_scripts_map_ and user_scripts_ (since
the script could already be loaded), but yeah - checking whether or not we
actually remove a script. If we don't, then there's no reason to add it to the
list of things that changes and we might even avoid an IPC.

Of course, even better would be deterministically not removing scripts that
weren't added (or queued to be added), which would also help in a couple other
cases.  I wonder if that's possible to do.

For now, a DCHECK here is fine to avoid any behavior changes.

[lazyboy, 2016/08/10 22:39:54]

OK, I've added a DCHECK and added a todo for this.

I'm trying to avoid behavior changes in this CL.
For the note, this would require keeping copies of script id-s from
|user_scripts_| since |user_scripts_| ownership is transferred to file thread
while the script contents load.

+    added_scripts_map_.erase(id_pair.id);
   }
   AttemptLoad();
 }
 
 void UserScriptLoader::ClearScripts() {
   clear_scripts_ = true;
-  added_scripts_.clear();
-  removed_scripts_.clear();
+  added_scripts_map_.clear();
+  removed_script_hosts_map_.clear();
   AttemptLoad();
 }
 
 void UserScriptLoader::Observe(int type,
                                const content::NotificationSource& source,
                                const content::NotificationDetails& details) {
   DCHECK_EQ(type, content::NOTIFICATION_RENDERER_PROCESS_CREATED);
   content::RenderProcessHost* process =
       content::Source<content::RenderProcessHost>(source).ptr();
   if (!ExtensionsBrowserClient::Get()->IsSameContext(
           browser_context_, process->GetBrowserContext()))
     return;
   if (scripts_ready()) {
     SendUpdate(process, shared_memory_.get(),
                std::set<HostID>());  // Include all hosts.
   }
 }
 
 bool UserScriptLoader::ScriptsMayHaveChanged() const {
   // Scripts may have changed if there are scripts added, scripts removed, or
   // if scripts were cleared and either:
   // (1) A load is in progress (which may result in a non-zero number of
   //     scripts that need to be cleared), or
   // (2) The current set of scripts is non-empty (so they need to be cleared).
-  return (added_scripts_.size() ||
-          removed_scripts_.size() ||
-          (clear_scripts_ &&
-           (is_loading() || user_scripts_->size())));
+  return (added_scripts_map_.size() || removed_script_hosts_map_.size() ||
+          (clear_scripts_ && (is_loading() || user_scripts_->size())));
 }
 
 void UserScriptLoader::AttemptLoad() {
   if (ready_ && ScriptsMayHaveChanged()) {
     if (is_loading())
       pending_load_ = true;
     else
       StartLoad();
   }
 }
 
 void UserScriptLoader::StartLoad() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(!is_loading());
 
   // If scripts were marked for clearing before adding and removing, then clear
   // them.
   if (clear_scripts_) {
     user_scripts_->clear();
   } else {
     for (UserScriptList::iterator it = user_scripts_->begin();
          it != user_scripts_->end();) {
-      if (removed_scripts_.count(*it))
+      if (removed_script_hosts_map_.count(it->id()) > 0u)
         it = user_scripts_->erase(it);
       else
         ++it;
     }
   }
 
-  user_scripts_->insert(

[lazyboy, 2016/08/09 00:48:51]

This is a major source of copy, because it might end up reallocating vector
user_scripts_, which might already contain "contents" loaded from script files.

-      user_scripts_->end(), added_scripts_.begin(), added_scripts_.end());
-
   std::set<int> added_script_ids;
-  for (std::set<UserScript>::const_iterator it = added_scripts_.begin();
-       it != added_scripts_.end();
-       ++it) {
-    added_script_ids.insert(it->id());
-  }
+  for (const auto& id_and_script : added_scripts_map_)
+    added_script_ids.insert(id_and_script.second.id());
 
   // Expand |changed_hosts_| for OnScriptsLoaded, which will use it in
   // its IPC message. This must be done before we clear |added_scripts_| and

[Devlin, 2016/08/09 23:28:54]

added_scripts_map_

[lazyboy, 2016/08/10 00:03:41]

Done.

-  // |removed_scripts_| below.
-  std::set<UserScript> changed_scripts(added_scripts_);
-  changed_scripts.insert(removed_scripts_.begin(), removed_scripts_.end());
-  for (const UserScript& script : changed_scripts)
-    changed_hosts_.insert(script.host_id());
+  // |removed_script_hosts_map_| below.
+  for (const auto& id_and_script : added_scripts_map_)
+    changed_hosts_.insert(id_and_script.second.host_id());
+  for (const std::pair<int, HostID>& host_id : removed_script_hosts_map_)
+    changed_hosts_.insert(host_id.second);
+
+  // Done with |added_scripts_map_|, now put them into |user_scripts_|.
+  user_scripts_->reserve(user_scripts_->size() + added_scripts_map_.size());
+  for (int id : added_script_ids)
+    user_scripts_->push_back(std::move(added_scripts_map_[id]));
 
   LoadScripts(std::move(user_scripts_), changed_hosts_, added_script_ids,
               base::Bind(&UserScriptLoader::OnScriptsLoaded,
                          weak_factory_.GetWeakPtr()));
 
   clear_scripts_ = false;
-  added_scripts_.clear();
-  removed_scripts_.clear();
+  added_scripts_map_.clear();
+  removed_script_hosts_map_.clear();
   user_scripts_.reset();
 }
 
 // static
 std::unique_ptr<base::SharedMemory> UserScriptLoader::Serialize(
     const UserScriptList& scripts) {
   base::Pickle pickle;
   pickle.WriteUInt32(scripts.size());
   for (const UserScript& script : scripts) {
     // TODO(aa): This can be replaced by sending content script metadata to
@@ skipping 115 matching lines @@
   if (!shared_memory->ShareToProcess(handle, &handle_for_process))
     return;  // This can legitimately fail if the renderer asserts at startup.
 
   if (base::SharedMemory::IsHandleValid(handle_for_process)) {
     process->Send(new ExtensionMsg_UpdateUserScripts(
         handle_for_process, host_id(), changed_hosts, whitelisted_only));
   }
 }
 
 }  // namespace extensions