Don't rely on guest_process_id for searching guest

atom/browser/atom_browser_client.cc

@@ -34,22 +34,33 @@ namespace {
 // Next navigation should not restart renderer process.
 bool g_suppress_renderer_process_restart = false;
 
-struct FindByProcessId {
-  explicit FindByProcessId(int child_process_id)
-      : child_process_id_(child_process_id) {
+// Find out the owner of the child process according to child_process_id.
+enum ChildProcessOwner {
+  OWNER_NATIVE_WINDOW,
+  OWNER_GUEST_WEB_CONTENTS,
+  OWNER_NONE,  // it might be devtools though.
+};
+ChildProcessOwner GetChildProcessOwner(int process_id,
+                                       NativeWindow** window,
+                                       WebViewManager::WebViewInfo* info) {
+  // First search for NativeWindow.
+  for (auto native_window : *WindowList::GetInstance()) {
+    content::WebContents* web_contents = native_window->GetWebContents();
+    if (web_contents &&
+        process_id == web_contents->GetRenderProcessHost()->GetID()) {
+      *window = native_window;
+      return OWNER_NATIVE_WINDOW;
+    }
   }
 
-  bool operator() (NativeWindow* const window) {
-    content::WebContents* web_contents = window->GetWebContents();
-    if (!web_contents)
-      return false;
-
-    int id = window->GetWebContents()->GetRenderProcessHost()->GetID();
-    return id == child_process_id_;
+  // Then search for guest WebContents.
+  auto process = content::RenderProcessHost::FromID(process_id);
+  if (WebViewManager::GetInfoForProcess(process, info)) {
+    return OWNER_GUEST_WEB_CONTENTS;
   }
 
-  int child_process_id_;
-};
+  return OWNER_NONE;
+}
 
 }  // namespace
 
@@ -82,8 +93,7 @@ content::AccessTokenStore* AtomBrowserClient::CreateAccessTokenStore() {
 }
 
 void AtomBrowserClient::OverrideWebkitPrefs(
-    content::RenderViewHost* render_view_host,
-    content::WebPreferences* prefs) {
+    content::RenderViewHost* host, content::WebPreferences* prefs) {
   prefs->javascript_enabled = true;
   prefs->web_security_enabled = true;
   prefs->javascript_can_open_windows_automatically = true;
@@ -102,17 +112,15 @@ void AtomBrowserClient::OverrideWebkitPrefs(
   prefs->allow_running_insecure_content = false;
 
   // Turn off web security for devtools.
-  auto web_contents = content::WebContents::FromRenderViewHost(
-      render_view_host);
+  auto web_contents = content::WebContents::FromRenderViewHost(host);
   if (web_contents && web_contents->GetURL().SchemeIs("chrome-devtools")) {
     prefs->web_security_enabled = false;
     return;
   }
 
   // Custom preferences of guest page.
-  auto process = render_view_host->GetProcess();
   WebViewManager::WebViewInfo info;
-  if (WebViewManager::GetInfoForProcess(process, &info)) {
+  if (WebViewManager::GetInfoForWebContents(web_contents, &info)) {
     prefs->web_security_enabled = !info.disable_web_security;
     return;
   }
@@ -136,73 +144,47 @@ void AtomBrowserClient::OverrideSiteInstanceForNavigation(
     return;
   }
 
+  // Restart renderer process for all navigations except "javacript:" scheme.
+  if (url.SchemeIs(url::kJavaScriptScheme))
+    return;
+
   if (current_instance->HasProcess())
     dying_render_process_ = current_instance->GetProcess();
 
-
-  if (!url.SchemeIs(url::kJavaScriptScheme)) {
-    // Restart renderer process for all navigations except javacript: scheme.
-    *new_instance = content::SiteInstance::CreateForURL(browser_context, url);
-  }
+  *new_instance = content::SiteInstance::CreateForURL(browser_context, url);
 }
 
 void AtomBrowserClient::AppendExtraCommandLineSwitches(
     base::CommandLine* command_line,
-    int child_process_id) {
+    int process_id) {
   std::string process_type = command_line->GetSwitchValueASCII("type");
   if (process_type != "renderer")
     return;
 
-  WindowList* list = WindowList::GetInstance();
-  NativeWindow* window = nullptr;
-
-  // Find the owner of this child process.
-  WindowList::const_iterator iter = std::find_if(
-      list->begin(), list->end(), FindByProcessId(child_process_id));
-  if (iter != list->end())
-    window = *iter;
+  NativeWindow* window;
+  WebViewManager::WebViewInfo info;
+  ChildProcessOwner owner = GetChildProcessOwner(process_id, &window, &info);
 
-  // If the render process is a newly started one, which means the window still
+  // If the render process is a newly started one, which means the owner still
   // uses the old going-to-be-swapped render process, then we try to find the
-  // window from the swapped render process.
-  if (!window && dying_render_process_) {
-    int dying_process_id = dying_render_process_->GetID();
-    WindowList::const_iterator iter = std::find_if(
-        list->begin(), list->end(), FindByProcessId(dying_process_id));
-    if (iter != list->end()) {
-      window = *iter;
-      child_process_id = dying_process_id;
-    } else {
-      // It appears that the dying process doesn't belong to a BrowserWindow,
-      // then it might be a guest process, if it is we should update its
-      // process ID in the WebViewManager.
-      auto child_process = content::RenderProcessHost::FromID(child_process_id);
-      // Update the process ID in webview guests.
-      WebViewManager::UpdateGuestProcessID(dying_render_process_,
-                                           child_process);
-    }
+  // owner from the swapped render process.
+  if (owner == OWNER_NONE) {
+    process_id = dying_render_process_->GetID();
+    owner = GetChildProcessOwner(process_id, &window, &info);
   }
 
-  if (window) {
-    window->AppendExtraCommandLineSwitches(command_line, child_process_id);
-  } else {
-    // Append commnad line arguments for guest web view.
-    auto child_process = content::RenderProcessHost::FromID(child_process_id);
-    WebViewManager::WebViewInfo info;
-    if (WebViewManager::GetInfoForProcess(child_process, &info)) {
-      command_line->AppendSwitchASCII(
-          switches::kGuestInstanceID,
-          base::IntToString(info.guest_instance_id));
-      command_line->AppendSwitchASCII(
-          switches::kNodeIntegration,
-          info.node_integration ? "true" : "false");
-      if (info.plugins)
-        command_line->AppendSwitch(switches::kEnablePlugins);
-      if (!info.preload_script.empty())
-        command_line->AppendSwitchPath(
-            switches::kPreloadScript,
-            info.preload_script);
-    }
+  if (owner == OWNER_NATIVE_WINDOW) {
+    window->AppendExtraCommandLineSwitches(command_line, process_id);
+  } else if (owner == OWNER_GUEST_WEB_CONTENTS) {
+    command_line->AppendSwitchASCII(
+        switches::kGuestInstanceID, base::IntToString(info.guest_instance_id));
+    command_line->AppendSwitchASCII(
+        switches::kNodeIntegration, info.node_integration ? "true" : "false");
+    if (info.plugins)
+      command_line->AppendSwitch(switches::kEnablePlugins);
+    if (!info.preload_script.empty())
+      command_line->AppendSwitchPath(
+          switches::kPreloadScript, info.preload_script);
   }
 
   dying_render_process_ = nullptr;

atom/browser/web_view_manager.cc

@@ -29,23 +29,27 @@ bool WebViewManager::GetInfoForProcess(content::RenderProcessHost* process,
   auto manager = GetManagerFromProcess(process);
   if (!manager)
     return false;
-  return manager->GetInfo(process->GetID(), info);
+  base::AutoLock auto_lock(manager->lock_);
+  for (auto iter : manager->webview_info_map_)
+    if (iter.first->GetRenderProcessHost() == process) {
+      *info = iter.second;
+      return true;
+    }
+  return false;
 }
 
 // static
-void WebViewManager::UpdateGuestProcessID(
-    content::RenderProcessHost* old_process,
-    content::RenderProcessHost* new_process) {
-  auto manager = GetManagerFromProcess(old_process);
-  if (manager) {
-    base::AutoLock auto_lock(manager->lock_);
-    int old_id = old_process->GetID();
-    int new_id = new_process->GetID();
-    if (!ContainsKey(manager->webview_info_map_, old_id))
-      return;
-    manager->webview_info_map_[new_id] = manager->webview_info_map_[old_id];
-    manager->webview_info_map_.erase(old_id);
-  }
+bool WebViewManager::GetInfoForWebContents(content::WebContents* web_contents,
+                                           WebViewInfo* info) {
+  auto manager = GetManagerFromProcess(web_contents->GetRenderProcessHost());
+  if (!manager)
+    return false;
+  base::AutoLock auto_lock(manager->lock_);
+  auto iter = manager->webview_info_map_.find(web_contents);
+  if (iter == manager->webview_info_map_.end())
+    return false;
+  *info = iter->second;
+  return true;
 }
 
 WebViewManager::WebViewManager(content::BrowserContext* context) {
@@ -61,9 +65,7 @@ void WebViewManager::AddGuest(int guest_instance_id,
                               const WebViewInfo& info) {
   base::AutoLock auto_lock(lock_);
   web_contents_embdder_map_[guest_instance_id] = { web_contents, embedder };
-
-  int guest_process_id = web_contents->GetRenderProcessHost()->GetID();
-  webview_info_map_[guest_process_id] = info;
+  webview_info_map_[web_contents] = info;
 
   // Map the element in embedder to guest.
   int owner_process_id = embedder->GetRenderProcessHost()->GetID();
@@ -78,9 +80,7 @@ void WebViewManager::RemoveGuest(int guest_instance_id) {
 
   auto web_contents = web_contents_embdder_map_[guest_instance_id].web_contents;
   web_contents_embdder_map_.erase(guest_instance_id);
-
-  int guest_process_id = web_contents->GetRenderProcessHost()->GetID();
-  webview_info_map_.erase(guest_process_id);
+  webview_info_map_.erase(web_contents);
 
   // Remove the record of element in embedder too.
   for (const auto& element : element_instance_id_to_guest_map_)
@@ -90,16 +90,6 @@ void WebViewManager::RemoveGuest(int guest_instance_id) {
     }
 }
 
-bool WebViewManager::GetInfo(int guest_process_id, WebViewInfo* webview_info) {
-  base::AutoLock auto_lock(lock_);
-  WebViewInfoMap::iterator iter = webview_info_map_.find(guest_process_id);
-  if (iter != webview_info_map_.end()) {
-    *webview_info = iter->second;
-    return true;
-  }
-  return false;
-}
-
 content::WebContents* WebViewManager::GetGuestByInstanceID(
     int owner_process_id,
     int element_instance_id) {

atom/browser/web_view_manager.h

@@ -34,9 +34,9 @@ class WebViewManager : public content::BrowserPluginGuestManager {
   static bool GetInfoForProcess(content::RenderProcessHost* process,
                                 WebViewInfo* info);
 
-  // Updates the guest process ID.
-  static void UpdateGuestProcessID(content::RenderProcessHost* old_process,
-                                   content::RenderProcessHost* new_process);
+  // Same with GetInfoForProcess but search for |web_contents| instead.
+  static bool GetInfoForWebContents(content::WebContents* web_contents,
+                                    WebViewInfo* info);
 
   explicit WebViewManager(content::BrowserContext* context);
   virtual ~WebViewManager();
@@ -48,10 +48,6 @@ class WebViewManager : public content::BrowserPluginGuestManager {
                 const WebViewInfo& info);
   void RemoveGuest(int guest_instance_id);
 
-  // Looks up the information for the embedder <webview> for a given render
-  // view, if one exists. Called on the IO thread.
-  bool GetInfo(int guest_process_id, WebViewInfo* webview_info);
-
  protected:
   // content::BrowserPluginGuestManager:
   content::WebContents* GetGuestByInstanceID(int owner_process_id,
@@ -89,8 +85,8 @@ class WebViewManager : public content::BrowserPluginGuestManager {
   // (embedder_process_id, element_instance_id) => guest_instance_id
   std::map<ElementInstanceKey, int> element_instance_id_to_guest_map_;
 
-  typedef std::map<int, WebViewInfo> WebViewInfoMap;
-  // guest_process_id => (guest_instance_id, embedder, ...)
+  typedef std::map<content::WebContents*, WebViewInfo> WebViewInfoMap;
+  // web_contents => (guest_instance_id, embedder, ...)
   WebViewInfoMap webview_info_map_;
 
   base::Lock lock_;