Check for already existing entry when adding to SiteProcessMap.

content/browser/renderer_host/render_process_host_impl.cc

Index: content/browser/renderer_host/render_process_host_impl.cc
diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
index 2c48d780f0fd99df89d492dc61abe6e947cd9d78..631388e169c2878fa6a6475bf6a23525467571a1 100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -333,7 +333,9 @@ class SiteProcessMap : public base::SupportsUserData::Data {
   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) {