DOMWindow: Set wrapper reference to global proxy object
The JS global proxy object already points to the
{Local,Remote}DOMWindow via internal field. Make
DOMWindow::main_world_wrapper_ point to the global proxy object as
well. Avoid using the already existing AssociateWithWrapper() methods
as they require that a context is properly set up.
{Local,Remote}WindowProxy's global_proxy_ field becomes a regular
traced reference without setting up the class id. The GC treats it
the same way for liveness but heap snapshot generation would not
consider it as a candidate to merge nodes with a JS object as it's not
ScriptWrappable's::main_world_wrapper_.
This way Wrappable/Wrapper pairs (represented by ScriptWrappable/V8
objects) are properly set up in the entire codebase and may on a
high-level considered as one object or "entity". This allows heap
snapshot generation to merge the nodes without considering the class
id state on the reference. Ultimately, this allows us to get rid of
the class id eventually and speed up all TracedReference creation.
Delete WPT test:
performance-timeline/tentative/performance-entry-source-deleted-frame.html
Rationale:
- The test was added to check that a PerformanceEntry does not leak
Window objects via its source property when PerformanceEntry objects
are stashed away to be consumed later.
- The test merely checks that we return a null Window for a detached
Window. This is not what it was added for.
- The test fails for FF and Safari [1]
- Writing this test properly is really hard: The JS wrapper is already
materialized in the iframe via `performance.mark()`. Thus accessing
`childEntry` from the main frame leaks the other iframe via the JS
wrapper constructor (that points to its context). In order to avoid
this, one must stash the Window itself in a weak ref, perform GC at
the event loop, and then check the weakref. Performing GC at the
event loop is not supported in WPT as of today.
[1] https://wpt.fyi/results/performance-timeline/tentative?label=master&label=experimental&aligned&q=performance-timeline%2Ftentative%2Fperformance-entry-source-deleted-frame.html
Tests: DevTools frontend e2e memory/*
Bug: chromium:1218404
Change-Id: I4e3273c6ca7da8e3241b4f9ac78c76932c52bce2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2953842
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1237676}
diff --git a/third_party/blink/renderer/bindings/core/v8/local_window_proxy.cc b/third_party/blink/renderer/bindings/core/v8/local_window_proxy.cc
index e22ccf32..471d34b3 100644
--- a/third_party/blink/renderer/bindings/core/v8/local_window_proxy.cc
+++ b/third_party/blink/renderer/bindings/core/v8/local_window_proxy.cc
@@ -114,14 +114,14 @@
if (next_status == Lifecycle::kV8MemoryIsForciblyPurged ||
next_status == Lifecycle::kGlobalObjectIsDetached) {
// Clean up state on the global proxy, which will be reused.
+ v8::Local<v8::Object> global = context->Global();
if (!global_proxy_.IsEmpty()) {
- CHECK(global_proxy_ == context->Global());
- CHECK_EQ(ToScriptWrappable(context->Global()),
- ToScriptWrappable(
- context->Global()->GetPrototype().As<v8::Object>()));
- global_proxy_.SetWrapperClassId(0);
+ CHECK(global_proxy_ == global);
+ CHECK_EQ(ToScriptWrappable(global),
+ ToScriptWrappable(global->GetPrototype().As<v8::Object>()));
}
- V8DOMWrapper::ClearNativeInfo(GetIsolate(), context->Global());
+ V8DOMWrapper::ClearNativeInfo(GetIsolate(), global);
+ DOMWrapperWorld::ClearWrapperIfEqualTo(GetFrame()->DomWindow(), global);
script_state_->DetachGlobalObject();
#if DCHECK_IS_ON()
@@ -293,11 +293,15 @@
// The global proxy object. Note this is not the global object.
v8::Local<v8::Object> global_proxy = context->Global();
CHECK(global_proxy_ == global_proxy);
+ // Use the global proxy as window wrapper object.
V8DOMWrapper::SetNativeInfo(GetIsolate(), global_proxy, wrapper_type_info,
window);
// Mark the handle to be traced by Oilpan, since the global proxy has a
// reference to the DOMWindow.
- global_proxy_.SetWrapperClassId(wrapper_type_info->wrapper_class_id);
+ // global_proxy_.SetWrapperClassId(wrapper_type_info->wrapper_class_id);
+ CHECK(global_proxy_ == window->AssociateWithWrapper(GetIsolate(), world_,
+ wrapper_type_info,
+ global_proxy));
// The global object, aka window wrapper object.
v8::Local<v8::Object> window_wrapper =
diff --git a/third_party/blink/renderer/bindings/core/v8/remote_window_proxy.cc b/third_party/blink/renderer/bindings/core/v8/remote_window_proxy.cc
index a2af8728..59439de 100644
--- a/third_party/blink/renderer/bindings/core/v8/remote_window_proxy.cc
+++ b/third_party/blink/renderer/bindings/core/v8/remote_window_proxy.cc
@@ -73,9 +73,9 @@
next_status == Lifecycle::kGlobalObjectIsDetached) &&
!global_proxy_.IsEmpty()) {
v8::HandleScope handle_scope(GetIsolate());
- global_proxy_.SetWrapperClassId(0);
- V8DOMWrapper::ClearNativeInfo(GetIsolate(),
- global_proxy_.Get(GetIsolate()));
+ v8::Local<v8::Object> global = global_proxy_.Get(GetIsolate());
+ V8DOMWrapper::ClearNativeInfo(GetIsolate(), global);
+ DOMWrapperWorld::ClearWrapperIfEqualTo(GetFrame()->DomWindow(), global);
#if DCHECK_IS_ON()
DidDetachGlobalObject();
#endif
@@ -133,11 +133,9 @@
// The global proxy object. Note this is not the global object.
v8::Local<v8::Object> global_proxy = global_proxy_.Get(GetIsolate());
- V8DOMWrapper::SetNativeInfo(GetIsolate(), global_proxy, wrapper_type_info,
- window);
- // Mark the handle to be traced by Oilpan, since the global proxy has a
- // reference to the DOMWindow.
- global_proxy_.SetWrapperClassId(wrapper_type_info->wrapper_class_id);
+ CHECK(global_proxy == window->AssociateWithWrapper(GetIsolate(), world_,
+ wrapper_type_info,
+ global_proxy));
// The global object, aka window wrapper object.
v8::Local<v8::Object> window_wrapper =
diff --git a/third_party/blink/renderer/core/frame/dom_window.cc b/third_party/blink/renderer/core/frame/dom_window.cc
index 76e0ff9..b6cf89d 100644
--- a/third_party/blink/renderer/core/frame/dom_window.cc
+++ b/third_party/blink/renderer/core/frame/dom_window.cc
@@ -40,6 +40,7 @@
#include "third_party/blink/renderer/core/page/page.h"
#include "third_party/blink/renderer/core/probe/core_probes.h"
#include "third_party/blink/renderer/platform/bindings/source_location.h"
+#include "third_party/blink/renderer/platform/bindings/v8_dom_wrapper.h"
#include "third_party/blink/renderer/platform/heap/garbage_collected.h"
#include "third_party/blink/renderer/platform/instrumentation/use_counter.h"
#include "third_party/blink/renderer/platform/weborigin/kurl.h"
@@ -84,6 +85,9 @@
// TODO(yukishiino): Make this function always return the non-empty handle
// even if the frame is detached because the global proxy must always exist
// per spec.
+ //
+ // Getting the proxy also results in initializing it and eventually yields in
+ // `SetupWindowPrototypeChain()` calls for the window proxy.
return frame->GetWindowProxy(script_state->World())
->GlobalProxyIfNotDetached();
}
@@ -92,8 +96,21 @@
v8::Isolate*,
const WrapperTypeInfo*,
v8::Local<v8::Object> wrapper) {
- NOTREACHED();
- return v8::Local<v8::Object>();
+ NOTREACHED_NORETURN();
+}
+
+v8::Local<v8::Object> DOMWindow::AssociateWithWrapper(
+ v8::Isolate* isolate,
+ scoped_refptr<DOMWrapperWorld> world,
+ const WrapperTypeInfo* wrapper_type_info,
+ v8::Local<v8::Object> wrapper) {
+ // Using the world directly avoids fetching it from a potentially
+ // half-initialized context.
+ if (world->DomDataStore().Set(isolate, this, wrapper_type_info, wrapper)) {
+ V8DOMWrapper::SetNativeInfo(isolate, wrapper, wrapper_type_info, this);
+ DCHECK(V8DOMWrapper::HasInternalFieldsSet(wrapper));
+ }
+ return wrapper;
}
const AtomicString& DOMWindow::InterfaceName() const {
diff --git a/third_party/blink/renderer/core/frame/dom_window.h b/third_party/blink/renderer/core/frame/dom_window.h
index 920789d..19f7fd0 100644
--- a/third_party/blink/renderer/core/frame/dom_window.h
+++ b/third_party/blink/renderer/core/frame/dom_window.h
@@ -5,6 +5,7 @@
#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_FRAME_DOM_WINDOW_H_
#define THIRD_PARTY_BLINK_RENDERER_CORE_FRAME_DOM_WINDOW_H_
+#include "base/memory/scoped_refptr.h"
#include "services/network/public/mojom/cross_origin_opener_policy.mojom-blink.h"
#include "third_party/blink/public/common/messaging/message_port_channel.h"
#include "third_party/blink/public/common/tokens/tokens.h"
@@ -22,6 +23,7 @@
namespace blink {
class ContextLifecycleNotifier;
+class DOMWrapperWorld;
class InputDeviceCapabilitiesConstants;
class LocalDOMWindow;
class Location;
@@ -31,6 +33,7 @@
class UserActivation;
class WindowPostMessageOptions;
class WindowProxyManager;
+struct WrapperTypeInfo;
struct BlinkTransferableMessage;
@@ -76,6 +79,11 @@
v8::Isolate*,
const WrapperTypeInfo*,
v8::Local<v8::Object> wrapper) final;
+ v8::Local<v8::Object> AssociateWithWrapper(
+ v8::Isolate* isolate,
+ scoped_refptr<DOMWrapperWorld> world,
+ const WrapperTypeInfo* wrapper_type_info,
+ v8::Local<v8::Object> wrapper);
// EventTarget overrides:
const AtomicString& InterfaceName() const override;
diff --git a/third_party/blink/renderer/platform/bindings/dom_data_store.h b/third_party/blink/renderer/platform/bindings/dom_data_store.h
index 12c281e..14247bf 100644
--- a/third_party/blink/renderer/platform/bindings/dom_data_store.h
+++ b/third_party/blink/renderer/platform/bindings/dom_data_store.h
@@ -143,17 +143,15 @@
return result.is_new_entry;
}
- bool UnsetSpecificWrapperIfSet(
- ScriptWrappable* object,
- const v8::TracedReference<v8::Object>& handle) {
+ template <typename HandleType>
+ bool ClearWrapperIfEqualTo(ScriptWrappable* object,
+ const HandleType& handle) {
DCHECK(!is_main_world_);
const auto& it = wrapper_map_.find(object);
- if (it != wrapper_map_.end()) {
- if (it->value == handle) {
- it->value.Reset();
- wrapper_map_.erase(it);
- return true;
- }
+ if (it != wrapper_map_.end() && it->value == handle) {
+ it->value.Reset();
+ wrapper_map_.erase(it);
+ return true;
}
return false;
}
diff --git a/third_party/blink/renderer/platform/bindings/dom_wrapper_world.cc b/third_party/blink/renderer/platform/bindings/dom_wrapper_world.cc
index e732c31..f913c8d5 100644
--- a/third_party/blink/renderer/platform/bindings/dom_wrapper_world.cc
+++ b/third_party/blink/renderer/platform/bindings/dom_wrapper_world.cc
@@ -291,13 +291,27 @@
}
// static
-bool DOMWrapperWorld::UnsetNonMainWorldWrapperIfSet(
+bool DOMWrapperWorld::ClearNonMainWorldWrapperIfEqualTo(
+ ScriptWrappable* object,
+ const v8::Local<v8::Object>& handle) {
+ for (DOMWrapperWorld* world : GetWorldMap().Values()) {
+ DOMDataStore& data_store = world->DomDataStore();
+ if (data_store.ClearWrapperIfEqualTo(object, handle)) {
+ return true;
+ }
+ }
+ return false;
+}
+
+// static
+bool DOMWrapperWorld::ClearNonMainWorldWrapperIfEqualTo(
ScriptWrappable* object,
const v8::TracedReference<v8::Object>& handle) {
for (DOMWrapperWorld* world : GetWorldMap().Values()) {
DOMDataStore& data_store = world->DomDataStore();
- if (data_store.UnsetSpecificWrapperIfSet(object, handle))
+ if (data_store.ClearWrapperIfEqualTo(object, handle)) {
return true;
+ }
}
return false;
}
diff --git a/third_party/blink/renderer/platform/bindings/dom_wrapper_world.h b/third_party/blink/renderer/platform/bindings/dom_wrapper_world.h
index fe809207..50e8dee 100644
--- a/third_party/blink/renderer/platform/bindings/dom_wrapper_world.h
+++ b/third_party/blink/renderer/platform/bindings/dom_wrapper_world.h
@@ -172,17 +172,20 @@
}
// Clear the reference pointing from |object| to |handle| in any world.
- static bool UnsetSpecificWrapperIfSet(
- ScriptWrappable* object,
- const v8::TracedReference<v8::Object>& handle);
+ template <typename HandleType>
+ inline static bool ClearWrapperIfEqualTo(ScriptWrappable* object,
+ const HandleType& handle);
- // Clear the reference pointing from |object| to |handle| in any world.
- static bool UnsetMainWorldWrapperIfSet(
- ScriptWrappable* object,
- const v8::TracedReference<v8::Object>& handle);
+ // Clear the reference pointing from |object| to |handle| in the main world.
+ template <typename HandleType>
+ inline static bool ClearMainWorldWrapperIfEqualTo(ScriptWrappable* object,
+ const HandleType& handle);
private:
- static bool UnsetNonMainWorldWrapperIfSet(
+ static bool ClearNonMainWorldWrapperIfEqualTo(
+ ScriptWrappable* object,
+ const v8::Local<v8::Object>& handle);
+ static bool ClearNonMainWorldWrapperIfEqualTo(
ScriptWrappable* object,
const v8::TracedReference<v8::Object>& handle);
@@ -202,23 +205,21 @@
};
// static
-inline bool DOMWrapperWorld::UnsetMainWorldWrapperIfSet(
- ScriptWrappable* object,
- const v8::TracedReference<v8::Object>& handle) {
- return object->UnsetMainWorldWrapperIfSet(handle);
+template <typename HandleType>
+bool DOMWrapperWorld::ClearMainWorldWrapperIfEqualTo(ScriptWrappable* object,
+ const HandleType& handle) {
+ return object->ClearMainWorldWrapperIfEqualTo(handle);
}
// static
-inline bool DOMWrapperWorld::UnsetSpecificWrapperIfSet(
- ScriptWrappable* object,
- const v8::TracedReference<v8::Object>& handle) {
- // Fast path for main world.
- if (UnsetMainWorldWrapperIfSet(object, handle)) {
+template <typename HandleType>
+bool DOMWrapperWorld::ClearWrapperIfEqualTo(ScriptWrappable* object,
+ const HandleType& handle) {
+ if (ClearMainWorldWrapperIfEqualTo(object, handle)) {
return true;
}
-
// Slow path: |object| may point to |handle| in any non-main DOM world.
- return DOMWrapperWorld::UnsetNonMainWorldWrapperIfSet(object, handle);
+ return DOMWrapperWorld::ClearNonMainWorldWrapperIfEqualTo(object, handle);
}
} // namespace blink
diff --git a/third_party/blink/renderer/platform/bindings/script_wrappable.h b/third_party/blink/renderer/platform/bindings/script_wrappable.h
index f51c759..6584f383 100644
--- a/third_party/blink/renderer/platform/bindings/script_wrappable.h
+++ b/third_party/blink/renderer/platform/bindings/script_wrappable.h
@@ -211,8 +211,8 @@
}
// Clear the main world wrapper if it is set to |handle|.
- bool UnsetMainWorldWrapperIfSet(
- const v8::TracedReference<v8::Object>& handle);
+ template <typename HandleType>
+ inline bool ClearMainWorldWrapperIfEqualTo(const HandleType& handle);
static_assert(
std::is_trivially_destructible<
@@ -225,11 +225,10 @@
// world wrapper.
friend class DOMDataStore;
friend class DOMWrapperWorld;
- friend class HeapSnaphotWrapperVisitor;
};
-inline bool ScriptWrappable::UnsetMainWorldWrapperIfSet(
- const v8::TracedReference<v8::Object>& handle) {
+template <typename HandleType>
+bool ScriptWrappable::ClearMainWorldWrapperIfEqualTo(const HandleType& handle) {
if (main_world_wrapper_ == handle) {
main_world_wrapper_.Reset();
return true;
diff --git a/third_party/blink/renderer/platform/heap/thread_state.cc b/third_party/blink/renderer/platform/heap/thread_state.cc
index 00da6d87..5ba124e 100644
--- a/third_party/blink/renderer/platform/heap/thread_state.cc
+++ b/third_party/blink/renderer/platform/heap/thread_state.cc
@@ -50,7 +50,7 @@
DCHECK(handle.WrapperClassId() == WrapperTypeInfo::kNodeClassId ||
handle.WrapperClassId() == WrapperTypeInfo::kObjectClassId);
const v8::TracedReference<v8::Object>& traced = handle.As<v8::Object>();
- bool success = DOMWrapperWorld::UnsetSpecificWrapperIfSet(
+ bool success = DOMWrapperWorld::ClearWrapperIfEqualTo(
ToScriptWrappable(traced), traced);
// Since V8 found a handle, Blink needs to find it as well when trying to
// remove it.
@@ -61,7 +61,7 @@
DCHECK(handle.WrapperClassId() == WrapperTypeInfo::kNodeClassId ||
handle.WrapperClassId() == WrapperTypeInfo::kObjectClassId);
const v8::TracedReference<v8::Object>& traced = handle.As<v8::Object>();
- return DOMWrapperWorld::UnsetMainWorldWrapperIfSet(
+ return DOMWrapperWorld::ClearMainWorldWrapperIfEqualTo(
ToScriptWrappable(traced), traced);
}
};
diff --git a/third_party/blink/web_tests/external/wpt/performance-timeline/tentative/performance-entry-source-deleted-frame.html b/third_party/blink/web_tests/external/wpt/performance-timeline/tentative/performance-entry-source-deleted-frame.html
deleted file mode 100644
index 81970606..0000000
--- a/third_party/blink/web_tests/external/wpt/performance-timeline/tentative/performance-entry-source-deleted-frame.html
+++ /dev/null
@@ -1,39 +0,0 @@
-<!DOCTYPE html>
-
-<head>
- <script src="/resources/testharness.js"></script>
- <script src="/resources/testharnessreport.js"></script>
-</head>
-
-<body>
-</body>
-<script>
- promise_test(async () => {
- // Create child iframe.
- const childFrame = document.createElement('iframe');
- childFrame.src = '../resources/child-frame.html';
-
- let childEntry;
- // Load child frame.
- const loadChildFramePromise = new Promise(resolve => {
- childFrame.addEventListener('load', () => {
- childEntry = performance.getEntries(
- { name: 'mark_child_frame', includeChildFrames: true })[0];
-
- // Child PerformanceMark source should be the child window.
- assert_equals(childEntry.source, childFrame.contentWindow);
-
- resolve();
- });
- });
- document.body.appendChild(childFrame);
- await loadChildFramePromise;
-
- // Remove child frame;
- childFrame.parentNode.removeChild(childFrame);
-
- // Child PerformanceMark source should be null after the child frame is
- // removed.
- assert_equals(childEntry.source, null);
- }, 'PerformanceEntry source is null when the window it points to is removed.')
-</script>
\ No newline at end of file
