CrossThreadWeakPersistent: Restrict usage

CTWP is a weak ref used on a different thread than the object has been
original been allocated on. Using the object directly is inherently
unsafe as garbage collection only consider the stack of the thread the
heap is attached to. For that reason, working on an object must
require a strong reference.

Note that this does not rule out issues, as the originating thread may
terminate in which case even the strong CrossThreadPersistent does not
keep the object alive.

Nonetheless, the only valid use case of a CTWP should be weakly
pointing to an object. The CL introduces a Lock() call, similar to
std::weak_ptr::Lock() with the same semantics: The reference (CTP)
needs to be check against cleared'ness before the object can be used.

Blink: Most use cases rely on external synchronizations when accessing
a CTWP, so this change merely hardens the API but likely does not fix
any real issues.

Bug: 1125495
Change-Id: Ib859df5e9c5f418aa36797c4bd9a444de8c82b15
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2391231
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804864}

third_party/blink/renderer/core/loader/worker_resource_timing_notifier_impl.cc

@@ -10,6 +10,7 @@
 #include "third_party/blink/renderer/core/timing/dom_window_performance.h"
 #include "third_party/blink/renderer/core/timing/performance.h"
 #include "third_party/blink/renderer/core/timing/worker_global_scope_performance.h"
+#include "third_party/blink/renderer/platform/heap/persistent.h"
 #include "third_party/blink/renderer/platform/loader/fetch/resource_timing_info.h"
 #include "third_party/blink/renderer/platform/scheduler/public/post_cross_thread_task.h"
 #include "third_party/blink/renderer/platform/wtf/cross_thread_copier.h"
@@ -88,14 +89,15 @@ void WorkerResourceTimingNotifierImpl::AddCrossThreadResourceTiming(
     mojo::PendingReceiver<mojom::blink::WorkerTimingContainer>
         worker_timing_receiver) {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
-  if (!outside_execution_context_ ||
-      outside_execution_context_->IsContextDestroyed())
+  auto outside_execution_context = outside_execution_context_.Lock();
+  if (!outside_execution_context ||
+      outside_execution_context->IsContextDestroyed())
     return;
-  DCHECK(outside_execution_context_->IsContextThread());
-  GetPerformance(*outside_execution_context_)
+  DCHECK(outside_execution_context->IsContextThread());
+  GetPerformance(*outside_execution_context)
       ->AddResourceTiming(std::move(info), AtomicString(initiator_type),
                           std::move(worker_timing_receiver),
-                          outside_execution_context_);
+                          outside_execution_context);
 }
 
 void WorkerResourceTimingNotifierImpl::Trace(Visitor* visitor) const {

third_party/blink/renderer/modules/webaudio/cpu/arm/oscillator_kernel_neon.cc

@@ -33,7 +33,8 @@ std::tuple<int, double> OscillatorHandler::ProcessKRateVector(
     double virtual_read_index,
     float frequency,
     float rate_scale) const {
-  const unsigned periodic_wave_size = periodic_wave_->PeriodicWaveSize();
+  auto periodic_wave = periodic_wave_.Lock();
+  const unsigned periodic_wave_size = periodic_wave->PeriodicWaveSize();
   const double inv_periodic_wave_size = 1.0 / periodic_wave_size;
 
   float* higher_wave_data = nullptr;
@@ -42,7 +43,7 @@ std::tuple<int, double> OscillatorHandler::ProcessKRateVector(
   const float incr = frequency * rate_scale;
   DCHECK_GE(incr, kInterpolate2Point);
 
-  periodic_wave_->WaveDataForFundamentalFrequency(
+  periodic_wave->WaveDataForFundamentalFrequency(
       frequency, lower_wave_data, higher_wave_data, table_interpolation_factor);
 
   const float32x4_t v_wave_size = vdupq_n_f32(periodic_wave_size);

third_party/blink/renderer/modules/webaudio/cpu/x86/oscillator_kernel_sse2.cc

@@ -46,7 +46,8 @@ std::tuple<int, double> OscillatorHandler::ProcessKRateVector(
     double virtual_read_index,
     float frequency,
     float rate_scale) const {
-  const unsigned periodic_wave_size = periodic_wave_->PeriodicWaveSize();
+  auto periodic_wave = periodic_wave_.Lock();
+  const unsigned periodic_wave_size = periodic_wave->PeriodicWaveSize();
   const double inv_periodic_wave_size = 1.0 / periodic_wave_size;
 
   float* higher_wave_data = nullptr;
@@ -55,7 +56,7 @@ std::tuple<int, double> OscillatorHandler::ProcessKRateVector(
   float incr = frequency * rate_scale;
   DCHECK_GE(incr, kInterpolate2Point);
 
-  periodic_wave_->WaveDataForFundamentalFrequency(
+  periodic_wave->WaveDataForFundamentalFrequency(
       frequency, lower_wave_data, higher_wave_data, table_interpolation_factor);
 
   const __m128 v_wave_size = _mm_set1_ps(periodic_wave_size);

third_party/blink/renderer/modules/webaudio/media_stream_audio_destination_node.cc

@@ -33,6 +33,7 @@
 #include "third_party/blink/renderer/platform/bindings/exception_messages.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
 #include "third_party/blink/renderer/platform/heap/heap.h"
+#include "third_party/blink/renderer/platform/heap/persistent.h"
 #include "third_party/blink/renderer/platform/instrumentation/tracing/trace_event.h"
 #include "third_party/blink/renderer/platform/wtf/uuid.h"
 
@@ -63,7 +64,8 @@ MediaStreamAudioDestinationHandler::MediaStreamAudioDestinationHandler(
       source_(static_cast<MediaStreamAudioDestinationNode&>(node).source()),
       mix_bus_(AudioBus::Create(number_of_channels,
                                 audio_utilities::kRenderQuantumFrames)) {
-  source_->SetAudioFormat(number_of_channels, node.context()->sampleRate());
+  source_.Lock()->SetAudioFormat(number_of_channels,
+                                 node.context()->sampleRate());
   SetInternalChannelCountMode(kExplicit);
   Initialize();
 }
@@ -89,6 +91,8 @@ void MediaStreamAudioDestinationHandler::Process(uint32_t number_of_frames) {
   // Synchronize with possible dynamic changes to the channel count.
   MutexTryLocker try_locker(process_lock_);
 
+  auto source = source_.Lock();
+
   // If we can get the lock, we can process normally by updating the
   // mix bus to a new channel count, if needed.  If not, just use the
   // old mix bus to do the mixing; we'll update the bus next time
@@ -99,7 +103,7 @@ void MediaStreamAudioDestinationHandler::Process(uint32_t number_of_frames) {
       mix_bus_ = AudioBus::Create(count, audio_utilities::kRenderQuantumFrames);
       // setAudioFormat has an internal lock.  This can cause audio to
       // glitch.  This is outside of our control.
-      source_->SetAudioFormat(count, Context()->sampleRate());
+      source->SetAudioFormat(count, Context()->sampleRate());
     }
   }
 
@@ -107,7 +111,7 @@ void MediaStreamAudioDestinationHandler::Process(uint32_t number_of_frames) {
 
   // consumeAudio has an internal lock (also used by setAudioFormat).
   // This can cause audio to glitch.  This is outside of our control.
-  source_->ConsumeAudio(mix_bus_.get(), number_of_frames);
+  source->ConsumeAudio(mix_bus_.get(), number_of_frames);
 }
 
 void MediaStreamAudioDestinationHandler::SetChannelCount(

third_party/blink/renderer/modules/webaudio/oscillator_node.cc

@@ -34,6 +34,7 @@
 #include "third_party/blink/renderer/platform/audio/vector_math.h"
 #include "third_party/blink/renderer/platform/bindings/enumeration_base.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
+#include "third_party/blink/renderer/platform/heap/persistent.h"
 #include "third_party/blink/renderer/platform/wtf/math_extras.h"
 #include "third_party/blink/renderer/platform/wtf/std_lib_extras.h"
 
@@ -197,7 +198,7 @@ bool OscillatorHandler::CalculateSampleAccuratePhaseIncrements(
   bool has_frequency_changes = false;
   float* phase_increments = phase_increments_.Data();
 
-  float final_scale = periodic_wave_->RateScale();
+  float final_scale = periodic_wave_.Lock()->RateScale();
 
   if (frequency_->HasSampleAccurateValues() && frequency_->IsAudioRate()) {
     has_sample_accurate_values = true;
@@ -437,7 +438,8 @@ double OscillatorHandler::ProcessKRateScalar(int start,
                                              double virtual_read_index,
                                              float frequency,
                                              float rate_scale) const {
-  const unsigned periodic_wave_size = periodic_wave_->PeriodicWaveSize();
+  auto periodic_wave = periodic_wave_.Lock();
+  const unsigned periodic_wave_size = periodic_wave->PeriodicWaveSize();
   const double inv_periodic_wave_size = 1.0 / periodic_wave_size;
   const unsigned read_index_mask = periodic_wave_size - 1;
 
@@ -445,7 +447,7 @@ double OscillatorHandler::ProcessKRateScalar(int start,
   float* lower_wave_data = nullptr;
   float table_interpolation_factor = 0;
 
-  periodic_wave_->WaveDataForFundamentalFrequency(
+  periodic_wave->WaveDataForFundamentalFrequency(
       frequency, lower_wave_data, higher_wave_data, table_interpolation_factor);
 
   const float incr = frequency * rate_scale;
@@ -491,7 +493,8 @@ double OscillatorHandler::ProcessKRateScalar(int start,
 double OscillatorHandler::ProcessKRate(int n,
                                        float* dest_p,
                                        double virtual_read_index) const {
-  const unsigned periodic_wave_size = periodic_wave_->PeriodicWaveSize();
+  auto periodic_wave = periodic_wave_.Lock();
+  const unsigned periodic_wave_size = periodic_wave->PeriodicWaveSize();
   const double inv_periodic_wave_size = 1.0 / periodic_wave_size;
   const unsigned read_index_mask = periodic_wave_size - 1;
 
@@ -503,10 +506,10 @@ double OscillatorHandler::ProcessKRate(int n,
   const float detune_scale = DetuneToFrequencyMultiplier(detune_->FinalValue());
   frequency *= detune_scale;
   ClampFrequency(&frequency, 1, Context()->sampleRate() / 2);
-  periodic_wave_->WaveDataForFundamentalFrequency(
+  periodic_wave->WaveDataForFundamentalFrequency(
       frequency, lower_wave_data, higher_wave_data, table_interpolation_factor);
 
-  const float rate_scale = periodic_wave_->RateScale();
+  const float rate_scale = periodic_wave->RateScale();
   const float incr = frequency * rate_scale;
 
   if (incr >= kInterpolate2Point) {
@@ -552,9 +555,10 @@ std::tuple<int, double> OscillatorHandler::ProcessARateVector(
     float* destination,
     double virtual_read_index,
     const float* phase_increments) const {
-  float rate_scale = periodic_wave_->RateScale();
+  auto periodic_wave = periodic_wave_.Lock();
+  float rate_scale = periodic_wave->RateScale();
   float inv_rate_scale = 1 / rate_scale;
-  unsigned periodic_wave_size = periodic_wave_->PeriodicWaveSize();
+  unsigned periodic_wave_size = periodic_wave->PeriodicWaveSize();
   double inv_periodic_wave_size = 1.0 / periodic_wave_size;
   unsigned read_index_mask = periodic_wave_size - 1;
 
@@ -576,9 +580,9 @@ std::tuple<int, double> OscillatorHandler::ProcessARateVector(
       frequency[m] = inv_rate_scale * phase_incr;
     }
 
-    periodic_wave_->WaveDataForFundamentalFrequency(frequency, lower_wave_data,
-                                                    higher_wave_data,
-                                                    table_interpolation_factor);
+    periodic_wave->WaveDataForFundamentalFrequency(frequency, lower_wave_data,
+                                                   higher_wave_data,
+                                                   table_interpolation_factor);
 
     // If all the phase increments are large enough, we can use linear
     // interpolation with a possibly vectorized implementation.  If not, we need
@@ -616,9 +620,10 @@ double OscillatorHandler::ProcessARateScalar(
     float* destination,
     double virtual_read_index,
     const float* phase_increments) const {
-  float rate_scale = periodic_wave_->RateScale();
+  auto periodic_wave = periodic_wave_.Lock();
+  float rate_scale = periodic_wave->RateScale();
   float inv_rate_scale = 1 / rate_scale;
-  unsigned periodic_wave_size = periodic_wave_->PeriodicWaveSize();
+  unsigned periodic_wave_size = periodic_wave->PeriodicWaveSize();
   double inv_periodic_wave_size = 1.0 / periodic_wave_size;
   unsigned read_index_mask = periodic_wave_size - 1;
 
@@ -630,9 +635,9 @@ double OscillatorHandler::ProcessARateScalar(
     float incr = phase_increments[m];
 
     float frequency = inv_rate_scale * incr;
-    periodic_wave_->WaveDataForFundamentalFrequency(frequency, lower_wave_data,
-                                                    higher_wave_data,
-                                                    table_interpolation_factor);
+    periodic_wave->WaveDataForFundamentalFrequency(frequency, lower_wave_data,
+                                                   higher_wave_data,
+                                                   table_interpolation_factor);
 
     float sample = DoInterpolation(virtual_read_index, fabs(incr),
                                    read_index_mask, table_interpolation_factor,
@@ -684,8 +689,10 @@ void OscillatorHandler::Process(uint32_t frames_to_process) {
     return;
   }
 
+  auto periodic_wave = periodic_wave_.Lock();
+
   // We must access m_periodicWave only inside the lock.
-  if (!periodic_wave_.Get()) {
+  if (!periodic_wave.Get()) {
     output_bus->Zero();
     return;
   }
@@ -703,7 +710,7 @@ void OscillatorHandler::Process(uint32_t frames_to_process) {
     return;
   }
 
-  unsigned periodic_wave_size = periodic_wave_->PeriodicWaveSize();
+  unsigned periodic_wave_size = periodic_wave->PeriodicWaveSize();
 
   float* dest_p = output_bus->Channel(0)->MutableData();
 
@@ -712,7 +719,7 @@ void OscillatorHandler::Process(uint32_t frames_to_process) {
   // We keep virtualReadIndex double-precision since we're accumulating values.
   double virtual_read_index = virtual_read_index_;
 
-  float rate_scale = periodic_wave_->RateScale();
+  float rate_scale = periodic_wave->RateScale();
   bool has_sample_accurate_values =
       CalculateSampleAccuratePhaseIncrements(frames_to_process);
 
@@ -727,9 +734,9 @@ void OscillatorHandler::Process(uint32_t frames_to_process) {
     float detune_scale = DetuneToFrequencyMultiplier(detune);
     frequency *= detune_scale;
     ClampFrequency(&frequency, 1, Context()->sampleRate() / 2);
-    periodic_wave_->WaveDataForFundamentalFrequency(frequency, lower_wave_data,
-                                                    higher_wave_data,
-                                                    table_interpolation_factor);
+    periodic_wave->WaveDataForFundamentalFrequency(frequency, lower_wave_data,
+                                                   higher_wave_data,
+                                                   table_interpolation_factor);
   }
 
   float* phase_increments = phase_increments_.Data();

third_party/blink/renderer/platform/heap/persistent.h

@@ -750,6 +750,28 @@ class CrossThreadWeakPersistent
     Parent::operator=(other);
     return *this;
   }
+
+  // Create a CrossThreadPersistent that keeps the underlying object alive if
+  // there is still on set. Can be used to work with an object on a different
+  // thread than it was allocated. Note that CTP does not block threads from
+  // terminating, in which case the reference would still be invalid.
+  const CrossThreadPersistent<T> Lock() const {
+    return CrossThreadPersistent<T>(*this);
+  }
+
+  // Disallow directly using CrossThreadWeakPersistent. Users must go through
+  // CrossThreadPersistent to access the pointee. Note that this does not
+  // guarantee that the object is still alive at that point. Users must check
+  // the state of CTP manually before invoking any calls.
+  T* operator->() const = delete;
+  T& operator*() const = delete;
+  // TODO(mlippautz): Also hide the following calls:
+  // - Parent::Get;
+  // - Parent::operator T*;
+
+ private:
+  template <typename U>
+  friend class CrossThreadPersistent;
 };
 
 template <typename T>

third_party/blink/renderer/platform/heap/test/heap_thread_test.cc

@@ -248,7 +248,7 @@ class CrossThreadWeakPersistentTester : public AlternatingThreadTester {
     SwitchToWorkerThread();
 
     // Step 5: Make sure the weak persistent is cleared.
-    EXPECT_FALSE(object_.Get());
+    EXPECT_FALSE(object_);
     EXPECT_EQ(1, DestructorLockingObject::destructor_calls_);
 
     SwitchToWorkerThread();

third_party/blink/renderer/platform/peerconnection/rtc_event_log_output_sink_proxy.cc

@@ -25,7 +25,7 @@ bool RtcEventLogOutputSinkProxy::Write(const std::string& output) {
   WTF::Vector<uint8_t> converted_output;
   converted_output.AppendRange(output.begin(), output.end());
 
-  sink_->OnWebRtcEventLogWrite(converted_output);
+  sink_.Lock()->OnWebRtcEventLogWrite(converted_output);
   return true;
 }