Skip to content

Commit

Permalink
TaskAttribution: Ensure CPED is cleared for upcoming per-Isolate CPED
Browse files Browse the repository at this point in the history
[Get|Set]ContinuationPreservedEmbedderData is being refactored to store
CPED on the isolate rather than the context in v8. But without changing
blink to use the new API, we can have a situation where resetting the
CPED when a task scope is destroyed fails if the context is gone (e.g.
detached frame). Previously this wasn't a problem because the data was
stored on the context, but with per-isolate CPED, this can leak the
context / propagate wrong values.

This CL works around this by clearing the utility context's CPED. With
per-Context CPED, this essentially does nothing, but when the
per-Isolate CPED v8 changes land, this will clear the per-Isolate data.

Note: this is only temporary and will be removed once the v8 changes
land, in favor of using the new API.

Bug: 1351643
Change-Id: Idafc7131067ccb0dd079715ed729e45e88d105c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5031759
Commit-Queue: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1225095}
  • Loading branch information
shaseley authored and Chromium LUCI CQ committed Nov 15, 2023
1 parent cc315ac commit c52e619
Showing 1 changed file with 31 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,22 @@
#include "third_party/blink/renderer/platform/bindings/exception_state.h"
#include "third_party/blink/renderer/platform/bindings/script_forbidden_scope.h"
#include "third_party/blink/renderer/platform/bindings/script_state.h"
#include "third_party/blink/renderer/platform/bindings/v8_per_isolate_data.h"

namespace blink {

namespace {

void ClearContinuationPreservedEmbedderData(v8::Isolate* isolate) {
v8::HandleScope handle_scope(isolate);
v8::Local<v8::Context> context =
V8PerIsolateData::From(isolate)->EnsureScriptRegexpContext();
v8::Context::Scope context_scope(context);
context->SetContinuationPreservedEmbedderData(v8::Local<v8::Value>());
}

} // namespace

ScriptWrappableTaskState::ScriptWrappableTaskState(
scheduler::TaskAttributionInfo* task,
AbortSignal* abort_source,
Expand Down Expand Up @@ -64,19 +77,31 @@ void ScriptWrappableTaskState::SetCurrent(
ScriptState* script_state,
ScriptWrappableTaskState* task_state) {
DCHECK(script_state);
if (!script_state->ContextIsValid()) {
return;
}
CHECK(!ScriptForbiddenScope::IsScriptForbidden());
ScriptState::Scope scope(script_state);
v8::Isolate* isolate = script_state->GetIsolate();
DCHECK(isolate);
if (isolate->IsExecutionTerminating()) {
return;
}
CHECK(!ScriptForbiddenScope::IsScriptForbidden());
if (!script_state->ContextIsValid()) {
// TODO(crbug.com/1351643): This is a temporary workaround for detached
// contexts while transitioning to per-isolate CPED. When v8 switches to
// per-isolate CPED, we won't restore the previous value if the context is
// detached, which can result in propagating the wrong value or leaking the
// context.
//
// The following prevents this by clearing the CPED on an arbitrary context
// associated with the isolate. Before the v8 API changes, this is a no-op
// (aside from potentially creating the context). After it changes, this
// will clear the per-isolate CPED since
// Context::SetContinuationPreservedEmbedderData() will delegate to
// Isolate::SetContinuationPreservedEmbedderData().
ClearContinuationPreservedEmbedderData(isolate);
return;
}
ScriptState::Scope scope(script_state);
v8::Local<v8::Context> context = script_state->GetContext();
DCHECK(!context.IsEmpty());

if (task_state) {
context->SetContinuationPreservedEmbedderData(
ToV8Traits<ScriptWrappableTaskState>::ToV8(script_state, task_state)
Expand Down

0 comments on commit c52e619

Please sign in to comment.