My favorites | Sign in
Project Home Downloads Wiki Issues Code Search
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 123007: Clean up RenderViewHostManager swapping logic
5 people starred this issue and may be notified of changes. Back to list
 
Project Member Reported by creis@chromium.org, Apr 11, 2012
The ShouldSwapProcessesForNavigation and GetSiteInstanceForEntry methods in RenderViewHostManager are currently subtle and fragile.  They need to handle transitions between a large number of page types, and corner cases often lead to renderer kills (like issue 102408) and back button problems (like  issue 93427 ).

I propose cleaning up that code to be more explicit about its assumptions and enforce CHECKs to catch bugs earlier.  This should reduce some of the maintenance burden for cross-process navigations.

I'm aiming to make the functionality change as small as possible, but I want to distinguish between navigations that should force a new BrowsingInstance (where existing tabs can't script the new page, currently handled by ShouldSwapProcessesForNavigation) and those that should force a new SiteInstance (currently handled by GetSiteInstanceForEntry).  WebUI, view-source, extensions, and isolated apps should force a new BrowsingInstance, while cross-site pages and hosted apps should force a new SiteInstance.
May 31, 2012
#1 bugdro...@chromium.org
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=139933

------------------------------------------------------------------------
r139933 | creis@chromium.org | Thu May 31 17:12:28 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/chrome_content_browser_client.h?r1=139933&r2=139932&pathrev=139933
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/public/browser/content_browser_client.cc?r1=139933&r2=139932&pathrev=139933
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/public/browser/content_browser_client.h?r1=139933&r2=139932&pathrev=139933
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/render_view_host_manager_unittest.cc?r1=139933&r2=139932&pathrev=139933
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/chrome_content_browser_client.cc?r1=139933&r2=139932&pathrev=139933
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/debugger/devtools_manager_unittest.cc?r1=139933&r2=139932&pathrev=139933
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/render_view_host_manager.h?r1=139933&r2=139932&pathrev=139933
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/render_view_host_manager.cc?r1=139933&r2=139932&pathrev=139933

Clean up RenderViewHostManager swapping logic.

Makes the difference between swapping SiteInstances and swapping
BrowsingInstances explicit, and adds CHECKs to enforce invariants
more effectively.

BUG=123007
TEST=No functionality change.

Review URL: https://chromiumcodereview.appspot.com/9965091
------------------------------------------------------------------------
Jun 1, 2012
#2 creis@chromium.org
(No comment was entered for this change.)
Status: Fixed
Jun 4, 2012
#3 creis@chromium.org
Found some crash dumps with failures of the new CHECKs.  They're not too common, so I'll try to get a fix in soon rather than reverting right away.

One crash happens when you prepend view-source: to any URL that would use a process-per-site mode.  For example, visit chrome://settings and then type in view-source:chrome://settings in the same tab.  That fails a CHECK because we're using the same SiteInstance even though force_swap is true.  (This is legitimately unsafe because we'll have two RenderViews for the same tab in the same SiteInstance, causing page IDs to conflict.)

I think the right way to fix it is to use "view-source:" as the SiteInstance's site, rather than the URL of the page whose source will be shown.  This means all view-source pages could share a process with each other rather than with the active versions of the actual URLs.

I think this should be safe because view-source pages are passive and don't run script, so they don't pose much of a risk.

There's one other WebUI CHECK failing in RenderViewHostManager::InitRenderView that I'm still tracking down.  I can revert this CL if I don't find it soon.
Status: Started
Jun 6, 2012
#4 bugdro...@chromium.org
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=140795

------------------------------------------------------------------------
r140795 | creis@chromium.org | Wed Jun 06 11:17:26 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/chrome_content_browser_client.h?r1=140795&r2=140794&pathrev=140795
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/public/browser/content_browser_client.cc?r1=140795&r2=140794&pathrev=140795
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/public/browser/content_browser_client.h?r1=140795&r2=140794&pathrev=140795
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/render_view_host_manager_unittest.cc?r1=140795&r2=140794&pathrev=140795
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/chrome_content_browser_client.cc?r1=140795&r2=140794&pathrev=140795
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/debugger/devtools_manager_unittest.cc?r1=140795&r2=140794&pathrev=140795
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/render_view_host_manager.h?r1=140795&r2=140794&pathrev=140795
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/render_view_host_manager.cc?r1=140795&r2=140794&pathrev=140795

Revert 139933 - Clean up RenderViewHostManager swapping logic.
Reverting due to http://crbug.com/131376.

Makes the difference between swapping SiteInstances and swapping
BrowsingInstances explicit, and adds CHECKs to enforce invariants
more effectively.

BUG=123007
TEST=No functionality change.

Review URL: https://chromiumcodereview.appspot.com/9965091

TBR=creis@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10546029
------------------------------------------------------------------------
Jun 7, 2012
#5 creis@chromium.org
This was reverted because of its impact on browser action popups ( issue 131376 ).  Apparently the Extension RenderViews weren't being initialized properly.  I'll try to track down the cause and fix before continuing.

On the plus side, this change has already helped catch another bug:  issue 131676 .  We're killing renderers because of view-source pages in process-per-site mode (which applies to WebUI pages by default).  I'll also need to get that fixed before re-landing this.
Blockedon: 131676
Oct 23, 2012
#6 bugdro...@chromium.org
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=163698

------------------------------------------------------------------------
r163698 | jianli@chromium.org | 2012-10-23T22:09:07.593632Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/extensions/native_shell_window.h?r1=163698&r2=163697&pathrev=163698
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h?r1=163698&r2=163697&pathrev=163698
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/extensions/shell_window_views.cc?r1=163698&r2=163697&pathrev=163698
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/gtk/extensions/shell_window_gtk.h?r1=163698&r2=163697&pathrev=163698
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/extensions/shell_window_views.h?r1=163698&r2=163697&pathrev=163698
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/extensions/shell_window.cc?r1=163698&r2=163697&pathrev=163698

Fix  bug 145175 : [Win] Frameless app windows are not resizable if no draggable region is specified

This happens because RWHV is created for the new RVH instead of the existing RVH associated with the web contents when navigating a web contents to extension URL (see http://crbug.com/123007). When ShellWindowViews tries to set the clickthrough region for resizing purpose, it fails due to no RWHV for the existing RVH. The fix is to make the native window do it again when RWHV is available after RVH is updated.

BUG=145175
TEST=Manual test by launching and resizing frameless window without draggable region specified


Review URL: https://chromiumcodereview.appspot.com/11205004
------------------------------------------------------------------------
Blockedon: -chromium:131676 chromium:131676
Mar 9, 2013
#7 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Type-Cleanup -Pri-2 -Area-Internals -Feature-Navigation Cr-Internals Pri-3 Cr-UI-Browser-Navigation Type-Bug
Oct 2, 2013
#8 bugdro...@chromium.org
------------------------------------------------------------------------
r226637 | avi@chromium.org | 2013-10-03T00:57:28.462406Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/apps/native_app_window_views.cc?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/apps/native_app_window_views.h?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/android/content_view_core_impl.cc?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/public/browser/web_contents_observer.h?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/apps/native_app_window.h?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/render_view_host_manager_unittest.cc?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/render_view_host_manager.cc?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/gtk/apps/native_app_window_gtk.cc?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/apps/app_window_contents.cc?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/public/browser/notification_types.h?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/render_view_host_manager.h?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/gtk/apps/native_app_window_gtk.h?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/controls/webview/webview.cc?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/web_drag_source_win.cc?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/panels/panel.cc?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/browser_plugin/browser_plugin_host_browsertest.cc?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/task_manager/tab_contents_resource_provider.cc?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/apps/shell_window.cc?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/web_contents_impl.cc?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_process_manager.cc?r1=226637&r2=226636&pathrev=226637
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/web_contents_impl.h?r1=226637&r2=226636&pathrev=226637

Merge NOTIFICATION_WEB_CONTENTS_SWAPPED into NOTIFICATION_RENDER_VIEW_HOST_CHANGED.

NOTIFICATION_WEB_CONTENTS_SWAPPED was fired in a strict subset of times that NOTIFICATION_RENDER_VIEW_HOST_CHANGED was fired, but everyone was listening for NOTIFICATION_WEB_CONTENTS_SWAPPED since it showed up in the types file first.

BUG=170921,123007
TEST=no functional change

Review URL: https://codereview.chromium.org/23618036
------------------------------------------------------------------------
Nov 5, 2013
#9 bugdro...@chromium.org
------------------------------------------------------------------------
r232916 | creis@chromium.org | 2013-11-05T05:25:26.147558Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/devtools/devtools_manager_unittest.cc?r1=232916&r2=232915&pathrev=232916

Remove unnecessary ContentBrowserClient in test.

DevToolsManagerTest was forcing a process swap using
ShouldSwapProcessesForNavigation, but this was unnecessary since the
navigation was cross-site anyway.  Removing the extra code so that it doesn't
force an unnecessary swap on the first navigation.

BUG=123007
TEST=DevToolsManagerTest.ReattachOnCancelPendingNavigation

Review URL: https://codereview.chromium.org/53293004
------------------------------------------------------------------------
Dec 2, 2013
#12 bugdro...@chromium.org
------------------------------------------------------------------------
r238304 | creis@chromium.org | 2013-12-03T05:16:07.730317Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/app_process_apitest.cc?r1=238304&r2=238303&pathrev=238304
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/chrome_content_browser_client.cc?r1=238304&r2=238303&pathrev=238304

Clean up ChromeContentBrowserClient::ShouldSwapProcessesForNavigation.

The current logic forces a BrowsingInstance swap when going to/from
hosted apps, which breaks postMessage.  Rely on RenderViewHostManager
to do a SiteInstance swap instead.

BUG=123007
TEST=postMessage works after navigating a hosted app popup cross-process.

Review URL: https://codereview.chromium.org/86973002
------------------------------------------------------------------------
Mar 26, 2014
#13 creis@chromium.org
I consider this resolved after the last CL in r238304 (back in December).  We've added a bunch of CHECKs that have caught real bugs and made the APIs more explicit about what they do.  This was useful before transitioning RenderViewHostManager to RenderFrameHostManager in  issue 314791 .
Status: Fixed
Jul 15, 2014
#14 mgiuca@chromium.org
I have a CL out to clean up some code in AppWindowContents that should not be needed any more since this was fixed:
https://codereview.chromium.org/378193002/
Jul 15, 2014
#15 mgiuca@chromium.org
(No comment was entered for this change.)
Cc: mgiuca@chromium.org
Jul 22, 2014
#16 bugdro...@chromium.org
------------------------------------------------------------------
r284866 | mgiuca@chromium.org | 2014-07-23T06:37:24.411112Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/apps/app_window_contents.cc?r1=284866&r2=284865&pathrev=284866
   M http://src.chromium.org/viewvc/chrome/trunk/src/apps/app_window_contents.h?r1=284866&r2=284865&pathrev=284866

AppWindowContents: Clean up unnecessary SuspendRenderViewHost.

This is no longer necessary since crbug.com/123007 was fixed.

BUG=123007

Review URL: https://codereview.chromium.org/378193002
-----------------------------------------------------------------
Jul 22, 2014
#17 bugdro...@chromium.org
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6691c1970cc88db1178f669605965bb26d02a9d2

commit 6691c1970cc88db1178f669605965bb26d02a9d2
Author: mgiuca@chromium.org <mgiuca@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Wed Jul 23 06:37:24 2014

AppWindowContents: Clean up unnecessary SuspendRenderViewHost.

This is no longer necessary since crbug.com/123007 was fixed.

BUG=123007

Review URL: https://codereview.chromium.org/378193002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@284866 0039d316-1c4b-4281-b951-d872f2087c98


Sign in to add a comment

Powered by Google Project Hosting