New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 859422 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug
Flaky-Test: FeedbackPrivateApiTest.Basic



Sign in to add a comment

FeedbackPrivateApiTest.Basic is Flaky

Project Member Reported by Findit, Jul 2

Issue description

Log from a failure:
[ RUN      ] FeedbackPrivateApiTest.Basic
[7273:7284:0701/230103.588769:3926601633:ERROR:network_service.cc(68)] Not implemented reached in std::unique_ptr<net::NetworkChangeNotifier> network::(anonymous namespace)::CreateNetworkChangeNotifierIfNeeded()

DevTools listening on ws://127.0.0.1:44033/devtools/browser/748b35e2-2e5f-47a7-b79e-856755b3f0cf
[7273:7273:0701/230103.616797:3926629675:ERROR:proxy_resolution_service.cc(1528)] ProxyConfigService for ChromeOS should be created in profile_io_data.cc::CreateProxyConfigService and this should be used only for examples.
[7273:7273:0701/230103.666763:3926679626:INFO:CONSOLE(0)] "[SUCCESS] getUserEmailTest", source: chrome-extension://gfdkimpbcpahaombhbimeihdjnejgicl/_generated_background_page.html (0)
[7273:7273:0701/230103.673154:3926686016:INFO:CONSOLE(0)] "[SUCCESS] getSystemInfoTest", source: chrome-extension://gfdkimpbcpahaombhbimeihdjnejgicl/_generated_background_page.html (0)
[7273:7273:0701/230103.675446:3926688307:ERROR:shell_feedback_private_delegate.cc(48)] Not implemented reached in virtual void extensions::ShellFeedbackPrivateDelegate::FetchAndMergeIwlwifiDumpLogsIfPresent(std::unique_ptr<FeedbackCommon::SystemLogsMap>, content::BrowserContext *, system_logs::SysLogsFetcherCallback) const
[7273:7273:0701/230103.676059:3926688921:INFO:CONSOLE(0)] "[SUCCESS] sendFeedbackTest", source: chrome-extension://gfdkimpbcpahaombhbimeihdjnejgicl/_generated_background_page.html (0)
BrowserTestBase received signal: Segmentation fault. Backtrace:
#0 0x000001f09d7c base::debug::StackTrace::StackTrace()
#1 0x00000342c3a5 content::(anonymous namespace)::DumpStackTraceSignalHandler()
#2 0x7ff0a8ec2cb0 <unknown>
#3 0x0000020c2d63 net::NetLog::AddEntry()
#4 0x0000020c3b45 net::NetLogWithSource::AddEntry()
#5 0x0000005702df net::HostResolverImpl::ProcTask::OnLookupAttemptComplete()
#6 0x0000005705da _ZN4base8internal7InvokerINS0_9BindStateIPFvNS_7WeakPtrIN3net16HostResolverImpl8ProcTaskEEERKNS_9TimeTicksEjPKNS_9TickClockENS4_16NetLogWithSourceERKNS4_11AddressListEiiEJS7_S8_jSD_SE_EEEFvSH_iiEE7RunOnceEPNS0_13BindStateBaseESH_ii
#7 0x000000570cf8 _ZN4base8internal7InvokerINS0_9BindStateINS_12OnceCallbackIFvRKN3net11AddressListEiiEEEJS5_iiEEEFvvEE7RunOnceEPNS0_13BindStateBaseE
#8 0x000001ea1ce9 base::debug::TaskAnnotator::RunTask()
#9 0x000001eb1847 base::MessageLoop::RunTask()
#10 0x000001eb1d08 base::MessageLoop::DoWork()
#11 0x000001f18fd9 base::MessagePumpLibevent::Run()
#12 0x000001ec8615 base::RunLoop::Run()
#13 0x000000e74d64 content::BrowserProcessSubThread::IOThreadRun()
#14 0x000001ee94bf base::Thread::ThreadMain()
#15 0x000001f14d8d base::(anonymous namespace)::ThreadFunc()
#16 0x7ff0ac029184 start_thread
#17 0x7ff0a8f8a03d clone
[1/1] FeedbackPrivateApiTest.Basic (CRASHED)
1 test crashed:
    FeedbackPrivateApiTest.Basic (../../extensions/browser/api/feedback_private/feedback_private_apitest.cc:12)
[1/1] FeedbackPrivateApiTest.Basic (191 ms)
SUCCESS: all tests passed.
[1/1] FeedbackPrivateApiTest.Basic (191 ms)

Stacktrace shows method: OnLookupAttemptComplete which was introduced on crrev.com/c/1108078 as pointed by Findit, so I'll try to revert it.
Cc: isherman@chromium.org xunji...@chromium.org
Labels: -Sheriff-Chromium
Owner: ericorth@chromium.org
Cc: -isherman@chromium.org
Investigated for a while.  Findit does seem to show pretty clearly that my CL is the culprit, but how the crash happens is not at all clear.  By the stack traces, the crash seems to be occurring in NetLog::AddEntry calls that seem to be happening pretty much the same before and after the CL.  I don't see anything obviously wrong with any of the code in question.  This should generally be a pretty safe method to call, but it then calls an observer list, so who knows what could be crashing there.

I think I'm going to have to try to get a repro so I can investigate more closely.
Status: Started (was: Available)
Still no luck getting a local repro.

But on a closer examination, I see that I missed one potentially significant difference before and after the CL:
With the culprit CL, we started calling NetLog::AddEntry after all DNS attempts, but before we would skip the call if the request were already cancelled.  There's some room for this to be problematic because the NetLog reference is not refcounted. (Anybody know why not?)

So if we're in a situation where the request was cancelled, or a retry was started but not needed so the retry attempt was effectively cancelled, we could dereference and use the NetLog that might not actually be valid anymore.

I think that explains things nicely, so I'm going to rewrite the culprit CL to ensure the NetLog is no longer dereferenced after call completion and resubmit.
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9a037565fd39121bf9db03691d14e6052573730f

commit 9a037565fd39121bf9db03691d14e6052573730f
Author: Eric Orth <ericorth@chromium.org>
Date: Tue Jul 03 21:24:38 2018

Retry "Cleanup HostResolver timeout/retry tests."

This reverts commit b4426085d22f8ba805af6c3c419bea9a14471562.
Original commit cb8862fc8f300814ebd07982373f5fc1defdb081.

Patchset 1 is a clean revert of the revert with all changes in
subsequent patchsets.

Only change from original is to skip logging attempt completions after
cancellation.  At that point, our NetLog reference should no longer be
trusted.

Bug:  859422 
Change-Id: I39e2dcdf9e415f7f707411773627ab6015e45f57
TBR: isherman@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/1124770
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572358}
[modify] https://crrev.com/9a037565fd39121bf9db03691d14e6052573730f/net/dns/host_resolver_impl.cc
[modify] https://crrev.com/9a037565fd39121bf9db03691d14e6052573730f/net/dns/host_resolver_impl_unittest.cc
[modify] https://crrev.com/9a037565fd39121bf9db03691d14e6052573730f/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Started)
Hopefully no flakes this time.

Sign in to add a comment