Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1408743005:
Properly recreate swapped out RenderView. (Closed)
|
Created:
5 years, 2 months ago by nasko Modified:
5 years, 2 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. |
DescriptionProperly recreate swapped out RenderView.
This CL fixes how RenderView is recreated after a process crash. Due to not recreating the RenderFrameProxy in the case the RenderView is swapped out, the WebView ends up with no mainFrame() and crashes in various ways.
BUG=357747, 544271
Committed: https://crrev.com/ae13c59638df3adf7c9a206f82680faf86d52310
Cr-Commit-Position: refs/heads/master@{#354658}
Patch Set 1 #Patch Set 2 : Rebase. #Patch Set 3 : Clear main RFH on cancelling the pending RFH. #
Total comments: 14
Patch Set 4 : A different approach. #
Total comments: 9
Patch Set 5 : Fixes based on Charlie's review. #
Total comments: 1
Patch Set 6 : Fix formatting. #
Messages
Total messages: 15 (4 generated)
creis@chromium.org changed reviewers: + creis@chromium.org
Not sure I totally understand this yet. Could you post a bit of an explanation of the bug to 543222? https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:918: // be cleared, as it will be deleted at the end of this method. s/, it must be cleared/, then the RenderViewHost must forget about it/ https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1842: if (!render_view_host->GetMainFrame()) { What would happen if it had a main frame routing ID for a different frame, or a non-existing frame? https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2166: // Tests navigating cross-process and reusing an existing RenderViewHost, nit: Tests that https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2167: // which process has been killed/crashed, doesn't result in creating nit: Put the killed/crash phrase in parentheses, not commas. nit: s/which/whose/ https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2168: // a RenderView with no valid RenderFrame or RenderFrameProxy. nit: Mention https://crbug.com/543222 here. https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2191: shell()->web_contents()->GetSiteInstance())); I'm not sure I understand the bug, but it seems like the part we care about is that shell() has a swapped out RVH for b.com (rather than the RVH for a.com in the popup). According to the comments, that's the one that causes the crash, right? https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2194: RenderProcessHost* process = popup_root->current_frame_host()->GetProcess(); Wait, are the comments wrong? This is killing the popup's process, not the main tab. https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2204: // RenderView for b.com in the main tab to be created. Can you add some explanation of what this is verifying? Maybe mention that this used to crash because the RenderView was created without a frame or a proxy, if that's what was happening. Also, does this actually fail without your fix? I know a lot of tests pass even if the renderer crashes. If it does fail, mention how.
I've rewritten the fix and improved the test readability. The test itself should be good to review, as it is good at reproing the problem. However, the fix is failing some tests in --site-per-process, so no need for review until I find what it is and fix it. https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2166: // Tests navigating cross-process and reusing an existing RenderViewHost, On 2015/10/16 17:02:23, Charlie Reis wrote: > nit: Tests that Done. https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2167: // which process has been killed/crashed, doesn't result in creating On 2015/10/16 17:02:23, Charlie Reis wrote: > nit: Put the killed/crash phrase in parentheses, not commas. > nit: s/which/whose/ Done. https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2168: // a RenderView with no valid RenderFrame or RenderFrameProxy. On 2015/10/16 17:02:24, Charlie Reis wrote: > nit: Mention https://crbug.com/543222 here. I ended up filing a master bug (https://crbug.com/544271) and explains the root cause of the bug much better than the actual crashes. https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2191: shell()->web_contents()->GetSiteInstance())); On 2015/10/16 17:02:24, Charlie Reis wrote: > I'm not sure I understand the bug, but it seems like the part we care about is > that shell() has a swapped out RVH for b.com (rather than the RVH for a.com in > the popup). According to the comments, that's the one that causes the crash, > right? Yes, you are correct. This RVH was a copy/paste from another test. Removed. https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2194: RenderProcessHost* process = popup_root->current_frame_host()->GetProcess(); On 2015/10/16 17:02:23, Charlie Reis wrote: > Wait, are the comments wrong? This is killing the popup's process, not the main > tab. Yes, fixed. https://codereview.chromium.org/1408743005/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2204: // RenderView for b.com in the main tab to be created. On 2015/10/16 17:02:23, Charlie Reis wrote: > Can you add some explanation of what this is verifying? Maybe mention that this > used to crash because the RenderView was created without a frame or a proxy, if > that's what was happening. > > Also, does this actually fail without your fix? I know a lot of tests pass even > if the renderer crashes. If it does fail, mention how. The process crashes, so we don't complete the navigation, therefore NavigateToURL fails. I'll double check again.
Ok, I understand now that the tests are actually passing, I forgot to look at the exclusion filter for --site-per-process. Ready to review : ).
creis@chromium.org changed reviewers: + alexmos@chromium.org
LGTM with some nits and a fix for the test. Might want Alex to confirm that the tweak I suggested for InitRenderView is correct. https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (left): https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1830: proxy_routing_id = proxy->GetRoutingID(); For posterity: this is the key part of the fix. We had a proxy but we're no longer swapped_out, so we weren't passing the proxy_routing_id. We need to create both the RenderView and RenderFrameProxy when recreating the process, since the swap hasn't happened yet. https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2007: DCHECK_EQ(render_view_host->is_active(), !proxy); These are no longer equal, as your test shows. Suppose that b.com were a WebUI page. I think we would want to grant the bindings based on whether the RVH is active (and thus for the main frame), regardless of whether there's a leftover proxy around from before. You can see my discussion with Alex here: https://codereview.chromium.org/1403343002/ https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:673: // RenderFrameProxy in the target renderer process with the given valid This comment makes it sound like the proxy is required. Can you clarify when it should be real vs null? https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2167: // (which process has been killed/crashed) recreates properly the RenderView and nit: s/which/whose/
Fixed! https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2007: DCHECK_EQ(render_view_host->is_active(), !proxy); On 2015/10/16 23:09:57, Charlie Reis wrote: > These are no longer equal, as your test shows. > > Suppose that b.com were a WebUI page. I think we would want to grant the > bindings based on whether the RVH is active (and thus for the main frame), > regardless of whether there's a leftover proxy around from before. > > You can see my discussion with Alex here: > https://codereview.chromium.org/1403343002/ I think this is correct. I've removed the DCHECK. https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2036: if (proxy) I also noticed that the bool is set unconditionally, which seems wrong. It used to be the case before, so we probably had a bug even before I moved the code here. Now it is set only if the creation above succeeded. https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.h:673: // RenderFrameProxy in the target renderer process with the given valid On 2015/10/16 23:09:57, Charlie Reis wrote: > This comment makes it sound like the proxy is required. Can you clarify when it > should be real vs null? Done. https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2167: // (which process has been killed/crashed) recreates properly the RenderView and On 2015/10/16 23:09:57, Charlie Reis wrote: > nit: s/which/whose/ Done.
LGTM https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1408743005/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2036: if (proxy) On 2015/10/16 23:42:09, nasko (slow to review) wrote: > I also noticed that the bool is set unconditionally, which seems wrong. It used > to be the case before, so we probably had a bug even before I moved the code > here. > > Now it is set only if the creation above succeeded. Acknowledged. https://codereview.chromium.org/1408743005/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/1408743005/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2188: embedded_test_server()->GetURL("b.com", "/title2.html"))); nit: indent seems off
Fixed formatting. Was due to adding EXPECT_TRUE after running git cl format.
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/1408743005/#ps100001 (title: "Fix formatting.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1408743005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1408743005/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ae13c59638df3adf7c9a206f82680faf86d52310 Cr-Commit-Position: refs/heads/master@{#354658}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1412333002/ by reillyg@chromium.org. The reason for reverting is: This appears to be causing a set of two memory leaks on the Webkit Linux Valgrind bots: 22,008 (48 direct, 21,960 indirect) bytes in 1 blocks are definitely lost in loss record 2,955 of 2,972 operator new(unsigned long) (m_replacemalloc/vg_replace_malloc.c:1140) content::RenderProcessHostImpl::Init() (content/browser/renderer_host/render_process_host_impl.cc:710) content::RenderFrameHostManager::InitRenderView(content::RenderViewHostImpl*, content::RenderFrameProxyHost*) (content/browser/frame_host/render_frame_host_manager.cc:1995) content::RenderFrameHostManager::Navigate(GURL const&, content::FrameNavigationEntry const&, content::NavigationEntryImpl const&) (content/browser/frame_host/render_frame_host_manager.cc:421) content::NavigatorImpl::NavigateToEntry(content::FrameTreeNode*, content::FrameNavigationEntry const&, content::NavigationEntryImpl const&, content::NavigationController::ReloadType, bool) (content/browser/frame_host/navigator_impl.cc:312) content::NavigatorImpl::NavigateToPendingEntry(content::FrameTreeNode*, content::FrameNavigationEntry const&, content::NavigationController::ReloadType, bool) (/b/build/slave/webkit-rel-linux-valgrind-layout/build/src/out/Release/content_shell) content::NavigationControllerImpl::NavigateToPendingEntryInternal(content::NavigationController::ReloadType) (content/browser/frame_host/navigation_controller_impl.cc:1785) content::NavigationControllerImpl::NavigateToPendingEntry(content::NavigationController::ReloadType) (content/browser/frame_host/navigation_controller_impl.cc:1762) content::NavigationControllerImpl::LoadEntry(scoped_ptr<content::NavigationEntryImpl, base::DefaultDeleter<content::NavigationEntryImpl> >) (content/browser/frame_host/navigation_controller_impl.cc:430) content::NavigationControllerImpl::LoadURLWithParams(content::NavigationController::LoadURLParams const&) (content/browser/frame_host/navigation_controller_impl.cc:795) content::Shell::LoadURLForFrame(GURL const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (content/shell/browser/shell.cc:190) content::Shell::LoadURL(GURL const&) (content/shell/browser/shell.cc:182) content::BlinkTestController::PrepareForLayoutTest(GURL const&, base::FilePath const&, bool, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (content/shell/browser/blink_test_controller.cc:274) (anonymous namespace)::RunOneTest(test_runner::TestInfo const&, bool*, scoped_ptr<content::BrowserMainRunner, base::DefaultDeleter<content::BrowserMainRunner> > const&) (content/shell/browser/layout_test/layout_test_browser_main.cc:40) (anonymous namespace)::RunTests(scoped_ptr<content::BrowserMainRunner, base::DefaultDeleter<content::BrowserMainRunner> > const&) (content/shell/browser/layout_test/layout_test_browser_main.cc:87) LayoutTestBrowserMain(content::MainFunctionParams const&, scoped_ptr<content::BrowserMainRunner, base::DefaultDeleter<content::BrowserMainRunner> > const&) (content/shell/browser/layout_test/layout_test_browser_main.cc:141) content::ShellMainDelegate::RunProcess(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, content::MainFunctionParams const&) (content/shell/app/shell_main_delegate.cc:271) content::RunNamedProcessTypeMain(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) (content/app/content_main_runner.cc:365) content::ContentMainRunnerImpl::Run() (content/app/content_main_runner.cc:798) content::ContentMain(content::ContentMainParams const&) (content/app/content_main.cc:19) 48 (40 direct, 8 indirect) bytes in 1 blocks are definitely lost in loss record 1,603 of 2,979 operator new(unsigned long) (m_replacemalloc/vg_replace_malloc.c:1140) content::PushMessagingMessageFilter::PushMessagingMessageFilter(int, content::ServiceWorkerContextWrapper*) (content/browser/push_messaging/push_messaging_message_filter.cc:213) content::RenderProcessHostImpl::CreateMessageFilters() (content/browser/renderer_host/render_process_host_impl.cc:939) content::RenderProcessHostImpl::Init() (content/browser/renderer_host/render_process_host_impl.cc:661) content::RenderFrameHostManager::InitRenderView(content::RenderViewHostImpl*, content::RenderFrameProxyHost*) (content/browser/frame_host/render_frame_host_manager.cc:1995) content::RenderFrameHostManager::Navigate(GURL const&, content::FrameNavigationEntry const&, content::NavigationEntryImpl const&) (content/browser/frame_host/render_frame_host_manager.cc:421) content::NavigatorImpl::NavigateToEntry(content::FrameTreeNode*, content::FrameNavigationEntry const&, content::NavigationEntryImpl const&, content::NavigationController::ReloadType, bool) (content/browser/frame_host/navigator_impl.cc:312) content::NavigatorImpl::NavigateToPendingEntry(content::FrameTreeNode*, content::FrameNavigationEntry const&, content::NavigationController::ReloadType, bool) (/b/build/slave/webkit-rel-linux-valgrind-layout/build/src/out/Release/content_shell) content::NavigationControllerImpl::NavigateToPendingEntryInternal(content::NavigationController::ReloadType) (content/browser/frame_host/navigation_controller_impl.cc:1785) content::NavigationControllerImpl::NavigateToPendingEntry(content::NavigationController::ReloadType) (content/browser/frame_host/navigation_controller_impl.cc:1762) content::NavigationControllerImpl::LoadEntry(scoped_ptr<content::NavigationEntryImpl, base::DefaultDeleter<content::NavigationEntryImpl> >) (content/browser/frame_host/navigation_controller_impl.cc:430) content::NavigationControllerImpl::LoadURLWithParams(content::NavigationController::LoadURLParams const&) (content/browser/frame_host/navigation_controller_impl.cc:795) content::Shell::LoadURLForFrame(GURL const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (content/shell/browser/shell.cc:190) content::Shell::LoadURL(GURL const&) (content/shell/browser/shell.cc:182) content::BlinkTestController::PrepareForLayoutTest(GURL const&, base::FilePath const&, bool, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (content/shell/browser/blink_test_controller.cc:274) (anonymous namespace)::RunOneTest(test_runner::TestInfo const&, bool*, scoped_ptr<content::BrowserMainRunner, base::DefaultDeleter<content::BrowserMainRunner> > const&) (content/shell/browser/layout_test/layout_test_browser_main.cc:40) (anonymous namespace)::RunTests(scoped_ptr<content::BrowserMainRunner, base::DefaultDeleter<content::BrowserMainRunner> > const&) (content/shell/browser/layout_test/layout_test_browser_main.cc:87) LayoutTestBrowserMain(content::MainFunctionParams const&, scoped_ptr<content::BrowserMainRunner, base::DefaultDeleter<content::BrowserMainRunner> > const&) (content/shell/browser/layout_test/layout_test_browser_main.cc:141) content::ShellMainDelegate::RunProcess(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, content::MainFunctionParams const&) (content/shell/app/shell_main_delegate.cc:271) content::RunNamedProcessTypeMain(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) (content/app/content_main_runner.cc:365) content::ContentMainRunnerImpl::Run() (content/app/content_main_runner.cc:798) content::ContentMain(content::ContentMainParams const&) (content/app/content_main.cc:19). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
