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

Issue 822975 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug



Sign in to add a comment

"PolicyPrefsTest.PolicyToPrefsMapping" is flaky

Project Member Reported by chromium...@appspot.gserviceaccount.com, Mar 16 2018

Issue description

"PolicyPrefsTest.PolicyToPrefsMapping" is flaky.

This issue was created automatically by the chromium-try-flakes app. Please find the right owner to fix the respective test/step and assign this issue to them. If the step/test is infrastructure-related, please add Infra-Troopers label and change issue status to Untriaged. When done, please remove the issue from Sheriff Bug Queue by removing the Sheriff-Chromium label.

We have detected 3 recent flakes. List of all flakes can be found at https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyLwsSBUZsYWtlIiRQb2xpY3lQcmVmc1Rlc3QuUG9saWN5VG9QcmVmc01hcHBpbmcM.

Flaky tests should be disabled within 30 minutes unless culprit CL is found and reverted. Please see more details here: https://sites.google.com/a/chromium.org/dev/developers/tree-sheriffs/sheriffing-bug-queues#triaging-auto-filed-flakiness-bugs
 

Comment 1 by dvadym@chromium.org, Mar 19 2018

The CL for disabling this test is in commit queue
https://chromium-review.googlesource.com/c/chromium/src/+/968507

Comment 2 by dvadym@chromium.org, Mar 19 2018

Cc: pastarmovj@chromium.org emaxx@chromium.org
Labels: -Sheriff-Chromium OS-Windows
Owner: rhalavati@chromium.org
Status: Assigned (was: Untriaged)
It looks that the problem is in DCHECK in network annotation. Error message

5676:6396:0316/032955.663:FATAL:network_traffic_annotation.h(226)] Check failed: is_valid().
Backtrace:
	base::debug::StackTrace::StackTrace [0x00007FF63E2BA6E4+36]
	logging::LogMessage::~LogMessage [0x00007FF63E211A93+99]
	net::MutableNetworkTrafficAnnotationTag::operator net::NetworkTrafficAnnotationTag [0x00007FF63C5468C2+98]
	net::InitSocketHandleForHttpRequest [0x00007FF63E5FE657+2167]
	net::InitSocketHandleForHttpRequest [0x00007FF63E5FDF6C+396]
	net::HttpStreamFactoryImpl::Job::DoInitConnectionImpl [0x00007FF63E5F6405+2885]
	net::HttpStreamFactoryImpl::Job::DoInitConnection [0x00007FF63E5F4752+34]
	net::HttpStreamFactoryImpl::Job::DoLoop [0x00007FF63E5F4060+640]
	net::HttpStreamFactoryImpl::Job::RunLoop [0x00007FF63E5F1EC0+96]
	net::HttpStreamFactoryImpl::Job::StartInternal [0x00007FF63E5F1C15+133]
	net::HttpStreamFactoryImpl::JobController::DoCreateJobs [0x00007FF63E5FC6A4+2932]
	net::HttpStreamFactoryImpl::JobController::DoLoop [0x00007FF63E5FB6CB+347]
	net::HttpStreamFactoryImpl::JobController::RunLoop [0x00007FF63E5F83B8+40]
	net::ProxyResolutionService::Request::QueryComplete [0x00007FF63E49B0D4+84]
	net::ProxyResolutionService::SetReady [0x00007FF63E49873B+235]
	net::ProxyResolutionService::InitializeUsingLastFetchedConfig [0x00007FF63E49899E+446]
	net::ProxyResolutionService::OnProxyConfigChanged [0x00007FF63E49A2F9+409]
	network::ProxyConfigServiceMojo::OnProxyConfigUpdated [0x00007FF63F744D4A+186]
	network::mojom::ProxyConfigClientStubDispatch::Accept [0x00007FF63C54E852+402]
	mojo::InterfaceEndpointClient::HandleValidatedMessage [0x00007FF63EDFC3C0+656]
	mojo::FilterChain::Accept [0x00007FF63EE018F7+151]
	mojo::InterfaceEndpointClient::HandleIncomingMessage [0x00007FF63EDFD37A+122]
	mojo::internal::MultiplexRouter::ProcessIncomingMessage [0x00007FF63EDF5649+729]
	mojo::internal::MultiplexRouter::Accept [0x00007FF63EDF515C+348]
	mojo::FilterChain::Accept [0x00007FF63EE018F7+151]
	mojo::Connector::ReadSingleMessage [0x00007FF63EDF9B48+440]
	mojo::Connector::ReadAllAvailableMessages [0x00007FF63EDFA390+128]
	mojo::Connector::OnHandleReadyInternal [0x00007FF63EDFA1FB+139]
	mojo::SimpleWatcher::OnHandleReady [0x00007FF63ED4A522+258]
	base::debug::TaskAnnotator::RunTask [0x00007FF63E2B6788+296]
	base::internal::IncomingTaskQueue::RunTask [0x00007FF63E2DDF4E+126]
	base::MessageLoop::RunTask [0x00007FF63E24E079+681]
	base::MessageLoop::DeferOrRunPendingTask [0x00007FF63E24E4C7+183]
	base::MessageLoop::DoWork [0x00007FF63E24E70E+542]
	base::MessagePumpForIO::DoRunLoop [0x00007FF63E294215+165]
	base::MessagePumpWin::Run [0x00007FF63E2932F8+104]
	base::MessageLoop::Run [0x00007FF63E24D8D9+201]
	base::RunLoop::Run [0x00007FF63E22296C+252]
	base::Thread::Run [0x00007FF63E269B98+200]
	content::BrowserThreadImpl::IOThreadRun [0x00007FF63CD23477+39]
	content::BrowserThreadImpl::Run [0x00007FF63CD2361E+302]
	base::Thread::ThreadMain [0x00007FF63E26A08A+730]
	base::PlatformThread::GetCurrentThreadPriority [0x00007FF63E24201C+572]
	BaseThreadInitThunk [0x00007FFAC5E18364+20]
	RtlUserThreadStart [0x00007FFAC83B5E91+33]


Project Member

Comment 3 by bugdroid1@chromium.org, Mar 19 2018

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

commit 788a292e3ec841a0ab78a2186af38cdc980ced8a
Author: Vadym Doroshenko <dvadym@chromium.org>
Date: Mon Mar 19 14:20:20 2018

Disabling flaky test PolicyPrefsTest.PolicyToPrefsMapping on Windows.

TBR=emaxx@chromium.org
TBR=pastarmovj@chromium.org

Bug:  822975 
Change-Id: I03062d8eb1c8c3221cf07eb08587e4d9c97ed5e2
Reviewed-on: https://chromium-review.googlesource.com/968507
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544023}
[modify] https://crrev.com/788a292e3ec841a0ab78a2186af38cdc980ced8a/chrome/browser/policy/policy_prefs_browsertest.cc

Hi Ramin,

can you please prioritize taking a stab at that one? This test is a critical piece of our policy verification infrastructure. It is leaving a big gap on Windows testing currently by not having this test.

Comment 5 by emaxx@chromium.org, Mar 28 2018

Cc: rhalavati@chromium.org
Owner: msramek@chromium.org
msramek@: Could you please take a look? Ramin is OOO for the next few days.
I'd be nice to fix "PolicyPrefsTest.PolicyToPrefsMapping" ASAP, because, as noted in comment 4, it verifies one of the core pieces in the admin policy subsystem.
Do you think that commit e91bd630d3038038491462bb28f63d701850c430 can be related?
It doesn't look related. It basically removes a declaration that is not used anywhere anymore.

The stack trace shows "MutableNetworkTrafficAnnotationTag". Most network traffic annotations are immutable, but there are a few instances that had to be mutable for one reason or another.

The DCHECK(is_valid()) means that there's an annotation with the value TRAFFIC_ANNOTATION_UNINITIALIZED. So there is some mishandling of a mutable annotation somewhere in the policy code.
Actually, we're not looking for a mutable annotation necessarily. The mutable annotation in the stack trace is only there because we need to convert to mutable when passing it over mojo.

We're simply looking for an unitialized annotation.
Well, it doesn't look like this happened in enterprise code. I think you're just unlucky that that the test is exercising some code path which triggers that. The policy is a red herring.

Based on the time when this started flaking and a bit of digging around the stack trace, my guess is that there's a mistake in one of these two CLs:

https://chromium.googlesource.com/chromium/src/+/ca8d525d49c7f122c84ec4531bdc6ef2d8aae363
https://chromium.googlesource.com/chromium/src/+/921731ea1b3df56cee22f7a9268505cae56b7a90

Annotations are attached to objects and passed around, there's probably some forgotten assignment.
Cc: xunji...@chromium.org mmenke@chromium.org
I don't see it yet. +mmenke@, +xunjieli@ in case they remember those CLs and have a hunch.

If we want to be safe, we could just revert (starting with e91bd630d3038038491462bb28f63d701850c430 which you mentioned, as that landed after them), but a lot of files are affected, so it's likely that this won't revert cleanly.

Alternatively, I could disable the validation and let Ramin re-enable it when he returns and can debug. The worst thing that can happen is that people will break some annotations, or add invalid ones, while the validation is disabled. But that's probably acceptable for a short amount of time.
I'm fine with disabling the validation - another test ran into this, too (See  issue 823077 ), and disabling the DCHECK would fix that one, too, until rhalavati gets back.

Comment 11 by emaxx@chromium.org, Mar 28 2018

Thanks Martin for taking look at this quickly!

I think your proposal in comment 9 about the validation disabling makes sense, as this allows to re-enable our test immediately and as you estimate the risk for the annotations code as not very high.
Owner: emaxx@chromium.org
https://chromium-review.googlesource.com/c/chromium/src/+/984355 will disable the validation once it lands.

After that, Maksim, go ahead and re-enable the tests.

I'll follow up with Ramin once he returns.
Project Member

Comment 13 by bugdroid1@chromium.org, Mar 29 2018

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

commit 8255d029d7226bf51208d894e96478ec505d2a53
Author: Maksim Ivanov <emaxx@chromium.org>
Date: Thu Mar 29 22:42:38 2018

Revert "Disabling flaky test PolicyPrefsTest.PolicyToPrefsMapping on Windows."

This reverts commit 788a292e3ec841a0ab78a2186af38cdc980ced8a.

Reason for revert: Re-enabling the PolicyPrefsTest.PolicyToPrefsMapping test, as the flaky DCHECK has been disabled in  https://crbug.com/826744 .

Original change's description:
> Disabling flaky test PolicyPrefsTest.PolicyToPrefsMapping on Windows.
> 
> TBR=emaxx@chromium.org
> TBR=pastarmovj@chromium.org
> 
> Bug:  822975 
> Change-Id: I03062d8eb1c8c3221cf07eb08587e4d9c97ed5e2
> Reviewed-on: https://chromium-review.googlesource.com/968507
> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
> Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#544023}

TBR=pastarmovj@chromium.org,emaxx@chromium.org,dvadym@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  822975 
Change-Id: I5721a14572d7f7a5b019fd5508ced218f19790a5
Reviewed-on: https://chromium-review.googlesource.com/986097
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546994}
[modify] https://crrev.com/8255d029d7226bf51208d894e96478ec505d2a53/chrome/browser/policy/policy_prefs_browsertest.cc

Cc: -rhalavati@chromium.org
Owner: rhalavati@chromium.org
Status: Started (was: Assigned)
Hi, I am back.

I take it emaxx@.
Project Member

Comment 15 by bugdroid1@chromium.org, Apr 9 2018

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

commit 4196b8354cd1bf4aed168dfc0f21074893059303
Author: Ramin Halavati <rhalavati@chromium.org>
Date: Mon Apr 09 08:55:34 2018

Fix ProxyResolutionService annotation bug and reactivate tests.

Network traffic annotation in ProxyResolutionService was wrongly set in
cases where proxy info was fetched through completely synchronous
method, causing flaky test failures.
The bug is fixed and tests are re-enabled.

Bug:  822975 
Bug:  826744 
Bug:  823077 
Change-Id: Ic63301ff053fc9160b1a96372af1ee289342396c
Reviewed-on: https://chromium-review.googlesource.com/997656
Reviewed-by: Eric Roman <eroman@chromium.org>
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549130}
[modify] https://crrev.com/4196b8354cd1bf4aed168dfc0f21074893059303/chrome/browser/net/network_context_configuration_browsertest.cc
[modify] https://crrev.com/4196b8354cd1bf4aed168dfc0f21074893059303/net/proxy_resolution/proxy_resolution_service.cc
[modify] https://crrev.com/4196b8354cd1bf4aed168dfc0f21074893059303/net/traffic_annotation/network_traffic_annotation.h

Status: Fixed (was: Started)
There seems to be no more errors. Marking it fixed.

Sign in to add a comment