Commit 3ceb81ba authored by Ken Rockot's avatar Ken Rockot Committed by Commit Bot

[mojo-bindings] CHECK for possible double-free

Adds a CHECK to SimpleWatcher's internal thread-safe Context state to
investigate possible incorrect cross-thread bindings usage leading to
double-free of a SimpleWatcher. This is highly speculative and might not
lead anywhere.

Bug: 896419
Change-Id: I33c937208152a9d5cc0fdfc7831935e1d3e3bb88
Reviewed-on: https://chromium-review.googlesource.com/c/1372269Reviewed-by: default avatarReilly Grant <reillyg@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#615652}
parent 1e6252d9
...@@ -42,6 +42,8 @@ class SimpleWatcher::Context : public base::RefCountedThreadSafe<Context> { ...@@ -42,6 +42,8 @@ class SimpleWatcher::Context : public base::RefCountedThreadSafe<Context> {
*result = MojoAddTrigger(trap_handle.value(), handle.value(), signals, *result = MojoAddTrigger(trap_handle.value(), handle.value(), signals,
condition, context->value(), nullptr); condition, context->value(), nullptr);
if (*result != MOJO_RESULT_OK) { if (*result != MOJO_RESULT_OK) {
context->cancelled_ = true;
// Balanced by the AddRef() above since MojoAddTrigger failed. // Balanced by the AddRef() above since MojoAddTrigger failed.
context->Release(); context->Release();
return nullptr; return nullptr;
...@@ -76,7 +78,17 @@ class SimpleWatcher::Context : public base::RefCountedThreadSafe<Context> { ...@@ -76,7 +78,17 @@ class SimpleWatcher::Context : public base::RefCountedThreadSafe<Context> {
: weak_watcher_(weak_watcher), : weak_watcher_(weak_watcher),
task_runner_(task_runner), task_runner_(task_runner),
watch_id_(watch_id) {} watch_id_(watch_id) {}
~Context() {}
~Context() {
// TODO(https://crbug.com/896419): Remove this once it's been live for a
// while. This is intended to catch possible double-frees of SimpleWatchers,
// due to, e.g., invalid cross-thread usage of bindings endpoints. If this
// CHECK fails, then the Context is being destroyed before a cancellation
// notification fired. In that case we know a Context ref has been
// double-released and we can catch its stack.
base::AutoLock lock(lock_);
CHECK(cancelled_);
}
void Notify(MojoResult result, void Notify(MojoResult result,
MojoHandleSignalsState signals_state, MojoHandleSignalsState signals_state,
...@@ -90,6 +102,7 @@ class SimpleWatcher::Context : public base::RefCountedThreadSafe<Context> { ...@@ -90,6 +102,7 @@ class SimpleWatcher::Context : public base::RefCountedThreadSafe<Context> {
// closed due to pipe error, all before the thread's TaskRunner has been // closed due to pipe error, all before the thread's TaskRunner has been
// properly initialized. // properly initialized.
base::AutoLock lock(lock_); base::AutoLock lock(lock_);
cancelled_ = true;
if (!enable_cancellation_notifications_) if (!enable_cancellation_notifications_)
return; return;
} }
...@@ -115,6 +128,7 @@ class SimpleWatcher::Context : public base::RefCountedThreadSafe<Context> { ...@@ -115,6 +128,7 @@ class SimpleWatcher::Context : public base::RefCountedThreadSafe<Context> {
const int watch_id_; const int watch_id_;
base::Lock lock_; base::Lock lock_;
bool cancelled_ = false;
bool enable_cancellation_notifications_ = true; bool enable_cancellation_notifications_ = true;
DISALLOW_COPY_AND_ASSIGN(Context); DISALLOW_COPY_AND_ASSIGN(Context);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment