Skip to content

Commit

Permalink
Revert "Use thread_local: base/threading/"
Browse files Browse the repository at this point in the history
This reverts commit 83dd35d.

Reason for revert:
LUCI Bisection identified this CL as the culprit of a build failure. See the analysis: https://luci-bisection.appspot.com/analysis/b/8786983103281946273

Sample failed build: https://ci.chromium.org/b/8786983103281946273

If this is a false positive, please report it at https://bugs.chromium.org/p/chromium/issues/entry?comment=Analysis%3A+https%3A%2F%2Fluci-bisection.appspot.com%2Fanalysis%2Fb%2F8786983103281946273&components=Tools%3ETest%3EFindit&labels=LUCI-Bisection-Wrong%2CPri-3%2CType-Bug&status=Available&summary=Wrongly+blamed+https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fsrc%2F%2B%2F4321126

Original change's description:
> Use thread_local: base/threading/
>
> This also marks various scopers as [[maybe_unused, nodiscard]] in hopes
> of better compiler warnings around (mis)use.
>
> Bug: 1416710
> Change-Id: I2cc8ac0b536fcd5e1c67530018f639f60483161d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4321126
> Commit-Queue: Peter Kasting <pkasting@chromium.org>
> Reviewed-by: danakj <danakj@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1115850}
>

Bug: 1416710
Change-Id: I7dfa72052b67c761e84d0bb46dcd1cc20ee1f4e8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4327906
Commit-Queue: luci-bisection@appspot.gserviceaccount.com <luci-bisection@appspot.gserviceaccount.com>
Bot-Commit: luci-bisection@appspot.gserviceaccount.com <luci-bisection@appspot.gserviceaccount.com>
Owners-Override: luci-bisection@appspot.gserviceaccount.com <luci-bisection@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1115856}
  • Loading branch information
luci-bisection@appspot.gserviceaccount.com authored and Chromium LUCI CQ committed Mar 10, 2023
1 parent 6494e5e commit d65d7bf
Show file tree
Hide file tree
Showing 14 changed files with 318 additions and 283 deletions.
66 changes: 44 additions & 22 deletions base/threading/hang_watcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "base/functional/callback_helpers.h"
#include "base/metrics/field_trial_params.h"
#include "base/metrics/histogram_macros.h"
#include "base/no_destructor.h"
#include "base/power_monitor/power_monitor.h"
#include "base/ranges/algorithm.h"
#include "base/strings/string_number_conversions.h"
Expand All @@ -30,7 +31,6 @@
#include "base/time/time.h"
#include "base/trace_event/base_tracing.h"
#include "build/build_config.h"
#include "third_party/abseil-cpp/absl/base/attributes.h"

namespace base {

Expand All @@ -44,8 +44,6 @@ namespace {
enum class LoggingLevel { kNone = 0, kUmaOnly = 1, kUmaAndCrash = 2 };

HangWatcher* g_instance = nullptr;
ABSL_CONST_INIT thread_local internal::HangWatchState* hang_watch_state =
nullptr;
std::atomic<bool> g_use_hang_watcher{false};
std::atomic<HangWatcher::ProcessType> g_hang_watcher_process_type{
HangWatcher::ProcessType::kBrowserProcess};
Expand Down Expand Up @@ -222,7 +220,9 @@ constexpr auto kMonitoringPeriod = base::Seconds(10);

WatchHangsInScope::WatchHangsInScope(TimeDelta timeout) {
internal::HangWatchState* current_hang_watch_state =
HangWatcher::IsEnabled() ? hang_watch_state : nullptr;
HangWatcher::IsEnabled()
? internal::HangWatchState::GetHangWatchStateForCurrentThread()->Get()
: nullptr;

DCHECK(timeout >= base::TimeDelta()) << "Negative timeouts are invalid.";

Expand Down Expand Up @@ -267,6 +267,9 @@ WatchHangsInScope::WatchHangsInScope(TimeDelta timeout) {
WatchHangsInScope::~WatchHangsInScope() {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);

internal::HangWatchState* current_hang_watch_state =
internal::HangWatchState::GetHangWatchStateForCurrentThread()->Get();

// If hang watching was not enabled at construction time there is nothing to
// validate or undo.
if (!took_effect_) {
Expand All @@ -275,43 +278,44 @@ WatchHangsInScope::~WatchHangsInScope() {

// If the thread was unregistered since construction there is also nothing to
// do .
if (!hang_watch_state) {
if (!current_hang_watch_state) {
return;
}

// If a hang is currently being captured we should block here so execution
// stops and we avoid recording unrelated stack frames in the crash.
if (hang_watch_state->IsFlagSet(
if (current_hang_watch_state->IsFlagSet(
internal::HangWatchDeadline::Flag::kShouldBlockOnHang)) {
base::HangWatcher::GetInstance()->BlockIfCaptureInProgress();
}

#if DCHECK_IS_ON()
// Verify that no Scope was destructed out of order.
DCHECK_EQ(this, hang_watch_state->GetCurrentWatchHangsInScope());
hang_watch_state->SetCurrentWatchHangsInScope(previous_watch_hangs_in_scope_);
DCHECK_EQ(this, current_hang_watch_state->GetCurrentWatchHangsInScope());
current_hang_watch_state->SetCurrentWatchHangsInScope(
previous_watch_hangs_in_scope_);
#endif

if (hang_watch_state->nesting_level() == 1) {
if (current_hang_watch_state->nesting_level() == 1) {
// If a call to InvalidateActiveExpectations() suspended hang watching
// during the lifetime of this or any nested WatchHangsInScope it can now
// safely be reactivated by clearing the ignore bit since this is the
// outer-most scope.
hang_watch_state->UnsetIgnoreCurrentWatchHangsInScope();
current_hang_watch_state->UnsetIgnoreCurrentWatchHangsInScope();
} else if (set_hangs_ignored_on_exit_) {
// Return to ignoring hangs since this was the previous state before hang
// watching was temporarily enabled for this WatchHangsInScope only in the
// constructor.
hang_watch_state->SetIgnoreCurrentWatchHangsInScope();
current_hang_watch_state->SetIgnoreCurrentWatchHangsInScope();
}

// Reset the deadline to the value it had before entering this
// WatchHangsInScope.
hang_watch_state->SetDeadline(previous_deadline_);
current_hang_watch_state->SetDeadline(previous_deadline_);
// TODO(crbug.com/1034046): Log when a WatchHangsInScope exits after its
// deadline and that went undetected by the HangWatcher.

hang_watch_state->DecrementNestingLevel();
current_hang_watch_state->DecrementNestingLevel();
}

// static
Expand Down Expand Up @@ -437,11 +441,13 @@ bool HangWatcher::IsCrashReportingEnabled() {

// static
void HangWatcher::InvalidateActiveExpectations() {
if (!hang_watch_state) {
internal::HangWatchState* current_hang_watch_state =
internal::HangWatchState::GetHangWatchStateForCurrentThread()->Get();
if (!current_hang_watch_state) {
// If the current thread is not under watch there is nothing to invalidate.
return;
}
hang_watch_state->SetIgnoreCurrentWatchHangsInScope();
current_hang_watch_state->SetIgnoreCurrentWatchHangsInScope();
}

HangWatcher::HangWatcher()
Expand Down Expand Up @@ -975,8 +981,15 @@ void HangWatcher::BlockIfCaptureInProgress() {
void HangWatcher::UnregisterThread() {
AutoLock auto_lock(watch_state_lock_);

auto it = ranges::find(watch_states_, hang_watch_state,
&std::unique_ptr<internal::HangWatchState>::get);
internal::HangWatchState* current_hang_watch_state =
internal::HangWatchState::GetHangWatchStateForCurrentThread()->Get();

auto it = ranges::find_if(
watch_states_,
[current_hang_watch_state](
const std::unique_ptr<internal::HangWatchState>& state) {
return state.get() == current_hang_watch_state;
});

// Thread should be registered to get unregistered.
DCHECK(it != watch_states_.end());
Expand Down Expand Up @@ -1152,7 +1165,10 @@ uint64_t HangWatchDeadline::SwitchBitsForTesting() {
}

HangWatchState::HangWatchState(HangWatcher::ThreadType thread_type)
: resetter_(&hang_watch_state, this, nullptr), thread_type_(thread_type) {
: thread_type_(thread_type) {
// There should not exist a state object for this thread already.
DCHECK(!GetHangWatchStateForCurrentThread()->Get());

// TODO(crbug.com/1223033): Remove this once macOS uses system-wide ids.
// On macOS the thread ids used by CrashPad are not the same as the ones
// provided by PlatformThread. Make sure to use the same for correct
Expand All @@ -1164,12 +1180,16 @@ HangWatchState::HangWatchState(HangWatcher::ThreadType thread_type)
#else
thread_id_ = PlatformThread::CurrentId();
#endif

// Bind the new instance to this thread.
GetHangWatchStateForCurrentThread()->Set(this);
}

HangWatchState::~HangWatchState() {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);

DCHECK_EQ(hang_watch_state, this);
DCHECK_EQ(GetHangWatchStateForCurrentThread()->Get(), this);
GetHangWatchStateForCurrentThread()->Set(nullptr);

#if DCHECK_IS_ON()
// Destroying the HangWatchState should not be done if there are live
Expand All @@ -1187,7 +1207,7 @@ HangWatchState::CreateHangWatchStateForCurrentThread(
std::make_unique<HangWatchState>(thread_type);

// Setting the thread local worked.
DCHECK_EQ(hang_watch_state, hang_state.get());
DCHECK_EQ(GetHangWatchStateForCurrentThread()->Get(), hang_state.get());

// Transfer ownership to caller.
return hang_state;
Expand Down Expand Up @@ -1253,8 +1273,10 @@ void HangWatchState::DecrementNestingLevel() {
}

// static
HangWatchState* HangWatchState::GetHangWatchStateForCurrentThread() {
return hang_watch_state;
ThreadLocalPointer<HangWatchState>*
HangWatchState::GetHangWatchStateForCurrentThread() {
static NoDestructor<ThreadLocalPointer<HangWatchState>> hang_watch_state;
return hang_watch_state.get();
}

PlatformThreadId HangWatchState::GetThreadID() const {
Expand Down
9 changes: 4 additions & 5 deletions base/threading/hang_watcher.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include <vector>

#include "base/atomicops.h"
#include "base/auto_reset.h"
#include "base/base_export.h"
#include "base/bits.h"
#include "base/compiler_specific.h"
Expand All @@ -32,6 +31,7 @@
#include "base/threading/platform_thread.h"
#include "base/threading/simple_thread.h"
#include "base/threading/thread_checker.h"
#include "base/threading/thread_local.h"
#include "base/time/tick_clock.h"
#include "base/time/time.h"
#include "build/build_config.h"
Expand Down Expand Up @@ -64,7 +64,7 @@ namespace base {
// member but special care is required when doing so as a WatchHangsInScope
// that stays alive longer than intended will generate non-actionable hang
// reports.
class BASE_EXPORT [[maybe_unused, nodiscard]] WatchHangsInScope {
class BASE_EXPORT WatchHangsInScope {
public:
// A good default value needs to be large enough to represent a significant
// hang and avoid noise while being small enough to not exclude too many
Expand Down Expand Up @@ -581,7 +581,8 @@ class BASE_EXPORT HangWatchState {
// Retrieves the hang watch state associated with the calling thread.
// Returns nullptr if no HangWatchState exists for the current thread (see
// CreateHangWatchStateForCurrentThread()).
static HangWatchState* GetHangWatchStateForCurrentThread();
static ThreadLocalPointer<HangWatchState>*
GetHangWatchStateForCurrentThread();

// Returns the current deadline. Use this function if you need to
// store the value. To test if the deadline has expired use IsOverDeadline().
Expand Down Expand Up @@ -649,8 +650,6 @@ class BASE_EXPORT HangWatchState {
// the deadline.
THREAD_CHECKER(thread_checker_);

const AutoReset<HangWatchState*> resetter_;

// If the deadline fails to be updated before TimeTicks::Now() ever
// reaches the value contained in it this constistutes a hang.
HangWatchDeadline deadline_;
Expand Down
2 changes: 1 addition & 1 deletion base/threading/hang_watcher_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,7 @@ TEST_F(HangWatcherSnapshotTest, NonActionableReport) {
WatchHangsInScope expires_instantly(base::TimeDelta{});

internal::HangWatchState* current_hang_watch_state =
internal::HangWatchState::GetHangWatchStateForCurrentThread();
internal::HangWatchState::GetHangWatchStateForCurrentThread()->Get();

// Simulate the deadline changing concurrently during the capture. This
// makes the capture fail since marking of the deadline fails.
Expand Down
34 changes: 28 additions & 6 deletions base/threading/platform_thread.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@

#include "base/threading/platform_thread.h"

#include "base/no_destructor.h"
#include "base/task/current_thread.h"
#include "third_party/abseil-cpp/absl/base/attributes.h"
#include "base/threading/thread_local_storage.h"

#if BUILDFLAG(IS_FUCHSIA)
#include "base/fuchsia/scheduler.h"
Expand All @@ -15,8 +16,30 @@ namespace base {

namespace {

ABSL_CONST_INIT thread_local ThreadType current_thread_type =
ThreadType::kDefault;
// Returns ThreadLocalStorage slot used to store type of the current thread.
// The value is stored as an integer value converted to a pointer. 1 is added to
// the integer value in order to distinguish the case when the TLS slot is not
// initialized.
base::ThreadLocalStorage::Slot* GetThreadTypeTlsSlot() {
static base::NoDestructor<base::ThreadLocalStorage::Slot> tls_slot;
return tls_slot.get();
}

void SaveThreadTypeToTls(ThreadType thread_type) {
GetThreadTypeTlsSlot()->Set(
reinterpret_cast<void*>(static_cast<uintptr_t>(thread_type) + 1));
}

ThreadType GetThreadTypeFromTls() {
uintptr_t value = reinterpret_cast<uintptr_t>(GetThreadTypeTlsSlot()->Get());

// Thread type is set to kNormal by default.
if (value == 0)
return ThreadType::kDefault;

DCHECK_LE(value - 1, static_cast<uintptr_t>(ThreadType::kMaxValue));
return static_cast<ThreadType>(value - 1);
}

} // namespace

Expand All @@ -36,7 +59,7 @@ void PlatformThread::SetCurrentThreadType(ThreadType thread_type) {

// static
ThreadType PlatformThread::GetCurrentThreadType() {
return current_thread_type;
return GetThreadTypeFromTls();
}

// static
Expand All @@ -56,9 +79,8 @@ namespace internal {

void SetCurrentThreadType(ThreadType thread_type,
MessagePumpType pump_type_hint) {
CHECK_LE(thread_type, ThreadType::kMaxValue);
SetCurrentThreadTypeImpl(thread_type, pump_type_hint);
current_thread_type = thread_type;
SaveThreadTypeToTls(thread_type);
}

} // namespace internal
Expand Down
22 changes: 14 additions & 8 deletions base/threading/scoped_blocking_call.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,6 @@
#include "third_party/perfetto/protos/perfetto/trace/track_event/source_location.pbzero.h" // nogncheck
#endif // BUILDFLAG(ENABLE_BASE_TRACING)

#if DCHECK_IS_ON()
#include "base/auto_reset.h"
#include "third_party/abseil-cpp/absl/base/attributes.h"
#endif

namespace base {

namespace {
Expand All @@ -29,7 +24,8 @@ namespace {
// Used to verify that the trace events used in the constructor do not result in
// instantiating a ScopedBlockingCall themselves (which would cause an infinite
// reentrancy loop).
ABSL_CONST_INIT thread_local bool construction_in_progress = false;
LazyInstance<ThreadLocalBoolean>::Leaky tls_construction_in_progress =
LAZY_INSTANCE_INITIALIZER;
#endif

} // namespace
Expand All @@ -40,7 +36,8 @@ ScopedBlockingCall::ScopedBlockingCall(const Location& from_here,
blocking_type,
UncheckedScopedBlockingCall::BlockingCallType::kRegular) {
#if DCHECK_IS_ON()
const AutoReset<bool> resetter(&construction_in_progress, true, false);
DCHECK(!tls_construction_in_progress.Get().Get());
tls_construction_in_progress.Get().Set(true);
#endif

internal::AssertBlockingAllowed();
Expand All @@ -49,6 +46,10 @@ ScopedBlockingCall::ScopedBlockingCall(const Location& from_here,
ctx.event()->set_source_location_iid(
base::trace_event::InternedSourceLocation::Get(&ctx, from_here));
});

#if DCHECK_IS_ON()
tls_construction_in_progress.Get().Set(false);
#endif
}

ScopedBlockingCall::~ScopedBlockingCall() {
Expand All @@ -64,7 +65,8 @@ ScopedBlockingCallWithBaseSyncPrimitives::
blocking_type,
UncheckedScopedBlockingCall::BlockingCallType::kBaseSyncPrimitives) {
#if DCHECK_IS_ON()
const AutoReset<bool> resetter(&construction_in_progress, true, false);
DCHECK(!tls_construction_in_progress.Get().Get());
tls_construction_in_progress.Get().Set(true);
#endif

internal::AssertBaseSyncPrimitivesAllowed();
Expand All @@ -76,6 +78,10 @@ ScopedBlockingCallWithBaseSyncPrimitives::
source_location_data->set_file_name(from_here.file_name());
source_location_data->set_function_name(from_here.function_name());
});

#if DCHECK_IS_ON()
tls_construction_in_progress.Get().Set(false);
#endif
}

ScopedBlockingCallWithBaseSyncPrimitives::
Expand Down

0 comments on commit d65d7bf

Please sign in to comment.