Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: cherry-pick 1856470e25 and 9037876f53 from chromium #29787

Merged
merged 2 commits into from Jun 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions patches/chromium/.patches
Expand Up @@ -168,3 +168,5 @@ media_feeds_disable_media_feeds_and_related_features.patch
remove_tabs_and_line_breaks_from_the_middle_of_app_names_when.patch
autofill_fixed_refill_of_changed_form.patch
x11_fix_window_enumeration_order_when_wm_doesn_t_set.patch
m86-lts_longtaskdetector_remove_container_mutation_during.patch
m86-lts_reduce_memory_consumption_on.patch
@@ -0,0 +1,99 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Yutaka Hirano <yhirano@chromium.org>
Date: Fri, 11 Jun 2021 08:07:55 +0000
Subject: Remove container mutation during iteration
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On LongTaskDetector, we call OnLongTaskDetected for all registered
observers. Some observers call LongTaskDetector::UnregisterObserver
in the callback, which is problematic because container mutation is
not allowed during iteration.

Copy the observer set to avoid the violation.

(cherry picked from commit 702f4d4ddb963cafb0d133972282dfc803510b75)

(cherry picked from commit e88c656a9fb4a7bb1c66ddcedae8049a448ebef4)

Bug: 1210487
Change-Id: Iccea748ac144def6884be8cf542cdc3572bed81a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2909934
Reviewed-by: Deep Roy <dproy@chromium.org>
Reviewed-by: Nicolás Peña Moreno <npm@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Original-Original-Commit-Position: refs/heads/master@{#885033}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2939704
Auto-Submit: Yutaka Hirano <yhirano@chromium.org>
Owners-Override: Prudhvi Kumar Bommana <pbommana@google.com>
Reviewed-by: Prudhvi Kumar Bommana <pbommana@google.com>
Cr-Original-Commit-Position: refs/branch-heads/4472@{#1443}
Cr-Original-Branched-From: 3d60439cfb36485e76a1c5bb7f513d3721b20da1-refs/heads/master@{#870763}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2945787
Owners-Override: Victor-Gabriel Savu <vsavu@google.com>
Reviewed-by: Artem Sumaneev <asumaneev@google.com>
Commit-Queue: Victor-Gabriel Savu <vsavu@google.com>
Cr-Commit-Position: refs/branch-heads/4240@{#1669}
Cr-Branched-From: f297677702651916bbf65e59c0d4bbd4ce57d1ee-refs/heads/master@{#800218}

diff --git a/third_party/blink/renderer/core/loader/long_task_detector.cc b/third_party/blink/renderer/core/loader/long_task_detector.cc
index 7e1499b1ddde30db2344db6fd9a9d3e7be574033..f040ae5053265fb136c629f106aeefa0b01130f1 100644
--- a/third_party/blink/renderer/core/loader/long_task_detector.cc
+++ b/third_party/blink/renderer/core/loader/long_task_detector.cc
@@ -43,7 +43,10 @@ void LongTaskDetector::DidProcessTask(base::TimeTicks start_time,
if ((end_time - start_time) < LongTaskDetector::kLongTaskThreshold)
return;

- for (auto& observer : observers_) {
+ // We copy `observers_` because it might be mutated in OnLongTaskDetected,
+ // and container mutation is not allowed during iteration.
+ const HeapHashSet<Member<LongTaskObserver>> observers = observers_;
+ for (auto& observer : observers) {
observer->OnLongTaskDetected(start_time, end_time);
}
}
diff --git a/third_party/blink/renderer/core/loader/long_task_detector_test.cc b/third_party/blink/renderer/core/loader/long_task_detector_test.cc
index 3384fa8ebfb0bd3ad1c408390db3fcb26edc4118..04959d3b682ddbf40577adc5799fe57a9ae9d500 100644
--- a/third_party/blink/renderer/core/loader/long_task_detector_test.cc
+++ b/third_party/blink/renderer/core/loader/long_task_detector_test.cc
@@ -27,9 +27,24 @@ class TestLongTaskObserver :
last_long_task_start = start_time;
last_long_task_end = end_time;
}
-}; // Anonymous namespace
+};
+
+class SelfUnregisteringObserver
+ : public GarbageCollected<SelfUnregisteringObserver>,
+ public LongTaskObserver {
+ public:
+ void OnLongTaskDetected(base::TimeTicks, base::TimeTicks) override {
+ called_ = true;
+ LongTaskDetector::Instance().UnregisterObserver(this);
+ }
+ bool IsCalled() const { return called_; }
+
+ private:
+ bool called_ = false;
+};

} // namespace
+
class LongTaskDetectorTest : public testing::Test {
public:
// Public because it's executed on a task queue.
@@ -126,4 +141,13 @@ TEST_F(LongTaskDetectorTest, RegisterSameObserverTwice) {
long_task_end_when_registered);
}

+TEST_F(LongTaskDetectorTest, SelfUnregisteringObserver) {
+ auto* observer = MakeGarbageCollected<SelfUnregisteringObserver>();
+
+ LongTaskDetector::Instance().RegisterObserver(observer);
+ SimulateTask(LongTaskDetector::kLongTaskThreshold +
+ base::TimeDelta::FromMilliseconds(10));
+ EXPECT_TRUE(observer->IsCalled());
+}
+
} // namespace blink
118 changes: 118 additions & 0 deletions patches/chromium/m86-lts_reduce_memory_consumption_on.patch
@@ -0,0 +1,118 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Yutaka Hirano <yhirano@chromium.org>
Date: Fri, 11 Jun 2021 08:42:36 +0000
Subject: Reduce memory consumption on LongTaskObserver::DidProcessTask

https://crrev.com/c/2909934 fixed a security issue, but it introduced a
copy operation for each DidProcessTask for a long task. We see a memory
regression on the change, and this is an attempt to mitigate the
regression.

(cherry picked from commit 8097e73295a88e64d8318d982847a5e4f2bcc4d2)

(cherry picked from commit 7be6a34fe2f01af881bb074bc616bf5b6b5f7c31)

Bug: 1210487, 1211539
Change-Id: Ib9101e29d70fadb11b7967754e847bb5cc754feb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2915153
Reviewed-by: Benoit L <lizeb@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Original-Original-Commit-Position: refs/heads/master@{#886221}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2944320
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Original-Commit-Position: refs/branch-heads/4472@{#1460}
Cr-Original-Branched-From: 3d60439cfb36485e76a1c5bb7f513d3721b20da1-refs/heads/master@{#870763}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2952502
Owners-Override: Victor-Gabriel Savu <vsavu@google.com>
Reviewed-by: Artem Sumaneev <asumaneev@google.com>
Commit-Queue: Victor-Gabriel Savu <vsavu@google.com>
Cr-Commit-Position: refs/branch-heads/4240@{#1670}
Cr-Branched-From: f297677702651916bbf65e59c0d4bbd4ce57d1ee-refs/heads/master@{#800218}

diff --git a/third_party/blink/renderer/core/loader/long_task_detector.cc b/third_party/blink/renderer/core/loader/long_task_detector.cc
index f040ae5053265fb136c629f106aeefa0b01130f1..3816779cfafaef06295734b4a8a2f033bf752691 100644
--- a/third_party/blink/renderer/core/loader/long_task_detector.cc
+++ b/third_party/blink/renderer/core/loader/long_task_detector.cc
@@ -24,6 +24,7 @@ LongTaskDetector::LongTaskDetector() = default;
void LongTaskDetector::RegisterObserver(LongTaskObserver* observer) {
DCHECK(IsMainThread());
DCHECK(observer);
+ DCHECK(!iterating_);
if (observers_.insert(observer).is_new_entry && observers_.size() == 1) {
// Number of observers just became non-zero.
Thread::Current()->AddTaskTimeObserver(this);
@@ -32,6 +33,10 @@ void LongTaskDetector::RegisterObserver(LongTaskObserver* observer) {

void LongTaskDetector::UnregisterObserver(LongTaskObserver* observer) {
DCHECK(IsMainThread());
+ if (iterating_) {
+ observers_to_be_removed_.push_back(observer);
+ return;
+ }
observers_.erase(observer);
if (observers_.size() == 0) {
Thread::Current()->RemoveTaskTimeObserver(this);
@@ -43,16 +48,21 @@ void LongTaskDetector::DidProcessTask(base::TimeTicks start_time,
if ((end_time - start_time) < LongTaskDetector::kLongTaskThreshold)
return;

- // We copy `observers_` because it might be mutated in OnLongTaskDetected,
- // and container mutation is not allowed during iteration.
- const HeapHashSet<Member<LongTaskObserver>> observers = observers_;
- for (auto& observer : observers) {
+ iterating_ = true;
+ for (auto& observer : observers_) {
observer->OnLongTaskDetected(start_time, end_time);
}
+ iterating_ = false;
+
+ for (const auto& observer : observers_to_be_removed_) {
+ UnregisterObserver(observer);
+ }
+ observers_to_be_removed_.clear();
}

void LongTaskDetector::Trace(Visitor* visitor) const {
visitor->Trace(observers_);
+ visitor->Trace(observers_to_be_removed_);
}

} // namespace blink
diff --git a/third_party/blink/renderer/core/loader/long_task_detector.h b/third_party/blink/renderer/core/loader/long_task_detector.h
index dc6f0dbab5c059b83bfe4212f0126e9690ab1a7f..5fd4bb8d2abcc67dd4e47927daa260fa37bc4aca 100644
--- a/third_party/blink/renderer/core/loader/long_task_detector.h
+++ b/third_party/blink/renderer/core/loader/long_task_detector.h
@@ -49,6 +49,8 @@ class CORE_EXPORT LongTaskDetector final
base::TimeTicks end_time) override;

HeapHashSet<Member<LongTaskObserver>> observers_;
+ HeapVector<Member<LongTaskObserver>> observers_to_be_removed_;
+ bool iterating_ = false;

DISALLOW_COPY_AND_ASSIGN(LongTaskDetector);
};
diff --git a/third_party/blink/renderer/core/loader/long_task_detector_test.cc b/third_party/blink/renderer/core/loader/long_task_detector_test.cc
index 04959d3b682ddbf40577adc5799fe57a9ae9d500..403ba452362a3fa2a6b24f238bad35d9eb1bac0c 100644
--- a/third_party/blink/renderer/core/loader/long_task_detector_test.cc
+++ b/third_party/blink/renderer/core/loader/long_task_detector_test.cc
@@ -39,6 +39,8 @@ class SelfUnregisteringObserver
}
bool IsCalled() const { return called_; }

+ void Reset() { called_ = false; }
+
private:
bool called_ = false;
};
@@ -148,6 +150,11 @@ TEST_F(LongTaskDetectorTest, SelfUnregisteringObserver) {
SimulateTask(LongTaskDetector::kLongTaskThreshold +
base::TimeDelta::FromMilliseconds(10));
EXPECT_TRUE(observer->IsCalled());
+ observer->Reset();
+
+ SimulateTask(LongTaskDetector::kLongTaskThreshold +
+ base::TimeDelta::FromMilliseconds(10));
+ EXPECT_FALSE(observer->IsCalled());
}

} // namespace blink