Skip to content

Commit

Permalink
chore: cherry-pick 4 changes from Release-3-M114 (#38949)
Browse files Browse the repository at this point in the history
* chore: [22-x-y] cherry-pick 4 changes from Release-3-M114

* 85beff6fd302 from chromium
* 60b93798c991 from chromium
* a1efa5343880 from v8
* d20849d07107 from webrtc

* chore: update patches

---------

Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
  • Loading branch information
ppontes and patchup[bot] committed Jul 6, 2023
1 parent 4ade2a6 commit 1f91895
Show file tree
Hide file tree
Showing 9 changed files with 552 additions and 0 deletions.
2 changes: 2 additions & 0 deletions patches/chromium/.patches
Original file line number Diff line number Diff line change
Expand Up @@ -164,3 +164,5 @@ cherry-pick-48785f698b1c.patch
m108-lts_return_after_readycommitnavigation_call_in_commiterrorpage.patch
m114_merge_fix_a_crash_caused_by_calling_trace_event.patch
base_do_not_use_va_args_twice_in_asprintf.patch
cherry-pick-85beff6fd302.patch
m114_webcodecs_fix_crash_when_changing_temporal_layer_count_in_av1.patch
215 changes: 215 additions & 0 deletions patches/chromium/cherry-pick-85beff6fd302.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,215 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Kevin McNee <mcnee@chromium.org>
Date: Wed, 14 Jun 2023 01:10:19 +0000
Subject: M114: Don't recursively destroy guests when clearing unattached
guests

Don't recursively destroy guests when clearing unattached guests

When an embedder process is destroyed, we also destroy any unattached
guests associated with that process. This is currently done with a
single call to `owned_guests_.erase`. However, it's possible that two
unattached guests could have an opener relationship, which causes the
destruction of the opener guest to also destroy the other guest, during
the call to `erase`, which is unsafe.

We now separate the steps of erasing `owned_guests_` and destroying the
guests, to avoid this recursive guest destruction.

This also fixes the WaitForNumGuestsCreated test method to not
return prematurely.

(cherry picked from commit 6345e7871e8197af92f9c6158b06c6e197f87945)

Bug: 1450397
Change-Id: Ifef5ec9ff3a1e6952ff56ec279e29e8522625ac0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4589949
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Auto-Submit: Kevin McNee <mcnee@chromium.org>
Reviewed-by: James Maclean <wjmaclean@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1153396}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4611152
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/branch-heads/5735@{#1292}
Cr-Branched-From: 2f562e4ddbaf79a3f3cb338b4d1bd4398d49eb67-refs/heads/main@{#1135570}

diff --git a/chrome/browser/apps/guest_view/web_view_browsertest.cc b/chrome/browser/apps/guest_view/web_view_browsertest.cc
index 7159cf6af5cfd0ad5b9e5ba526043a4407a5399d..e43966f43f7ae551b3ea335a3f4222887071a75e 100644
--- a/chrome/browser/apps/guest_view/web_view_browsertest.cc
+++ b/chrome/browser/apps/guest_view/web_view_browsertest.cc
@@ -2731,6 +2731,22 @@ IN_PROC_BROWSER_TEST_P(WebViewNewWindowTest,
EXPECT_TRUE(content::NavigateToURLFromRenderer(guest2, coop_url));
}

+// This test creates a situation where we have two unattached webviews which
+// have an opener relationship, and ensures that we can shutdown safely. See
+// https://crbug.com/1450397.
+IN_PROC_BROWSER_TEST_P(WebViewNewWindowTest, DestroyOpenerBeforeAttachment) {
+ TestHelper("testDestroyOpenerBeforeAttachment", "web_view/newwindow",
+ NEEDS_TEST_SERVER);
+ GetGuestViewManager()->WaitForNumGuestsCreated(2);
+
+ content::RenderProcessHost* embedder_rph =
+ GetEmbedderWebContents()->GetPrimaryMainFrame()->GetProcess();
+ content::RenderProcessHostWatcher kill_observer(
+ embedder_rph, content::RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+ EXPECT_TRUE(embedder_rph->Shutdown(content::RESULT_CODE_KILLED));
+ kill_observer.Wait();
+}
+
IN_PROC_BROWSER_TEST_P(WebViewTest, ContextMenuInspectElement) {
LoadAppWithGuest("web_view/context_menus/basic");
content::RenderFrameHost* guest_rfh = GetGuestRenderFrameHost();
diff --git a/chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js b/chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js
index 900911f4963d23d74225868dce01326ba533f63a..4dd25d8849b0b13957ab7fa2912c0a158d3cd244 100644
--- a/chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js
+++ b/chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js
@@ -34,6 +34,9 @@ embedder.setUp_ = function(config) {
embedder.guestWithLinkURL = embedder.baseGuestURL +
'/extensions/platform_apps/web_view/newwindow' +
'/guest_with_link.html';
+ embedder.guestOpenOnLoadURL = embedder.baseGuestURL +
+ '/extensions/platform_apps/web_view/newwindow' +
+ '/guest_opener_open_on_load.html';
};

/** @private */
@@ -652,6 +655,24 @@ function testNewWindowDeferredAttachmentIndefinitely() {
embedder.setUpNewWindowRequest_(webview, 'guest.html', '', testName);
}

+// This is not a test in and of itself, but a means of creating a webview that
+// is left in an unattached state while its opener webview is also in an
+// unattached state, so that the C++ side can test it in that state.
+function testDestroyOpenerBeforeAttachment() {
+ embedder.test.succeed();
+
+ let webview = new WebView();
+ webview.src = embedder.guestOpenOnLoadURL;
+ document.body.appendChild(webview);
+
+ // By spinning forever here, we prevent `webview` from completing the
+ // attachment process. But since the guest is still created and it calls
+ // window.open, we have a situation where two unattached webviews have an
+ // opener relationship. The C++ side will test that we can shutdown safely in
+ // this case.
+ while (true) {}
+}
+
embedder.test.testList = {
'testNewWindowAttachAfterOpenerDestroyed':
testNewWindowAttachAfterOpenerDestroyed,
@@ -675,7 +696,9 @@ embedder.test.testList = {
testNewWindowWebViewNameTakesPrecedence,
'testNewWindowAndUpdateOpener': testNewWindowAndUpdateOpener,
'testNewWindowDeferredAttachmentIndefinitely':
- testNewWindowDeferredAttachmentIndefinitely
+ testNewWindowDeferredAttachmentIndefinitely,
+ 'testDestroyOpenerBeforeAttachment':
+ testDestroyOpenerBeforeAttachment
};

onload = function() {
diff --git a/chrome/test/data/extensions/platform_apps/web_view/newwindow/guest_opener_open_on_load.html b/chrome/test/data/extensions/platform_apps/web_view/newwindow/guest_opener_open_on_load.html
new file mode 100644
index 0000000000000000000000000000000000000000..e961feb3c6487066801adf414bf4a2746c50a3f6
--- /dev/null
+++ b/chrome/test/data/extensions/platform_apps/web_view/newwindow/guest_opener_open_on_load.html
@@ -0,0 +1,13 @@
+<!--
+Copyright 2023 The Chromium Authors
+Use of this source code is governed by a BSD-style license that can be
+found in the LICENSE file.
+-->
+<html>
+<body>
+<script>
+ // A guest that opens a new window on load.
+ window.open('guest.html');
+</script>
+</body>
+</html>
diff --git a/components/guest_view/browser/guest_view_manager.cc b/components/guest_view/browser/guest_view_manager.cc
index 38f0f12e65009c660a6dba262617d48c10ff72ea..129443365f474b840e2ddc61868e89af2851a892 100644
--- a/components/guest_view/browser/guest_view_manager.cc
+++ b/components/guest_view/browser/guest_view_manager.cc
@@ -324,7 +324,20 @@ void GuestViewManager::RemoveGuest(int guest_instance_id) {

void GuestViewManager::EmbedderProcessDestroyed(int embedder_process_id) {
embedders_observed_.erase(embedder_process_id);
+
+ // We can't just call std::multimap::erase here because destroying a guest
+ // could trigger the destruction of another guest which is also owned by
+ // `owned_guests_`. Recursively calling std::multimap::erase is unsafe (see
+ // https://crbug.com/1450397). So we take ownership of all of the guests that
+ // will be destroyed before erasing the entries from the map.
+ std::vector<std::unique_ptr<GuestViewBase>> guests_to_destroy;
+ const auto destroy_range = owned_guests_.equal_range(embedder_process_id);
+ for (auto it = destroy_range.first; it != destroy_range.second; ++it) {
+ guests_to_destroy.push_back(std::move(it->second));
+ }
owned_guests_.erase(embedder_process_id);
+ guests_to_destroy.clear();
+
CallViewDestructionCallbacks(embedder_process_id);
}

diff --git a/components/guest_view/browser/test_guest_view_manager.cc b/components/guest_view/browser/test_guest_view_manager.cc
index ab703db51b5ecd33e5fabd831ba121a2b5047d93..877f3eea7b440ed0a860253f85108c2442afee2e 100644
--- a/components/guest_view/browser/test_guest_view_manager.cc
+++ b/components/guest_view/browser/test_guest_view_manager.cc
@@ -36,7 +36,6 @@ TestGuestViewManager::TestGuestViewManager(
num_guests_created_(0),
expected_num_guests_created_(0),
num_views_garbage_collected_(0),
- waiting_for_guests_created_(false),
waiting_for_attach_(nullptr) {}

TestGuestViewManager::~TestGuestViewManager() = default;
@@ -127,14 +126,15 @@ GuestViewBase* TestGuestViewManager::WaitForNextGuestViewCreated() {
}

void TestGuestViewManager::WaitForNumGuestsCreated(size_t count) {
- if (count == num_guests_created_)
+ if (count == num_guests_created_) {
return;
+ }

- waiting_for_guests_created_ = true;
expected_num_guests_created_ = count;

num_created_run_loop_ = std::make_unique<base::RunLoop>();
num_created_run_loop_->Run();
+ num_created_run_loop_ = nullptr;
}

void TestGuestViewManager::WaitUntilAttached(GuestViewBase* guest_view) {
@@ -179,13 +179,11 @@ void TestGuestViewManager::AddGuest(int guest_instance_id,
created_run_loop_->Quit();

++num_guests_created_;
- if (!waiting_for_guests_created_ &&
- num_guests_created_ != expected_num_guests_created_) {
- return;
- }

- if (num_created_run_loop_)
+ if (num_created_run_loop_ &&
+ num_guests_created_ == expected_num_guests_created_) {
num_created_run_loop_->Quit();
+ }
}

void TestGuestViewManager::AttachGuest(int embedder_process_id,
diff --git a/components/guest_view/browser/test_guest_view_manager.h b/components/guest_view/browser/test_guest_view_manager.h
index 75015f30cfaf8f4bb427b360951f01c729f37308..557afdbdcde44062f13dd981aacd56b8c17d35d6 100644
--- a/components/guest_view/browser/test_guest_view_manager.h
+++ b/components/guest_view/browser/test_guest_view_manager.h
@@ -121,7 +121,6 @@ class TestGuestViewManager : public GuestViewManager {
size_t num_guests_created_;
size_t expected_num_guests_created_;
int num_views_garbage_collected_;
- bool waiting_for_guests_created_;

// Tracks the life time of the GuestView's main FrameTreeNode. The main FTN
// has the same lifesspan as the GuestView.
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Eugene Zemtsov <eugene@chromium.org>
Date: Wed, 21 Jun 2023 17:57:52 +0000
Subject: webcodecs: Fix crash when changing temporal layer count in AV1
encoder

(cherry picked from commit f312efac1b90117729e8961b58c643fc0eae1fbd)

Bug: 1447568
Change-Id: I4ecb02ed956707571573a65ade17fdffe676b502
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4554300
Auto-Submit: Eugene Zemtsov <eugene@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1148041}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4610718
Cr-Commit-Position: refs/branch-heads/5735@{#1360}
Cr-Branched-From: 2f562e4ddbaf79a3f3cb338b4d1bd4398d49eb67-refs/heads/main@{#1135570}

diff --git a/media/video/av1_video_encoder.cc b/media/video/av1_video_encoder.cc
index 71e4fa09a650183c3cd2ef84520d9493f2655bba..266170a524bc8f4317bec941714fe0be9333de37 100644
--- a/media/video/av1_video_encoder.cc
+++ b/media/video/av1_video_encoder.cc
@@ -118,6 +118,7 @@ EncoderStatus SetUpAomConfig(const VideoEncoder::Options& opts,
svc_params = {};
svc_params.framerate_factor[0] = 1;
svc_params.number_spatial_layers = 1;
+ svc_params.number_temporal_layers = 1;
if (opts.scalability_mode.has_value()) {
switch (opts.scalability_mode.value()) {
case SVCScalabilityMode::kL1T2:
diff --git a/media/video/software_video_encoder_test.cc b/media/video/software_video_encoder_test.cc
index 318c743bddcf9bec3a994f125ea329d45f8c4376..7a27b9326a5df556b1a6bacdad71081d46bcc781 100644
--- a/media/video/software_video_encoder_test.cc
+++ b/media/video/software_video_encoder_test.cc
@@ -602,6 +602,63 @@ TEST_P(SVCVideoEncoderTest, EncodeClipTemporalSvc) {
}
}

+TEST_P(SVCVideoEncoderTest, ChangeLayers) {
+ VideoEncoder::Options options;
+ options.frame_size = gfx::Size(640, 480);
+ options.bitrate = Bitrate::ConstantBitrate(1000000u); // 1Mbps
+ options.framerate = 25;
+ options.scalability_mode = GetParam().scalability_mode;
+ std::vector<scoped_refptr<VideoFrame>> frames_to_encode;
+
+ std::vector<VideoEncoderOutput> chunks;
+ size_t total_frames_count = 80;
+
+ // Encoder all frames with 3 temporal layers and put all outputs in |chunks|
+ auto frame_duration = base::Seconds(1.0 / options.framerate.value());
+
+ VideoEncoder::OutputCB encoder_output_cb = base::BindLambdaForTesting(
+ [&](VideoEncoderOutput output,
+ absl::optional<VideoEncoder::CodecDescription> desc) {
+ chunks.push_back(std::move(output));
+ });
+
+ encoder_->Initialize(profile_, options, /*info_cb=*/base::DoNothing(),
+ std::move(encoder_output_cb),
+ ValidatingStatusCB(/* quit_run_loop_on_call */ true));
+ RunUntilQuit();
+
+ uint32_t color = 0x964050;
+ for (auto frame_index = 0u; frame_index < total_frames_count; frame_index++) {
+ auto timestamp = frame_index * frame_duration;
+
+ const bool reconfigure = (frame_index == total_frames_count / 2);
+ if (reconfigure) {
+ encoder_->Flush(ValidatingStatusCB(/* quit_run_loop_on_call */ true));
+ RunUntilQuit();
+
+ // Ask encoder to change SVC mode, empty output callback
+ // means the encoder should keep the old one.
+ options.scalability_mode = SVCScalabilityMode::kL1T1;
+ encoder_->ChangeOptions(
+ options, VideoEncoder::OutputCB(),
+ ValidatingStatusCB(/* quit_run_loop_on_call */ true));
+ RunUntilQuit();
+ }
+
+ auto frame =
+ CreateFrame(options.frame_size, pixel_format_, timestamp, color);
+ color = (color << 1) + frame_index;
+ frames_to_encode.push_back(frame);
+ encoder_->Encode(frame, VideoEncoder::EncodeOptions(false),
+ ValidatingStatusCB(/* quit_run_loop_on_call */ true));
+ RunUntilQuit();
+ }
+
+ encoder_->Flush(ValidatingStatusCB(/* quit_run_loop_on_call */ true));
+ RunUntilQuit();
+ EXPECT_EQ(chunks.size(), total_frames_count);
+}
+
TEST_P(H264VideoEncoderTest, ReconfigureWithResize) {
VideoEncoder::Options options;
gfx::Size size1(320, 200), size2(400, 240);
3 changes: 3 additions & 0 deletions patches/v8/.patches
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,6 @@ cherry-pick-73af1a19a901.patch
merged_regexp_fix_clobbered_register_in_global_unicode_special.patch
m108-lts_api_fix_v8_object_setaccessorproperty.patch
cherry-pick-2e76270cf65e.patch
utf-8_q_shared-struct_20fix_20using_20shared_20objects_20as.patch
merged_runtime_set_instance_prototypes_directly_on_maps.patch
merged_compiler_stackcheck_can_have_side_effects.patch
31 changes: 31 additions & 0 deletions patches/v8/merged_compiler_stackcheck_can_have_side_effects.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Tobias Tebbi <tebbi@chromium.org>
Date: Tue, 13 Jun 2023 17:08:59 +0200
Subject: Merged: [compiler] StackCheck can have side effects

Bug: chromium:1452137
(cherry picked from commit e548943e473b020fdc1de6e5543ca31b24d8b7f9)

Change-Id: Ibd7c9b02efd12341b452e4c34a635a58a817649f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4637129
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Auto-Submit: Tobias Tebbi <tebbi@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/branch-heads/11.4@{#49}
Cr-Branched-From: 8a8a1e7086dacc426965d3875914efa66663c431-refs/heads/11.4.183@{#1}
Cr-Branched-From: 5483d8e816e0bbce865cbbc3fa0ab357e6330bab-refs/heads/main@{#87241}

diff --git a/src/compiler/js-operator.cc b/src/compiler/js-operator.cc
index 8af8e7d32fb68fdcaedaf08427103c893d23098d..e60c8a2c78970c89c4f52a6dc34a97d56e602e26 100644
--- a/src/compiler/js-operator.cc
+++ b/src/compiler/js-operator.cc
@@ -1396,7 +1396,7 @@ const Operator* JSOperatorBuilder::CloneObject(FeedbackSource const& feedback,
const Operator* JSOperatorBuilder::StackCheck(StackCheckKind kind) {
return zone()->New<Operator1<StackCheckKind>>( // --
IrOpcode::kJSStackCheck, // opcode
- Operator::kNoWrite, // properties
+ Operator::kNoProperties, // properties
"JSStackCheck", // name
0, 1, 1, 0, 1, 2, // counts
kind); // parameter
Loading

0 comments on commit 1f91895

Please sign in to comment.