|
|
Created:
5 years, 6 months ago by nasko Modified:
5 years, 2 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable support for swapped out RenderFrame(Host) on desktop.
This CL disables the usage of swapped out RenderFrame(Host) objects. Instead
RenderFrameProxy(Host) is used. Disabling is done through setting a
boolean value in checking for swapped out support and the actual code
will be removed in follow up CLs, once this one sticks and no issues are found.
Android is left alone, as troubleshooting why content_browsertests is failing
on the bots is very time consuming. Follow up patch will disable it there too.
BUG=357747
Committed: https://crrev.com/b8cc9ba3b9d0b8478bfd5d63130dfcfc69fbe6e6
Cr-Commit-Position: refs/heads/master@{#351446}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebased on ToT. #Patch Set 3 : Flip the swapped out forbidden flag. #Patch Set 4 : Fixing some webview/mime_handler_view cases. #Patch Set 5 : Rebase on ToT. #Patch Set 6 : Removed logging and rebased on ToT. #Patch Set 7 : Fix CreateOpenerProxiesForFrameTree and MimeHandlerViewContainer #Patch Set 8 : Rebase on ToT (resolve conflicts). #Patch Set 9 : Rebase on ToT to pick up split out patches. #Patch Set 10 : Rebase on ToT. #Patch Set 11 : Rebase to fix patch conflicts #Patch Set 12 : Disable text autosizer on cross-process navigations. #Patch Set 13 : Rebase on TextAutosizer fix. #Patch Set 14 : Leave swapped out in Android. #
Total comments: 2
Patch Set 15 : Add a TODO. #Patch Set 16 : Rebase on ToT. #Messages
Total messages: 34 (16 generated)
avi@chromium.org changed reviewers: + avi@chromium.org
Nasko: You asked me to look at the HistoryController, but your change isn't about that, it's about switching out the use of "render_view_->GetMainRenderFrame()->GetWebFrame()" in the HistoryController with the passed-in value of "frame_" from the RenderFrame. That code path only happens if both PlzNavigate is off as well as OOPIF is off. In that case, we're coming in from OnNavigate, via IPC from RFHI::Navigate. That comes from RFHI::NavigateToURL and NavigatorImpl::NavigateToEntry. The former's sole caller is InterstitialPageImpl::Show(), which only calls it on top-level frames. The latter's sole caller is NavigatorImpl::NavigateToPendingEntry, which in non-OOPIF is only called with top-level frames. It appears you're safe here. This patch is obviously a work in progress, so I didn't comment extensively, but I made a few comments in the area in which you asked me to look. https://codereview.chromium.org/1199313006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1199313006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:54: bool RenderFrameHostManager::IsSwappedOutStateForbidden() { Looking forward to this function going away. :) https://codereview.chromium.org/1199313006/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1199313006/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:3951: int WebContentsImpl::CreateSwappedOutRenderView( You remove all callers of this function; you should remove it too. https://codereview.chromium.org/1199313006/diff/1/content/renderer/history_co... File content/renderer/history_controller.h (right): https://codereview.chromium.org/1199313006/diff/1/content/renderer/history_co... content/renderer/history_controller.h:118: void GoToEntry(blink::WebLocalFrame* frame, name this |main_frame| to match the .cc file, as well as to document its meaning
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1199313006/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1199313006/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1199313006/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1199313006/180001
The CQ bit was unchecked by nasko@chromium.org
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1199313006/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1199313006/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
nasko@chromium.org changed reviewers: - avi@chromium.org
nasko@chromium.org changed reviewers: + creis@chromium.org
Hey Charlie, Can you review this CL for me? As we chatted, this CL still leaves swapped out into Android, until I can go through and figure out why the bots are failing. Thanks in advance! Nasko
I'm so excited to LGTM this! CL description nit: Add "on desktop" to the end of the subject line. s/intact/alone/ https://codereview.chromium.org/1199313006/diff/260001/content/common/site_is... File content/common/site_isolation_policy.cc (right): https://codereview.chromium.org/1199313006/diff/260001/content/common/site_is... content/common/site_isolation_policy.cc:75: #if defined(OS_ANDROID) nit: TODO(nasko): Fix Android issues and remove this ifdef.
https://codereview.chromium.org/1199313006/diff/260001/content/common/site_is... File content/common/site_isolation_policy.cc (right): https://codereview.chromium.org/1199313006/diff/260001/content/common/site_is... content/common/site_isolation_policy.cc:75: #if defined(OS_ANDROID) On 2015/09/29 19:58:22, Charlie Reis wrote: > nit: TODO(nasko): Fix Android issues and remove this ifdef. Done.
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1199313006/#ps280001 (title: "Add a TODO.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1199313006/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1199313006/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1199313006/#ps300001 (title: "Rebase on ToT.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1199313006/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1199313006/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1199313006/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1199313006/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/b8cc9ba3b9d0b8478bfd5d63130dfcfc69fbe6e6 Cr-Commit-Position: refs/heads/master@{#351446}
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/1374543004/ by zhaoqin@chromium.org. The reason for reverting is: This CL is the culprit of RenderFrameHostManagerTest.NoScriptAccessAfterSwapOut content_browsertests failure on TSan bots BUG=537689.
Message was sent while issue was closed.
On 2015/09/30 18:09:22, zhaoqin1 wrote: > A revert of this CL (patchset #16 id:300001) has been created in > https://codereview.chromium.org/1374543004/ by mailto:zhaoqin@chromium.org. > > The reason for reverting is: This CL is the culprit of > RenderFrameHostManagerTest.NoScriptAccessAfterSwapOut content_browsertests > failure on TSan bots > > BUG=537689. Can you please include a link to the failing bot and/or copy relevant logs here? |