Keep track of pending delete RenderFrameHosts |
||||||||||||
Issue descriptionVersion: 52.0.2726.0 OS: All RenderFrameHostManagerTest.RestoreSubframeFileAccessForHistoryNavigation is failing when run with --isolate-sites-for-testing=*.is or --site-per-process. (Same in any mode that sets UseSubframeNavigationEntries to true.) ../../content/browser/frame_host/render_frame_host_manager_browsertest.cc:1948: Failure Value of: files.size() Actual: 0 Expected: 1U Which is: 1 This happens because RenderFrameImpl::OnSwapOut only sends an UpdateState message for the frame being swapped. The test navigates the main frame and expects a PageState to be sent for the subframe (which is detached as a result of the swap). It fails because we don't update the subframe's FrameNavigationEntry. (This isn't an issue in default Chrome because the UpdateState message contains info for all frames in that mode.) It's fairly easy to send the UpdateState message for any RenderFrame before its deleted, but that's only part of the problem. In the browser process, we call FrameTreeNode::ResetForNewProcess at commit time and delete all subframe FrameTreeNodes and RenderFrameHosts, before giving them a chance to run their unload handlers or send UpdateState IPCs. This means no one is listening in the browser process when the subframe's UpdateState IPC arrives, so the test continues to fail. In a practical sense, this is fairly low impact to the user-- only the last 1 second of changes to a subframe's PageState are lost if its parent navigates cross-process. (We batch up and send UpdateState changes after 1 second, so this will rarely impact users.) However, we've known that the ResetForNewProcess issue could lead to problems like this, and we should investigate ways to deal with it.
,
Jun 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8aa4ed83d329e038980afa0fd10d880415b853a9 commit 8aa4ed83d329e038980afa0fd10d880415b853a9 Author: creis <creis@chromium.org> Date: Mon Jun 20 23:24:04 2016 Remove RenderFrameImpl::is_detaching_, since it appears no longer needed. ajwong@ added this in r219965 when making RFHs routable, without an explanation at the time. https://codereview.chromium.org/22876014/diff/76001/content/renderer/render_frame_impl.cc ajwong@ later added a comment in r234992 about how it was necessary to prevent observers from sending IPC messages after detach: https://codereview.chromium.org/67313010/diff/160001/content/renderer/render_frame_impl.cc nasko@ preserved this comment in r244023 when switching us over to always have subframe RFHs: https://codereview.chromium.org/117603002/diff/960001/content/renderer/render_frame_impl.cc However, gcasto removed the observer behavior in r320838. Since that CL, observers have been able to send IPCs during detach and (apparently) almost nothing is disabled by is_detaching_: https://codereview.chromium.org/1014703003/diff/20001/content/renderer/render_frame_impl.cc As a result, this appears to be mostly dead code at this point, for over a year. Since I'd like to add more IPCs after detach, I'm removing this restriction. BUG=609963 TEST=Existing tests still pass. Review-Url: https://codereview.chromium.org/2073413002 Cr-Commit-Position: refs/heads/master@{#400829} [modify] https://crrev.com/8aa4ed83d329e038980afa0fd10d880415b853a9/content/renderer/render_frame_impl.cc [modify] https://crrev.com/8aa4ed83d329e038980afa0fd10d880415b853a9/content/renderer/render_frame_impl.h
,
Jun 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dab7c8f09b323e70dbcd457963e84d35276e0cea commit dab7c8f09b323e70dbcd457963e84d35276e0cea Author: creis <creis@chromium.org> Date: Wed Jun 22 03:41:46 2016 Cache RenderFrameHost's parent so it doesn't change over time. RenderFrameHost::GetParent() should always return the same value for a given frame. However, we will soon allow RenderFrameHosts to continue to exist briefly in the background in a pending deletion state, such that we can wait for their unload handlers, PageState, etc. In these cases, the parent FrameTreeNode will have already moved to a new current RenderFrameHost. Rather than asking the parent FrameTreeNode, just cache the parent RenderFrameHost in the child. BUG=609963 TEST=No behavior change. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2086083003 Cr-Commit-Position: refs/heads/master@{#401198} [modify] https://crrev.com/dab7c8f09b323e70dbcd457963e84d35276e0cea/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/dab7c8f09b323e70dbcd457963e84d35276e0cea/content/browser/frame_host/render_frame_host_impl.h
,
Jun 24 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/75504cc6c988e7765a7c61e7c0251b9886cd84dc commit 75504cc6c988e7765a7c61e7c0251b9886cd84dc Author: creis <creis@chromium.org> Date: Fri Jun 24 19:20:07 2016 Make RestoreSubframeFileAccessForHistoryNavigation pass in OOPIF modes. Fixing the test properly will require tracking FrameTreeNodes that are pending deletion, so that we can hear UpdateState messages from subframes after doing a cross-process navigation in their ancestor. Since we're only losing the last second of PageState changes at the moment, we can work around this for now to let us proceed with moving to the new navigation path. Adding an in-page navigation ensures that the PageState arrives in the browser process, letting the test pass. BUG=609963 TEST=RestoreSubframeFileAccessForHistoryNavigation passes in --isolate-sites-for-testing=*.is or --site-per-process. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2093123002 Cr-Commit-Position: refs/heads/master@{#401924} [modify] https://crrev.com/75504cc6c988e7765a7c61e7c0251b9886cd84dc/content/browser/frame_host/render_frame_host_manager_browsertest.cc [modify] https://crrev.com/75504cc6c988e7765a7c61e7c0251b9886cd84dc/testing/buildbot/filters/isolate-extensions.content_browsertests.filter [modify] https://crrev.com/75504cc6c988e7765a7c61e7c0251b9886cd84dc/testing/buildbot/filters/site-per-process.content_browsertests.filter
,
Sep 12 2017
It's unclear if this is causing issues in practice, but lukasza@ found it may be useful for issue 763549.
,
Nov 7 2017
,
Nov 7 2017
Apologies, applied the wrong component in bulk.
,
Nov 7 2017
,
Jun 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1bfc151cd3805a81e6488dd7b4cd65fd08f3584e commit 1bfc151cd3805a81e6488dd7b4cd65fd08f3584e Author: Alex Moshchuk <alexmos@chromium.org> Date: Thu Jun 14 20:49:51 2018 Delay subframe process shutdown to allow subframe unload handlers to execute. Currently, if a page with an OOPIF navigates cross-process, we immediately destroy the OOPIF's RenderFrameHost when its parent commits (see FrameTreeNode::ResetForNewProcess()). The RFH destruction triggers a message to the renderer to delete the RenderFrame, which also causes it to run its unload handler. However, if the subframe was the last active frame in its process, the process will proceed to shut down without waiting for the unload handler to run at all, and in such a case, the subframe unload handler won't really be able to do anything, including termination pings. This CL fixes this by delaying subframe process shutdown in such situations by one second (which matches the existing unload timeout that we use for pending delete RenderFrameHosts). Bug: 852204 , 609963 Change-Id: I142ef4e6f59df465c6ed213df02daf492e989c2d Reviewed-on: https://chromium-review.googlesource.com/1094917 Reviewed-by: Steven Holte <holte@chromium.org> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Commit-Queue: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#567395} [modify] https://crrev.com/1bfc151cd3805a81e6488dd7b4cd65fd08f3584e/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/1bfc151cd3805a81e6488dd7b4cd65fd08f3584e/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/1bfc151cd3805a81e6488dd7b4cd65fd08f3584e/content/browser/frame_host/render_frame_host_manager_browsertest.cc [modify] https://crrev.com/1bfc151cd3805a81e6488dd7b4cd65fd08f3584e/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/1bfc151cd3805a81e6488dd7b4cd65fd08f3584e/content/browser/renderer_host/render_process_host_impl.h [modify] https://crrev.com/1bfc151cd3805a81e6488dd7b4cd65fd08f3584e/content/public/browser/render_process_host.h [modify] https://crrev.com/1bfc151cd3805a81e6488dd7b4cd65fd08f3584e/tools/metrics/histograms/histograms.xml
,
Jun 15 2018
Thanks Alex! Temporarily assigning to Nasko since he's working on plan #2 from comment 1 in the following CLs: https://chromium-review.googlesource.com/c/chromium/src/+/1098562 https://chromium-review.googlesource.com/c/chromium/src/+/1101297 Nasko, feel free to reassign if needed.
,
Jun 15 2018
,
Jun 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/16e6577a8fa81b76b044373c5116f73fc89d09f6 commit 16e6577a8fa81b76b044373c5116f73fc89d09f6 Author: Alex Moshchuk <alexmos@chromium.org> Date: Mon Jun 18 21:31:26 2018 Delay subframe process shutdown to allow subframe unload handlers to execute. (Merge to M68) Currently, if a page with an OOPIF navigates cross-process, we immediately destroy the OOPIF's RenderFrameHost when its parent commits (see FrameTreeNode::ResetForNewProcess()). The RFH destruction triggers a message to the renderer to delete the RenderFrame, which also causes it to run its unload handler. However, if the subframe was the last active frame in its process, the process will proceed to shut down without waiting for the unload handler to run at all, and in such a case, the subframe unload handler won't really be able to do anything, including termination pings. This CL fixes this by delaying subframe process shutdown in such situations by one second (which matches the existing unload timeout that we use for pending delete RenderFrameHosts). TBR=alexmos@chromium.org (cherry picked from commit 1bfc151cd3805a81e6488dd7b4cd65fd08f3584e) Bug: 852204 , 609963 Change-Id: I142ef4e6f59df465c6ed213df02daf492e989c2d Reviewed-on: https://chromium-review.googlesource.com/1094917 Reviewed-by: Steven Holte <holte@chromium.org> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Commit-Queue: Alex Moshchuk <alexmos@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#567395} Reviewed-on: https://chromium-review.googlesource.com/1105179 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/branch-heads/3440@{#422} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} [modify] https://crrev.com/16e6577a8fa81b76b044373c5116f73fc89d09f6/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/16e6577a8fa81b76b044373c5116f73fc89d09f6/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/16e6577a8fa81b76b044373c5116f73fc89d09f6/content/browser/frame_host/render_frame_host_manager_browsertest.cc [modify] https://crrev.com/16e6577a8fa81b76b044373c5116f73fc89d09f6/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/16e6577a8fa81b76b044373c5116f73fc89d09f6/content/browser/renderer_host/render_process_host_impl.h [modify] https://crrev.com/16e6577a8fa81b76b044373c5116f73fc89d09f6/content/public/browser/render_process_host.h [modify] https://crrev.com/16e6577a8fa81b76b044373c5116f73fc89d09f6/tools/metrics/histograms/histograms.xml
,
Jun 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf commit 92b72c7ca498e61a6e7afacb300b0246a0d5b9cf Author: Alex Moshchuk <alexmos@chromium.org> Date: Fri Jun 22 00:30:25 2018 Delay subframe process shutdown to allow subframe unload handlers to execute. (Merge to M67) Currently, if a page with an OOPIF navigates cross-process, we immediately destroy the OOPIF's RenderFrameHost when its parent commits (see FrameTreeNode::ResetForNewProcess()). The RFH destruction triggers a message to the renderer to delete the RenderFrame, which also causes it to run its unload handler. However, if the subframe was the last active frame in its process, the process will proceed to shut down without waiting for the unload handler to run at all, and in such a case, the subframe unload handler won't really be able to do anything, including termination pings. This CL fixes this by delaying subframe process shutdown in such situations by one second (which matches the existing unload timeout that we use for pending delete RenderFrameHosts). TBR=alexmos@chromium.org (cherry picked from commit 1bfc151cd3805a81e6488dd7b4cd65fd08f3584e) Bug: 852204 , 609963 Change-Id: I142ef4e6f59df465c6ed213df02daf492e989c2d Reviewed-on: https://chromium-review.googlesource.com/1094917 Reviewed-by: Steven Holte <holte@chromium.org> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Commit-Queue: Alex Moshchuk <alexmos@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#567395} Reviewed-on: https://chromium-review.googlesource.com/1111331 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#789} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf/content/browser/frame_host/render_frame_host_manager_browsertest.cc [modify] https://crrev.com/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf/content/browser/renderer_host/render_process_host_impl.h [modify] https://crrev.com/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf/content/public/browser/render_process_host.h [modify] https://crrev.com/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf/tools/metrics/histograms/histograms.xml
,
Jun 28 2018
Nasko, Arthur and I chatted today, and Arthur graciously agreed to keep pushing the plan from comment #11 forward while Nasko is on vacation.
,
Jul 9
I have a WIP patch here: https://chromium-review.googlesource.com/c/chromium/src/+/1122629 This is a continuation of Nasko's patch, but with all the tests fixed except one: https://chromium-review.googlesource.com/c/chromium/src/+/1101297/3 The remaining test is: ``` SitePerProcessBrowserTest.ParentOriginDoesNotChangeInUnloadHandler // Ensure that after a main frame with an OOPIF is navigated cross-site, the // unload handler in the OOPIF sees correct main frame origin, namely the old // and not the new origin. See https://crbug.com/825283. ``` When navigating from A(B) to C, when B executes its unload handler. Its parents (WebRemoteFrame) origin is now C instead of A. Doing it correctly was done in: https://chromium-review.googlesource.com/c/chromium/src/+/984729 In NavigationImpl::DidNavigate(), removing the iframes's RenderFrameHosts was done before calling FrameTreeNode::SetCurrentOrigin(). They are no more removed, so this test regressed. This one looks harder to fix. I am not really sure how to fix it.
,
Aug 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4973b39587712fa4f01c88efc9069ffa1aa43421 commit 4973b39587712fa4f01c88efc9069ffa1aa43421 Author: Arthur Sonzogni <arthursonzogni@chromium.org> Date: Mon Aug 20 18:14:45 2018 Remove renderer-initiated Shutdown. The browser process is the sole responsible for shutting down its renderer processes. Surprisingly, there was some logic in the renderer process to request self shutdown. There were race conditions. For instance: 1) The renderer process requests to shutdown itself. 2) The browser process accepts the request, but don't update any of its internal state. 3) The browser process reuse that process. 4) The renderer process shutdown itself. The CL removes the capability for a renderer process to shutdown itself. This CL is part of https://crbug.com/609963. This CL may also fix bug https://crbug.com/873541. Bug: 873541, 609963, 535246 Change-Id: Idd498497acc6106fe8c87e994239bbde9b3453c3 Reviewed-on: https://chromium-review.googlesource.com/1174713 Commit-Queue: Nasko Oskov <nasko@chromium.org> Reviewed-by: Nasko Oskov <nasko@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#584502} [modify] https://crrev.com/4973b39587712fa4f01c88efc9069ffa1aa43421/content/browser/renderer_host/render_process_host_browsertest.cc [modify] https://crrev.com/4973b39587712fa4f01c88efc9069ffa1aa43421/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/4973b39587712fa4f01c88efc9069ffa1aa43421/content/browser/renderer_host/render_process_host_impl.h [modify] https://crrev.com/4973b39587712fa4f01c88efc9069ffa1aa43421/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/4973b39587712fa4f01c88efc9069ffa1aa43421/content/common/renderer_host.mojom [modify] https://crrev.com/4973b39587712fa4f01c88efc9069ffa1aa43421/content/public/browser/render_process_host_observer.h [modify] https://crrev.com/4973b39587712fa4f01c88efc9069ffa1aa43421/content/renderer/render_thread_impl.cc
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/22e314b01f55988d41b17f831c313cb82b3f1d4d commit 22e314b01f55988d41b17f831c313cb82b3f1d4d Author: Arthur Sonzogni <arthursonzogni@chromium.org> Date: Thu Aug 23 13:55:56 2018 Fix fullscreen not removed for frame in pending deletion. RenderFrameHostImpl are removed from the set of frame in fullscreen when they are deleted. It means they are still in this set in between their swap out and their deletion. As a result this test was flaky on slow devices: WebContentsImplbrowserTest.NotifyFullscreenAcquired_Navigate This CL: * Fix this issue by removing the frame immediatly when it is swapped out. * Renable the flaky test on Android. * Add a regression test. Bug: 855018 , 609963 Change-Id: Ic858acd462cd8f80f39b76c09d5fbd8f9c3110b1 Reviewed-on: https://chromium-review.googlesource.com/1181433 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#585473} [modify] https://crrev.com/22e314b01f55988d41b17f831c313cb82b3f1d4d/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/22e314b01f55988d41b17f831c313cb82b3f1d4d/content/browser/frame_host/render_frame_host_manager.cc [modify] https://crrev.com/22e314b01f55988d41b17f831c313cb82b3f1d4d/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/22e314b01f55988d41b17f831c313cb82b3f1d4d/content/browser/web_contents/web_contents_impl.h [modify] https://crrev.com/22e314b01f55988d41b17f831c313cb82b3f1d4d/content/browser/web_contents/web_contents_impl_browsertest.cc [modify] https://crrev.com/22e314b01f55988d41b17f831c313cb82b3f1d4d/content/test/content_browser_test_utils_internal.cc [modify] https://crrev.com/22e314b01f55988d41b17f831c313cb82b3f1d4d/content/test/content_browser_test_utils_internal.h
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a commit c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a Author: Arthur Sonzogni <arthursonzogni@chromium.org> Date: Wed Sep 05 08:51:33 2018 Remove reference to AddRefProcess/ReleaseProcess() in the renderer. The browser process is the sole responsible for shutting down its renderer processes. Surprisingly, there was some logic in the renderer process to request self shutdown. Previous CL disabled renderer process initiated shutdown. This CL removes associated logic to increment/decrement the reference counter. Bug: 873541, 609963, 535246 Change-Id: I06bc6670d84dcad597bf98eea6c3e07242f341b6 Reviewed-on: https://chromium-review.googlesource.com/1177748 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Makoto Shimazu <shimazu@chromium.org> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Cr-Commit-Position: refs/heads/master@{#588799} [modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/child/child_process.h [modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/loader/web_url_loader_impl.cc [modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/render_frame_impl.cc [modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/render_process_impl.cc [modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/render_process_impl.h [modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/render_thread_impl.cc [modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/render_widget.cc [modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/renderer_blink_platform_impl.cc [modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/renderer_blink_platform_impl.h [modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/service_worker/embedded_worker_instance_client_impl.cc [modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/service_worker/embedded_worker_instance_client_impl.h [modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/shared_worker/embedded_shared_worker_stub.cc [modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/shared_worker/embedded_shared_worker_stub.h [modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/third_party/blink/public/platform/platform.h [modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/third_party/blink/renderer/platform/blob/blob_bytes_provider.cc
,
Sep 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/929d29fa8985277d1846d9a96807ec1e1b862edd commit 929d29fa8985277d1846d9a96807ec1e1b862edd Author: Arthur Sonzogni <arthursonzogni@chromium.org> Date: Mon Sep 17 14:19:44 2018 Refactor the set of frames in fullscreen. Instead of tracking FrameTreeNode, tracks RenderFrameImpl. There are several reasons: 1) WebContentsImpl::FullscreenStateChanged() takes RenderFrameHostImpl* in input and notifies its observers using a RenderFrameHostImpl*. There is no real need to convert from/to a FrameTreeNode. 2) This function traverses the tree of FrameTreeNode, but is called in RenderFrameHostImpl* destructor. It means the tree is in the middle of modifying itself. This is hard to reason about. For instance: rfh->frame_tree_node()->current_frame_host() may be null here, even when rfh is the current frame host. Traversing the tree using only rfh->GetParent() is easier. Bug: 609963, 881849 Change-Id: I2e74324f83ddb2800b4dd177b561ff987c2c0a5f Reviewed-on: https://chromium-review.googlesource.com/1207753 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Becca Hughes <beccahughes@chromium.org> Cr-Commit-Position: refs/heads/master@{#591667} [modify] https://crrev.com/929d29fa8985277d1846d9a96807ec1e1b862edd/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/929d29fa8985277d1846d9a96807ec1e1b862edd/content/browser/web_contents/web_contents_impl.h [modify] https://crrev.com/929d29fa8985277d1846d9a96807ec1e1b862edd/content/browser/web_contents/web_contents_impl_browsertest.cc
,
Sep 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/da90ece84fb07b4c548b127580428b7f5378c78f commit da90ece84fb07b4c548b127580428b7f5378c78f Author: Arthur Sonzogni <arthursonzogni@chromium.org> Date: Fri Sep 21 09:09:05 2018 Update GetRootRenderWidgetHostView() to explore the tree using RFH. GetRootRenderWidgetHostView() and GetParentRenderWidget() explore the FrameTree using FrameTreeNode. Once it is done, it returns the RenderWidgetHostView associated with the current RenderFrameHost on that FrameTreeNode. The issue: there is no 1:1 association between the FrameTreeNode and the RenderFrameHost. A FrameTreeNode, contains several RenderframeHost: * The current one. * The pending one, not displayed and used for loading a new document, but this one has still not committed. * The ones in pending deletion. Those are no more displayed and are in progress executing their javascript unload handlers. Instead of exploring the tree using FrameTreeNode::parent(), explore the tree using RenderFrameHost::GetParent(). That way, it is guaranteed the functions will always returns the same object. Bug: 609963 Change-Id: Ic8972180b94763bd6a831f2754d1e3c661c0bbc4 Reviewed-on: https://chromium-review.googlesource.com/1210942 Reviewed-by: Ken Buchanan <kenrb@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Lucas Gadani <lfg@chromium.org> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Cr-Commit-Position: refs/heads/master@{#593130} [modify] https://crrev.com/da90ece84fb07b4c548b127580428b7f5378c78f/content/browser/frame_host/cross_process_frame_connector.cc
,
Oct 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/252ae04baf912b8d734b03f29d0be98a5244a5ac commit 252ae04baf912b8d734b03f29d0be98a5244a5ac Author: Nasko Oskov <nasko@chromium.org> Date: Thu Oct 04 21:44:12 2018 Move subframe FrameTreeNode ownership RenderFrameHost. In order to better handle messages sent from RenderFrames in the renderer process during unload event, we should keep the browser side objects alive as long as possible. This CL moves ownership of subframes to RenderFrameHost, which can allow the subframes to live until the unload handler of the top parent has completed running. The CL is just a mechanical move of the ownership and does not change the lifetime of subframes for now, which will come in a follow up CL. Bug: 609963 Change-Id: I52456dc1bf68cb15caab527c16c96ddd13a80838 Reviewed-on: https://chromium-review.googlesource.com/c/1098562 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Commit-Queue: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#596869} [modify] https://crrev.com/252ae04baf912b8d734b03f29d0be98a5244a5ac/content/browser/find_request_manager_browsertest.cc [modify] https://crrev.com/252ae04baf912b8d734b03f29d0be98a5244a5ac/content/browser/frame_host/frame_tree.cc [modify] https://crrev.com/252ae04baf912b8d734b03f29d0be98a5244a5ac/content/browser/frame_host/frame_tree_node.cc [modify] https://crrev.com/252ae04baf912b8d734b03f29d0be98a5244a5ac/content/browser/frame_host/frame_tree_node.h [modify] https://crrev.com/252ae04baf912b8d734b03f29d0be98a5244a5ac/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/252ae04baf912b8d734b03f29d0be98a5244a5ac/content/browser/frame_host/render_frame_host_impl.h
,
Oct 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/97739b2d9b25f3b77e5f3ab658e2c4061926c761 commit 97739b2d9b25f3b77e5f3ab658e2c4061926c761 Author: Nasko Oskov <nasko@chromium.org> Date: Fri Oct 05 21:14:18 2018 Use the RenderFrameHost SiteInstance when creating subframes. This is a follow up on a comment from https://chromium-review.googlesource.com/c/chromium/src/+/1098562 Bug: 609963 Change-Id: I8ba97bcbb8961f1dbacd4d3fe5c1766c44c20042 Reviewed-on: https://chromium-review.googlesource.com/c/1265023 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#597299} [modify] https://crrev.com/97739b2d9b25f3b77e5f3ab658e2c4061926c761/content/browser/frame_host/render_frame_host_impl.cc
,
Oct 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/969a794e014a8851806cc6792d7b453a070c1de7 commit 969a794e014a8851806cc6792d7b453a070c1de7 Author: Arthur Sonzogni <arthursonzogni@chromium.org> Date: Thu Oct 11 08:56:09 2018 Replace FTN::IsDescendantOf by RFH::IsDescendantOf A FrameTreeNode hosts several RenderFrameHost like: * The current RenderFrameHost. * The future one that is loading but hasn't committed yet. * The ones pending deletion. On its turn a RenderFrameHost contains several children frames, represented by FrameTreeNode. This CL replace FTN::IsDescendantOf by RFH::IsDescendantOf. * The new form of IsDescendantOf is used in follow up CLs. * In the case of one RenderFrameHost being pending deletion, the meaning is a bit different. For instance during a navigation from A(B) to C. Both RenderFrameHost A and C are on the same FrameTreeNode. The FrameTreeNode of B is a child of the FrameTreeNode of C, but B is not a child of C, only a child of A. Add a content/public API so that embedders won't have to manually iterate on RenderFrameHost::GetParent() themselves to get this information. This will be used in a follow-up CL. Bug: 609963 Change-Id: Iedb4a7acf17c7782cd9e91dd529a2736c1fd51d4 Reviewed-on: https://chromium-review.googlesource.com/c/1269874 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#598702} [modify] https://crrev.com/969a794e014a8851806cc6792d7b453a070c1de7/content/browser/frame_host/frame_tree_node.cc [modify] https://crrev.com/969a794e014a8851806cc6792d7b453a070c1de7/content/browser/frame_host/frame_tree_node.h [modify] https://crrev.com/969a794e014a8851806cc6792d7b453a070c1de7/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/969a794e014a8851806cc6792d7b453a070c1de7/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/969a794e014a8851806cc6792d7b453a070c1de7/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/969a794e014a8851806cc6792d7b453a070c1de7/content/public/browser/render_frame_host.h
,
Oct 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3bcb8b871a8a6adb3db213b591e830ab2ed98441 commit 3bcb8b871a8a6adb3db213b591e830ab2ed98441 Author: Arthur Sonzogni <arthursonzogni@chromium.org> Date: Thu Oct 11 12:35:10 2018 Task manager: Make it work with iframes in pending deletion. This CL is a sub-part of: [Don't delete subframes on CommitPending] (https://chromium-review.googlesource.com/c/chromium/src/+/1122629/18) RenderFrameHosts (subframes) are allowed to stay alive longer in the background, the time needed for them to execute their unload handler. The WebContentsTaskProvider needs to stop tracking all the frames in pending deletion, not only the navigating one, but also its subframes. Not doing it properly triggers a DCHECK: --- // Whenever we have a task, we should have a main frame site instance. DCHECK(tasks_by_frames_.empty() == (main_frame_site_instance_ == nullptr)); --- With tasks_by_frames_ not being empty, but main_frame_site_instance being null after removing the old_frame in RenderFrameHostChanged. Bug: 609963 Change-Id: I6e4ad45fb0d47ab378396e9ea1ad97bfc27c9c31 Reviewed-on: https://chromium-review.googlesource.com/c/1163502 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Ahmed Fakhry <afakhry@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#598732} [modify] https://crrev.com/3bcb8b871a8a6adb3db213b591e830ab2ed98441/chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc
,
Oct 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5e34dd12472f056e7f8833da88c1a3c412d187a5 commit 5e34dd12472f056e7f8833da88c1a3c412d187a5 Author: Arthur Sonzogni <arthursonzogni@chromium.org> Date: Thu Oct 11 16:04:56 2018 FileSelectHelper: Don't give file access to a frame in pending deletion. This is a prerequisite to: https://chromium-review.googlesource.com/c/chromium/src/+/1122629 When a file chooser dialog is opened for an active iframe, but is closed when the iframe is in pending deletion, it should not continue. Bug: 609963 Change-Id: I897258f642e812de4b3abf117a2a83815568aba8 Reviewed-on: https://chromium-review.googlesource.com/c/1253783 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#598785} [modify] https://crrev.com/5e34dd12472f056e7f8833da88c1a3c412d187a5/chrome/browser/file_select_helper.cc
,
Oct 15
Reading through two last patches, I am concerned that current WebContentsObserver API is becoming unclear. I think it's hard to understand that RenderFrameHostChanged implies all children are pending deletion, and that client might need to do something about it. We might have some tests which fail for _some_ components, but there can easily be many related hidden bugs, which we just don't know about. Should we consider some other way? It would be much more convenient, for example, if RenderFrameHostChanged would be immediately followed (or preceeded) by FrameDeleted on all the child frames. Even if the RFH objects won't be deleted immediately - as a client of this API, I don't really care. WDYT? Maybe something similar?
,
Oct 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/06b336896d5384a8df0b3affc2424a97ce372759 commit 06b336896d5384a8df0b3affc2424a97ce372759 Author: Arthur Sonzogni <arthursonzogni@chromium.org> Date: Mon Oct 15 17:56:52 2018 FindRequestManager: Stop tracking subframe. This is a prerequisite for: https://chromium-review.googlesource.com/c/chromium/src/+/1122629 It makes subframes to stay alive after the navigation commit. Find-in-page needs to stop tracking these frames immediately when they are in pending deletion, not when they are deleted. Bug: 609963 Change-Id: I9c8fa05ec3d571e79ecc22c1609ee59ff90b2b45 Reviewed-on: https://chromium-review.googlesource.com/c/1254262 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Paul Meyer <paulmeyer@chromium.org> Cr-Commit-Position: refs/heads/master@{#599679} [modify] https://crrev.com/06b336896d5384a8df0b3affc2424a97ce372759/content/browser/find_request_manager.cc
,
Oct 15
On comment 27, we had discussed having a RenderFrameHostStateChanged event to signal the move from being an active RFH to one pending deletion. This would make it very clear what is happening and can be extended in the future to account for more lifecycle state transitions. Would this make the API more clear?
,
Oct 15
I also think we should update the current documentation on WCO::RenderFrameHostChanged, which currently doesn't explain what happens to old_rfh's subframe RFHs at all. The current behavior is that they're deleted (and will get FrameDeleted notifications) at the same time as RenderFrameHostChanged, but this has always been kind of implicit in the public API and not guaranteed AFAICT. With Arthur's change, the FrameDeleted for subframes will just happen later, after they complete running their unload handlers. > We might have some tests which fail for _some_ components, but there can easily be many related hidden bugs, which we just don't know about. Should we consider some other way? We were concerned about this too, which is why Arthur audited all uses of RenderFrameHostChanged and has a spreadsheet somewhere. There actually aren't that many uses, so it was feasible to go through them all. I think most observers he's updating now weren't actually resulting in test failures.
,
Oct 15
> On comment 27, we had discussed having a RenderFrameHostStateChanged event to signal the move from being an active RFH to one pending deletion. This would make it very clear what is happening and can be extended in the future to account for more lifecycle state transitions. Would this make the API more clear? Perhaps something like that will help. From my (limited) experience, most users just want to know when to stop tracking something related to RFH, and use RenderFrameHostChanged + FrameDeleted as instructed by WCO. If all of that can be replaced with a single callback, perhaps it could work out for most usecases. > I also think we should update the current documentation on WCO::RenderFrameHostChanged, which currently doesn't explain what happens to old_rfh's subframe RFHs at all. The current behavior is that they're deleted (and will get FrameDeleted notifications) at the same time as RenderFrameHostChanged, but this has always been kind of implicit in the public API and not guaranteed AFAICT. With Arthur's change, the FrameDeleted for subframes will just happen later, after they complete running their unload handlers. Out of curiosity, is it possible to keep this contract (and add comments to WCO): call FrameDeleted immediately, and delete later? Such a change definitely keeps API backwards-compatible. Would it break something else? > We were concerned about this too, which is why Arthur audited all uses of RenderFrameHostChanged and has a spreadsheet somewhere. There actually aren't that many uses, so it was feasible to go through them all. Glad to hear that, thank you! I didn't find the spreadsheet, that's why I've asked.
,
Oct 16
Here is the spreadsheet I made to check all users of RenderFrameHost: https://docs.google.com/spreadsheets/d/1T3COzhjzXXJplGE46FTniNI7fFRiBV30cUbOtJVsnv0/edit?usp=sharing I had failing tests for two of them (task manager + find-in-page). For the other, there was not failing tests. [Dmitry] > From my (limited) experience, most users just want to know when to stop tracking something related to RFH, and use RenderFrameHostChanged + FrameDeleted as instructed by WCO. If all of that can be replaced with a single callback, perhaps it could work out for most usecases. +1: Most users just want this. It would be very nice for them to have only one method to watch for instead of two. I think it might be possible to call FrameDeleted each time. What do you think? I think I am going to try this and see. [Dmitry] > Out of curiosity, is it possible to keep this contract (and add comments to WCO): call FrameDeleted immediately, and delete later? Such a change definitely keeps API backwards-compatible. Would it break something else? I don't know if it breaks something, but if it is possible, I would prefer avoiding "not to tell the truth" to embedders. Ideally I would like something like: * RenderFrameHostChange(_, new_host) to know when a new RenderFrameHost starts to be used. (!= created) * FrameDeleted(old_host) to know when it is deleted. (Always called) * [Optionally] RenderFrameHostStateChange(host, new_state) to know when it goes in pending deletion. Better, but I think this is very unlikely to happen. Instead of having all of this in WebContentsObserver, having a RenderFrameHostObserver. It will allow embedders to track a particular RenderFrameHost.
,
Oct 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c6ada4673960c873c61318f5ee6ab850bea49490 commit c6ada4673960c873c61318f5ee6ab850bea49490 Author: Arthur Sonzogni <arthursonzogni@chromium.org> Date: Wed Oct 17 11:07:18 2018 WebNavigationAPI. Do not track subframe in pending deletion. Current state: ============== After a cross-process navigation, content/ swaps the old and the new RenderFrameHost. The old RenderFrameHost goes into pending deletion. Its subframe are immediately deleted. So after a RenderFrameHost swap, WebNavigationAPI stops tracking: * the navigating frame in RenderFrameHostChanged(). * its subframe in FrameDeleted() Future: ====== In a near future, the subframe of |old_host| will go into pending deletion as well. As a result, FrameDeleted() will not be immediately called. See: https://chromium-review.googlesource.com/1122629 What this CL does: ================== Stop tracking the subframes of |old_host| when it goes into pending deletion. Bug: 609963 Change-Id: Ifba038af87a74f3b8e68794b62d3d44055efecad Reviewed-on: https://chromium-review.googlesource.com/c/1278170 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Nasko Oskov <nasko@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#600346} [modify] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc [modify] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/browser/extensions/api/web_navigation/web_navigation_api.h [modify] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc [modify] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/test/data/extensions/api_test/webnavigation/framework.js [add] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/test/data/extensions/api_test/webnavigation/pendingDeletion/iframe_slow_to_unload.html [add] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/test/data/extensions/api_test/webnavigation/pendingDeletion/manifest.json [add] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/test/data/extensions/api_test/webnavigation/pendingDeletion/slow_to_unload.html [add] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/test/data/extensions/api_test/webnavigation/pendingDeletion/test_pendingDeletion.html [add] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/test/data/extensions/api_test/webnavigation/pendingDeletion/test_pendingDeletion.js
,
Oct 17
> +1: Most users just want this. It would be very nice for them to have only one method to watch for instead of two. I think it might be possible to call FrameDeleted each time. What do you think? I think I am going to try this and see. Could you elaborate on "call FrameDeleted each time"? This sounds similar to my proposal, but you seem to imply something else. > Better, but I think this is very unlikely to happen. Instead of having all of this in WebContentsObserver, having a RenderFrameHostObserver. It will allow embedders to track a particular RenderFrameHost. My understanding is that embedders don't want to track RenderFrameHost. Basically, they want a signal "RenderFrameHost is not useful anymore". I don't see an issue if that doesn't particularly correspond to the destruction of RenderFrameHost object itself, as long as it does not appear in other APIs anymore. Strictly speaking, FrameDeleted(RFH*) is called before RFH is deleted anyway, so we won't change anything.
,
Oct 18
> Could you elaborate on "call FrameDeleted each time"? This sounds similar to my proposal, but you seem to imply something else. I was just approving your idea. I am talking about the same thing. > My understanding is that embedders don't want to track RenderFrameHost. Basically, they want a signal "RenderFrameHost is not useful anymore". I see. Maybe notifying them earlier than at deletion time is okay for all of them.
,
Oct 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9f370484d7256ce8e6b3d6955175edd4cc1faeb6 commit 9f370484d7256ce8e6b3d6955175edd4cc1faeb6 Author: Arthur Sonzogni <arthursonzogni@chromium.org> Date: Thu Oct 18 10:44:54 2018 RenderFrameHostDelegate: Fix documentation. Update the documentation of RenderFrame{Created,Deleted} The comments said it was about the creation/deletion of RenderFrameHost, but in reality, it is about its RenderFrame. This is different. Bug: 609963 Change-Id: Id83af9e82e7d544e68759dff2ffcefabb62c3374 Reviewed-on: https://chromium-review.googlesource.com/c/1286143 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#600720} [modify] https://crrev.com/9f370484d7256ce8e6b3d6955175edd4cc1faeb6/content/browser/frame_host/render_frame_host_delegate.h
,
Oct 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5a9a88c1b0b1e6913ed2e5b0349b17bfcd4f4c30 commit 5a9a88c1b0b1e6913ed2e5b0349b17bfcd4f4c30 Author: Arthur Sonzogni <arthursonzogni@chromium.org> Date: Thu Oct 18 13:16:00 2018 Remove FrameTreeNode::ResetForNewProcess(). Currently, this method is simply calling ResetChildren() on the current RenderFrameHost. It used to do more, but this is no longer the case. This CL removes this method. Seeing RenderFrameHostImpl::ResetChildren() in the code is more explicit about what happens. It is easier to understand. Bug: 609963 Change-Id: Ia82726f445b127ccca3758d7578e71eb32a49cbc Reviewed-on: https://chromium-review.googlesource.com/c/1286421 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#600742} [modify] https://crrev.com/5a9a88c1b0b1e6913ed2e5b0349b17bfcd4f4c30/content/browser/frame_host/frame_tree_node.cc [modify] https://crrev.com/5a9a88c1b0b1e6913ed2e5b0349b17bfcd4f4c30/content/browser/frame_host/frame_tree_node.h [modify] https://crrev.com/5a9a88c1b0b1e6913ed2e5b0349b17bfcd4f4c30/content/browser/frame_host/interstitial_page_impl.cc [modify] https://crrev.com/5a9a88c1b0b1e6913ed2e5b0349b17bfcd4f4c30/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/5a9a88c1b0b1e6913ed2e5b0349b17bfcd4f4c30/content/browser/frame_host/render_frame_host_manager.cc [modify] https://crrev.com/5a9a88c1b0b1e6913ed2e5b0349b17bfcd4f4c30/content/browser/web_contents/web_contents_impl.cc
,
Nov 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f8840b9771e2686d3054a311440abb48ac636e65 commit f8840b9771e2686d3054a311440abb48ac636e65 Author: Arthur Sonzogni <arthursonzogni@chromium.org> Date: Wed Nov 07 14:10:35 2018 Keep subframe alive in pending deletion. After a cross process navigation, the old RenderFrameHostImpl is kept alive the time needed for executing its unload handler. This CL generalize this by also keeping its subframes alive. This fixes unload handler not always being run. RenderFrameHostImpl is now delete as soon as: * All of its children have been deleted. * In case of an unload handler, wait FrameHostMsg_Detach. Explainer: https://goo.gl/85KSff Bug: 609963 Change-Id: Ie5b5585e54b9d1740798d649fcfac5dd2666920f Reviewed-on: https://chromium-review.googlesource.com/c/1122629 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#606036} [modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/frame_tree_node.cc [modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/frame_tree_node.h [modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/render_frame_host_manager.cc [modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/render_frame_host_manager_browsertest.cc [modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/render_frame_host_manager_unittest.cc [modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/render_frame_proxy_host.cc [modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/renderer/render_frame_impl.cc [modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/renderer/render_frame_impl.h [modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/test/test_render_frame_host.cc [modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/test/web_contents_observer_sanity_checker.cc
,
Jan 21
(2 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/549302586868ec5254ac03dcc3181051f3858990 commit 549302586868ec5254ac03dcc3181051f3858990 Author: Arthur Sonzogni <arthursonzogni@chromium.org> Date: Mon Jan 21 12:37:48 2019 Properly execute unload handlers on iframe after parent destruction. On iframe destruction, wait for its children to delete properly before deleting their parent. This CL is simply reusing what was introduced in : https://chromium-review.googlesource.com/1122629 Bug: 609963 Change-Id: Ic5825f586d73f76551c2ad7cb161e636f383d37f Reviewed-on: https://chromium-review.googlesource.com/c/1354922 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#624568} [modify] https://crrev.com/549302586868ec5254ac03dcc3181051f3858990/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/549302586868ec5254ac03dcc3181051f3858990/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/549302586868ec5254ac03dcc3181051f3858990/content/browser/frame_host/render_frame_proxy_host.cc [modify] https://crrev.com/549302586868ec5254ac03dcc3181051f3858990/content/browser/site_per_process_browsertest.cc |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by creis@chromium.org
, May 6 2016