Clean up and remove ShouldFork logic from the renderer |
|
Issue descriptionRenderFrameImpl::DecidePolicyForNavigation has a bunch of logic to decide whether or not to send the navigation to the browser process via the OpenURL IPC. ShouldFork logic is very complex, as it tries to deal with WebUI, prerender, instant, and crossing web/extension process boundaries. It has bitten us with bugs, and with PlzNavigate and site isolation, most of this logic should be redundant, so we should look into removing it. As an experiment, I tried ignoring the should_fork value in DecidePolicyForNavigation in https://chromium-review.googlesource.com/c/chromium/src/+/1222292, and saw some test failures: WebNavigationApiTest.CrossProcessHistory NetInternalsTest.netInternalsPrerenderViewSucceed ExtensionBrowserTest.NavigateToInaccessibleResourceFromChromeURL PrerenderBrowserTest.PrerenderServerRedirectNavigateToSecondViaClick PrerenderBrowserTest.PrerenderCancelReferrerPolicy WebNavigationApiTest.CrossProcess PrerenderBrowserTest.PrerenderClickClickGoBack DevToolsSanityTest.TestDevToolsExternalNavigation PrerenderBrowserTest.PrerenderClientRedirectNavigateToSecondViaClick NavigatingExtensionPopupBrowserTest.Webpage PrerenderBrowserTest.PrerenderPing ExtensionBrowserTest.WindowOpenInvalidExtension PrerenderBrowserTest.PrerenderClickNavigateGoBack ExtensionResourceRequestPolicyTest.LinkToWebAccessibleResources PrerenderBrowserTest.PrerenderCancelReferrer NavigatingExtensionPopupBrowserTest.PageInOtherExtension ExtensionBrowserTest.WindowOpenInaccessibleResourceFromDataURL Ignoring ShouldFork only for extensions (https://chromium-review.googlesource.com/c/chromium/src/+/1222087) fared better, but still had some failures: WebNavigationApiTest.CrossProcess NavigatingExtensionPopupBrowserTest.Webpage ExtensionBrowserTest.WindowOpenInvalidExtension ExtensionResourceRequestPolicyTest.LinkToWebAccessibleResources WebNavigationApiTest.CrossProcessHistory NavigatingExtensionPopupBrowserTest.PageInOtherExtension ExtensionBrowserTest.WindowOpenInaccessibleResourceFromDataURL So some ShouldFork logic might be still applicable, and we should take a look at why that's the case and whether some of it should move to the browser process.
,
Sep 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/11135685753f0ae02c2ae9c8ad45c6ed2d135bbb commit 11135685753f0ae02c2ae9c8ad45c6ed2d135bbb Author: Alex Moshchuk <alexmos@chromium.org> Date: Fri Sep 14 22:53:25 2018 Remove ShellContentRendererClient::ShouldFork. This only checked for the kContentShellAlwaysFork switch, which seems to be unused. Bug: 883549 Change-Id: Ib9e5e9df79caaa0e2d6cda8457b84cf4d391064c Reviewed-on: https://chromium-review.googlesource.com/1226462 Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Commit-Queue: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#591509} [modify] https://crrev.com/11135685753f0ae02c2ae9c8ad45c6ed2d135bbb/content/shell/common/shell_switches.cc [modify] https://crrev.com/11135685753f0ae02c2ae9c8ad45c6ed2d135bbb/content/shell/common/shell_switches.h [modify] https://crrev.com/11135685753f0ae02c2ae9c8ad45c6ed2d135bbb/content/shell/renderer/shell_content_renderer_client.cc [modify] https://crrev.com/11135685753f0ae02c2ae9c8ad45c6ed2d135bbb/content/shell/renderer/shell_content_renderer_client.h
,
Sep 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d97d105fb1575487cfce16135bbc1faf9642e14b commit d97d105fb1575487cfce16135bbc1faf9642e14b Author: Alex Moshchuk <alexmos@chromium.org> Date: Fri Sep 14 23:24:25 2018 Remove hosted app reload logic from ShouldFork. This CL removes the logic that triggered process swaps in ChromeExtensionsRendererClient::ShouldFork() for the case when the main frame reloads a URL for which a hosted app was just installed or uninstalled. This logic isn't needed with --site-per-process, as that mode will already force a process transfer. This is covered by AppApiTest.ReloadIntoAppProcess and AppApiTest.ReloadIntoAppProcessWithJavaScript, which still pass with that logic removed. This CL also makes a fix on the browser side to keep this logic working even without --site-per-process, by unconditionally checking HasWrongProcessForURL() and forcing a process transfer if needed in IsRendererTransferNeededForNavigation(). This is possible because with PlzNavigate we will always go to the browser process and check for transfers, even without --site-per-process. With that fix, the above two tests also pass without --site-per-process. The above tweak also has a side effect of swapping processes when an app opens a cross-site, non-app popup (also made possible by work on issue 794315 ) in non-site-per-process mode, which seems acceptable and desirable - no reason for the cross-site popup to be in a process with app permissions. Bug: 883550 , 883549, 718516 Test: AppApiTest.ReloadIntoAppProcess and AppApiTest.ReloadIntoAppProcessWithJavaScript keep working, with and without --site-per-process. Change-Id: Ibac63fff3a36318fabceb593ca7ae7967eefad89 Reviewed-on: https://chromium-review.googlesource.com/1226075 Reviewed-by: Nasko Oskov <nasko@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Commit-Queue: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#591521} [modify] https://crrev.com/d97d105fb1575487cfce16135bbc1faf9642e14b/chrome/browser/ui/extensions/hosted_app_browsertest.cc [modify] https://crrev.com/d97d105fb1575487cfce16135bbc1faf9642e14b/chrome/renderer/extensions/chrome_extensions_renderer_client.cc [modify] https://crrev.com/d97d105fb1575487cfce16135bbc1faf9642e14b/content/browser/frame_host/render_frame_host_manager.cc |
|
►
Sign in to add a comment |
|
Comment 1 by lukasza@chromium.org
, Sep 12