chrome/browser/task_management/providers/web_contents/guest_task.cc - Issue 2086423005: Using WebContents::UpdateTitleForEntry() instead of NavigationEntry::SetTitle()

Unified Diff: chrome/browser/task_management/providers/web_contents/guest_task.cc

Issue 2086423005: Using WebContents::UpdateTitleForEntry() instead of NavigationEntry::SetTitle() (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: DidFinishNavigation is received when RenderFrameHostImpl is destroyed!! Created 4 years, 6 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

« no previous file with comments | « no previous file | chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc » ('j') | chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/browser/task_management/providers/web_contents/guest_task.cc

diff --git a/chrome/browser/task_management/providers/web_contents/guest_task.cc b/chrome/browser/task_management/providers/web_contents/guest_task.cc

index bf092cbbe190a032b4adbc9781aa6b055fc6084c..5155401f948b5f70557658e0d2b9c062670f5b97 100644

--- a/chrome/browser/task_management/providers/web_contents/guest_task.cc

+++ b/chrome/browser/task_management/providers/web_contents/guest_task.cc

@@ -40,7 +40,8 @@ base::string16 GuestTask::GetCurrentTitle(

guest_view::GuestViewBase* guest =

guest_view::GuestViewBase::FromWebContents(web_contents);

- DCHECK(guest);

+ if (!guest)

+ return base::string16();

Charlie Reis 2016/06/24 22:21:17 Just curious, in which case did this fail? Might

afakhry 2016/06/27 14:29:18 This caused crashes in some browsertests when I ch

This caused crashes in some browsertests when I changed the task manager from listening to DidNavigateMainFrame() to listening to DidFinishNavigation() as Nick told me the former will be deprecated. Not related to interstitial titles. Here are the failing tests: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... With the following stack trace: [13384:13384:0623/120511:FATAL:guest_task.cc(43)] Check failed: guest. #0 0x000004c7a1ee base::debug::StackTrace::StackTrace() #1 0x000004c9024a logging::LogMessage::~LogMessage() #2 0x000004e9fac7 task_management::GuestTask::GetCurrentTitle() #3 0x000004e9fb6b task_management::GuestTask::UpdateTitle() #4 0x000004ea2998 task_management::WebContentsEntry::DidFinishNavigation() #5 0x0000025fa347 content::WebContentsImpl::DidFinishNavigation() #6 0x000002364e18 content::NavigationHandleImpl::~NavigationHandleImpl() #7 0x000002365199 content::NavigationHandleImpl::~NavigationHandleImpl() #8 0x00000236ca4a content::RenderFrameHostImpl::~RenderFrameHostImpl() #9 0x00000236ce49 content::RenderFrameHostImpl::~RenderFrameHostImpl() #10 0x00000237d128 content::RenderFrameHostManager::~RenderFrameHostManager() #11 0x000002356556 content::FrameTreeNode::~FrameTreeNode() #12 0x000002356897 content::FrameTreeNode::ResetForNewProcess() #13 0x0000025e895a content::WebContentsImpl::~WebContentsImpl() #14 0x0000025e9739 content::WebContentsImpl::~WebContentsImpl() #15 0x0000036472eb guest_view::GuestViewBase::Destroy() #16 0x0000025e8fa4 content::WebContentsImpl::~WebContentsImpl() #17 0x0000025e9739 content::WebContentsImpl::~WebContentsImpl() #18 0x0000035b03e3 extensions::AppWindowContentsImpl::~AppWindowContentsImpl() #19 0x0000035ad91e extensions::AppWindow::~AppWindow() #20 0x0000035adad9 extensions::AppWindow::~AppWindow() #21 0x0000035ae497 extensions::AppWindow::OnNativeClose() #22 0x0000058aaa66 views::Widget::OnNativeWidgetDestroyed() #23 0x0000058b66cc views::NativeWidgetAura::OnWindowDestroyed() #24 0x0000061f629e aura::Window::~Window() #25 0x0000061f69a9 aura::Window::~Window() #26 0x000004d074a9 base::debug::TaskAnnotator::RunTask() #27 0x000004c97a45 base::MessageLoop::RunTask() #28 0x000004c97d38 base::MessageLoop::DeferOrRunPendingTask() #29 0x000004c9805b base::MessageLoop::DoWork() #30 0x000004c99b79 base::MessagePumpGlib::Run() #31 0x000004c9757d base::MessageLoop::RunHandler() #32 0x000004cba500 base::RunLoop::Run() #33 0x0000075fe99c content::RunThisRunLoop() #34 0x0000075fe93a content::RunMessageLoop() #35 0x0000015a2741 InProcessBrowserTest::QuitBrowsers() #36 0x0000015a24e5 InProcessBrowserTest::RunTestOnMainThreadLoop() #37 0x0000016842f4 content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() #38 0x000004d38da9 ChromeBrowserMainParts::PreMainMessageLoopRunImpl() #39 0x000004d37ea5 ChromeBrowserMainParts::PreMainMessageLoopRun() #40 0x000001759e31 chromeos::ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() #41 0x00000229a3d5 content::BrowserMainLoop::PreMainMessageLoopRun() #42 0x0000025c3517 content::StartupTaskRunner::RunAllTasksNow() #43 0x000002298b94 content::BrowserMainLoop::CreateStartupTasks() #44 0x00000229d182 content::BrowserMainRunnerImpl::Initialize() #45 0x000002296336 content::BrowserMain() #46 0x000004b8149e content::ContentMainRunnerImpl::Run() #47 0x000004b7fe80 content::ContentMain() #48 0x000001683ca8 content::BrowserTestBase::SetUp() #49 0x0000015a09eb InProcessBrowserTest::SetUp() #50 0x0000057c2cc8 testing::Test::Run() #51 0x0000057c3a3a testing::TestInfo::Run() #52 0x0000057c3ec3 testing::TestCase::Run() #53 0x0000057cb0b9 testing::internal::UnitTestImpl::RunAllTests() #54 0x0000057cad5e testing::UnitTest::Run() #55 0x0000015acb66 base::TestSuite::Run() #56 0x000004c6f59b ChromeTestSuiteRunner::RunTestSuite() #57 0x0000075fb7ce content::LaunchTests() #58 0x0000005fedc1 main #59 0x7f780ddf67ed __libc_start_main #60 0x0000005fec81 <unknown> I'm not sure of the meaning of that when it happens, so I don't know what's the right comment to write here. I also changed it from returning an empty string16() to returning the original cached title().

Charlie Reis 2016/06/28 00:47:09 Ack. For posterity (when the link above expires),

On 2016/06/27 14:29:18, afakhry wrote: > On 2016/06/24 22:21:17, Charlie Reis (slow) wrote: > > Just curious, in which case did this fail? Might be nice to have a comment > > about it, unless it's covered by one of your new tests. > > This caused crashes in some browsertests when I changed the task manager from > listening to DidNavigateMainFrame() to listening to DidFinishNavigation() as > Nick told me the former will be deprecated. Not related to interstitial titles. > > Here are the failing tests: > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... > > With the following stack trace: > > [13384:13384:0623/120511:FATAL:guest_task.cc(43)] Check failed: guest. > #0 0x000004c7a1ee base::debug::StackTrace::StackTrace() > #1 0x000004c9024a logging::LogMessage::~LogMessage() > #2 0x000004e9fac7 task_management::GuestTask::GetCurrentTitle() > #3 0x000004e9fb6b task_management::GuestTask::UpdateTitle() > #4 0x000004ea2998 task_management::WebContentsEntry::DidFinishNavigation() > #5 0x0000025fa347 content::WebContentsImpl::DidFinishNavigation() > #6 0x000002364e18 content::NavigationHandleImpl::~NavigationHandleImpl() > #7 0x000002365199 content::NavigationHandleImpl::~NavigationHandleImpl() > #8 0x00000236ca4a content::RenderFrameHostImpl::~RenderFrameHostImpl() > #9 0x00000236ce49 content::RenderFrameHostImpl::~RenderFrameHostImpl() > #10 0x00000237d128 content::RenderFrameHostManager::~RenderFrameHostManager() > #11 0x000002356556 content::FrameTreeNode::~FrameTreeNode() > #12 0x000002356897 content::FrameTreeNode::ResetForNewProcess() > #13 0x0000025e895a content::WebContentsImpl::~WebContentsImpl() > #14 0x0000025e9739 content::WebContentsImpl::~WebContentsImpl() > #15 0x0000036472eb guest_view::GuestViewBase::Destroy() > #16 0x0000025e8fa4 content::WebContentsImpl::~WebContentsImpl() > #17 0x0000025e9739 content::WebContentsImpl::~WebContentsImpl() > #18 0x0000035b03e3 extensions::AppWindowContentsImpl::~AppWindowContentsImpl() > #19 0x0000035ad91e extensions::AppWindow::~AppWindow() > #20 0x0000035adad9 extensions::AppWindow::~AppWindow() > #21 0x0000035ae497 extensions::AppWindow::OnNativeClose() > #22 0x0000058aaa66 views::Widget::OnNativeWidgetDestroyed() > #23 0x0000058b66cc views::NativeWidgetAura::OnWindowDestroyed() > #24 0x0000061f629e aura::Window::~Window() > #25 0x0000061f69a9 aura::Window::~Window() > #26 0x000004d074a9 base::debug::TaskAnnotator::RunTask() > #27 0x000004c97a45 base::MessageLoop::RunTask() > #28 0x000004c97d38 base::MessageLoop::DeferOrRunPendingTask() > #29 0x000004c9805b base::MessageLoop::DoWork() > #30 0x000004c99b79 base::MessagePumpGlib::Run() > #31 0x000004c9757d base::MessageLoop::RunHandler() > #32 0x000004cba500 base::RunLoop::Run() > #33 0x0000075fe99c content::RunThisRunLoop() > #34 0x0000075fe93a content::RunMessageLoop() > #35 0x0000015a2741 InProcessBrowserTest::QuitBrowsers() > #36 0x0000015a24e5 InProcessBrowserTest::RunTestOnMainThreadLoop() > #37 0x0000016842f4 content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() > #38 0x000004d38da9 ChromeBrowserMainParts::PreMainMessageLoopRunImpl() > #39 0x000004d37ea5 ChromeBrowserMainParts::PreMainMessageLoopRun() > #40 0x000001759e31 > chromeos::ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() > #41 0x00000229a3d5 content::BrowserMainLoop::PreMainMessageLoopRun() > #42 0x0000025c3517 content::StartupTaskRunner::RunAllTasksNow() > #43 0x000002298b94 content::BrowserMainLoop::CreateStartupTasks() > #44 0x00000229d182 content::BrowserMainRunnerImpl::Initialize() > #45 0x000002296336 content::BrowserMain() > #46 0x000004b8149e content::ContentMainRunnerImpl::Run() > #47 0x000004b7fe80 content::ContentMain() > #48 0x000001683ca8 content::BrowserTestBase::SetUp() > #49 0x0000015a09eb InProcessBrowserTest::SetUp() > #50 0x0000057c2cc8 testing::Test::Run() > #51 0x0000057c3a3a testing::TestInfo::Run() > #52 0x0000057c3ec3 testing::TestCase::Run() > #53 0x0000057cb0b9 testing::internal::UnitTestImpl::RunAllTests() > #54 0x0000057cad5e testing::UnitTest::Run() > #55 0x0000015acb66 base::TestSuite::Run() > #56 0x000004c6f59b ChromeTestSuiteRunner::RunTestSuite() > #57 0x0000075fb7ce content::LaunchTests() > #58 0x0000005fedc1 main > #59 0x7f780ddf67ed __libc_start_main > #60 0x0000005fec81 <unknown> > > I'm not sure of the meaning of that when it happens, so I don't know what's the > right comment to write here. I also changed it from returning an empty > string16() to returning the original cached title().

Ack. For posterity (when the link above expires), the failing tests were: WebViewTests_WebViewTest.Shim_TestNestedCrossOriginSubframes_0 WebViewTests_WebViewTest.Shim_TestNestedCrossOriginSubframes_1 WebViewTests_WebViewTest.Shim_TestNestedSubframes_0 WebViewTests_WebViewTest.Shim_TestNestedSubframes_1

base::string16 title =

l10n_util::GetStringFUTF16(guest->GetTaskPrefix(),