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 ec0cce63f47d from chromium #34232

Merged
merged 3 commits into from May 16, 2022
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
1 change: 1 addition & 0 deletions patches/chromium/.patches
Expand Up @@ -135,4 +135,5 @@ cherry-pick-5be8e065f43e.patch
cherry-pick-12ba78f3fa7a.patch
reland_fix_noopener_case_for_user_activation_consumption.patch
fsa_pass_file_ownership_to_worker_for_async_fsarfd_file_operations.patch
cherry-pick-ec0cce63f47d.patch
cherry-pick-99c3f3bfd507.patch
160 changes: 160 additions & 0 deletions patches/chromium/cherry-pick-ec0cce63f47d.patch
@@ -0,0 +1,160 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Corentin Pescheloche <cpescheloche@fb.com>
Date: Tue, 10 May 2022 14:05:53 +0000
Subject: Cleanup profiler group detached profilers

ProfilerGroup keeps track of detached profilers to be able to gracefully
stop leaked profilers when the corresponding ExecutionContext is
destroyed.

(cherry picked from commit 9f9d5fd2f3085414fc8776bf556fb5c4fa2dac2c)

Change-Id: I4fdbbc3a5208819397d742c9ecbff117f839691c
Bug: chromium:1297283
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3594570
Commit-Queue: Corentin Pescheloche <cpescheloche@fb.com>
Cr-Original-Commit-Position: refs/heads/main@{#994316}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3620956
Reviewed-by: Oleh Lamzin <lamzin@google.com>
Owners-Override: Oleh Lamzin <lamzin@google.com>
Commit-Queue: Roger Felipe Zanoni da Silva <rzanoni@google.com>
Auto-Submit: Roger Felipe Zanoni da Silva <rzanoni@google.com>
Cr-Commit-Position: refs/branch-heads/4664@{#1629}
Cr-Branched-From: 24dc4ee75e01a29d390d43c9c264372a169273a7-refs/heads/main@{#929512}

diff --git a/third_party/blink/renderer/core/timing/profiler_group.cc b/third_party/blink/renderer/core/timing/profiler_group.cc
index 6638ee38519674036c6c45cf6cd1a4eff023289b..f498a122f5a491226720a36dc729364028148eaa 100644
--- a/third_party/blink/renderer/core/timing/profiler_group.cc
+++ b/third_party/blink/renderer/core/timing/profiler_group.cc
@@ -110,8 +110,7 @@ ProfilerGroup::ProfilerGroup(v8::Isolate* isolate)
: isolate_(isolate),
cpu_profiler_(nullptr),
next_profiler_id_(0),
- num_active_profilers_(0) {
-}
+ num_active_profilers_(0) {}

void DiscardedSamplesDelegate::Notify() {
if (profiler_group_) {
@@ -234,6 +233,8 @@ void ProfilerGroup::WillBeDestroyed() {
DCHECK(!profilers_.Contains(profiler));
}

+ StopDetachedProfilers();
+
if (cpu_profiler_)
TeardownV8Profiler();
}
@@ -304,18 +305,49 @@ void ProfilerGroup::CancelProfiler(Profiler* profiler) {

void ProfilerGroup::CancelProfilerAsync(ScriptState* script_state,
Profiler* profiler) {
+ DCHECK(IsMainThread());
DCHECK(cpu_profiler_);
DCHECK(!profiler->stopped());
profilers_.erase(profiler);

+ // register the profiler to be cleaned up in case its associated context
+ // gets destroyed before the cleanup task is executed.
+ detached_profiler_ids_.push_back(profiler->ProfilerId());
+
// Since it's possible for the profiler to get destructed along with its
// associated context, dispatch a task to cleanup context-independent isolate
// resources (rather than use the context's task runner).
ThreadScheduler::Current()->V8TaskRunner()->PostTask(
- FROM_HERE, WTF::Bind(&ProfilerGroup::CancelProfilerImpl,
+ FROM_HERE, WTF::Bind(&ProfilerGroup::StopDetachedProfiler,
WrapPersistent(this), profiler->ProfilerId()));
}

+void ProfilerGroup::StopDetachedProfiler(String profiler_id) {
+ DCHECK(IsMainThread());
+
+ // we use a vector instead of a map because the expected number of profiler
+ // is expected to be very small
+ auto* it = std::find(detached_profiler_ids_.begin(),
+ detached_profiler_ids_.end(), profiler_id);
+
+ if (it == detached_profiler_ids_.end()) {
+ // Profiler already stopped
+ return;
+ }
+
+ CancelProfilerImpl(profiler_id);
+ detached_profiler_ids_.erase(it);
+}
+
+void ProfilerGroup::StopDetachedProfilers() {
+ DCHECK(IsMainThread());
+
+ for (auto& detached_profiler_id : detached_profiler_ids_) {
+ CancelProfilerImpl(detached_profiler_id);
+ }
+ detached_profiler_ids_.clear();
+}
+
void ProfilerGroup::CancelProfilerImpl(String profiler_id) {
if (!cpu_profiler_)
return;
diff --git a/third_party/blink/renderer/core/timing/profiler_group.h b/third_party/blink/renderer/core/timing/profiler_group.h
index 2cdbd795f4265086073b8c378ede4f7c6c60e3e3..ede25fa5656f32d1982077eda43585b70743fd63 100644
--- a/third_party/blink/renderer/core/timing/profiler_group.h
+++ b/third_party/blink/renderer/core/timing/profiler_group.h
@@ -81,6 +81,10 @@ class CORE_EXPORT ProfilerGroup
// Internal implementation of cancel.
void CancelProfilerImpl(String profiler_id);

+ // Clean context independent resources for leaked profilers
+ void StopDetachedProfiler(String profiler_id);
+ void StopDetachedProfilers();
+
// Generates an unused string identifier to use for a new profiling session.
String NextProfilerId();

@@ -88,9 +92,11 @@ class CORE_EXPORT ProfilerGroup
v8::CpuProfiler* cpu_profiler_;
int next_profiler_id_;
int num_active_profilers_;
-
HeapHashSet<WeakMember<Profiler>> profilers_;

+ // Store the ids of leaked collected profilers that needs to be stopped
+ Vector<String> detached_profiler_ids_;
+
// A set of observers, one for each ExecutionContext that has profiling
// enabled.
HeapHashSet<Member<ProfilingContextObserver>> context_observers_;
diff --git a/third_party/blink/renderer/core/timing/profiler_group_test.cc b/third_party/blink/renderer/core/timing/profiler_group_test.cc
index adedff31f007ca2deff401ec93b39641f4ea516e..1c6dd092c36a27710085a03c74fa12e212adccb4 100644
--- a/third_party/blink/renderer/core/timing/profiler_group_test.cc
+++ b/third_party/blink/renderer/core/timing/profiler_group_test.cc
@@ -262,4 +262,29 @@ TEST(ProfilerGroupTest, LeakProfilerWithContext) {
test::RunPendingTasks();
}

+// Tests that a ProfilerGroup doesn't crash if the ProfilerGroup is destroyed
+// before a Profiler::Dispose is ran.
+TEST(ProfilerGroupTest, Bug1297283) {
+ {
+ V8TestingScope scope;
+ ProfilerGroup* profiler_group = ProfilerGroup::From(scope.GetIsolate());
+ profiler_group->OnProfilingContextAdded(scope.GetExecutionContext());
+
+ ProfilerInitOptions* init_options = ProfilerInitOptions::Create();
+ init_options->setSampleInterval(0);
+ init_options->setMaxBufferSize(0);
+ Profiler* profiler = profiler_group->CreateProfiler(
+ scope.GetScriptState(), *init_options, base::TimeTicks(),
+ scope.GetExceptionState());
+ EXPECT_FALSE(profiler->stopped());
+
+ // Force a collection of the underlying Profiler
+ profiler = nullptr;
+ ThreadState::Current()->CollectAllGarbageForTesting();
+ // Exit Scope deallocating Context triggering ProfilerGroup::WillBeDestroyed
+ // Ensure doesn't crash.
+ }
+ test::RunPendingTasks();
+}
+
} // namespace blink