Merge pull request #1850 from atom/webview-update-process-id

Don't search NativeWindow or guest view by child process ID

atom/browser/api/atom_api_web_contents.cc

@@ -85,7 +85,7 @@ v8::Persistent<v8::ObjectTemplate> template_;
 // Get the window that has the |guest| embedded.
 NativeWindow* GetWindowFromGuest(const content::WebContents* guest) {
   WebViewManager::WebViewInfo info;
-  if (WebViewManager::GetInfoForProcess(guest->GetRenderProcessHost(), &info))
+  if (WebViewManager::GetInfoForWebContents(guest, &info))
     return NativeWindow::FromWebContents(info.embedder);
   else
     return nullptr;

atom/browser/atom_browser_client.cc

@@ -31,25 +31,41 @@ namespace atom {
 
 namespace {
 
+// The default routing id of WebContents.
+// In Electron each RenderProcessHost only has one WebContents, so this ID is
+// same for every WebContents.
+int kDefaultRoutingID = 2;
+
 // Next navigation should not restart renderer process.
 bool g_suppress_renderer_process_restart = false;
 
-struct FindByProcessId {
+// Find out the owner of the child process according to |process_id|.
-  explicit FindByProcessId(int child_process_id)
+enum ProcessOwner {
-      : child_process_id_(child_process_id) {
+  OWNER_NATIVE_WINDOW,
-  }
+  OWNER_GUEST_WEB_CONTENTS,
-
+  OWNER_NONE,  // it might be devtools though.
-  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_;
-  }
-
-  int child_process_id_;
 };
+ProcessOwner GetProcessOwner(int process_id,
+                             NativeWindow** window,
+                             WebViewManager::WebViewInfo* info) {
+  auto web_contents = content::WebContents::FromRenderViewHost(
+      content::RenderViewHost::FromID(process_id, kDefaultRoutingID));
+  if (!web_contents)
+    return OWNER_NONE;
+
+  // First search for NativeWindow.
+  for (auto native_window : *WindowList::GetInstance())
+    if (web_contents == native_window->GetWebContents()) {
+      *window = native_window;
+      return OWNER_NATIVE_WINDOW;
+    }
+
+  // Then search for guest WebContents.
+  if (WebViewManager::GetInfoForWebContents(web_contents, info))
+    return OWNER_GUEST_WEB_CONTENTS;
+
+  return OWNER_NONE;
+}
 
 }  // namespace
 
@@ -58,8 +74,7 @@ void AtomBrowserClient::SuppressRendererProcessRestartForOnce() {
   g_suppress_renderer_process_restart = true;
 }
 
-AtomBrowserClient::AtomBrowserClient()
+AtomBrowserClient::AtomBrowserClient() {
-    : dying_render_process_(nullptr) {
 }
 
 AtomBrowserClient::~AtomBrowserClient() {
@@ -82,8 +97,7 @@ content::AccessTokenStore* AtomBrowserClient::CreateAccessTokenStore() {
 }
 
 void AtomBrowserClient::OverrideWebkitPrefs(
-    content::RenderViewHost* render_view_host,
+    content::RenderViewHost* host, content::WebPreferences* prefs) {
-    content::WebPreferences* prefs) {
   prefs->javascript_enabled = true;
   prefs->web_security_enabled = true;
   prefs->javascript_can_open_windows_automatically = true;
@@ -101,18 +115,10 @@ void AtomBrowserClient::OverrideWebkitPrefs(
   prefs->allow_displaying_insecure_content = false;
   prefs->allow_running_insecure_content = false;
 
-  // Turn off web security for devtools.
-  auto web_contents = content::WebContents::FromRenderViewHost(
-      render_view_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();
+  auto web_contents = content::WebContents::FromRenderViewHost(host);
   WebViewManager::WebViewInfo info;
-  if (WebViewManager::GetInfoForProcess(process, &info)) {
+  if (WebViewManager::GetInfoForWebContents(web_contents, &info)) {
     prefs->web_security_enabled = !info.disable_web_security;
     return;
   }
@@ -136,76 +142,37 @@ void AtomBrowserClient::OverrideSiteInstanceForNavigation(
     return;
   }
 
-  if (current_instance->HasProcess())
+  // Restart renderer process for all navigations except "javacript:" scheme.
-    dying_render_process_ = current_instance->GetProcess();
+  if (url.SchemeIs(url::kJavaScriptScheme))
+    return;
 
-
+  *new_instance = content::SiteInstance::CreateForURL(browser_context, url);
-  if (!url.SchemeIs(url::kJavaScriptScheme)) {
-    // Restart renderer process for all navigations except javacript: scheme.
-    *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;
-  NativeWindow* window = nullptr;
+  WebViewManager::WebViewInfo info;
+  ProcessOwner owner = GetProcessOwner(process_id, &window, &info);
 
-  // Find the owner of this child process.
+  if (owner == OWNER_NATIVE_WINDOW) {
-  WindowList::const_iterator iter = std::find_if(
+    window->AppendExtraCommandLineSwitches(command_line);
-      list->begin(), list->end(), FindByProcessId(child_process_id));
+  } else if (owner == OWNER_GUEST_WEB_CONTENTS) {
-  if (iter != list->end())
+    command_line->AppendSwitchASCII(
-    window = *iter;
+        switches::kGuestInstanceID, base::IntToString(info.guest_instance_id));
-
+    command_line->AppendSwitchASCII(
-  // If the render process is a newly started one, which means the window still
+        switches::kNodeIntegration, info.node_integration ? "true" : "false");
-  // uses the old going-to-be-swapped render process, then we try to find the
+    if (info.plugins)
-  // window from the swapped render process.
+      command_line->AppendSwitch(switches::kEnablePlugins);
-  if (!window && dying_render_process_) {
+    if (!info.preload_script.empty())
-    int dying_process_id = dying_render_process_->GetID();
+      command_line->AppendSwitchPath(
-    WindowList::const_iterator iter = std::find_if(
+          switches::kPreloadScript, info.preload_script);
-        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);
-    }
   }
-
-  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);
-    }
-  }
-
-  dying_render_process_ = nullptr;
 }
 
 void AtomBrowserClient::DidCreatePpapiPlugin(

atom/browser/atom_browser_client.h

@@ -46,9 +46,6 @@ class AtomBrowserClient : public brightray::BrowserClient {
   brightray::BrowserMainParts* OverrideCreateBrowserMainParts(
       const content::MainFunctionParams&) override;
 
-  // The render process which would be swapped out soon.
-  content::RenderProcessHost* dying_render_process_;
-
   DISALLOW_COPY_AND_ASSIGN(AtomBrowserClient);
 };
 

atom/browser/native_window.cc

@@ -422,7 +422,7 @@ content::WebContents* NativeWindow::GetDevToolsWebContents() const {
 }
 
 void NativeWindow::AppendExtraCommandLineSwitches(
-    base::CommandLine* command_line, int child_process_id) {
+    base::CommandLine* command_line) {
   // Append --node-integration to renderer process.
   command_line->AppendSwitchASCII(switches::kNodeIntegration,
                                   node_integration_ ? "true" : "false");

atom/browser/native_window.h

@@ -192,8 +192,7 @@ class NativeWindow : public brightray::DefaultWebContentsDelegate,
   content::WebContents* GetDevToolsWebContents() const;
 
   // Called when renderer process is going to be started.
-  void AppendExtraCommandLineSwitches(base::CommandLine* command_line,
+  void AppendExtraCommandLineSwitches(base::CommandLine* command_line);
-                                      int child_process_id);
   void OverrideWebkitPrefs(content::WebPreferences* prefs);
 
   // Set fullscreen mode triggered by html api.

atom/browser/web_view_manager.cc

@@ -12,10 +12,9 @@ namespace atom {
 
 namespace {
 
-WebViewManager* GetManagerFromProcess(content::RenderProcessHost* process) {
+WebViewManager* GetManagerFromWebContents(
-  if (!process)
+    const content::WebContents* web_contents) {
-    return nullptr;
+  auto context = web_contents->GetBrowserContext();
-  auto context = process->GetBrowserContext();
   if (!context)
     return nullptr;
   return static_cast<WebViewManager*>(context->GetGuestManager());
@@ -24,28 +23,17 @@ WebViewManager* GetManagerFromProcess(content::RenderProcessHost* process) {
 }  // namespace
 
 // static
-bool WebViewManager::GetInfoForProcess(content::RenderProcessHost* process,
+bool WebViewManager::GetInfoForWebContents(
-                                       WebViewInfo* info) {
+    const content::WebContents* web_contents, WebViewInfo* info) {
-  auto manager = GetManagerFromProcess(process);
+  auto manager = GetManagerFromWebContents(web_contents);
   if (!manager)
     return false;
-  return manager->GetInfo(process->GetID(), info);
+  base::AutoLock auto_lock(manager->lock_);
-}
+  auto iter = manager->webview_info_map_.find(web_contents);
-
+  if (iter == manager->webview_info_map_.end())
-// static
+    return false;
-void WebViewManager::UpdateGuestProcessID(
+  *info = iter->second;
-    content::RenderProcessHost* old_process,
+  return true;
-    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);
-  }
 }
 
 WebViewManager::WebViewManager(content::BrowserContext* context) {
@@ -61,9 +49,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 };
-
+  webview_info_map_[web_contents] = info;
-  int guest_process_id = web_contents->GetRenderProcessHost()->GetID();
-  webview_info_map_[guest_process_id] = info;
 
   // Map the element in embedder to guest.
   int owner_process_id = embedder->GetRenderProcessHost()->GetID();
@@ -78,9 +64,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);
-
+  webview_info_map_.erase(web_contents);
-  int guest_process_id = web_contents->GetRenderProcessHost()->GetID();
-  webview_info_map_.erase(guest_process_id);
 
   // Remove the record of element in embedder too.
   for (const auto& element : element_instance_id_to_guest_map_)
@@ -90,16 +74,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

@@ -29,14 +29,10 @@ class WebViewManager : public content::BrowserPluginGuestManager {
     base::FilePath preload_script;
   };
 
-  // Finds the WebViewManager attached with |process| and returns the
+  // Finds the WebViewManager attached with |web_contents| and returns the
   // WebViewInfo of it.
-  static bool GetInfoForProcess(content::RenderProcessHost* process,
+  static bool GetInfoForWebContents(const content::WebContents* web_contents,
-                                WebViewInfo* info);
+                                    WebViewInfo* info);
-
-  // Updates the guest process ID.
-  static void UpdateGuestProcessID(content::RenderProcessHost* old_process,
-                                   content::RenderProcessHost* new_process);
 
   explicit WebViewManager(content::BrowserContext* context);
   virtual ~WebViewManager();
@@ -48,10 +44,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 +81,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;
+  typedef std::map<const content::WebContents*, WebViewInfo> WebViewInfoMap;
-  // guest_process_id => (guest_instance_id, embedder, ...)
+  // web_contents => (guest_instance_id, embedder, ...)
   WebViewInfoMap webview_info_map_;
 
   base::Lock lock_;

atom/renderer/lib/web-view/web-view-attributes.coffee

@@ -28,7 +28,7 @@ class WebViewAttribute
   # Changes the attribute's value without triggering its mutation handler.
   setValueIgnoreMutation: (value) ->
     @ignoreMutation = true
-    @webViewImpl.webviewNode.setAttribute(@name, value || '')
+    @setValue value
     @ignoreMutation = false
 
   # Defines this attribute as a property on the webview node.
@@ -119,6 +119,14 @@ class SrcAttribute extends WebViewAttribute
     else
       ''
 
+  setValueIgnoreMutation: (value) ->
+    WebViewAttribute::setValueIgnoreMutation value
+    # takeRecords() is needed to clear queued up src mutations. Without it, it
+    # is possible for this change to get picked up asyncronously by src's
+    # mutation observer |observer|, and then get handled even though we do not
+    # want to handle this mutation.
+    @observer.takeRecords()
+
   handleMutation: (oldValue, newValue) ->
     # Once we have navigated, we don't allow clearing the src attribute.
     # Once <webview> enters a navigated state, it cannot return to a
@@ -138,7 +146,10 @@ class SrcAttribute extends WebViewAttribute
   setupMutationObserver: ->
     @observer = new MutationObserver (mutations) =>
       for mutation in mutations
-        @handleMutation mutation.oldValue, @getValue()
+        oldValue = mutation.oldValue
+        newValue = @getValue()
+        return if oldValue isnt newValue
+        @handleMutation oldValue, newValue
     params =
       attributes: true,
       attributeOldValue: true,