New issue
Advanced search Search tips

Issue 805814 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Add Abseil's lock annotation to //base

Project Member Reported by pwnall@chromium.org, Jan 25 2018

Issue description

Discussion: https://groups.google.com/a/chromium.org/d/topic/cxx/oTvaE-JrRBs/discussion

Although most of Chromium code relies on message-passing rather than mutexes, we still use mutexes in the codebase.

Abseil has some very useful (imho) macros for annotating Mutex usage:
https://github.com/abseil/abseil-cpp/blob/master/absl/base/thread_annotations.h
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 25 2018

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

commit eea5ec40b383287eeaf5a48f6ab5c213b6fc4498
Author: Victor Costan <pwnall@chromium.org>
Date: Thu Jan 25 07:53:50 2018

Blink: Make ThreadCondition only work with Mutex.

The interaction between ThreadCondition (condition variables) and
recursive (a.k.a. re-entrant) mutexes is difficult to reason about.
Details in the "Why can the holder of a Lock not reacquire it?" section
of https://www.chromium.org/developers/lock-and-condition-variable

Bug:  805814 
Change-Id: Ie0b5e026a43768108011ca8f2f6fd3fe0e1f4b03
Reviewed-on: https://chromium-review.googlesource.com/885862
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531846}
[modify] https://crrev.com/eea5ec40b383287eeaf5a48f6ab5c213b6fc4498/third_party/WebKit/Source/platform/wtf/ThreadingPrimitives.h
[modify] https://crrev.com/eea5ec40b383287eeaf5a48f6ab5c213b6fc4498/third_party/WebKit/Source/platform/wtf/ThreadingPthreads.cpp
[modify] https://crrev.com/eea5ec40b383287eeaf5a48f6ab5c213b6fc4498/third_party/WebKit/Source/platform/wtf/ThreadingWin.cpp

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 27 2018

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

commit a0590b829d7bfab8e7c258de5b369dec363fcae6
Author: Victor Costan <pwnall@chromium.org>
Date: Sat Jan 27 07:44:27 2018

WebSQL: Switch mutex ownership DCHECK from TryLock() to Locked().

Bug:  805814 
Change-Id: Ia1cac2e6ce3da10997ff722b5d5095651c4ca586
Reviewed-on: https://chromium-review.googlesource.com/885921
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532195}
[modify] https://crrev.com/a0590b829d7bfab8e7c258de5b369dec363fcae6/third_party/WebKit/Source/modules/webdatabase/Database.cpp

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 31 2018

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

commit 70f81b0a7fecac2469b060d7418f61d9c3e3ed63
Author: Victor Costan <pwnall@chromium.org>
Date: Wed Jan 31 23:09:50 2018

Add lock annotations from Abseil C++.

The annotations are documented in "base/thread_annotations.h". The
header location matches Abseil by design. Additional documentation at
https://abseil.io/docs/cpp/guides/synchronization#thread-annotations

This takes advantage of Clang's built-in support for static lock analysis,
documented at http://clang.llvm.org/docs/ThreadSafetyAnalysis.html

This CL ensures that the lock annotations work as intended by applying them to
Blink's WTF::Mutex and WTF::MutexLocker, and by marking one member
variable in a Blink class (BlobData) as guarded by a Mutex. This is the
minimum necessary to open up the way for other CLs to add annotations to
the rest of the codebase.

Bug:  805814 
Change-Id: I34159fccd34a8db749f75992e0d160f83d222d68
Reviewed-on: https://chromium-review.googlesource.com/885506
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533465}
[modify] https://crrev.com/70f81b0a7fecac2469b060d7418f61d9c3e3ed63/base/BUILD.gn
[add] https://crrev.com/70f81b0a7fecac2469b060d7418f61d9c3e3ed63/base/thread_annotations.h
[modify] https://crrev.com/70f81b0a7fecac2469b060d7418f61d9c3e3ed63/build/config/compiler/BUILD.gn
[modify] https://crrev.com/70f81b0a7fecac2469b060d7418f61d9c3e3ed63/third_party/WebKit/Source/DEPS
[modify] https://crrev.com/70f81b0a7fecac2469b060d7418f61d9c3e3ed63/third_party/WebKit/Source/modules/webaudio/MediaElementAudioSourceNode.h
[modify] https://crrev.com/70f81b0a7fecac2469b060d7418f61d9c3e3ed63/third_party/WebKit/Source/modules/webdatabase/Database.cpp
[modify] https://crrev.com/70f81b0a7fecac2469b060d7418f61d9c3e3ed63/third_party/WebKit/Source/platform/blob/BlobData.h
[modify] https://crrev.com/70f81b0a7fecac2469b060d7418f61d9c3e3ed63/third_party/WebKit/Source/platform/heap/Heap.cpp
[modify] https://crrev.com/70f81b0a7fecac2469b060d7418f61d9c3e3ed63/third_party/WebKit/Source/platform/heap/HeapTest.cpp
[modify] https://crrev.com/70f81b0a7fecac2469b060d7418f61d9c3e3ed63/third_party/WebKit/Source/platform/heap/Persistent.h
[modify] https://crrev.com/70f81b0a7fecac2469b060d7418f61d9c3e3ed63/third_party/WebKit/Source/platform/heap/PersistentNode.cpp
[modify] https://crrev.com/70f81b0a7fecac2469b060d7418f61d9c3e3ed63/third_party/WebKit/Source/platform/heap/PersistentNode.h
[modify] https://crrev.com/70f81b0a7fecac2469b060d7418f61d9c3e3ed63/third_party/WebKit/Source/platform/heap/ThreadState.cpp
[modify] https://crrev.com/70f81b0a7fecac2469b060d7418f61d9c3e3ed63/third_party/WebKit/Source/platform/wtf/ThreadingPrimitives.h

Comment 4 by pwnall@chromium.org, Jan 31 2018

Status: Fixed (was: Started)
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 16 2018

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

commit 63cd2c2dfef0dbc7a588006b72e0ec358026cf11
Author: Victor Costan <pwnall@chromium.org>
Date: Fri Feb 16 02:29:07 2018

Unit tests for base/thread_annotations.h.

Bug:  805814 
Change-Id: Ib2441c3ed2060e73062c3993a6876314cac9a669
Reviewed-on: https://chromium-review.googlesource.com/921321
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537110}
[modify] https://crrev.com/63cd2c2dfef0dbc7a588006b72e0ec358026cf11/base/BUILD.gn
[add] https://crrev.com/63cd2c2dfef0dbc7a588006b72e0ec358026cf11/base/thread_annotations_unittest.cc
[add] https://crrev.com/63cd2c2dfef0dbc7a588006b72e0ec358026cf11/base/thread_annotations_unittest.nc
[modify] https://crrev.com/63cd2c2dfef0dbc7a588006b72e0ec358026cf11/build/nocompile.gni

Project Member

Comment 6 by bugdroid1@chromium.org, Feb 16 2018

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

commit 3884f1c01ae0d33e0697ad5322de7ffdc949bd81
Author: Victor Costan <pwnall@chromium.org>
Date: Fri Feb 16 03:20:18 2018

Enable thread safety analysis on Windows clang.

Bug:  805814 
Change-Id: I58df2b83b9caa6bd049e14cdfae67e259eb3bba5
Reviewed-on: https://chromium-review.googlesource.com/921341
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537169}
[modify] https://crrev.com/3884f1c01ae0d33e0697ad5322de7ffdc949bd81/build/config/compiler/BUILD.gn

Sign in to add a comment