SBNavigationObserverBrowserTest download tests are flaky with browser_side_navigation |
|||||||
Issue descriptionThis test periodically fails, like so: [ RUN ] SBNavigationObserverBrowserTest.SubFrameDirectDownload Xlib: extension "RANDR" missing on display ":99". [31654:31654:0407/090220.307144:WARNING:audio_manager.cc(295)] Multiple instances of AudioManager detected [31654:31654:0407/090220.307191:WARNING:audio_manager.cc(254)] Multiple instances of AudioManager detected [31654:31654:0407/090220.431373:WARNING:password_store_factory.cc(249)] Using basic (unencrypted) store for password storage. See https://chromium.googlesource.com/chromium/src/+/master/docs/linux_password_storage.md for more information about password storage options. [31654:31712:0407/090220.892363:WARNING:embedded_test_server.cc(219)] Request not handled. Returning 404: /favicon.ico ../../chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc:228: Failure Value of: download_items.size() Actual: 0 Expected: 1U Which is: 1 BrowserTestBase received signal: Segmentation fault. Backtrace: #0 0x000002904067 base::debug::StackTrace::StackTrace() #1 0x000002e6b546 content::(anonymous namespace)::DumpStackTraceSignalHandler() #2 0x7f050d8f6cb0 <unknown> #3 0x00000135e613 safe_browsing::SBNavigationObserverBrowserTest::GetDownload() #4 0x00000136914d safe_browsing::SBNavigationObserverBrowserTest_SubFrameDirectDownload_Test::RunTestOnMainThread() #5 0x0000029aef9f InProcessBrowserTest::RunTestOnMainThreadLoop() #6 0x000002e6b2a8 content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() #7 0x000002cdbbc4 ChromeBrowserMainParts::PreMainMessageLoopRunImpl() #8 0x000002cdac6b ChromeBrowserMainParts::PreMainMessageLoopRun() #9 0x00000197207f content::BrowserMainLoop::PreMainMessageLoopRun() #10 0x000001c380a7 content::StartupTaskRunner::RunAllTasksNow() #11 0x000001970335 content::BrowserMainLoop::CreateStartupTasks() #12 0x000001974934 content::BrowserMainRunnerImpl::Initialize() #13 0x00000196da92 content::BrowserMain() #14 0x0000028ee44c content::ContentMainRunnerImpl::Run() #15 0x000003ebe76f service_manager::Main() #16 0x0000028ed4c2 content::ContentMain() #17 0x000002e6abe9 content::BrowserTestBase::SetUp() #18 0x0000029ad690 InProcessBrowserTest::SetUp() #19 0x00000347144e testing::Test::Run() #20 0x000003471fc0 testing::TestInfo::Run() #21 0x0000034724e7 testing::TestCase::Run() #22 0x000003479527 testing::internal::UnitTestImpl::RunAllTests() #23 0x0000034791a7 testing::UnitTest::Run() #24 0x0000029be881 base::TestSuite::Run() #25 0x0000028f8c48 ChromeTestSuiteRunner::RunTestSuite() #26 0x000002e8edba content::LaunchTests() #27 0x0000028f8bc7 main #28 0x7f050d8e1f45 __libc_start_main #29 0x0000005fe80d <unknown> For example, see: https://uberchromegw.corp.google.com/i/chromium.linux/builders/Linux%20Tests/builds/54497 mattm@, can you take a peek at this?
,
Apr 7 2017
I'll take a look ASAP. Thanks!
,
Apr 25 2017
Issue 715076 has been merged into this issue.
,
Apr 25 2017
Looks like a similar flake is happening with DirectDownloadNoReferrerTargetBlank, which I have made worse with a recent CL. Merging the two issues together for now. I think this might have something to do with PlzNavigate + Downloads complexity.
,
Apr 25 2017
Issue 715019 has been merged into this issue.
,
Apr 25 2017
Yes we have some issues with streams & downloads. In particular, NavigationResourceHandler should pause the navigation on WillProcessResponse while Navigationtrhottles execute on the UI thread, but it doesn't because InterceptResourceHandler doesn't expect it. I'm working on a patch to fix that. I suspect this is the root cause behind issue 709771 (which also interacts with SafeBrowsing in some ways).
,
Apr 25 2017
Thanks clamy that makes sense, though I'm not quite convinced that the production code is at issue here. I wonder if this is just a problem with downloads not behaving properly with the TestNavigationObserver class? Given that the test is flaking it makes me feel like there is a timing issue with the waiting mechanism.
,
Apr 26 2017
https://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x64_rel_ng/builds/852 had a SBNavigationObserverBrowserTest.DirectDownloadNoReferrerTargetBlank failure on linux -- comment 4 sounds like this is the bug for that too? If so, bug should be retitled.
,
Apr 26 2017
,
Apr 26 2017
clamy: would you mind owning this issue?
,
Apr 26 2017
Retitling
,
Apr 26 2017
I have https://codereview.chromium.org/2847443002/ which should fix the race conditions issues. Could you check if it works?
,
Apr 26 2017
Change owner to clamy@. Please feel free to change the component label. Thanks for addressing this issue!
,
Apr 26 2017
clamy: I haven't tried to reproduce this locally but I'll try now (with + without your patch)
,
Apr 26 2017
Sorry to say but I have confirmed that the CL in comment 12 does not de-flake these tests :( We still have 0 download items when the navigation is complete.
,
Apr 26 2017
Looks like we post a task to start the download on the UI thread in DownloadResourceHandler::OnResponseStarted but that task is not serviced by the time the run loop quits using the test navigation observer. My gut instinct is that this isn't really PlzNavigate's fault and this test harness should just explicitly wait for the download or pump the task queue before trying to check the download manager.
,
Apr 26 2017
Tentative CL: https://codereview.chromium.org/2845593003
,
Apr 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8865b3ae437986ee8449db0facfd9c1f28d668d5 commit 8865b3ae437986ee8449db0facfd9c1f28d668d5 Author: csharrison <csharrison@chromium.org> Date: Wed Apr 26 19:35:33 2017 Properly wait for download items in SBNavigationObserverBrowserTests This fixes flakiness with PlzNavigate completing navigations before the download item reaches the UI thread. BUG= 709508 Review-Url: https://codereview.chromium.org/2845593003 Cr-Commit-Position: refs/heads/master@{#467413} [modify] https://crrev.com/8865b3ae437986ee8449db0facfd9c1f28d668d5/chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc
,
Apr 26 2017
The CL that landed in #18 should fix things, let's keep an eye on builds to make sure.
,
Jun 5 2017
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by mattm@chromium.org
, Apr 7 2017Owner: jialiul@chromium.org