-
Notifications
You must be signed in to change notification settings - Fork 15k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
chore: cherry-pick 1856470e25 and 9037876f53 from chromium (#29787)
* cherry-pick 1856470e25 and 9037876f53 from chromium * chore: update patches Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
- Loading branch information
1 parent
0724cac
commit 4106956
Showing
3 changed files
with
219 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
99 changes: 99 additions & 0 deletions
99
patches/chromium/m86-lts_longtaskdetector_remove_container_mutation_during.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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
118
patches/chromium/m86-lts_reduce_memory_consumption_on.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |