Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1066823004:
PlzNavigate: improve tracking of improper subframe swap-out. (Closed)
|
Created:
5 years, 8 months ago by carlosk Modified:
5 years, 8 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, creis+watch_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: improve tracking of improper subframe swap-out.
Upon a FrameTreeNode destruction a speculative RenderFrameHost might
be swapped-out instead of being destroyed along with the node. This
behavior differs from the current navigation implementation and was
causing a leak of a RenderFrameProxyHost.
This change adds a CHECK to stop the leak the same way
RenderFrameHostManager::SwapOutOldFrame does.
BUG=439423
Committed: https://crrev.com/508434a400b2284b82c89d01e2f1970eb7a15546
Cr-Commit-Position: refs/heads/master@{#327044}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Revers the FTN change and adds a check for leaked RFPH. #Patch Set 3 : Rebase #Patch Set 4 : Changed NI::GetNavigationRequestForNodeForTesting signature. #
Total comments: 12
Patch Set 5 : Rebase and changes from CR coments (sorry about the mixed patch set) #
Total comments: 2
Patch Set 6 : Revert test changes #Messages
Total messages: 19 (4 generated)
carlosk@chromium.org changed reviewers: + clamy@chromium.org, fdegans@chromium.org
clamy@, fdegans@: PTAL. https://codereview.chromium.org/1066823004/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (left): https://codereview.chromium.org/1066823004/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:77: } N::CancelNavigation -> RFHM::CleanUpNavigation -> RFHM::DiscardUnusedFrame, which would swap out the RFH if there should be more references to its SiteInstance. So in RenderFrameHostManagerTest::DetachPendingChild, when it sends the first FrameHostMsg_Detach an extra reference to the SiteInstance was kept by the swapped out RFH when it shouldn't. This caused the last check in this test to fail (site_instance->HasOneRef()). https://codereview.chromium.org/1066823004/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/1066823004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl_unittest.cc:371: ASSERT_FALSE(navigator->GetNavigationRequestForNodeForTesting(subframe_node)); As I removed the N::CancelNavigation from FTN::~FrameTreeNode I wanted to be sure subframe navigations were still correctly cleaned up. Hence these additions to this test.
Thanks! It seems that this prevent the navigation request for the subframe from being properly cleaned up though. I think we may have to think of a different way to address the problem. https://codereview.chromium.org/1066823004/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (left): https://codereview.chromium.org/1066823004/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:77: } On 2015/04/08 09:01:48, carlosk wrote: > N::CancelNavigation -> RFHM::CleanUpNavigation -> RFHM::DiscardUnusedFrame, > which would swap out the RFH if there should be more references to its > SiteInstance. > > So in RenderFrameHostManagerTest::DetachPendingChild, when it sends the first > FrameHostMsg_Detach an extra reference to the SiteInstance was kept by the > swapped out RFH when it shouldn't. This caused the last check in this test to > fail (site_instance->HasOneRef()). I think this needs to remain in place otherwise the navigation request for the subframes does not get cleaned up. We ran into very similar issues when I was fixing this test in the first place, and Nasko mentioned that creating the swapped out RFH in the parent there might not make much sense. I suggest you talk to him to see how to address the problem, because I don't think this is the best approach. https://codereview.chromium.org/1066823004/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/1066823004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl_unittest.cc:371: ASSERT_FALSE(navigator->GetNavigationRequestForNodeForTesting(subframe_node)); On 2015/04/08 09:01:48, carlosk wrote: > As I removed the N::CancelNavigation from FTN::~FrameTreeNode I wanted to be > sure subframe navigations were still correctly cleaned up. Hence these additions > to this test. Well since it's failing they are not.
Thanks. No new upload yet. I'll talk to Nasko as suggested. https://codereview.chromium.org/1066823004/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (left): https://codereview.chromium.org/1066823004/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:77: } On 2015/04/08 11:29:11, clamy wrote: > On 2015/04/08 09:01:48, carlosk wrote: > > N::CancelNavigation -> RFHM::CleanUpNavigation -> RFHM::DiscardUnusedFrame, > > which would swap out the RFH if there should be more references to its > > SiteInstance. > > > > So in RenderFrameHostManagerTest::DetachPendingChild, when it sends the first > > FrameHostMsg_Detach an extra reference to the SiteInstance was kept by the > > swapped out RFH when it shouldn't. This caused the last check in this test to > > fail (site_instance->HasOneRef()). > > I think this needs to remain in place otherwise the navigation request for the > subframes does not get cleaned up. We ran into very similar issues when I was > fixing this test in the first place, and Nasko mentioned that creating the > swapped out RFH in the parent there might not make much sense. I suggest you > talk to him to see how to address the problem, because I don't think this is the > best approach. Acknowledged. https://codereview.chromium.org/1066823004/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/1066823004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl_unittest.cc:371: ASSERT_FALSE(navigator->GetNavigationRequestForNodeForTesting(subframe_node)); On 2015/04/08 11:29:11, clamy wrote: > On 2015/04/08 09:01:48, carlosk wrote: > > As I removed the N::CancelNavigation from FTN::~FrameTreeNode I wanted to be > > sure subframe navigations were still correctly cleaned up. Hence these > additions > > to this test. > > Well since it's failing they are not. Yeah. It did pass on my workstation but I only tested in Debug. Now I see it does not in Release...
carlosk@chromium.org changed reviewers: + nasko@chromium.org
nasko@: PTAL As discussed yesterday I updated this change to add the CHECK to avoid the leak and removed my previous "fix". I left the test changes in place as it's an improvement over what we had before.
Patchset #4 (id:60001) has been deleted
I updated the signature of NavigatorImpl::GetNavigationRequestForNodeForTesting to directly take the FrameTreeNode ID to allow using it for already destroyed frames. This required a few more changes to other tests using it but it seems to still make sense to leave that in this CL. The ASAN trybot found that issue for me (thankfully).
Thanks! A few comments. https://codereview.chromium.org/1066823004/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/1066823004/diff/80001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:278: process()->sink().ClearMessages(); Why this line? https://codereview.chromium.org/1066823004/diff/80001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:372: // Commit the navigation. Add that the subframe will be destroyed as a result. https://codereview.chromium.org/1066823004/diff/80001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:375: subframe_node = nullptr; // Was just destroyed. I don't think this line is needed. https://codereview.chromium.org/1066823004/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1066823004/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:668: CHECK(!GetRenderFrameProxyHost(site_instance)); Maybe move the CHECK above the creation of the new proxy?
https://codereview.chromium.org/1066823004/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.h (right): https://codereview.chromium.org/1066823004/diff/80001/content/browser/frame_h... content/browser/frame_host/navigator_impl.h:90: NavigationRequest* GetNavigationRequestForNodeForTesting( nit: I'd drop the "ForNode" part of the name, as it is no longer for a node, but it looks up by id. It will make it easier to read and won't make it less clear what it does. https://codereview.chromium.org/1066823004/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/1066823004/diff/80001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:368: // The subframe request is still there as the subframe still exists. This doesn't match reality and I'm not sure it is a good idea to include. When a parent frame commits a new document, all of the existing subframes will disappear, as the old document is being destroyed. Is it possible to split in a different test case what you are trying to test for subframes?
Thanks for the latest comments. With clamy@'s change that moved the ownership of NavigationRequest instances to the FrameTreeNode (http://crrev.com/1080073004) the initial issue of this change was solved: now a destroyed subframe properly cleans up its own NavigationRequest. But the added CHECK to RFHM::DiscardUnusedFrame and the test modifications still look useful to me. WDYT? https://chromiumcodereview.appspot.com/1066823004/diff/80001/content/browser/... File content/browser/frame_host/navigator_impl.h (right): https://chromiumcodereview.appspot.com/1066823004/diff/80001/content/browser/... content/browser/frame_host/navigator_impl.h:90: NavigationRequest* GetNavigationRequestForNodeForTesting( On 2015/04/14 16:06:29, nasko (very slow until 04-27) wrote: > nit: I'd drop the "ForNode" part of the name, as it is no longer for a node, but > it looks up by id. It will make it easier to read and won't make it less clear > what it does. This method doesn't exist anymore. https://chromiumcodereview.appspot.com/1066823004/diff/80001/content/browser/... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/1066823004/diff/80001/content/browser/... content/browser/frame_host/navigator_impl_unittest.cc:278: process()->sink().ClearMessages(); On 2015/04/14 15:32:03, clamy wrote: > Why this line? As there's more than one commit happening and we check for a single commit messages later on to determine which RFH committed (DidRenderFrameHostRequestCommit) this cleanup is necessary at this point. https://chromiumcodereview.appspot.com/1066823004/diff/80001/content/browser/... content/browser/frame_host/navigator_impl_unittest.cc:368: // The subframe request is still there as the subframe still exists. On 2015/04/14 16:06:29, nasko (very slow until 04-27) wrote: > This doesn't match reality and I'm not sure it is a good idea to include. When a > parent frame commits a new document, all of the existing subframes will > disappear, as the old document is being destroyed. > > Is it possible to split in a different test case what you are trying to test for > subframes? The request still exists because at this stage we're still at the old document. The subframe is only eliminated when the navigated RFH finally navigates (in the SendNavigate call below). Instead of creating a new test I think this one should be renamed to better match what it's actually testing: a complex use-case of simultaneous navigation at the root and subframe levels. I made this change in the latest patch set so WDYT? https://chromiumcodereview.appspot.com/1066823004/diff/80001/content/browser/... content/browser/frame_host/navigator_impl_unittest.cc:372: // Commit the navigation. On 2015/04/14 15:32:03, clamy wrote: > Add that the subframe will be destroyed as a result. This code was removed. https://chromiumcodereview.appspot.com/1066823004/diff/80001/content/browser/... content/browser/frame_host/navigator_impl_unittest.cc:375: subframe_node = nullptr; // Was just destroyed. On 2015/04/14 15:32:03, clamy wrote: > I don't think this line is needed. I was copying a pattern I saw in RenderFrameHostManagerTest::DetachPendingChild (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...) where the subframe pointer was set to NULL. It makes it clear that the pointer is now invalid and will SEGFAULT if one should inadvertently try to use it (faster feedback than waiting for ASAN trybot results). But this code was removed anyway. https://chromiumcodereview.appspot.com/1066823004/diff/80001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/1066823004/diff/80001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:668: CHECK(!GetRenderFrameProxyHost(site_instance)); On 2015/04/14 15:32:04, clamy wrote: > Maybe move the CHECK above the creation of the new proxy? Done.
Thanks! I'm a bit skeptical of the test changes, since we are essentially testing that a subframe exists until commit. This seems like something already tested somewhere else. https://codereview.chromium.org/1066823004/diff/100001/content/browser/frame_... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/1066823004/diff/100001/content/browser/frame_... content/browser/frame_host/navigator_impl_unittest.cc:263: TEST_F(NavigatorTestWithBrowserSideNavigation, Note that the original goal of this test is to check whether we create proper NavigationRequests for root/subframe navigations. In particular, computing the first_party_for_cookies/is_main_frame/parent_is_main_frame part. I'm not sure we need to expend it. https://codereview.chromium.org/1066823004/diff/100001/content/browser/frame_... content/browser/frame_host/navigator_impl_unittest.cc:344: // Have the navigation commit for the root node's speculative RenderFrameHost. This comment is misleading. If you want to simulate a full navigation commit, then you need to add a DidNavigate at the end. This will destroy the subframe, making the last check non-relevant.
On 2015/04/27 12:05:20, clamy wrote: > Thanks! I'm a bit skeptical of the test changes, since we are essentially > testing that a subframe exists until commit. This seems like something already > tested somewhere else. Thanks. I agree that the test changes were finally not very relevant so I reverted them (so no use in replying to the other comments). All that is left is the new CHECK added to RFHM::DiscardUnusedFrame.
LGTM
Thanks1 lgtm.
The CQ bit was checked by carlosk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1066823004/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/508434a400b2284b82c89d01e2f1970eb7a15546 Cr-Commit-Position: refs/heads/master@{#327044} |
