New issue
Advanced search Search tips

Issue 832728 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

base_unittests and unit_tests failures in win-asan official builder.

Project Member Reported by w...@chromium.org, Apr 13 2018

Issue description

This builder produces an official/Release build with dcheck_is_configurable=true. Death-test failures typically mean the test's expectations do not correctly match the [D]CHECKs in the code, e.g. they use EXPECT_DCHECK_DEATH() but the code under test uses CHECK.

base_unittests failing:
RunLoopDeathTest.MustRegisterBeforeInstantiating
ObserverListTest.NonReentrantObserverList
SampleVectorTest.AddSubtractBucketNotMatchDeath
SampleVectorTest.BucketIndexDeath
TestMockTimeTaskRunnerTest.DefaultUnbound

unit_tests failing:
ExtensionUpdaterTest.TestExtensionUpdateCheckRequestsPending
ExtensionUpdaterTest.TestInstallSource
ExtensionUpdaterTest.TestUpdateUrlData
ExtensionUpdaterTest.TestExtensionUpdateCheckRequests
ExtensionUpdaterTest.TestInstallLocation

 

Comment 1 by w...@chromium.org, Apr 13 2018

Description: Show this description

Comment 2 by w...@chromium.org, Apr 13 2018

Cc: gab@chromium.org
RunLoopDeathTest.MustRegisterBeforeInstantiating fails because it EXPECT_DCHECK_DEATH()s but the code-under-test hits a CHECK() before reaching the intended DCHECK().

+gab@, who added this test in https://chromium-review.googlesource.com/594352

Comment 3 by w...@chromium.org, Apr 14 2018

Cc: rdevlin....@chromium.org
ExtensionUpdaterTest.* tests are all failing because:
1. |prodchannel| is populated from channel_info::GetChannelInfo(), which returns " DCheck" in these builds, instead of "".
2. The tests verify that the UpdateQueryParams() are correctly copied into the query URL, but something in that path seems to strip the leading space.

+rdevlin.cronin@ - Devlin, how is the |prodchannel| field used..? Does returning "<channel> DCheck" here actually pose a problem in production?

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 14 2018

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

commit 39ee610f7bfa84da3bcdb33c31d126cfc4081905
Author: Wez <wez@chromium.org>
Date: Sat Apr 14 21:33:59 2018

Fix RunLoopDeathTest to expect CHECK rather than DCHECK failure.

Bug:  832728 ,  703346 
Change-Id: I4f400986db39b957095a30dc7770f6e6e541a8ae
Reviewed-on: https://chromium-review.googlesource.com/1012568
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550908}
[modify] https://crrev.com/39ee610f7bfa84da3bcdb33c31d126cfc4081905/base/run_loop_unittest.cc

@3 Sorry, I don't quite follow.

I don't see any field called prodchannel in the extension updater, or any assertions for dcheck vs not in ExtensionUpdaterTest.  I also can't find any calls to GetChannelInfo() in the updater.

Can you include some code pointers to the places you're referencing?
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/39ee610f7bfa84da3bcdb33c31d126cfc4081905

commit 39ee610f7bfa84da3bcdb33c31d126cfc4081905
Author: Wez <wez@chromium.org>
Date: Sat Apr 14 21:33:59 2018

Fix RunLoopDeathTest to expect CHECK rather than DCHECK failure.

Bug:  832728 ,  703346 
Change-Id: I4f400986db39b957095a30dc7770f6e6e541a8ae
Reviewed-on: https://chromium-review.googlesource.com/1012568
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550908}
[modify] https://crrev.com/39ee610f7bfa84da3bcdb33c31d126cfc4081905/base/run_loop_unittest.cc

Comment 7 by w...@chromium.org, Apr 17 2018

Re #5: Sorry, I'm referring to the update query URL verification at https://cs.chromium.org/chromium/src/chrome/browser/extensions/updater/extension_updater_unittest.cc?l=623.  The function is basically checking that the general update query parameters are all present and correct in extension updater query URLs, in addition to the extension-specific bits.

For the "expected" query params, "prodchannel" is a field inserted into the update query URL retrieved from UpdateQueryParams::Get() (see https://cs.chromium.org/chromium/src/components/update_client/update_query_params.cc?l=75) by the UpdateQueryParamsDelegate (see Chrome's impl https://cs.chromium.org/chromium/src/chrome/browser/update_client/chrome_update_query_params_delegate.cc?l=32 which adds "prodchannel" based on GetChannelInfo()).

The failing tests are calling the same UpdateQueryParams::Get() function to get the query parameters (see https://cs.chromium.org/chromium/src/chrome/browser/extensions/updater/extension_updater_unittest.cc?l=2013) to pass when creating the ManifestFetchData helper. Internally that helper calls GURL::ReplaceComponents() (see https://cs.chromium.org/chromium/src/extensions/browser/updater/manifest_fetch_data.cc?sq=package:chromium&l=95), which I suspect is where the space is getting stripped, from " DCheck" to "DCheck".

This may be as simple as changing the " DCheck" channel suffix to e.g. "-DCheck", but I'm surprised that ReplaceComponents() seems to be stripping characters from elements.
Cc: waff...@chromium.org mxnguyen@chromium.org
+component updater experts
Project Member

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

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

commit f0acd4fcf1939a152c2787f625644a8e57947d28
Author: Wez <wez@chromium.org>
Date: Tue Apr 17 21:39:44 2018

Fix SampleVector tests to expect CHECK rather than DCHECK deaths.

Bug:  832728 
Change-Id: I9a2d2d330aa4d250c925b7a614780acbf40dce3f
Reviewed-on: https://chromium-review.googlesource.com/1012732
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551483}
[modify] https://crrev.com/f0acd4fcf1939a152c2787f625644a8e57947d28/base/metrics/sample_vector_unittest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 17 2018

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

commit a10be3237416a9a944d4cc024e8b4c6bbaad5d33
Author: Findit <findit-for-me@appspot.gserviceaccount.com>
Date: Tue Apr 17 22:58:32 2018

Revert "Fix SampleVector tests to expect CHECK rather than DCHECK deaths."

This reverts commit f0acd4fcf1939a152c2787f625644a8e57947d28.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 551483 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2YwYWNkNGZjZjE5MzlhMTUyYzI3ODdmNjI1NjQ0YThlNTc5NDdkMjgM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-rel/7001

Sample Failed Step: base_unittests

Original change's description:
> Fix SampleVector tests to expect CHECK rather than DCHECK deaths.
> 
> Bug:  832728 
> Change-Id: I9a2d2d330aa4d250c925b7a614780acbf40dce3f
> Reviewed-on: https://chromium-review.googlesource.com/1012732
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Commit-Queue: Wez <wez@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#551483}

Change-Id: Ie2ad7fb9ebd4c214a7dd2cebe0763b2f41793f01
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  832728 
Reviewed-on: https://chromium-review.googlesource.com/1015194
Cr-Commit-Position: refs/heads/master@{#551512}
[modify] https://crrev.com/a10be3237416a9a944d4cc024e8b4c6bbaad5d33/base/metrics/sample_vector_unittest.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Apr 18 2018

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

commit 8aefefd90c62830a6b17606011c2c1fea542f2b3
Author: Wez <wez@chromium.org>
Date: Wed Apr 18 19:17:30 2018

Reland "Fix SampleVector tests to expect CHECK rather than DCHECK deaths."

This is a reland of f0acd4fcf1939a152c2787f625644a8e57947d28, which
mistakenly replaced some EXPECT_DCHECK_DEATH() checks with EXPECT_DEATH()
even though the code under test really was DCHECKing.

Original change's description:
> Fix SampleVector tests to expect CHECK rather than DCHECK deaths.
>
> Bug:  832728 
> Change-Id: I9a2d2d330aa4d250c925b7a614780acbf40dce3f
> Reviewed-on: https://chromium-review.googlesource.com/1012732
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Commit-Queue: Wez <wez@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#551483}

Bug:  832728 
Change-Id: I046d3ad35a9b9b7e6d3f214029dfcfaeba81243d
Reviewed-on: https://chromium-review.googlesource.com/1016861
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551776}
[modify] https://crrev.com/8aefefd90c62830a6b17606011c2c1fea542f2b3/base/metrics/sample_vector_unittest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Apr 24 2018

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

commit c536c3c91a02d679f532a46db12aa5e91178a174
Author: Wez <wez@chromium.org>
Date: Tue Apr 24 01:22:25 2018

Don't use whitespace in ChannelName() under Windows Official DCHECK.

Using whitespace breaks ExtensionUpdater query parameter generation.

Bug:  832728 
Change-Id: I191d175d623803d4655d22caf422bf7127bdc57c
Reviewed-on: https://chromium-review.googlesource.com/1016138
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552942}
[modify] https://crrev.com/c536c3c91a02d679f532a46db12aa5e91178a174/chrome/common/channel_info_win.cc

Just to update the latest behavior of this issue, still below failures is observed for win-asan official.desktop builder.

Failures:
-----------
TestMockTimeTaskRunnerTest.DefaultUnbound
ObserverListTest.NonReentrantObserverList

Link to builder:
---------------
https://uberchromegw.corp.google.com/i/official.desktop/builders/win-asan/builds/945

Link to log:
------------
https://logs.chromium.org/v/?s=chrome%2Fbb%2Fofficial.desktop%2Fwin-asan%2F945%2F%2B%2Frecipes%2Fsteps%2Fbase_unittests%2F0%2Fstdout

Thanks!
  
Project Member

Comment 14 by bugdroid1@chromium.org, Apr 26 2018

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

commit 0c9c93cef24767d3bc2f069d727aee144f693c50
Author: Wez <wez@chromium.org>
Date: Thu Apr 26 16:23:24 2018

Tweak some death test expectations to pass dcheck_is_configurable.

- Fixes TestMockTimeTaskRunnerTest to use EXPECT_DEATH().
- Switches ObserverListTest.NonReentrantObserverList to use the
  EXPECT_DCHECK_DEATH() macro rather than the ScopedLogAssertHandler,
  so that LOG_DCHECK will be set correctly for the to-die child process.

Bug:  832728 
Change-Id: I7f90bfd49491520c49505dc1500d3934aad81494
Reviewed-on: https://chromium-review.googlesource.com/1027990
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554046}
[modify] https://crrev.com/0c9c93cef24767d3bc2f069d727aee144f693c50/base/observer_list_unittest.cc
[modify] https://crrev.com/0c9c93cef24767d3bc2f069d727aee144f693c50/base/test/test_mock_time_task_runner_unittest.cc

Comment 15 by w...@chromium.org, Apr 27 2018

Status: Fixed (was: Started)

Sign in to add a comment