Fix iterator invalidation issue

If a RemotePlayback availabilityCallback invokes watchAvailability(),
it may cause changes to the underlying |availability_callbacks_|. This
can invalidate the iterator we are using to loop over the callbacks.

This CL copies the callbacks to a vector before invoking them, allowing
them to add/remove callbacks without problem.

Bug: 1108497
Change-Id: I78220da0b8e10c1d6c0e4fa5e15ada81f10f8fc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2314981
Auto-Submit: Thomas Guilbert <tguilbert@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Commit-Queue: Thomas Guilbert <tguilbert@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791472}

third_party/blink/renderer/modules/remoteplayback/remote_playback.cc

@@ -502,7 +502,12 @@ void RemotePlayback::AvailabilityChanged(
   if (new_availability == old_availability)
     return;
 
-  for (auto& callback : availability_callbacks_.Values())
+  // Copy the callbacks to a temporary vector to prevent iterator invalidations,
+  // in case the JS callbacks invoke watchAvailability().
+  HeapVector<Member<AvailabilityCallbackWrapper>> callbacks;
+  CopyValuesToVector(availability_callbacks_, callbacks);
+
+  for (auto& callback : callbacks)
     callback->Run(this, new_availability);
 }
 

third_party/blink/renderer/modules/remoteplayback/remote_playback_test.cc

@@ -319,6 +319,59 @@ TEST_F(RemotePlaybackTest, DisableRemotePlaybackCancelsAvailabilityCallbacks) {
   testing::Mock::VerifyAndClear(callback_function);
 }
 
+TEST_F(RemotePlaybackTest, CallingWatchAvailabilityFromAvailabilityCallback) {
+  V8TestingScope scope;
+
+  auto page_holder = std::make_unique<DummyPageHolder>();
+
+  auto* element =
+      MakeGarbageCollected<HTMLVideoElement>(page_holder->GetDocument());
+  RemotePlayback& remote_playback = RemotePlayback::From(*element);
+
+  MockFunction* callback_function =
+      MockFunction::Create(scope.GetScriptState());
+  V8RemotePlaybackAvailabilityCallback* availability_callback =
+      V8RemotePlaybackAvailabilityCallback::Create(callback_function->Bind());
+
+  const int kNumberCallbacks = 10;
+  for (int i = 0; i < kNumberCallbacks; ++i) {
+    remote_playback.watchAvailability(scope.GetScriptState(),
+                                      availability_callback);
+  }
+
+  auto add_callback_lambda = [&]() {
+    remote_playback.watchAvailability(scope.GetScriptState(),
+                                      availability_callback);
+    return blink::ScriptValue::CreateNull(scope.GetScriptState()->GetIsolate());
+  };
+
+  // When the availability changes, we should get exactly kNumberCallbacks
+  // calls, due to the kNumberCallbacks initial current callbacks. The extra
+  // callbacks we are adding should not be executed.
+  EXPECT_CALL(*callback_function, Call(testing::_))
+      .Times(kNumberCallbacks)
+      .WillRepeatedly(testing::InvokeWithoutArgs(add_callback_lambda));
+
+  remote_playback.AvailabilityChangedForTesting(true);
+
+  v8::MicrotasksScope::PerformCheckpoint(scope.GetIsolate());
+  testing::Mock::VerifyAndClear(callback_function);
+
+  // We now have twice as many callbacks as we started with, and should get
+  // twice as many calls, but no more.
+  EXPECT_CALL(*callback_function, Call(testing::_))
+      .Times(kNumberCallbacks * 2)
+      .WillRepeatedly(testing::InvokeWithoutArgs(add_callback_lambda));
+
+  remote_playback.AvailabilityChangedForTesting(false);
+
+  v8::MicrotasksScope::PerformCheckpoint(scope.GetIsolate());
+
+  // Verify mock expectations explicitly as the mock objects are garbage
+  // collected.
+  testing::Mock::VerifyAndClear(callback_function);
+}
+
 TEST_F(RemotePlaybackTest, PromptThrowsWhenBackendDisabled) {
   ScopedRemotePlaybackBackendForTest remote_playback_backend(false);
   V8TestingScope scope;