Check for already existing entry when adding to SiteProcessMap.

content/browser/renderer_host/render_process_host_impl.cc

 // Copyright 2012 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.
 
 // Represents the browser side of the browser <--> renderer communication
 // channel. There will be one RenderProcessHost per renderer process.
 
 #include "content/browser/renderer_host/render_process_host_impl.h"
 
 #include <algorithm>
@@ skipping 315 matching lines @@
     LAZY_INSTANCE_INITIALIZER;
 
 // Map of site to process, to ensure we only have one RenderProcessHost per
 // site in process-per-site mode.  Each map is specific to a BrowserContext.
 class SiteProcessMap : public base::SupportsUserData::Data {
  public:
   typedef base::hash_map<std::string, RenderProcessHost*> SiteToProcessMap;
   SiteProcessMap() {}
 
   void RegisterProcess(const std::string& site, RenderProcessHost* process) {
-    map_[site] = process;
+    SiteToProcessMap::iterator i = map_.find(site);

[Charlie Reis, 2017/03/13 19:57:30]

This should have a comment, since at first glance it's not obvious why this can
happen or what the right thing to do is (e.g., ignore, overwrite, or crash).

And actually, that makes me realize there may be a problem here.  Thanks for the
detailed explanation of the scenario in https://crbug.com/700514#c1, which helps
a lot.

Suppose in that case, we left out steps 1 and 2 and someone else (perhaps the
user) navigated to the blocked URL in a blank SiteInstance.  Wouldn't that mean
we'd register the blocked error page's SiteInstance in this map, preventing a
real extension process from being registered later?  (Or would real extension
pages get forced into this error page SiteInstance, which might lack bindings
and/or be sharing a process with web pages?)

It seems like the real problem here is that we're treating the error URL as the
SiteInstance's site URL.  I know we want to change the committed URL for blocked
navigations at some point after this CL and
https://codereview.chromium.org/2738643002/, but I wonder if we can do a better
job fixing this aspect now.

For example, maybe the blank SiteInstance's site URL shouldn't be derived from
the "committed" URL if it was an unsuccessful navigation?

(If we did go that route, we'd probably still want to decide what the right
behavior here is, whether's it's NOTREACHED, ignore, or overwrite.  I suppose we
can also get here if we race two blank SiteInstances to the same site?)

[nasko, 2017/03/13 21:06:19]

Comment added.
I think the latter will happen and we will put the extension in a process
without
bindings. It is the safer fallback though, so it shouldn't result in security 
bugs.
Indeed. And as discussed, I'm adding code to not call SetSite on the empty 
SiteInstance in the case of an error page.
 
I think a race can indeed cause us to get here. I think not overwriting is
the approach easiest to reason about.

+    if (i == map_.end())
+      map_[site] = process;
   }
 
   RenderProcessHost* FindProcess(const std::string& site) {
     SiteToProcessMap::iterator i = map_.find(site);
     if (i != map_.end())
       return i->second;
     return NULL;
   }
 
   void RemoveProcess(RenderProcessHost* host) {
@@ skipping 2719 matching lines @@
   LOG(ERROR) << "Terminating render process for bad Mojo message: " << error;
 
   // The ReceivedBadMessage call below will trigger a DumpWithoutCrashing.
   // Capture the error message in a crash key value.
   base::debug::ScopedCrashKey error_key_value("mojo-message-error", error);
   bad_message::ReceivedBadMessage(render_process_id,
                                   bad_message::RPH_MOJO_PROCESS_ERROR);
 }
 
 }  // namespace content