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

Unified Diff: content/browser/site_per_process_browsertest.cc

Issue 1886413002: Always swap with a replacement proxy in OnSwapOut. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Charlie's comments Created 4 years, 8 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: content/browser/site_per_process_browsertest.cc
diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc
index 80c4d169bf440321d0aa58edef178fe177dd45d4..fef1749b8dd88ddb52dd2d8276430538a41be3de 100644
--- a/content/browser/site_per_process_browsertest.cc
+++ b/content/browser/site_per_process_browsertest.cc
@@ -31,8 +31,10 @@
#include "content/browser/renderer_host/render_view_host_impl.h"
#include "content/browser/renderer_host/render_widget_host_input_event_router.h"
#include "content/browser/renderer_host/render_widget_host_view_aura.h"
+#include "content/common/child_process_messages.h"
#include "content/common/frame_messages.h"
#include "content/common/input/synthetic_tap_gesture_params.h"
+#include "content/common/input_messages.h"
#include "content/common/view_messages.h"
#include "content/public/browser/cert_store.h"
#include "content/public/browser/notification_observer.h"
@@ -4121,11 +4123,12 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
}
// Test for https://crbug.com/515302. Perform two navigations, A->B->A, and
-// delay the SwapOut ACK from the A->B navigation, so that the second B->A
+// drop the SwapOut ACK from the A->B navigation, so that the second B->A
// navigation is initiated before the first page receives the SwapOut ACK.
-// Ensure that the RVH(A) that's pending deletion is not reused in that case.
+// Ensure that this doesn't crash and that the RVH(A) is not reused in that
+// case.
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
- RenderViewHostPendingDeletionIsNotReused) {
+ RenderViewHostIsNotReusedAfterDelayedSwapOutACK) {
GURL a_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
NavigateToURL(shell(), a_url);
@@ -4142,9 +4145,9 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
rfh->GetProcess()->AddFilter(filter.get());
rfh->DisableSwapOutTimerForTesting();
- // Navigate to B. This must wait for DidCommitProvisionalLoad, as opposed to
- // DidStopLoading, since otherwise the SwapOut timer might call OnSwappedOut
- // and destroy |rvh| before its pending deletion status is checked.
+ // Navigate to B. This must wait for DidCommitProvisionalLoad and not
+ // DidStopLoading, so that the SwapOut timer doesn't call OnSwappedOut and
+ // destroy |rfh| and |rvh| before they are checked in the test.
GURL b_url(embedded_test_server()->GetURL("b.com", "/title2.html"));
TestFrameNavigationObserver commit_observer(root);
shell()->LoadURL(b_url);
@@ -4152,18 +4155,28 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
EXPECT_FALSE(deleted_observer.deleted());
// Since the SwapOut ACK for A->B is dropped, the first page's
- // RenderFrameHost and RenderViewHost should be pending deletion after the
- // last navigation.
+ // RenderFrameHost should be pending deletion after the last navigation.
EXPECT_FALSE(rfh->is_active());
- EXPECT_TRUE(root->render_manager()->IsViewPendingDeletion(rvh));
// Wait for process A to exit so we can reinitialize it cleanly for the next
- // navigation. This can be removed once https://crbug.com/535246 is fixed.
+ // navigation. Since process A doesn't have any active views, it will
+ // initiate shutdown via ChildProcessHostMsg_ShutdownRequest. After process
+ // A shuts down, the |rfh| and |rvh| should get destroyed via
+ // OnRenderProcessGone.
+ //
+ // Not waiting for process shutdown here could lead to the |rvh| being
+ // reused, now that there is no notion of pending deletion RenderViewHosts.
+ // This would also be fine; however, the race in https://crbug.com/535246
+ // still needs to be addressed and tested in that case.
RenderProcessHostWatcher process_exit_observer(
rvh->GetProcess(),
RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
process_exit_observer.Wait();
+ // Verify that the RVH and RFH for A were cleaned up.
+ EXPECT_FALSE(root->frame_tree()->GetRenderViewHost(site_instance));
+ EXPECT_TRUE(deleted_observer.deleted());
+
// Start a navigation back to A and check that the RenderViewHost wasn't
// reused.
TestNavigationObserver navigation_observer(shell()->web_contents());
@@ -4175,21 +4188,15 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
EXPECT_EQ(site_instance, pending_rvh->GetSiteInstance());
EXPECT_NE(rvh_routing_id, pending_rvh->GetRoutingID());
- // TODO(alexmos, creis): Once https://crbug.com/535246 is fixed and the
- // process_exit_observer is not needed above, we'll need to simulate that the
- // dropped SwapOut ACK message arrives now on the original RenderFrameHost,
- // causing it to be deleted.
- EXPECT_TRUE(deleted_observer.deleted());
-
// Make sure the last navigation finishes without crashing.
navigation_observer.Wait();
}
// Test for https://crbug.com/591478, where navigating to a cross-site page with
-// a subframe on the old site could cause the old RenderViewHost (now pending
-// deletion) to be reused.
+// a subframe on the old site caused a crash while trying to reuse the old
+// RenderViewHost.
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
- DontReusePendingDeleteRenderViewHostForSubframe) {
+ ReusePendingDeleteRenderViewHostForSubframe) {
GURL main_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
EXPECT_TRUE(NavigateToURL(shell(), main_url));
@@ -4206,8 +4213,12 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
EXPECT_TRUE(NavigateToURL(shell(), second_url));
// If the subframe is created while the main frame is pending deletion, then
- // the RVH will be different.
- // TODO(creis, alexmos): Find a way to assert this that isn't flaky. For now,
+ // the RVH will be reused. The main frame should've been swapped with a
+ // proxy despite being the last active frame in the progress (see
+ // https://crbug.com/568836), and this proxy should also be reused by the new
+ // page.
+ //
+ // TODO(creis, alexmos): Find a way to assert this that isn't flaky. For now,
// the test is just likely (not certain) to catch regressions by crashing.
}
@@ -6093,4 +6104,90 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
EXPECT_FALSE(rvh->is_swapped_out_);
}
+// Helper class to wait for a ChildProcessHostMsg_ShutdownRequest message to
+// arrive.
+class ShutdownRequestMessageFilter : public BrowserMessageFilter {
+ public:
+ ShutdownRequestMessageFilter()
+ : BrowserMessageFilter(ChildProcessMsgStart),
+ message_loop_runner_(new MessageLoopRunner) {}
+
+ bool OnMessageReceived(const IPC::Message& message) override {
+ if (message.type() == ChildProcessHostMsg_ShutdownRequest::ID) {
+ content::BrowserThread::PostTask(
+ content::BrowserThread::UI, FROM_HERE,
+ base::Bind(&ShutdownRequestMessageFilter::OnShutdownRequest, this));
+ }
+ return false;
+ }
+
+ void OnShutdownRequest() { message_loop_runner_->Quit(); }
+
+ void Wait() { message_loop_runner_->Run(); }
+
+ private:
+ ~ShutdownRequestMessageFilter() override {}
+
+ scoped_refptr<MessageLoopRunner> message_loop_runner_;
+
+ DISALLOW_COPY_AND_ASSIGN(ShutdownRequestMessageFilter);
+};
+
+// Test for https://crbug.com/568836. From an A-embed-B page, navigate the
+// subframe from B to A. This cleans up the process for B, but the test delays
+// the browser side from killing the B process right away. This allows the
+// B process to process two ViewMsg_Close messages sent to the subframe's
+// RenderWidget and to the RenderView, in that order. In the bug, the latter
+// crashed while detaching the subframe's LocalFrame (triggered as part of
+// closing the RenderView), because this tried to access the subframe's
+// WebFrameWidget (from RenderFrameImpl::didChangeSelection), which had already
+// been cleared by the former.
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
+ CloseSubframeWidgetAndViewOnProcessExit) {
+ GURL main_url(embedded_test_server()->GetURL(
+ "a.com", "/cross_site_iframe_factory.html?a(b)"));
+ EXPECT_TRUE(NavigateToURL(shell(), main_url));
+
+ FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+ ->GetFrameTree()
+ ->root();
+
+ // "Select all" in the subframe. The bug only happens if there's a selection
+ // change, which triggers the path through didChangeSelection.
+ root->child_at(0)->current_frame_host()->Send(new InputMsg_SelectAll(
+ root->child_at(0)->current_frame_host()->GetRoutingID()));
+
+ // Prevent b.com process from terminating right away once the subframe
+ // navigates away from b.com below. This is necessary so that the renderer
+ // process has time to process the closings of RenderWidget and RenderView,
+ // which is where the original bug was triggered. Incrementing worker
+ // RefCount will cause RenderProcessHostImpl::Cleanup to forego process
+ // termination.
+ RenderProcessHost* subframe_process =
+ root->child_at(0)->current_frame_host()->GetProcess();
+ subframe_process->IncrementWorkerRefCount();
+
+ // Navigate the subframe away from b.com. Since this is the last active
+ // frame in the b.com process, this causes the RenderWidget and RenderView to
+ // be closed. If this succeeds without crashing, the renderer will release
+ // the process and send a ChildProcessHostMsg_ShutdownRequest to the browser
+ // process to ask whether it's ok to terminate. Thus, wait for this message
+ // to ensure that the RenderView and widget were closed without crashing.
+ scoped_refptr<ShutdownRequestMessageFilter> filter =
+ new ShutdownRequestMessageFilter();
+ subframe_process->AddFilter(filter.get());
+ NavigateFrameToURL(root->child_at(0),
+ embedded_test_server()->GetURL("a.com", "/title1.html"));
+ filter->Wait();
+
+ // TODO(alexmos): Navigating the subframe back to b.com at this point would
+ // trigger the race in https://crbug.com/535246, where the browser process
+ // tries to reuse the b.com process thinking it's still initialized, whereas
+ // the process has actually been destroyed by the renderer (but the browser
+ // process hasn't heard the OnChannelError yet). This race will need to be
+ // fixed.
+
+ subframe_process->DecrementWorkerRefCount();
+}
+
} // namespace content

Powered by Google App Engine
This is Rietveld 408576698