Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(199)

Issue 1886413002: Always swap with a replacement proxy in OnSwapOut. (Closed)

Created:
4 years, 8 months ago by alexmos
Modified:
4 years, 8 months ago
Reviewers:
Charlie Reis, nasko
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, mlamouri+watch-blink_chromium.org, nasko+codewatch_chromium.org, jam, dcheng, blink-reviews, darin-cc_chromium.org, ajwong+watch_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.

Description

Always swap with a replacement proxy in OnSwapOut. Currently, if a frame is the last active frame in the process and it gets swapped out, OnSwapOut is not passed a proxy routing ID, and so it leaves the RenderFrame alone without swapping it or destroying it. (Later on, closing the RenderView detaches all nodes in the frame tree, which cleans up that RenderFrame.) This leads to the complexity of dealing with this special case and has indirectly led to various races involving reuse of the RenderViewHost for a RFH pending shutdown. See https://codereview.chromium.org/1835833002 and issues 515302, 581912, 544755, and 591478 for some context. Additionally, this led to problems of trying to later use the RenderFrame that was left hanging around by OnSwapOut. For instance, when this is done for a subframe, there is a race where the subframe's WebFrameWidget is cleaned up before the RenderView is closed, and the RenderFrame tried to reference the null widget as part of being detached when the RenderView is closed (see https://crbug.com/568836#c15). This CL changes SwapOut to always swap with a proxy. In the cases when the last active frame in the process is going away, the proxy will be short-lived and will be deleted as soon as the RFH pending shutdown is deleted. To facilitate this, the decrementing of active frame count moves from RFH::SwapOut to RFH::OnSwappedOut. This also lets us get rid of the whole concept of RenderViewHosts pending shutdown, making it fine to reuse a RVH when its RFH is pending shutdown and just rely on RVH's refcount to keep it alive and destroy it. The extra short-lived proxy should be well worth significantly reduced code complexity. BUG=568836, 515302, 581912, 544755, 591478 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/9aa6123974ab0f85aecf20cfc6e377071f99b386 Cr-Commit-Position: refs/heads/master@{#389904}

Patch Set 1 #

Patch Set 2 : Fix test #

Patch Set 3 : Fix ExtensionWebRequestApiTest.WebRequestBlocking #

Patch Set 4 : Fix test for Android and cleanup #

Patch Set 5 : Nits #

Total comments: 10

Patch Set 6 : Try crazy approach: always create proxy in SwapOut #

Patch Set 7 : Remove RVH pending deletion logic completely #

Patch Set 8 : Undo unintentional test change #

Patch Set 9 : Nits #

Patch Set 10 : Fix compile #

Total comments: 19

Patch Set 11 : Charlie's comments #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+217 lines, -188 lines) Patch
M content/browser/frame_host/frame_tree.h View 1 2 3 4 5 6 7 8 9 2 chunks +0 lines, -8 lines 0 comments Download
M content/browser/frame_host/frame_tree.cc View 1 2 3 4 5 6 7 8 9 3 chunks +16 lines, -52 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +13 lines, -18 lines 1 comment Download
M content/browser/frame_host/render_frame_host_manager.h View 1 2 3 4 5 6 1 chunk +0 lines, -4 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager.cc View 1 2 3 4 5 6 2 chunks +11 lines, -36 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager_browsertest.cc View 1 2 3 4 5 6 7 8 4 chunks +22 lines, -9 lines 0 comments Download
M content/browser/renderer_host/render_view_host_impl.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/site_per_process_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 7 chunks +118 lines, -21 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +34 lines, -37 lines 0 comments Download

Messages

Total messages: 26 (12 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1886413002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1886413002/40001
4 years, 8 months ago (2016-04-14 22:41:06 UTC) #3
commit-bot: I haz the power
Dry run: 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_compile_dbg_ng/builds/174939)
4 years, 8 months ago (2016-04-14 22:48:02 UTC) #5
alexmos
Charlie, Nasko - what do you think about this? The full description of the problem ...
4 years, 8 months ago (2016-04-15 17:05:12 UTC) #10
Charlie Reis
Wow, nice investigation. Definitely a tricky situation. On 2016/04/15 17:05:12, alexmos wrote: > Charlie, Nasko ...
4 years, 8 months ago (2016-04-15 23:20:05 UTC) #11
nasko
Just some nits on the code itself. Charlie, I like the crazy idea. Tracing through ...
4 years, 8 months ago (2016-04-18 21:24:27 UTC) #12
alexmos
OK, I think this is ready for another look. The latest PS totally changes direction ...
4 years, 8 months ago (2016-04-22 05:09:35 UTC) #15
Charlie Reis
This is so nice!! I have two questions below, but I really like how this ...
4 years, 8 months ago (2016-04-22 19:11:34 UTC) #16
alexmos
Thanks for reviewing! https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_host/frame_tree.cc File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_host/frame_tree.cc#newcode343 content/browser/frame_host/frame_tree.cc:343: CHECK(iter != render_view_host_map_.end()); On 2016/04/22 19:11:33, ...
4 years, 8 months ago (2016-04-25 18:17:08 UTC) #17
Charlie Reis
I think it makes sense to proceed and file a bug to consider any issues ...
4 years, 8 months ago (2016-04-26 05:06:37 UTC) #19
nasko
LGTM https://codereview.chromium.org/1886413002/diff/200001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1886413002/diff/200001/content/browser/frame_host/render_frame_host_impl.cc#newcode1222 content/browser/frame_host/render_frame_host_impl.cc:1222: set_render_frame_proxy_host(proxy); Ugh, this isn't needed, but I'll get ...
4 years, 8 months ago (2016-04-26 19:41:02 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1886413002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1886413002/200001
4 years, 8 months ago (2016-04-26 19:52:17 UTC) #22
alexmos
Thanks for the reviews! I've filed https://crbug.com/606926 to track RVH reuse issues, and I'll go ...
4 years, 8 months ago (2016-04-26 20:02:57 UTC) #23
commit-bot: I haz the power
Committed patchset #11 (id:200001)
4 years, 8 months ago (2016-04-26 21:54:13 UTC) #24
commit-bot: I haz the power
4 years, 8 months ago (2016-04-26 21:55:38 UTC) #26
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/9aa6123974ab0f85aecf20cfc6e377071f99b386
Cr-Commit-Position: refs/heads/master@{#389904}

Powered by Google App Engine
This is Rietveld 408576698