Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
196133032:
Add test for filtering of IPC messages when RenderFrameHost is swapped out. (Closed)
|
Created:
6 years, 9 months ago by nasko Modified:
6 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, dcheng, site-isolation-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd test for filtering of IPC messages when RenderFrameHost is swapped out.
This is a followup CL to add test for the actual fix of filtering IPCs from swapped out RFHs. I've fixed the state transitions in RVH, since we had an illegal one and fixed up the unit tests to account for that.
BUG=351815
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259466
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix unit tests for cross-site navigation and SwapOut. #
Total comments: 17
Patch Set 3 : Fixes based on Charlie's review. #Patch Set 4 : Rebase on ToT. #Patch Set 5 : Add a test bypassing OnCrossSiteResponse. #
Total comments: 10
Patch Set 6 : Rebase on ToT. #Patch Set 7 : Another round of fixes. #
Total comments: 2
Patch Set 8 : Fix nits. #Patch Set 9 : Fix NavigationControllerTest.LoadURL_WithBindings #
Total comments: 3
Patch Set 10 : Fixing url_chain and comment. #
Messages
Total messages: 19 (0 generated)
Hey guys, Can you review this CL for me? Thanks, Nasko
lgtm
I like the update to the test! One question below. https://codereview.chromium.org/196133032/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/196133032/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_manager_unittest.cc:254: // which the test which follows it simulates being sent. The test is succsesful nit: which the FilterMessagesWhileSwappedOut test simulates nit: successful https://codereview.chromium.org/196133032/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_manager_unittest.cc:339: ntp_rvh->main_render_frame_host()->set_swapped_out(true); Is this line actually needed? You're checking for whether the RVH is swapped out in OnMessageReceived.
Charlie, Can you take another look? This CL fixes the state transition and the unit tests. It is rebased on top of the actual fix already. Thanks! Nasko https://codereview.chromium.org/196133032/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/196133032/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_manager_unittest.cc:254: // which the test which follows it simulates being sent. The test is succsesful On 2014/03/19 20:40:57, Charlie Reis wrote: > nit: which the FilterMessagesWhileSwappedOut test simulates > nit: successful Done. https://codereview.chromium.org/196133032/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_manager_unittest.cc:339: ntp_rvh->main_render_frame_host()->set_swapped_out(true); On 2014/03/19 20:40:57, Charlie Reis wrote: > Is this line actually needed? You're checking for whether the RVH is swapped > out in OnMessageReceived. Quite a bit has changed, so this is no longer applicable.
[+site-isolation-reviews] Glad to see the unit tests getting more realistic and checking the states-- thanks for adding that. A few thoughts below. https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:472: // happened yet. Please move "if it hasn't happened yet" to the end of the previous sentence, since it's about whether we need to run the unload handler, not whether it runs in the background. https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:476: RenderViewHostImpl::STATE_WAITING_FOR_COMMIT) { What if it were STATE_WAITING_FOR_UNLOAD_ACK? That means we've already called SwapOut and don't need to again. Presumably the same thing for PENDING_SWAP_OUT and PENDING_SHUTDOWN, right? Should this check be rvh_state() == STATE_DEFAULT? (Edit: I think so, since that's what we're checking for in RVHI::SwapOut.) https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:125: class IpcMessageObserver : public WebContentsObserver { This sounds like it's more general than just plugins and favicons. Maybe something like PluginFaviconMessageObserver? https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:178: void StartCrossSiteTransition(TestWebContents* contents) { Great, I'm a big fan of making most of the tests use this path instead of the shortcut in OnSwappedOut. Do we have any remaining tests for the OnSwappedOut path (i.e., without OnCrossSiteResponse)? If not, can we add one to make sure it's covered? https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:188: EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_UNLOAD_ACK, Great, glad to see these sorts of checks. https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:224: RenderViewHostDeletedObserver rvh_observer(contents()); I'm not thrilled with keeping the pointer to a deleted object and comparing it later, even if we don't dereference it. Here's one alternative: have the RenderViewHostDeletedObserver take in old_rvh and store the routing ID and process ID, plus a bool for whether it has been deleted. When the deletion occurs, flip the bool. You can then check the bool below. That has the added advantage of not getting fooled if two RVHs get deleted in a row. https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:391: // The old renderer, being slow, now updates the title. It should be filtered s/title/favicon/ https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:410: //EXPECT_TRUE(ntp_rvh->main_render_frame_host()->is_swapped_out()); Let's remove these lines until they're needed.
https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:472: // happened yet. On 2014/03/21 21:35:13, Charlie Reis wrote: > Please move "if it hasn't happened yet" to the end of the previous sentence, > since it's about whether we need to run the unload handler, not whether it runs > in the background. Done. https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:476: RenderViewHostImpl::STATE_WAITING_FOR_COMMIT) { On 2014/03/21 21:35:13, Charlie Reis wrote: > What if it were STATE_WAITING_FOR_UNLOAD_ACK? That means we've already called > SwapOut and don't need to again. Presumably the same thing for PENDING_SWAP_OUT > and PENDING_SHUTDOWN, right? > > Should this check be rvh_state() == STATE_DEFAULT? > > (Edit: I think so, since that's what we're checking for in RVHI::SwapOut.) Done. https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:125: class IpcMessageObserver : public WebContentsObserver { On 2014/03/21 21:35:13, Charlie Reis wrote: > This sounds like it's more general than just plugins and favicons. Maybe > something like PluginFaviconMessageObserver? Done. https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:178: void StartCrossSiteTransition(TestWebContents* contents) { On 2014/03/21 21:35:13, Charlie Reis wrote: > Great, I'm a big fan of making most of the tests use this path instead of the > shortcut in OnSwappedOut. > > Do we have any remaining tests for the OnSwappedOut path (i.e., without > OnCrossSiteResponse)? If not, can we add one to make sure it's covered? Does the NavigateWithEarlyReNavigation test case count? I've left it using the old way, since getting this test right using OnCrossSiteResponse wasn't trivial. https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:188: EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_UNLOAD_ACK, On 2014/03/21 21:35:13, Charlie Reis wrote: > Great, glad to see these sorts of checks. Done. https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:224: RenderViewHostDeletedObserver rvh_observer(contents()); On 2014/03/21 21:35:13, Charlie Reis wrote: > I'm not thrilled with keeping the pointer to a deleted object and comparing it > later, even if we don't dereference it. > > Here's one alternative: have the RenderViewHostDeletedObserver take in old_rvh > and store the routing ID and process ID, plus a bool for whether it has been > deleted. When the deletion occurs, flip the bool. You can then check the bool > below. > > That has the added advantage of not getting fooled if two RVHs get deleted in a > row. Done. https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:391: // The old renderer, being slow, now updates the title. It should be filtered On 2014/03/21 21:35:13, Charlie Reis wrote: > s/title/favicon/ Done. https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:410: //EXPECT_TRUE(ntp_rvh->main_render_frame_host()->is_swapped_out()); On 2014/03/21 21:35:13, Charlie Reis wrote: > Let's remove these lines until they're needed. Done.
LGTM. If we don't have test coverage for that case, let's add a test in a follow-up CL. https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/196133032/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:178: void StartCrossSiteTransition(TestWebContents* contents) { On 2014/03/24 17:48:11, nasko wrote: > On 2014/03/21 21:35:13, Charlie Reis wrote: > > Great, I'm a big fan of making most of the tests use this path instead of the > > shortcut in OnSwappedOut. > > > > Do we have any remaining tests for the OnSwappedOut path (i.e., without > > OnCrossSiteResponse)? If not, can we add one to make sure it's covered? > > Does the NavigateWithEarlyReNavigation test case count? I've left it using the > old way, since getting this test right using OnCrossSiteResponse wasn't trivial. Actually, that appears to be one of the few tests that doesn't hit the SwapOutOldPage call in DidNavigateMainFrame. I just locally changed DidNavigateMainFrame to have NOTREACHED instead and ran all the RenderFrameHostManagerTest.* tests, and that one didn't fail. If you try the same thing with this CL in place, do we hit still hit the NOTREACHED somewhere?
I've added a test as you suggested and in the meantime cleaned up an extra copy of an observer tracking RVH deletion. We have another instance of the same object in the _browsertest.cc file, so we should consolidate the implementation in one location in a follow up CL.
Thanks for the extra test! One question about it and one about the observer, plus a few nits. https://codereview.chromium.org/196133032/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/196133032/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:113: // pointers shouldn't be used, as the object is destroyed. I don't understand this comment. It says we shouldn't use the RVH pointers, but then we use them on the next line. If we're going to compare the pointers at all, then I don't think we need the process ID and routing ID check. At that point, we're essentially bringing RenderViewHostDestructionObserver up from below and using that plus the render_view_host_ = NULL line so that we don't have a dangling pointer afterward. https://codereview.chromium.org/196133032/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:1685: // Test that the RenderViewHost is properly swapped out if navigation in the a navigation https://codereview.chromium.org/196133032/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:1686: // new renderer commits before sendint the SwapOut message to the old renderer. sending https://codereview.chromium.org/196133032/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:1687: // This simulates a cross-site navigation to a synchronously committing URL a synchronously committing URL (e.g., a data URL) https://codereview.chromium.org/196133032/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:1722: // Simulate the effect of calling SwapOut in OnCrossSiteResponse. I thought we were testing the case without OnCrossSiteResponse. Won't DidNavigateFrame call SwapOut for us?
https://codereview.chromium.org/196133032/diff/80001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/196133032/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:113: // pointers shouldn't be used, as the object is destroyed. On 2014/03/24 23:24:43, Charlie Reis wrote: > I don't understand this comment. It says we shouldn't use the RVH pointers, but > then we use them on the next line. > > If we're going to compare the pointers at all, then I don't think we need the > process ID and routing ID check. > > At that point, we're essentially bringing RenderViewHostDestructionObserver up > from below and using that plus the render_view_host_ = NULL line so that we > don't have a dangling pointer afterward. Ok, got back to not using pointers. The aim of this check was to enforce the expectation that an RVH object with the same routing/process id pair is the exact same object in memory. https://codereview.chromium.org/196133032/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:1685: // Test that the RenderViewHost is properly swapped out if navigation in the On 2014/03/24 23:24:43, Charlie Reis wrote: > a navigation Done. https://codereview.chromium.org/196133032/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:1686: // new renderer commits before sendint the SwapOut message to the old renderer. On 2014/03/24 23:24:43, Charlie Reis wrote: > sending Done. https://codereview.chromium.org/196133032/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:1687: // This simulates a cross-site navigation to a synchronously committing URL On 2014/03/24 23:24:43, Charlie Reis wrote: > a synchronously committing URL (e.g., a data URL) Done. https://codereview.chromium.org/196133032/diff/80001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager_unittest.cc:1722: // Simulate the effect of calling SwapOut in OnCrossSiteResponse. On 2014/03/24 23:24:43, Charlie Reis wrote: > I thought we were testing the case without OnCrossSiteResponse. Won't > DidNavigateFrame call SwapOut for us? Done.
Great. LGTM. (Sorry to miss these nits in the previous patch.) https://codereview.chromium.org/196133032/diff/110001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/196133032/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:218: // Simulate the response comming from the pending renderer. nit: coming https://codereview.chromium.org/196133032/diff/110001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:933: // navigation has already cause the renderer to start swapping out, there nit: caused
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/196133032/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel
https://codereview.chromium.org/196133032/diff/350001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/196133032/diff/350001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:1066: url_chain.push_back(GURL()); I think we can just pass an empty vector, since this isn't a transfer. https://codereview.chromium.org/196133032/diff/350001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:1093: // transition, the running of unload handler, and set bindings on the pending nit: run the unload handler (Each phrase in the list so far starts with a verb.) https://codereview.chromium.org/196133032/diff/350001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/196133032/diff/350001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:188: url_chain.push_back(GURL()); Same here.
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/196133032/390001
Message was sent while issue was closed.
Change committed as 259466 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
