GitLab

PowerMonitorBroadcastSource suffers from raciness in that it can
procress incoming Mojo messages on one thread while both it and the
process-wide PowerMonitor instance are being destroyed on another
thread. A previous CL introduced a mitigation for this problem by
shutting down the processing of incoming Mojo messages in the
PowerMonitorBroadcastSource destructor before proceeding with
destruction (https://chromium-review.googlesource.com/1041215). However,
that turns out not to be sufficient to eliminate the race:
the PowerMonitorBroadcastSource instance can end up waiting for the
lock in its destructor while on another thread it is holding the lock
to process an incoming Mojo message. That second thread can then call
back into the process-wide PowerMonitor instance while it is being
torn down, resulting in crashes due to undefined state (specifically,
the PowerMonitor instance has already set its PowerMonitorSource
instance to nullptr, resulting in a crash when that instance is being
accessed -- ironically, the very instance that is currently blocked
in its destructor waiting for the processing of the incoming Mojo
message to finish :).

This CL removes the possibility of that race on shutdown by introducing
a two-phase shutdown process for PowerMonitorSource. In PowerMonitor's
destructor, it first invokes PowerMonitorSource::Shutdown() before
proceeding. The contract of PowerMonitorSource::Shutdown() is that
subclasses must take any necessary action to ensure that on return
from that method, they will no longer call into PowerMonitor.
PowerMonitorBroadcastSource fulfills this contract by stopping the
process of incoming Mojo messages in
PowerMonitorBroadcastSource::Shutdown(). In the above scenario,
the broadcast source will end up waiting for the processing of the
incoming Mojo message to finish *inside*
PowerMonitorBroadcastSource::Shutdown(), which is safe by design as it
is before PowerMonitor has done any teardown.

Note that it is possible that there are still other kinds of
raciness between PowerMonitorBroadcastSource and PowerMonitor; as I
expressed on
https://bugs.chromium.org/p/chromium/issues/detail?id=834312#c33, I am
not confident about the overall threading model here.

TBR=xjz@chromium.org, mmenke@chromium.org

Bug: 809031
Change-Id: I9d0f291995b8185e16dff1f88564b6e5344439b2
Reviewed-on: https://chromium-review.googlesource.com/1098932Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567302}