From 0bd1d986cb4babeeeea9d474334ea186f091d435 Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Tue, 7 Apr 2020 17:36:29 -0700 Subject: [PATCH 1/3] chore: cherry-pick 4c57222340cf from chromium --- patches/chromium/.patches | 1 + .../chromium/cherry-pick-4c57222340cf.patch | 77 +++++++++++++++++++ 2 files changed, 78 insertions(+) create mode 100644 patches/chromium/cherry-pick-4c57222340cf.patch diff --git a/patches/chromium/.patches b/patches/chromium/.patches index f640842db11ca..a30afcbdb96d5 100644 --- a/patches/chromium/.patches +++ b/patches/chromium/.patches @@ -100,3 +100,4 @@ move_readablestream_requests_onto_the_stack_before_iteration.patch streams_convert_state_dchecks_to_checks.patch audiocontext_haspendingactivity_unless_it_s_closed.patch protect_automatic_pull_handlers_with_mutex.patch +cherry-pick-4c57222340cf.patch diff --git a/patches/chromium/cherry-pick-4c57222340cf.patch b/patches/chromium/cherry-pick-4c57222340cf.patch new file mode 100644 index 0000000000000..74d39b0e0e048 --- /dev/null +++ b/patches/chromium/cherry-pick-4c57222340cf.patch @@ -0,0 +1,77 @@ +From 4c57222340cfb78edadf08532f4468c676df5395 Mon Sep 17 00:00:00 2001 +From: Raymond Toy +Date: Wed, 11 Mar 2020 20:23:49 +0000 +Subject: [PATCH] Make finished_source_handlers_ hold scoped_refptrs + +Previously, finished_source_handlers_ held raw pointers to +AudioHandlers and assumed that active_source_handlers_ also had a +copy. But when the context goes away, active_source_handlers_ would +be cleared, but not finished_source_handlers_, leaving pointers to +deleted objects. + +So do two things: +1. Change finished_source_handlers_ to hold scoped_refptrs to manage + lifetime of the objects +2. Clear finished_source_handler_ in ClearHandlersToBeDeleted() + +Either of these fix the repro case, but let's do both. Don't want to +leaving dangling objects. + +Manually tested the repro case which no longer reproduces. + +Bug: 1059686 +Change-Id: I2f30c996e8589fa5c3890d32500c4bb4f3bc4286 +Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2098260 +Reviewed-by: Hongchan Choi +Commit-Queue: Raymond Toy +Cr-Commit-Position: refs/heads/master@{#749302} +--- + .../blink/renderer/modules/webaudio/deferred_task_handler.cc | 5 ++--- + .../blink/renderer/modules/webaudio/deferred_task_handler.h | 4 ++-- + 2 files changed, 4 insertions(+), 5 deletions(-) + +diff --git a/third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc b/third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc +index d0cbb762a8de9..d02a7f69fe191 100644 +--- a/third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc ++++ b/third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc +@@ -77,9 +77,7 @@ void DeferredTaskHandler::BreakConnections() { + // connection. + wtf_size_t size = finished_source_handlers_.size(); + if (size > 0) { +- for (auto* finished : finished_source_handlers_) { +- // Break connection first and then remove from the list because that can +- // cause the handler to be deleted. ++ for (auto finished : finished_source_handlers_) { + finished->BreakConnectionWithLock(); + active_source_handlers_.erase(finished); + } +@@ -358,6 +356,7 @@ void DeferredTaskHandler::ClearHandlersToBeDeleted() { + deletable_orphan_handlers_.clear(); + automatic_pull_handlers_.clear(); + rendering_automatic_pull_handlers_.clear(); ++ finished_source_handlers_.clear(); + active_source_handlers_.clear(); + } + +diff --git a/third_party/blink/renderer/modules/webaudio/deferred_task_handler.h b/third_party/blink/renderer/modules/webaudio/deferred_task_handler.h +index 9d2ae46db53ff..3cf47ca77a8b1 100644 +--- a/third_party/blink/renderer/modules/webaudio/deferred_task_handler.h ++++ b/third_party/blink/renderer/modules/webaudio/deferred_task_handler.h +@@ -190,7 +190,7 @@ class MODULES_EXPORT DeferredTaskHandler final + return &active_source_handlers_; + } + +- Vector* GetFinishedSourceHandlers() { ++ Vector>* GetFinishedSourceHandlers() { + return &finished_source_handlers_; + } + +@@ -259,7 +259,7 @@ class MODULES_EXPORT DeferredTaskHandler final + // connection and elements here are removed from |active_source_handlers_|. + // + // This must be accessed only from the audio thread. +- Vector finished_source_handlers_; ++ Vector> finished_source_handlers_; + + scoped_refptr task_runner_; + From 508f9294ca8a0add46bc67162401401fb6154b13 Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Thu, 9 Apr 2020 13:08:52 -0700 Subject: [PATCH 2/3] resolve conflicts --- patches/chromium/.patches | 2 +- ...source_handlers_hold_scoped_refptrs.patch} | 29 +++++++++---------- 2 files changed, 14 insertions(+), 17 deletions(-) rename patches/chromium/{cherry-pick-4c57222340cf.patch => make_finished_source_handlers_hold_scoped_refptrs.patch} (74%) diff --git a/patches/chromium/.patches b/patches/chromium/.patches index a30afcbdb96d5..f8744564a644f 100644 --- a/patches/chromium/.patches +++ b/patches/chromium/.patches @@ -100,4 +100,4 @@ move_readablestream_requests_onto_the_stack_before_iteration.patch streams_convert_state_dchecks_to_checks.patch audiocontext_haspendingactivity_unless_it_s_closed.patch protect_automatic_pull_handlers_with_mutex.patch -cherry-pick-4c57222340cf.patch +make_finished_source_handlers_hold_scoped_refptrs.patch diff --git a/patches/chromium/cherry-pick-4c57222340cf.patch b/patches/chromium/make_finished_source_handlers_hold_scoped_refptrs.patch similarity index 74% rename from patches/chromium/cherry-pick-4c57222340cf.patch rename to patches/chromium/make_finished_source_handlers_hold_scoped_refptrs.patch index 74d39b0e0e048..804140aaff37f 100644 --- a/patches/chromium/cherry-pick-4c57222340cf.patch +++ b/patches/chromium/make_finished_source_handlers_hold_scoped_refptrs.patch @@ -1,7 +1,7 @@ -From 4c57222340cfb78edadf08532f4468c676df5395 Mon Sep 17 00:00:00 2001 +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Raymond Toy Date: Wed, 11 Mar 2020 20:23:49 +0000 -Subject: [PATCH] Make finished_source_handlers_ hold scoped_refptrs +Subject: Make finished_source_handlers_ hold scoped_refptrs Previously, finished_source_handlers_ held raw pointers to AudioHandlers and assumed that active_source_handlers_ also had a @@ -25,39 +25,36 @@ Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2098260 Reviewed-by: Hongchan Choi Commit-Queue: Raymond Toy Cr-Commit-Position: refs/heads/master@{#749302} ---- - .../blink/renderer/modules/webaudio/deferred_task_handler.cc | 5 ++--- - .../blink/renderer/modules/webaudio/deferred_task_handler.h | 4 ++-- - 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc b/third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc -index d0cbb762a8de9..d02a7f69fe191 100644 +index 6ecedccc14ed15b81e916f0e1ff8f635489dcecc..1cbae52a782e71773684af932f9fc5c554d0caf8 100644 --- a/third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc +++ b/third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc -@@ -77,9 +77,7 @@ void DeferredTaskHandler::BreakConnections() { +@@ -77,9 +77,9 @@ void DeferredTaskHandler::BreakConnections() { // connection. wtf_size_t size = finished_source_handlers_.size(); if (size > 0) { - for (auto* finished : finished_source_handlers_) { -- // Break connection first and then remove from the list because that can -- // cause the handler to be deleted. +- active_source_handlers_.erase(finished); + for (auto finished : finished_source_handlers_) { finished->BreakConnectionWithLock(); - active_source_handlers_.erase(finished); ++ active_source_handlers_.erase(finished); } -@@ -358,6 +356,7 @@ void DeferredTaskHandler::ClearHandlersToBeDeleted() { + finished_source_handlers_.clear(); + } +@@ -370,6 +370,7 @@ void DeferredTaskHandler::ClearHandlersToBeDeleted() { + rendering_orphan_handlers_.clear(); deletable_orphan_handlers_.clear(); automatic_pull_handlers_.clear(); - rendering_automatic_pull_handlers_.clear(); + finished_source_handlers_.clear(); active_source_handlers_.clear(); } diff --git a/third_party/blink/renderer/modules/webaudio/deferred_task_handler.h b/third_party/blink/renderer/modules/webaudio/deferred_task_handler.h -index 9d2ae46db53ff..3cf47ca77a8b1 100644 +index af384d0941da47bbe544be3c0ce5739ca4c384e1..5fbc449639c5c04c7ae85c9b65047a8e28bb2a2e 100644 --- a/third_party/blink/renderer/modules/webaudio/deferred_task_handler.h +++ b/third_party/blink/renderer/modules/webaudio/deferred_task_handler.h -@@ -190,7 +190,7 @@ class MODULES_EXPORT DeferredTaskHandler final +@@ -188,7 +188,7 @@ class MODULES_EXPORT DeferredTaskHandler final return &active_source_handlers_; } @@ -66,7 +63,7 @@ index 9d2ae46db53ff..3cf47ca77a8b1 100644 return &finished_source_handlers_; } -@@ -259,7 +259,7 @@ class MODULES_EXPORT DeferredTaskHandler final +@@ -257,7 +257,7 @@ class MODULES_EXPORT DeferredTaskHandler final // connection and elements here are removed from |active_source_handlers_|. // // This must be accessed only from the audio thread. From 3917e83cd9183f0a7c66ec1779959235e8a39f5b Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Thu, 9 Apr 2020 14:36:58 -0700 Subject: [PATCH 3/3] fix patch --- patches/chromium/.patches | 2 +- ...nished_source_handlers_hold_scoped_refptrs.patch | 13 ++++++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/patches/chromium/.patches b/patches/chromium/.patches index 801611e44d69f..2d1b661d950ab 100644 --- a/patches/chromium/.patches +++ b/patches/chromium/.patches @@ -101,8 +101,8 @@ streams_convert_state_dchecks_to_checks.patch -_point_usrsctp_to_a68325e7d9ed844cc84ec134192d788586ea6cc1.patch audiocontext_haspendingactivity_unless_it_s_closed.patch protect_automatic_pull_handlers_with_mutex.patch -make_finished_source_handlers_hold_scoped_refptrs.patch break_connections_before_removing_from_active_source_handlers.patch +make_finished_source_handlers_hold_scoped_refptrs.patch mojovideoencodeacceleratorservice_handle_potential_later.patch speculative_fix_for_crashes_in_filechooserimpl.patch reland_sequentialise_access_to_callbacks_in.patch diff --git a/patches/chromium/make_finished_source_handlers_hold_scoped_refptrs.patch b/patches/chromium/make_finished_source_handlers_hold_scoped_refptrs.patch index 804140aaff37f..2945ff7cff3dc 100644 --- a/patches/chromium/make_finished_source_handlers_hold_scoped_refptrs.patch +++ b/patches/chromium/make_finished_source_handlers_hold_scoped_refptrs.patch @@ -27,22 +27,21 @@ Commit-Queue: Raymond Toy Cr-Commit-Position: refs/heads/master@{#749302} diff --git a/third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc b/third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc -index 6ecedccc14ed15b81e916f0e1ff8f635489dcecc..1cbae52a782e71773684af932f9fc5c554d0caf8 100644 +index b4a376aa8f5cb06efcc71032ed089db46dadf902..1cbae52a782e71773684af932f9fc5c554d0caf8 100644 --- a/third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc +++ b/third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc -@@ -77,9 +77,9 @@ void DeferredTaskHandler::BreakConnections() { +@@ -77,9 +77,7 @@ void DeferredTaskHandler::BreakConnections() { // connection. wtf_size_t size = finished_source_handlers_.size(); if (size > 0) { - for (auto* finished : finished_source_handlers_) { -- active_source_handlers_.erase(finished); +- // Break connection first and then remove from the list because that can +- // cause the handler to be deleted. + for (auto finished : finished_source_handlers_) { finished->BreakConnectionWithLock(); -+ active_source_handlers_.erase(finished); + active_source_handlers_.erase(finished); } - finished_source_handlers_.clear(); - } -@@ -370,6 +370,7 @@ void DeferredTaskHandler::ClearHandlersToBeDeleted() { +@@ -372,6 +370,7 @@ void DeferredTaskHandler::ClearHandlersToBeDeleted() { rendering_orphan_handlers_.clear(); deletable_orphan_handlers_.clear(); automatic_pull_handlers_.clear();