Prevent creating a swapped out RVH in the same SiteInstance as the current one.

Prevent creating a swapped out RVH in the same SiteInstance as the current one.

BUG=123007, 319613
TEST=RenderViewHostManagerTest.CleanUpSwappedOutRVHOnProcessCrash

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235953

[Charlie Reis, 2013-11-05 17:55:38]

Another white whale.  This is the key part of http://crbug.com/123007, which
I've tried several times to fix-- we don't want to have two RenderViewHosts for
the same SiteInstance in the same tab.  I'd like to get this resolved before
moving RVHM to RFHM if possible.

I'm still trying to tell if there are further test failures or if the try
servers are just untrustworthy, but I'd like to get the review started.  Nasko,
can you take a look?  (It's similar to my previous attempt at
https://codereview.chromium.org/19021004/.)

[nasko, 2013-11-05 18:41:00]

Couple of clarifying comments.

https://codereview.chromium.org/57433010/diff/320001/content/browser/frame_ho...
File content/browser/frame_host/render_view_host_manager.cc (right):

https://codereview.chromium.org/57433010/diff/320001/content/browser/frame_ho...
content/browser/frame_host/render_view_host_manager.cc:574: // BrowsingInstance
to be different ones, using CreateForURL.
Why is that? Are we guaranteed in this case that there are no scripting
relationships we need to maintain?
If so, force_swap is a bit unclear name for this, since we can do process swap
but still be in the same BrowsingInstance, right?

https://codereview.chromium.org/57433010/diff/320001/content/browser/site_ins...
File content/browser/site_instance_impl.h (right):

https://codereview.chromium.org/57433010/diff/320001/content/browser/site_ins...
content/browser/site_instance_impl.h:70: // Get the effective URL for the given
actual URL.  Public so that we can make
If this becomes external to SiteInstanceImpl, it is useful to document what
effective URL is with possibly an example.

[Charlie Reis, 2013-11-05 19:13:15]

Thanks.  New patch uploaded.

https://codereview.chromium.org/57433010/diff/320001/content/browser/frame_ho...
File content/browser/frame_host/render_view_host_manager.cc (right):

https://codereview.chromium.org/57433010/diff/320001/content/browser/frame_ho...
content/browser/frame_host/render_view_host_manager.cc:574: // BrowsingInstance
to be different ones, using CreateForURL.
On 2013/11/05 18:41:00, nasko wrote:
> Why is that? Are we guaranteed in this case that there are no scripting
> relationships we need to maintain?

Yep, that's what the poorly named ShouldSwapProcessesForNavigation is actually
deciding.  (Note that the main change to this function is passing in the result
of that call as a parameter and doing the check up front, rather than calling
ShouldSwap again at the end.)

> If so, force_swap is a bit unclear name for this, since we can do process swap
> but still be in the same BrowsingInstance, right?

I agree.  I've switched the name of this parameter to
force_browsing_instance_swap, but the rest of the rename (including
ShouldSwapBrowsingInstancesForNavigation) is coming in a followup CL as I revive
https://codereview.chromium.org/10690048/.

https://codereview.chromium.org/57433010/diff/320001/content/browser/site_ins...
File content/browser/site_instance_impl.h (right):

https://codereview.chromium.org/57433010/diff/320001/content/browser/site_ins...
content/browser/site_instance_impl.h:70: // Get the effective URL for the given
actual URL.  Public so that we can make
On 2013/11/05 18:41:00, nasko wrote:
> If this becomes external to SiteInstanceImpl, it is useful to document what
> effective URL is with possibly an example.

Yeah, I wasn't thrilled about making it public, but I think it's necessary for
the process swap decisions.  I've updated the comment.

[nasko, 2013-11-05 19:15:23]

LGTM

[ncarter (slow), 2013-11-12 03:38:25]

Here's how to get this working on Windows. I've implemented these suggestions in
https://codereview.chromium.org/62523004/ which is running through the trybots
now.

https://codereview.chromium.org/57433010/diff/450002/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api_browsertest.cc
(right):

https://codereview.chromium.org/57433010/diff/450002/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api_browsertest.cc:258:
LOG(INFO) << "SetEnabled: setenabled with gesture error.";
Don't forget to remove these LOG(INFO)s

https://codereview.chromium.org/57433010/diff/450002/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api_browsertest.cc:269:
content::NOTIFICATION_RENDERER_PROCESS_CREATED,
Change this to chrome::NOTIFICATION_EXTENSION_HOST_CREATED to make sure that the
channel finishes establishing itself. Otherwise, it seems on Windows we kill the
process before such a point where we'd notice a broken pipe and trigger the
RENDERER_PROCESS_CLOSED notification that CrashTab is expecting.

https://codereview.chromium.org/57433010/diff/450002/content/browser/frame_ho...
File content/browser/frame_host/render_view_host_manager.h (right):

https://codereview.chromium.org/57433010/diff/450002/content/browser/frame_ho...
content/browser/frame_host/render_view_host_manager.h:313: // this is only
called when ShouldSwapProcessesForNavigation reutrns true.
typo "reutrns"

https://codereview.chromium.org/57433010/diff/450002/content/public/test/brow...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/57433010/diff/450002/content/public/test/brow...
content/public/test/browser_test_utils.cc:217:
NOTIFICATION_RENDERER_PROCESS_TERMINATED,
Change this back to to NOTIFICATION_RENDERER_PROCESS_CLOSED, otherwise you'll
see test hangs in various existing code.

[Charlie Reis, 2013-11-12 16:09:35]

Fantastic-- thanks a ton for the diagnosis, Nick!  The SetEnabled test does
appear to be passing now on all platforms.  Can you take a look and make sure I
covered all your comments?

Matt, can you review management_api_browsertest.cc for owners?

https://codereview.chromium.org/57433010/diff/450002/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api_browsertest.cc
(right):

https://codereview.chromium.org/57433010/diff/450002/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api_browsertest.cc:258:
LOG(INFO) << "SetEnabled: setenabled with gesture error.";
On 2013/11/12 03:38:26, ncarter wrote:
> Don't forget to remove these LOG(INFO)s

Done.

https://codereview.chromium.org/57433010/diff/450002/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api_browsertest.cc:269:
content::NOTIFICATION_RENDERER_PROCESS_CREATED,
On 2013/11/12 03:38:26, ncarter wrote:
> Change this to chrome::NOTIFICATION_EXTENSION_HOST_CREATED to make sure that
the
> channel finishes establishing itself. Otherwise, it seems on Windows we kill
the
> process before such a point where we'd notice a broken pipe and trigger the
> RENDERER_PROCESS_CLOSED notification that CrashTab is expecting.

Done.

https://codereview.chromium.org/57433010/diff/450002/content/browser/frame_ho...
File content/browser/frame_host/render_view_host_manager.h (right):

https://codereview.chromium.org/57433010/diff/450002/content/browser/frame_ho...
content/browser/frame_host/render_view_host_manager.h:313: // this is only
called when ShouldSwapProcessesForNavigation reutrns true.
On 2013/11/12 03:38:26, ncarter wrote:
> typo "reutrns"

Done.

https://codereview.chromium.org/57433010/diff/450002/content/public/test/brow...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/57433010/diff/450002/content/public/test/brow...
content/public/test/browser_test_utils.cc:217:
NOTIFICATION_RENDERER_PROCESS_TERMINATED,
On 2013/11/12 03:38:26, ncarter wrote:
> Change this back to to NOTIFICATION_RENDERER_PROCESS_CLOSED, otherwise you'll
> see test hangs in various existing code.

Done.

[Matt Perry, 2013-11-12 19:48:15]

lgtm

[Charlie Reis, 2013-11-15 18:26:40]

[+benwells, CC mlamouri]

Ben, can I take you up on the suggestion to disable the tests in
app_window_browsertest.cc for the time being?  Much appreciated.

[mlamouri (slow - plz ping), 2013-11-18 04:23:02]

You mentioned that you saw that problem only on Mac and locally on Linux. Is
that only on Mac and Linux Aura? I would be more comfortable disabling the tests
on some platforms instead of all of them.

[Charlie Reis, 2013-11-18 18:05:56]

On 2013/11/18 04:23:02, Mounir Lamouri wrote:
> You mentioned that you saw that problem only on Mac and locally on Linux. Is
> that only on Mac and Linux Aura? I would be more comfortable disabling the
tests
> on some platforms instead of all of them.

It was Mac and Linux (non-Aura, I believe).  I've limited the disabling to those
platforms, though I may have to disable further if it fails on Windows (etc) on
the build bots.

mlamouri and benwells, PTAL.

[Charlie Reis, 2013-11-18 19:59:31]

On 2013/11/18 18:05:56, creis wrote:
> On 2013/11/18 04:23:02, Mounir Lamouri wrote:
> > You mentioned that you saw that problem only on Mac and locally on Linux. Is
> > that only on Mac and Linux Aura? I would be more comfortable disabling the
> tests
> > on some platforms instead of all of them.
> 
> It was Mac and Linux (non-Aura, I believe).  I've limited the disabling to
those
> platforms, though I may have to disable further if it fails on Windows (etc)
on
> the build bots.
> 
> mlamouri and benwells, PTAL.

Nevermind.  Patch set 12 just failed AppWindowAPI.TestBounds on Windows:
http://build.chromium.org/p/tryserver.chromium/builders/win_swarm_triggered/b...

This means the tests are flaky on all platforms.  I've gone back to patch set
11, which disables the tests on all platforms.  PTAL.

[benwells, 2013-11-19 02:21:37]

lgtm.

Sorry about the delay, we were hoping to keep these enabled on some platforms.

[commit-bot: I haz the power, 2013-11-19 02:37:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/57433010/760001

[commit-bot: I haz the power, 2013-11-19 02:37:19]

Failed to apply patch for content/browser/site_instance_impl.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/site_instance_impl.h
  Hunk #1 succeeded at 66 (offset -1 lines).
  Hunk #2 FAILED at 89.
  1 out of 2 hunks FAILED -- saving rejects to file
content/browser/site_instance_impl.h.rej

Patch:       content/browser/site_instance_impl.h
Index: content/browser/site_instance_impl.h
diff --git a/content/browser/site_instance_impl.h
b/content/browser/site_instance_impl.h
index
013e65895a189c4c7ed418eeb05d0122e77c6b1f..658d8647c9bcb7901c08971af11040fb5ceece00
100644
--- a/content/browser/site_instance_impl.h
+++ b/content/browser/site_instance_impl.h
@@ -67,6 +67,15 @@ class CONTENT_EXPORT SiteInstanceImpl : public SiteInstance,
   static void set_render_process_host_factory(
       const RenderProcessHostFactory* rph_factory);
 
+  // Get the effective URL for the given actual URL.  This allows the
+  // ContentBrowserClient to override the SiteInstance's site for certain URLs.
+  // For example, Chrome uses this to replace hosted app URLs with extension
+  // hosts.
+  // Only public so that we can make a consistent process swap decision in
+  // RenderViewHostManager.
+  static GURL GetEffectiveURL(BrowserContext* browser_context,
+                              const GURL& url);
+
  protected:
   friend class BrowsingInstance;
   friend class SiteInstance;
@@ -80,10 +89,6 @@ class CONTENT_EXPORT SiteInstanceImpl : public SiteInstance,
   explicit SiteInstanceImpl(BrowsingInstance* browsing_instance);
 
  private:
-  // Get the effective URL for the given actual URL.
-  static GURL GetEffectiveURL(BrowserContext* browser_context,
-                              const GURL& url);
-
   // NotificationObserver implementation.
   virtual void Observe(int type,
                        const NotificationSource& source,

[commit-bot: I haz the power, 2013-11-19 02:47:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/57433010/1000001

[commit-bot: I haz the power, 2013-11-19 02:47:23]

Failed to apply patch for content/browser/site_instance_impl.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/site_instance_impl.h
  Hunk #1 succeeded at 66 (offset -1 lines).
  Hunk #2 FAILED at 89.
  1 out of 2 hunks FAILED -- saving rejects to file
content/browser/site_instance_impl.h.rej

Patch:       content/browser/site_instance_impl.h
Index: content/browser/site_instance_impl.h
diff --git a/content/browser/site_instance_impl.h
b/content/browser/site_instance_impl.h
index
013e65895a189c4c7ed418eeb05d0122e77c6b1f..658d8647c9bcb7901c08971af11040fb5ceece00
100644
--- a/content/browser/site_instance_impl.h
+++ b/content/browser/site_instance_impl.h
@@ -67,6 +67,15 @@ class CONTENT_EXPORT SiteInstanceImpl : public SiteInstance,
   static void set_render_process_host_factory(
       const RenderProcessHostFactory* rph_factory);
 
+  // Get the effective URL for the given actual URL.  This allows the
+  // ContentBrowserClient to override the SiteInstance's site for certain URLs.
+  // For example, Chrome uses this to replace hosted app URLs with extension
+  // hosts.
+  // Only public so that we can make a consistent process swap decision in
+  // RenderViewHostManager.
+  static GURL GetEffectiveURL(BrowserContext* browser_context,
+                              const GURL& url);
+
  protected:
   friend class BrowsingInstance;
   friend class SiteInstance;
@@ -80,10 +89,6 @@ class CONTENT_EXPORT SiteInstanceImpl : public SiteInstance,
   explicit SiteInstanceImpl(BrowsingInstance* browsing_instance);
 
  private:
-  // Get the effective URL for the given actual URL.
-  static GURL GetEffectiveURL(BrowserContext* browser_context,
-                              const GURL& url);
-
   // NotificationObserver implementation.
   virtual void Observe(int type,
                        const NotificationSource& source,

[commit-bot: I haz the power, 2013-11-19 02:51:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/57433010/990007

[commit-bot: I haz the power, 2013-11-19 09:50:34]

Change committed as 235953