re-use WebUIs

content/browser/web_contents/render_view_host_manager.h

Index: content/browser/web_contents/render_view_host_manager.h
diff --git a/content/browser/web_contents/render_view_host_manager.h b/content/browser/web_contents/render_view_host_manager.h
index 04825c5250076f499624aa33135e349ea6e49fc7..db1c2f613b60ca9fe7e14a26cd920257e1724756 100644
--- a/content/browser/web_contents/render_view_host_manager.h
+++ b/content/browser/web_contents/render_view_host_manager.h
@@ -124,7 +124,7 @@ class CONTENT_EXPORT RenderViewHostManager
   WebUIImpl* web_ui() const { return web_ui_.get(); }
 
   // Returns the Web UI for the pending navigation, or NULL of none applies.
-  WebUIImpl* pending_web_ui() const { return pending_web_ui_.get(); }
+  WebUIImpl* pending_web_ui() const { return pending_web_ui_; }
 
   // Called when we want to instruct the renderer to navigate to the given
   // navigation entry. It may create a new RenderViewHost or re-use an existing
@@ -219,6 +219,10 @@ class CONTENT_EXPORT RenderViewHostManager
       const content::NavigationEntry* cur_entry,
       const content::NavigationEntryImpl* new_entry) const;
 
+  bool ShouldReuseWebUI(
+      const content::NavigationEntry* curr_entry,

[Dan Beam, 2012/04/21 02:53:07]

can you change this to cur_entry or change above to curr_entry?

[Evan Stade, 2012/04/24 17:52:40]

Done.

+      const content::NavigationEntryImpl* new_entry) const;
+
   // Returns an appropriate SiteInstance object for the given NavigationEntry,
   // possibly reusing the current SiteInstance.
   // Never called if --process-per-tab is used.
@@ -251,6 +255,10 @@ class CONTENT_EXPORT RenderViewHostManager
   // schedule new navigations in its swapped out RenderViewHosts after this.
   void RendererProcessClosing(content::RenderProcessHost* render_process_host);
 
+  // Deletes |pending_web_ui_|, making sure not to double-free in the case where
+  // it is the same as |web_ui_|.
+  void FreePendingWebUI();
+
   // Our delegate, not owned by us. Guaranteed non-NULL.
   Delegate* delegate_;
 
@@ -274,12 +282,16 @@ class CONTENT_EXPORT RenderViewHostManager
   // have an associated Web UI, in which case the Web UI pointer will be non-
   // NULL.
   //
-  // The pending_web_ui may be non-NULL even when the pending_render_view_host_
+  // The pending_web_ui_ may be non-NULL even when the pending_render_view_host_

[Dan Beam, 2012/04/21 02:53:07]

nit: |pending_web_ui_| and |pending_render_view_host_| if you want to

[Evan Stade, 2012/04/24 17:52:40]

Done.

   // is. This will happen when we're transitioning between two Web UI pages:
   // the RVH won't be swapped, so the pending pointer will be unused, but there
   // will be a pending Web UI associated with the navigation.
   content::RenderViewHostImpl* pending_render_view_host_;
-  scoped_ptr<WebUIImpl> pending_web_ui_;
+
+  // If non-NULL, the pending navigation is to a chrome: page. The pointer may
+  // be a unique WebUIImpl, or may be a copy of web_ui_, in which case the same
+  // WebUIImpl* will be reused for the next navigation.
+  WebUIImpl* pending_web_ui_;

[stuartmorgan, 2012/04/24 09:12:20]

This memory model makes me *extremely* nervous; I think it's almost certain that
someone will introduce a leak or a crash eventually this way (doubly so since
this class is both very complex and weakly owned).

I can see two sane approaches here:
1) A trivial ref-counted class that wraps WebUIImpl*, so that you can use that
as the type for two scoped_refptrs, or
2) A little helper class that owns both WebUIImpl objects and has a handful of
public methods that do what you need (checks if there is a pending web ui,
returns the pending web ui, commits the pending web ui).

[Evan Stade, 2012/04/24 17:52:40]

Done.

 
   // A map of site instance ID to swapped out RenderViewHosts.
   typedef base::hash_map<int32, content::RenderViewHostImpl*> RenderViewHostMap;