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

Issue 836359 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 18
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug

Blocked on:
issue 837707



Sign in to add a comment

Race in NotificationImageReporterTest

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, Apr 24 2018

Issue description

Filed by sheriff-o-matic@appspot.gserviceaccount.com on behalf of thestig@chromium.org

unit_tests failing on chromium.memory/Linux TSan Tests

Builders failed on: 
- Linux TSan Tests: 
  https://build.chromium.org/p/chromium.memory/builders/Linux%20TSan%20Tests

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20TSan%20Tests/20707
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20TSan%20Tests/20715

[ RUN      ] NotificationImageReporterTest.MaxReportsPerDay
GMOCK WARNING:
Uninteresting mock function call - taking default action specified at:
../../chrome/browser/safe_browsing/notification_image_reporter_unittest.cc:152:
    Function call: CheckCsdWhitelistUrl(@0x7b2c00004b20 https://example.com/, 0x7b2800005820)
          Returns: 4-byte object <02-00 00-00>
NOTE: You can safely ignore the above warning unless this call should not happen.  Do not suppress it by blindly adding an EXPECT_CALL() if you don't mean to enforce the call.  See https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#knowing-when-to-expect for details.
==================
WARNING: ThreadSanitizer: data race (pid=23515)
  Write of size 1 at 0x00000f9f0930 by thread T14:
    #0 base::FeatureList::IsEnabled(base::Feature const&) base/feature_list.cc (unit_tests+0x7fc3011)
    #1 network::ResourceScheduler::ResourceScheduler(bool) services/network/resource_scheduler.cc:1023:11 (unit_tests+0x9d46e1c)
    #2 make_unique<network::ResourceScheduler, const bool &> buildtools/third_party/libc++/trunk/include/memory:3114:32 (unit_tests+0x9d2c42c)
...
  Previous write of size 1 at 0x00000f9f0930 by main thread:
    #0 base::FeatureList::IsEnabled(base::Feature const&) base/feature_list.cc (unit_tests+0x7fc3011)
    #1 GetExtendedReportingPrefName components/safe_browsing/common/safe_browsing_prefs.cc:228:7 (unit_tests+0xa9fa4c8)
    #2 safe_browsing::IsExtendedReportingEnabled(PrefService const&) components/safe_browsing/common/safe_browsing_prefs.cc:312 (unit_tests+0xa9fa4c8)
    #3 safe_browsing::GetExtendedReportingLevel(PrefService const&) components/safe_browsing/common/safe_browsing_prefs.cc:218:8 (unit_tests+0xa9fa3de)
    #4 safe_browsing::NotificationImageReporter::OnWhitelistCheckDone(Profile*, GURL const&, SkBitmap const&, bool) chrome/browser/safe_browsing/notification_image_reporter.cc:172:7 (unit_tests+0xab0ea61)
    #5 Invoke<base::WeakPtr<safe_browsing::NotificationImageReporter>, Profile *, GURL, SkBitmap, bool> base/bind_internal.h:447:12 (unit_tests+0xab0fd6a)
 
Cc: -thestig@chromium.org harkness@chromium.org lpz@chromium.org
Components: Services>Safebrowsing
Labels: Stability-ThreadSanitizer OS-Linux Type-Bug
Status: Untriaged (was: Available)
Summary: Race in NotificationImageReporterTest (was: Race in NotificationImageReporterTest.NoReportWithoutScout)
The stacktraces is actually mixed between NoReportWithoutScout and MaxReportsPerDay.
Labels: -Sheriff-Chromium
I'm going to disable the racing tests.
See also  bug 831563 .
Project Member

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

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

commit da17415d18884fb47451bb27fc9f3087fdb37915
Author: Lei Zhang <thestig@chromium.org>
Date: Tue Apr 24 19:30:06 2018

Disable NotificationImageReporterTest.

Its use of feature flags causes data races.

BUG= 836359 
TBR=xidachen@chromium.org
NOTRY=true

Change-Id: I5bc92a6fc9b9010b9863b363dc3033b95f777762
Reviewed-on: https://chromium-review.googlesource.com/1026462
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553230}
[modify] https://crrev.com/da17415d18884fb47451bb27fc9f3087fdb37915/chrome/browser/safe_browsing/notification_image_reporter_unittest.cc

Project Member

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

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

commit c2aa90817727c49eb306db48ef753c4a4eb9b264
Author: Findit <findit-for-me@appspot.gserviceaccount.com>
Date: Tue Apr 24 20:02:06 2018

Revert "Disable NotificationImageReporterTest."

This reverts commit da17415d18884fb47451bb27fc9f3087fdb37915.

Reason for revert:

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

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.linux/Linux%20Builder/101940

Sample Failed Step: compile

Original change's description:
> Disable NotificationImageReporterTest.
> 
> Its use of feature flags causes data races.
> 
> BUG= 836359 
> TBR=xidachen@chromium.org
> NOTRY=true
> 
> Change-Id: I5bc92a6fc9b9010b9863b363dc3033b95f777762
> Reviewed-on: https://chromium-review.googlesource.com/1026462
> Reviewed-by: Lei Zhang <thestig@chromium.org>
> Commit-Queue: Lei Zhang <thestig@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#553230}

No-Presubmit: true
No-Tree-Checks: true
No-Try: true
BUG= 836359 

Change-Id: I748cc7ee9c5dd97a74d10dfdd77161966cff86b8
Reviewed-on: https://chromium-review.googlesource.com/1026433
Cr-Commit-Position: refs/heads/master@{#553241}
[modify] https://crrev.com/c2aa90817727c49eb306db48ef753c4a4eb9b264/chrome/browser/safe_browsing/notification_image_reporter_unittest.cc

Project Member

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

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

commit 82a08db105afb0ad765fa5ee5c7c6370745f466e
Author: Lei Zhang <thestig@chromium.org>
Date: Tue Apr 24 20:45:44 2018

Disable NotificationImageReporterTest. (try 2)

Its use of feature flags causes data races.

The first attempt in https://chromium-review.googlesource.com/1026462
got incorrectly reverted by FindIt.

BUG= 836359 
TBR=xidachen@chromium.org
NOTRY=true

Change-Id: Icfeaf23ccb69b3b1e80f6df10556f5c75e663a71
Reviewed-on: https://chromium-review.googlesource.com/1026557
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553263}
[modify] https://crrev.com/82a08db105afb0ad765fa5ee5c7c6370745f466e/chrome/browser/safe_browsing/notification_image_reporter_unittest.cc

Labels: SafeBrowsing-Triaged
Owner: vakh@chromium.org
Status: Assigned (was: Untriaged)

Comment 9 by vakh@chromium.org, May 4 2018

Blockedon: 837707
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 18

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

commit d38b38428a8060ce1a2644a364b1466060d50aab
Author: Varun Khaneja <vakh@chromium.org>
Date: Wed Jul 18 19:53:49 2018

Remove NotificationImageReporter since it the backend support is not planned

R=nparker

Bug: 837707, 836359 , 831563 
Change-Id: Ice5c619948e47a3df6a257fc062ab9d38928bbab
Reviewed-on: https://chromium-review.googlesource.com/1139032
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Commit-Queue: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576175}
[modify] https://crrev.com/d38b38428a8060ce1a2644a364b1466060d50aab/android_webview/browser/aw_safe_browsing_ui_manager.cc
[modify] https://crrev.com/d38b38428a8060ce1a2644a364b1466060d50aab/android_webview/browser/aw_safe_browsing_ui_manager.h
[modify] https://crrev.com/d38b38428a8060ce1a2644a364b1466060d50aab/chrome/browser/notifications/platform_notification_service_impl.cc
[modify] https://crrev.com/d38b38428a8060ce1a2644a364b1466060d50aab/chrome/browser/safe_browsing/BUILD.gn
[delete] https://crrev.com/28ae7e13cf5b8ac2e51ccaed9c342934ff3a39e6/chrome/browser/safe_browsing/notification_image_reporter.cc
[delete] https://crrev.com/28ae7e13cf5b8ac2e51ccaed9c342934ff3a39e6/chrome/browser/safe_browsing/notification_image_reporter.h
[delete] https://crrev.com/28ae7e13cf5b8ac2e51ccaed9c342934ff3a39e6/chrome/browser/safe_browsing/notification_image_reporter_unittest.cc
[delete] https://crrev.com/28ae7e13cf5b8ac2e51ccaed9c342934ff3a39e6/chrome/browser/safe_browsing/ping_manager.cc
[delete] https://crrev.com/28ae7e13cf5b8ac2e51ccaed9c342934ff3a39e6/chrome/browser/safe_browsing/ping_manager.h
[modify] https://crrev.com/d38b38428a8060ce1a2644a364b1466060d50aab/chrome/browser/safe_browsing/safe_browsing_service.cc
[modify] https://crrev.com/d38b38428a8060ce1a2644a364b1466060d50aab/chrome/browser/safe_browsing/safe_browsing_service.h
[modify] https://crrev.com/d38b38428a8060ce1a2644a364b1466060d50aab/chrome/browser/safe_browsing/test_safe_browsing_service.cc
[modify] https://crrev.com/d38b38428a8060ce1a2644a364b1466060d50aab/chrome/browser/safe_browsing/ui_manager.cc
[modify] https://crrev.com/d38b38428a8060ce1a2644a364b1466060d50aab/chrome/test/BUILD.gn
[modify] https://crrev.com/d38b38428a8060ce1a2644a364b1466060d50aab/components/safe_browsing/BUILD.gn
[rename] https://crrev.com/d38b38428a8060ce1a2644a364b1466060d50aab/components/safe_browsing/ping_manager.cc
[rename] https://crrev.com/d38b38428a8060ce1a2644a364b1466060d50aab/components/safe_browsing/ping_manager.h
[rename] https://crrev.com/d38b38428a8060ce1a2644a364b1466060d50aab/components/safe_browsing/ping_manager_unittest.cc
[modify] https://crrev.com/d38b38428a8060ce1a2644a364b1466060d50aab/tools/traffic_annotation/summary/annotations.xml

Status: Fixed (was: Assigned)

Sign in to add a comment