PlzNavigate: Avoid duplicate SiteInstance creation during navigation.

content/browser/frame_host/render_frame_host_manager.h

 // Copyright 2013 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.
 
 #ifndef CONTENT_BROWSER_FRAME_HOST_RENDER_FRAME_HOST_MANAGER_H_
 #define CONTENT_BROWSER_FRAME_HOST_RENDER_FRAME_HOST_MANAGER_H_
 
 #include <list>
 
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "content/browser/renderer_host/render_view_host_delegate.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/global_request_id.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/common/referrer.h"
 #include "ui/base/page_transition_types.h"
 
 struct FrameMsg_Navigate_Params;
 
+namespace {
+struct SiteInstanceDescriptor;

[carlosk, 2015/03/23 14:31:18]

Instead of the lose |out_params| I preferred creating the struct to hold them
all concisely together.

[clamy, 2015/03/23 15:26:44]

Unnamed namespaces are forbidden in header files. If you want a struct for your
parameters that are only used by private functions, you should declare it as a
nested class inside the private part of your class.

[carlosk, 2015/03/23 17:39:19]

Done.

+}
+
 namespace content {
 class BrowserContext;
 class CrossProcessFrameConnector;
 class CrossSiteTransferringRequest;
 class FrameTreeNode;
 class InterstitialPageImpl;
 class NavigationControllerImpl;
 class NavigationEntry;
 class NavigationEntryImpl;
 class NavigationRequest;
@@ skipping 380 matching lines @@
 
   void EnsureRenderViewInitialized(FrameTreeNode* source,
                                    RenderViewHostImpl* render_view_host,
                                    SiteInstance* instance);
 
  private:
   friend class NavigatorTestWithBrowserSideNavigation;
   friend class RenderFrameHostManagerTest;
   friend class TestWebContents;
 
+  enum RenderFrameHostToUse {

[clamy, 2015/03/23 15:26:44]

This should be prefixed by a PlzNavigate comment to show that it is only used by
PlzNavigate.

[carlosk, 2015/03/23 17:39:19]

Done. I also moved the friend-related macro below to tag along the other friend
definitions.

+    INVALID,
+    REUSE_CURRENT,
+    NEW_SPECULATIVE,
+    REUSE_SPECULATIVE,
+  };
+
   FRIEND_TEST_ALL_PREFIXES(CrossProcessFrameTreeBrowserTest,
                            CreateCrossProcessSubframeProxies);
 
   // Used with FrameTree::ForEach to erase RenderFrameProxyHosts from a
   // FrameTreeNode's RenderFrameHostManager.
   static bool ClearProxiesInSiteInstance(int32 site_instance_id,
                                          FrameTreeNode* node);
   // Used with FrameTree::ForEach to reset initialized state of
   // RenderFrameProxyHosts from a FrameTreeNode's RenderFrameHostManager.
   static bool ResetProxiesInSiteInstance(int32 site_instance_id,
@@ skipping 27 matching lines @@
   // Creates a new Web UI, ensuring that the bindings are appropriate compared
   // to |bindings|.
   scoped_ptr<WebUIImpl> CreateWebUI(const GURL& url, int bindings);
 
   // Returns true if it is safe to reuse the current WebUI when navigating from
   // |current_entry| to |new_url|.
   bool ShouldReuseWebUI(
       const NavigationEntry* current_entry,
       const GURL& new_url) const;
 
+  // PlzNavigate
+  // Returns which RenderFrameHost should be used for the navigation among the
+  // possibilities defined by RenderFrameHostToUse.
+  scoped_refptr<SiteInstance> ChooseFrameHostForNavigation(
+      const NavigationRequest& request,
+      RenderFrameHostToUse* rfh_to_use);

[carlosk, 2015/03/23 14:31:18]

The added logic to reuse the speculative RFH made GetFrameHostForNavigation too
complex and it made sense to split it in two. Now we have: 
* ChooseFrameHostForNavigation that knows how to chose the RFH that should be
used for a navigation.
* GetFrameHostForNavigation that updates internal state depending on the RFH
selected by ChooseFrameHostForNavigation.

On this method signature: it follows the style guide but I don't like the way it
looks. The enum result, the more important information, is an out parameter but
the less important smart pointer is actually returned.

Possible alternatives: switch the pointer to an out param (sounds dangerous?),
return a std::pair with the enum and the pointer or return a struct (practically
the same as the std::pair). WDYT?

[clamy, 2015/03/23 15:26:44]

I don't like the signature of this method either, and I think we may not be
doing the best logical split here. In particular, I find the split
GetFrameHostForNavigation/ChooseFrameHostForNavigation hard to follow.

[carlosk, 2015/03/23 17:39:19]

What I think is nice about the two methods is that it's very clear what the
possible RFH choices are (due to the enum), how they are chosen and what happens
in each case (in the switch block). 

+
   // Returns the SiteInstance to use for the navigation.
   SiteInstance* GetSiteInstanceForNavigation(const GURL& dest_url,

[clamy, 2015/03/23 15:26:44]

Is GetSiteInstanceForNavigation idempotent in the end? I think the caller code
would be nicer if it were. In particular, we would not have to do the
ChooseFrameHostForNavigation/GetFrameHostForNavigation split.

How about we have GetSiteInstanceForNavigation take the current instances as
parameters (both the current or speculative). It would then check if either of
these can be used for the navigation through IsCorrectSiteInstanceForURL and
return it if true. If not, it would create a new one. Then we keep the logic in
GetFrameHostForNavigation untouched, while not making the one in
GetSiteInstanceForNavigation much more complex.

[carlosk, 2015/03/23 17:39:19]

GetSiteInstanceForNavigation is not idempotent as it must comply with the
current behavior (it will create new SiteInstances and has no knowledge of the
speculative RFH). IsCorrectSiteInstanceForURL and DetermineSiteInstanceForURL
are idempotent.
 
We would achieve what you are suggesting by merging IsCorrectSiteInstanceForURL
into GetSiteInstanceForNavigation and keeping |candidate_instance| as an
optional parameter (PlzNavigate would use it, normal navigations wouldn't). That
would indeed allow GetFrameHostForNavigation to be kept unchanged.

WDYT?

                                              SiteInstance* source_instance,
                                              SiteInstance* dest_instance,
                                              ui::PageTransition transition,
                                              bool dest_is_restore,
                                              bool dest_is_view_source_mode);
 
-  // Returns an appropriate SiteInstance object for the given |dest_url|,
-  // possibly reusing the current SiteInstance.  If --process-per-tab is used,
-  // this is only called when ShouldSwapBrowsingInstancesForNavigation returns
-  // true. |source_instance| is the SiteInstance of the frame that initiated the
-  // navigation. |current_instance| is the SiteInstance of the frame that is
-  // currently navigating. |dest_instance|, is a predetermined SiteInstance
-  // that'll be used if not null.
+  // PlzNavigate
+  // Returns true if |candidate_instance| is a valid SiteInstance for a
+  // navigation to |dest_url| given all the provided context.
+  bool IsCorrectSiteInstanceForURL(SiteInstance* candidate_instance,
+                                   const GURL& dest_url,
+                                   SiteInstance* source_instance,
+                                   SiteInstance* dest_instance,
+                                   ui::PageTransition transition,
+                                   bool dest_is_restore,
+                                   bool dest_is_view_source_mode);
+
+  // Returns a descriptor of the appropriate SiteInstance object for the given
+  // |dest_url|, possibly reusing the current, source or destination
+  // SiteInstance. It will check ShouldTransitionCrossSite and
+  // ShouldSwapBrowsingInstancesForNavigation.
+  // |source_instance| is the SiteInstance of the frame that initiated the
+  // navigation. |dest_instance|, is a predetermined SiteInstance
+  // that'll be used if not null. Internally, |current_instance| is the
+  // SiteInstance of the frame that is currently navigating.
   // For example, if you have a parent frame A, which has a child frame B, and
   // A is trying to change the src attribute of B, this will cause a navigation
   // where the source SiteInstance is A and B is the current SiteInstance.
-  // This is a helper function for GetSiteInstanceForNavigation.
-  SiteInstance* GetSiteInstanceForURL(const GURL& dest_url,
-                                      SiteInstance* source_instance,
-                                      SiteInstance* current_instance,
-                                      SiteInstance* dest_instance,
-                                      ui::PageTransition transition,
-                                      bool dest_is_restore,
-                                      bool dest_is_view_source_mode,
-                                      bool force_browsing_instance_swap);
+  // This is a helper function for GetSiteInstanceForNavigation and
+  // IsCorrectSiteInstanceForURL.
+  SiteInstanceDescriptor DetermineSiteInstanceForURL(
+      const GURL& dest_url,
+      SiteInstance* source_instance,
+      SiteInstance* dest_instance,
+      ui::PageTransition transition,
+      bool dest_is_restore,
+      bool dest_is_view_source_mode);
+  SiteInstanceDescriptor DetermineSiteInstanceForURLInternal(

[clamy, 2015/03/23 15:26:44]

Why do we have a DetermineSiteInstanceForURL and a DetermineSiteInstanceForURL
considering that they are both private?

[carlosk, 2015/03/23 17:39:19]

I guess you are asking about the DetermineSiteInstanceForURL and
DetermineSiteInstanceForURLInternal, right? These map directly from
GetSiteInstanceForNavigation and GetSiteInstanceForURL, respectively. The reason
why I kept this logic in two methods is explained in the comment below.

+      const GURL& dest_url,
+      SiteInstance* source_instance,
+      SiteInstance* current_instance,
+      SiteInstance* dest_instance,
+      ui::PageTransition transition,
+      bool dest_is_restore,
+      bool dest_is_view_source_mode,
+      bool force_browsing_instance_swap);

[carlosk, 2015/03/23 14:31:18]

I adapted this comment above so that it now fits what
DetermineSiteInstanceForURL does.

I tried merging the logic from GetSiteInstanceForNavigation and
GetSiteInstanceForURL into a single DetermineSiteInstanceForURL method but the
CHECK in the end of GetSiteInstanceForNavigation (asserts that the current SI is
not used when a browsing instance change should happen) gets too complex. So I
reverted back to having two separate methods as before.

 
   // Determines the appropriate url to use as the current url for SiteInstance
   // selection.
   const GURL& GetCurrentURLForSiteInstance(
       SiteInstance* current_instance,
       NavigationEntry* current_entry);
 
   // Creates a new RenderFrameHostImpl for the |new_instance| and assign it to
   // |pending_render_frame_host_| while respecting the opener route if needed
   // and stores it in pending_render_frame_host_.
@@ skipping 196 matching lines @@
   bool should_reuse_web_ui_;
 
   base::WeakPtrFactory<RenderFrameHostManager> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RenderFrameHostManager);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_FRAME_HOST_RENDER_FRAME_HOST_MANAGER_H_