MSE-in-Workers: Fix leak of HTMLME+MSE due to strong attachment ref
This change releases the MediaSourceHandle's reference to the CrossThreadMediaSourceAttachment when it is retrieved via TakeAttachment() (formerly named GetAttachment()).
TakeAttachment() is only called from 2 places:
1) HTMLMediaElement::LoadResource : if the retrieved attachment is
nullptr, then invalid attempt to reuse the attachment is detected
and usual load failure occurs.
2) Serialization of MediaSourceHandle : if the handle has already been
serialized or assigned as srcObject, then TakeAttachment() won't be
called.
The fix for the leak also adjusts the code to handle (1), allowing for
the possibility of a nullptr-valued attachment being retrieved that
leads to media element load failure.
Without this change, if a MediaSourceHandle has been used to attach
via srcObject ever, then the underlying CrossThreadMediaSourceAttachment
will have persistent Oilpan references to both the worker MediaSource and the main thread HTMLMediaElement. Those are only dropped if the refcounted attachment itself is destructed, which cannot happen if there remains any strong reference to it.
BUG=878133
TEST=local build with srcObject part 5 included with this no longer
reproduces the leak failure that caused revert [1] of part 5.
[1] https://chromium-review.googlesource.com/c/chromium/src/+/3695890
Change-Id: I1403e09dffdfddd4eae240b8e2345bd4672e874a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3704191
Auto-Submit: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1014736}
diff --git a/third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules.cc b/third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules.cc
index 46e9625..cc225c4 100644
--- a/third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules.cc
+++ b/third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules.cc
@@ -594,8 +594,15 @@
// MediaSourceHandleImpl. Add the internal state of |handle| to it and
// serialize it using the index of that state in the vector.
auto& attachments = attachment->Attachments();
+
+ scoped_refptr<MediaSourceAttachment> media_source_attachment =
+ handle->TakeAttachment();
+ // The two handle checks, above, (!is_serialized() and !is_used()) should
+ // prevent us from ever having a missing |media_source_attachment| here.
+ DCHECK(media_source_attachment);
+
attachments.push_back(MediaSourceHandleAttachment::HandleInternals{
- .attachment = handle->GetAttachment(),
+ .attachment = std::move(media_source_attachment),
.internal_blob_url = handle->GetInternalBlobURL()});
handle->mark_serialized();
const uint32_t index = static_cast<uint32_t>(attachments.size() - 1);
diff --git a/third_party/blink/renderer/core/html/media/html_media_element.cc b/third_party/blink/renderer/core/html/media/html_media_element.cc
index fa373a5..a033cf9 100644
--- a/third_party/blink/renderer/core/html/media/html_media_element.cc
+++ b/third_party/blink/renderer/core/html/media/html_media_element.cc
@@ -1383,8 +1383,13 @@
bool attempt_load = true;
if (src_object_media_source_handle_) {
- media_source_attachment_ = src_object_media_source_handle_->GetAttachment();
- DCHECK(media_source_attachment_);
+ media_source_attachment_ =
+ src_object_media_source_handle_->TakeAttachment();
+
+ // If the attachment is nullptr, then fail the load.
+ if (!media_source_attachment_) {
+ attempt_load = false;
+ }
} else {
media_source_attachment_ =
MediaSourceAttachment::LookupMediaSource(url.GetString());
diff --git a/third_party/blink/renderer/core/html/media/media_source_handle.h b/third_party/blink/renderer/core/html/media/media_source_handle.h
index acccad95..141ce60 100644
--- a/third_party/blink/renderer/core/html/media/media_source_handle.h
+++ b/third_party/blink/renderer/core/html/media/media_source_handle.h
@@ -18,7 +18,9 @@
MediaSourceHandle& operator=(const MediaSourceHandle&) = delete;
virtual ~MediaSourceHandle() = default;
- virtual scoped_refptr<MediaSourceAttachment> GetAttachment() = 0;
+ // Removes our reference on the attachment, giving it to the caller.
+ virtual scoped_refptr<MediaSourceAttachment> TakeAttachment() = 0;
+
virtual String GetInternalBlobURL() = 0;
void mark_used() { used_ = true; }
diff --git a/third_party/blink/renderer/modules/mediasource/media_source_handle_impl.cc b/third_party/blink/renderer/modules/mediasource/media_source_handle_impl.cc
index f59a2d6..5f83ec6 100644
--- a/third_party/blink/renderer/modules/mediasource/media_source_handle_impl.cc
+++ b/third_party/blink/renderer/modules/mediasource/media_source_handle_impl.cc
@@ -26,8 +26,8 @@
DVLOG(1) << __func__ << " this=" << this;
}
-scoped_refptr<MediaSourceAttachment> MediaSourceHandleImpl::GetAttachment() {
- return attachment_;
+scoped_refptr<MediaSourceAttachment> MediaSourceHandleImpl::TakeAttachment() {
+ return std::move(attachment_);
}
String MediaSourceHandleImpl::GetInternalBlobURL() {
@@ -37,6 +37,13 @@
void MediaSourceHandleImpl::mark_serialized() {
DCHECK(!serialized_);
serialized_ = true;
+
+ // Before being serialized, the serialization must have retrieved our
+ // reference to the |attachment_| precisely once. Note that immediately upon
+ // an instance of us being assigned to srcObject, that instance can no longer
+ // be serialized and there will be at most one async media element load that
+ // retrieves our attachment reference.
+ DCHECK(!attachment_);
}
void MediaSourceHandleImpl::Trace(Visitor* visitor) const {
diff --git a/third_party/blink/renderer/modules/mediasource/media_source_handle_impl.h b/third_party/blink/renderer/modules/mediasource/media_source_handle_impl.h
index 62bf6247..bd5ab93 100644
--- a/third_party/blink/renderer/modules/mediasource/media_source_handle_impl.h
+++ b/third_party/blink/renderer/modules/mediasource/media_source_handle_impl.h
@@ -23,7 +23,7 @@
String internal_blob_url);
~MediaSourceHandleImpl() override;
- scoped_refptr<MediaSourceAttachment> GetAttachment() override;
+ scoped_refptr<MediaSourceAttachment> TakeAttachment() override;
String GetInternalBlobURL() override;
void mark_serialized();
