[BreakoutBox] Invalidate frames when sending to the sink

This CL makes sure that the destroy method is automatically invoked on
VideoFrames when they are sent to a sink.
This automatic invalidation is important because there are cases when
frames are automatically sent to a sink without the possibility of
manual invalidation (e.g., when automatically piping a readable stream
to a writable stream).
This is also consistent with the automatic invalidation in
WebRTC Insertable Streams.

Drive-by: Re-enable a disabled test that flaked because of
a missing call to wait for a promise to be settled.

Bug: 1151913,1153092
Change-Id: I3bc40df3bb02a1ae9852b03af2c94898400a2d4d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587037
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836251}

third_party/blink/renderer/modules/mediastream/media_stream_video_track_underlying_sink.cc

@@ -29,7 +29,13 @@ ScriptPromise MediaStreamVideoTrackUnderlyingSink::write(
   VideoFrame* video_frame = V8VideoFrame::ToImplWithTypeCheck(
       script_state->GetIsolate(), chunk.V8Value());
   if (!video_frame) {
-    exception_state.ThrowTypeError("Invalid video frame.");
+    exception_state.ThrowTypeError("Null video frame.");
+    return ScriptPromise();
+  }
+
+  if (!video_frame->frame()) {
+    exception_state.ThrowDOMException(DOMExceptionCode::kOperationError,
+                                      "Empty video frame.");
     return ScriptPromise();
   }
 
@@ -41,6 +47,10 @@ ScriptPromise MediaStreamVideoTrackUnderlyingSink::write(
 
   base::TimeTicks estimated_capture_time = base::TimeTicks::Now();
   source_->PushFrame(video_frame->frame(), estimated_capture_time);
+  // Invalidate the JS |video_frame|. Otherwise, the media frames might not be
+  // released, which would leak resources and also cause some MediaStream
+  // sources such as cameras to drop frames.
+  video_frame->destroy();
 
   return ScriptPromise::CastUndefined(script_state);
 }

third_party/blink/renderer/modules/mediastream/media_stream_video_track_underlying_sink_test.cc

@@ -53,11 +53,14 @@ class MediaStreamVideoTrackUnderlyingSinkTest : public testing::Test {
         /*enabled=*/true);
   }
 
-  ScriptValue CreateVideoFrameChunk(ScriptState* script_state) {
+  ScriptValue CreateVideoFrameChunk(ScriptState* script_state,
+                                    VideoFrame** video_frame_out = nullptr) {
     const scoped_refptr<media::VideoFrame> media_frame =
         media::VideoFrame::CreateBlackFrame(gfx::Size(100, 50));
     VideoFrame* video_frame = MakeGarbageCollected<VideoFrame>(
         std::move(media_frame), ExecutionContext::From(script_state));
+    if (video_frame_out)
+      *video_frame_out = video_frame;
     return ScriptValue(script_state->GetIsolate(),
                        ToV8(video_frame, script_state->GetContext()->Global(),
                             script_state->GetIsolate()));
@@ -69,9 +72,8 @@ class MediaStreamVideoTrackUnderlyingSinkTest : public testing::Test {
   PushableMediaStreamVideoSource* pushable_video_source_;
 };
 
-// crbug.com/1153092: flaky on several platforms.
 TEST_F(MediaStreamVideoTrackUnderlyingSinkTest,
-       DISABLED_WriteToStreamForwardsToMediaStreamSink) {
+       WriteToStreamForwardsToMediaStreamSink) {
   V8TestingScope v8_scope;
   ScriptState* script_state = v8_scope.GetScriptState();
   auto* underlying_sink = CreateUnderlyingSink(script_state);
@@ -85,12 +87,17 @@ TEST_F(MediaStreamVideoTrackUnderlyingSinkTest,
   NonThrowableExceptionState exception_state;
   auto* writer = writable_stream->getWriter(script_state, exception_state);
 
+  VideoFrame* video_frame = nullptr;
+  auto video_frame_chunk = CreateVideoFrameChunk(script_state, &video_frame);
+  EXPECT_NE(video_frame, nullptr);
+  EXPECT_NE(video_frame->frame(), nullptr);
   EXPECT_CALL(media_stream_video_sink, OnVideoFrame(_));
   ScriptPromiseTester write_tester(
       script_state,
-      writer->write(script_state, CreateVideoFrameChunk(script_state),
-                    exception_state));
-  EXPECT_FALSE(write_tester.IsFulfilled());
+      writer->write(script_state, video_frame_chunk, exception_state));
+  write_tester.WaitUntilSettled();
+  // |video_frame| should be invalidated after sending it to the sink.
+  EXPECT_EQ(video_frame->frame(), nullptr);
 
   writer->releaseLock(script_state);
   ScriptPromiseTester close_tester(
@@ -116,6 +123,23 @@ TEST_F(MediaStreamVideoTrackUnderlyingSinkTest, WriteInvalidDataFails) {
   DummyExceptionStateForTesting dummy_exception_state;
   sink->write(script_state, v8_integer, nullptr, dummy_exception_state);
   EXPECT_TRUE(dummy_exception_state.HadException());
+
+  // Writing something that is not a VideoFrame to the sink should fail.
+  dummy_exception_state.ClearException();
+  EXPECT_FALSE(dummy_exception_state.HadException());
+  sink->write(script_state, ScriptValue::CreateNull(v8_scope.GetIsolate()),
+              nullptr, dummy_exception_state);
+  EXPECT_TRUE(dummy_exception_state.HadException());
+
+  // Writing a destroyed VideoFrame to the sink should fail.
+  dummy_exception_state.ClearException();
+  VideoFrame* video_frame = nullptr;
+  auto chunk = CreateVideoFrameChunk(script_state, &video_frame);
+  video_frame->destroy();
+  EXPECT_FALSE(dummy_exception_state.HadException());
+  sink->write(script_state, ScriptValue::CreateNull(v8_scope.GetIsolate()),
+              nullptr, dummy_exception_state);
+  EXPECT_TRUE(dummy_exception_state.HadException());
 }
 
 TEST_F(MediaStreamVideoTrackUnderlyingSinkTest, WriteToAbortedSinkFails) {