Revert of binding: Changes the association among global-proxy/global/window-instance (2nd attempt). (patchset #7 id:120001 of https://codereview.chromium.org/2693893007/ )
Reason for revert:
The change should be causing crash issues.
https://crbug.com/712638
https://crbug.com/713676
https://crbug.com/713667
https://crbug.com/715150
The root cause could be different but the CL should have at least triggered the crashes.
Original issue's description:
> binding: Changes the association among global-proxy/global/window-instance (2nd attempt).
>
> The first attempt is: https://crrev.com/2617733004
>
> Since V8 now supports internal fields for the global proxies,
> let's change the mappings among global proxy / global object /
> C++ instance in Blink.
>
> With this CL, the global proxy and C++ instance point to each
> other, and a global object points to the C++ instance (without
> reverse pointer).
>
> Note that this CL makes one big difference. For non-window objects,
> Blink objects always live longer than V8 wrappers. This CL
> introduces an exception; DOMWindow may be collected before the V8
> wrapper object gets collected because the V8 wrapper is the global
> proxy which remains the same while navigations.
>
> BUG=675872
> Review-Url: https://codereview.chromium.org/2693893007
> Cr-Commit-Position: refs/heads/master@{#465197}
> Committed: https://chromium.googlesource.com/chromium/src/+/ae0d679c61a85bc16722724a4a96e30a1dee9dd2
This CL reverts the following CL, too.
v8binding: Makes ToV8(window) work without a frame.
https://codereview.chromium.org/2713623002/
Original issue's description:
> v8binding: Makes ToV8(window) work without a frame.
>
> Makes DOMWindow hold a WindowProxyManager and makes
> it possible that ToV8(window) work without a frame.
>
> With this CL, all tests in
> external/wpt/html/webappapis/scripting/events/compile-event-handler-settings-objects.html
> pass without a failure.
>
> This CL is a preparation for
> https://crrev.com/2693893007
>
> BUG=
>
> Review-Url: https://codereview.chromium.org/2713623002
> Cr-Commit-Position: refs/heads/master@{#464907}
> Committed: https://chromium.googlesource.com/chromium/src/+/58af3664facba6332528fd983988d51cf3636af4
BUG=712638,713676,713667,715150
Review-Url: https://codereview.chromium.org/2848503002
Cr-Commit-Position: refs/heads/master@{#467623}
diff --git a/third_party/WebKit/LayoutTests/external/wpt/html/webappapis/scripting/events/compile-event-handler-settings-objects-expected.txt b/third_party/WebKit/LayoutTests/external/wpt/html/webappapis/scripting/events/compile-event-handler-settings-objects-expected.txt
new file mode 100644
index 0000000..60038a3
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/external/wpt/html/webappapis/scripting/events/compile-event-handler-settings-objects-expected.txt
@@ -0,0 +1,6 @@
+This is a testharness.js-based test.
+PASS The Function instance must be created in the Realm of the node document
+PASS The entry settings object while executing the compiled callback via Web IDL's invoke must be that of the node document
+FAIL The incumbent settings object while executing the compiled callback via Web IDL's invoke must be that of the node document Cannot read property 'name' of undefined
+Harness: the test ran to completion.
+
diff --git a/third_party/WebKit/Source/bindings/bindings.gni b/third_party/WebKit/Source/bindings/bindings.gni
index 6965ab9..55dc1e6 100644
--- a/third_party/WebKit/Source/bindings/bindings.gni
+++ b/third_party/WebKit/Source/bindings/bindings.gni
@@ -133,17 +133,18 @@
"core/v8/SharedPersistent.h",
"core/v8/SourceLocation.cpp",
"core/v8/SourceLocation.h",
+ "core/v8/ToV8.h",
+ "core/v8/ToV8ForCore.cpp",
+ "core/v8/ToV8ForCore.h",
"core/v8/TraceWrapperMember.h",
"core/v8/TraceWrapperV8Reference.h",
"core/v8/Transferables.h",
- "core/v8/ToV8.h",
- "core/v8/ToV8ForCore.h",
"core/v8/UseCounterCallback.cpp",
"core/v8/UseCounterCallback.h",
"core/v8/V8AbstractEventListener.cpp",
"core/v8/V8AbstractEventListener.h",
- "core/v8/V8Binding.h",
"core/v8/V8Binding.cpp",
+ "core/v8/V8Binding.h",
"core/v8/V8BindingForCore.cpp",
"core/v8/V8BindingForCore.h",
"core/v8/V8BindingMacros.h",
diff --git a/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp b/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp
index e8494d5..def966f 100644
--- a/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp
@@ -221,9 +221,8 @@
// The global proxy object. Note this is not the global object.
v8::Local<v8::Object> global_proxy = context->Global();
CHECK(global_proxy_ == global_proxy);
- v8::Local<v8::Object> associated_wrapper =
- AssociateWithWrapper(window, wrapper_type_info, global_proxy);
- DCHECK(associated_wrapper == global_proxy);
+ 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_.Get().SetWrapperClassId(wrapper_type_info->wrapper_class_id);
@@ -231,8 +230,9 @@
// The global object, aka window wrapper object.
v8::Local<v8::Object> window_wrapper =
global_proxy->GetPrototype().As<v8::Object>();
- V8DOMWrapper::SetNativeInfo(GetIsolate(), window_wrapper, wrapper_type_info,
- window);
+ v8::Local<v8::Object> associated_wrapper =
+ AssociateWithWrapper(window, wrapper_type_info, window_wrapper);
+ DCHECK(associated_wrapper == window_wrapper);
// The prototype object of Window interface.
v8::Local<v8::Object> window_prototype =
diff --git a/third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp b/third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp
index 3625ce5d..d10f735e 100644
--- a/third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp
@@ -125,9 +125,8 @@
// The global proxy object. Note this is not the global object.
v8::Local<v8::Object> global_proxy = global_proxy_.NewLocal(GetIsolate());
- v8::Local<v8::Object> associated_wrapper =
- AssociateWithWrapper(window, wrapper_type_info, global_proxy);
- DCHECK(associated_wrapper == global_proxy);
+ 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_.Get().SetWrapperClassId(wrapper_type_info->wrapper_class_id);
@@ -135,8 +134,9 @@
// The global object, aka window wrapper object.
v8::Local<v8::Object> window_wrapper =
global_proxy->GetPrototype().As<v8::Object>();
- V8DOMWrapper::SetNativeInfo(GetIsolate(), window_wrapper, wrapper_type_info,
- window);
+ v8::Local<v8::Object> associated_wrapper =
+ AssociateWithWrapper(window, wrapper_type_info, window_wrapper);
+ DCHECK(associated_wrapper == window_wrapper);
}
} // namespace blink
diff --git a/third_party/WebKit/Source/bindings/core/v8/ToV8ForCore.cpp b/third_party/WebKit/Source/bindings/core/v8/ToV8ForCore.cpp
new file mode 100644
index 0000000..442f1d9
--- /dev/null
+++ b/third_party/WebKit/Source/bindings/core/v8/ToV8ForCore.cpp
@@ -0,0 +1,48 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "bindings/core/v8/ToV8ForCore.h"
+
+#include "bindings/core/v8/WindowProxy.h"
+#include "core/events/EventTarget.h"
+#include "core/frame/DOMWindow.h"
+#include "core/frame/Frame.h"
+
+namespace blink {
+
+v8::Local<v8::Value> ToV8(DOMWindow* window,
+ v8::Local<v8::Object> creation_context,
+ v8::Isolate* isolate) {
+ // Notice that we explicitly ignore creationContext because the DOMWindow
+ // has its own creationContext.
+
+ if (UNLIKELY(!window))
+ return v8::Null(isolate);
+
+ // TODO(yukishiino): Get understanding of why it's possible to initialize
+ // the context after the frame is detached. And then, remove the following
+ // lines. See also https://crbug.com/712638 .
+ Frame* frame = window->GetFrame();
+ if (!frame)
+ return v8::Local<v8::Object>();
+
+ // 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.
+ return frame->GetWindowProxy(DOMWrapperWorld::Current(isolate))
+ ->GlobalProxyIfNotDetached();
+}
+
+v8::Local<v8::Value> ToV8(EventTarget* impl,
+ v8::Local<v8::Object> creation_context,
+ v8::Isolate* isolate) {
+ if (UNLIKELY(!impl))
+ return v8::Null(isolate);
+
+ if (impl->InterfaceName() == EventTargetNames::DOMWindow)
+ return ToV8(static_cast<DOMWindow*>(impl), creation_context, isolate);
+ return ToV8(static_cast<ScriptWrappable*>(impl), creation_context, isolate);
+}
+
+} // namespace blink
diff --git a/third_party/WebKit/Source/bindings/core/v8/ToV8ForCore.h b/third_party/WebKit/Source/bindings/core/v8/ToV8ForCore.h
index be7c1ebc..077c84a3 100644
--- a/third_party/WebKit/Source/bindings/core/v8/ToV8ForCore.h
+++ b/third_party/WebKit/Source/bindings/core/v8/ToV8ForCore.h
@@ -11,11 +11,26 @@
#include "bindings/core/v8/ToV8.h"
#include "bindings/core/v8/V8NodeFilterCondition.h"
#include "core/dom/ArrayBufferViewHelpers.h"
+#include "core/dom/Node.h"
#include "v8/include/v8.h"
namespace blink {
class Dictionary;
+class DOMWindow;
+class EventTarget;
+
+CORE_EXPORT v8::Local<v8::Value> ToV8(DOMWindow*,
+ v8::Local<v8::Object> creation_context,
+ v8::Isolate*);
+CORE_EXPORT v8::Local<v8::Value> ToV8(EventTarget*,
+ v8::Local<v8::Object> creation_context,
+ v8::Isolate*);
+inline v8::Local<v8::Value> ToV8(Node* node,
+ v8::Local<v8::Object> creation_context,
+ v8::Isolate* isolate) {
+ return ToV8(static_cast<ScriptWrappable*>(node), creation_context, isolate);
+}
inline v8::Local<v8::Value> ToV8(const Dictionary& value,
v8::Local<v8::Object> creation_context,
diff --git a/third_party/WebKit/Source/bindings/core/v8/V8BindingForCore.h b/third_party/WebKit/Source/bindings/core/v8/V8BindingForCore.h
index a5433d2..a8bdd423 100644
--- a/third_party/WebKit/Source/bindings/core/v8/V8BindingForCore.h
+++ b/third_party/WebKit/Source/bindings/core/v8/V8BindingForCore.h
@@ -40,6 +40,7 @@
#include "bindings/core/v8/ScriptState.h"
#include "bindings/core/v8/ScriptValue.h"
#include "bindings/core/v8/ScriptWrappable.h"
+#include "bindings/core/v8/ToV8ForCore.h"
#include "bindings/core/v8/V8Binding.h"
#include "bindings/core/v8/V8BindingMacros.h"
#include "bindings/core/v8/V8PerIsolateData.h"
@@ -49,6 +50,7 @@
#include "bindings/core/v8/V8ValueCache.h"
#include "core/CoreExport.h"
#include "core/dom/ArrayBufferViewHelpers.h"
+#include "core/dom/Node.h"
#include "platform/heap/Handle.h"
#include "platform/wtf/text/AtomicString.h"
#include "platform/wtf/text/StringView.h"
@@ -63,6 +65,7 @@
class DOMWindow;
class EventListener;
+class EventTarget;
class ExceptionState;
class ExecutionContext;
class FlexibleArrayBufferView;
@@ -72,6 +75,88 @@
class NodeFilter;
class XPathNSResolver;
+template <typename CallbackInfo>
+inline void V8SetReturnValue(const CallbackInfo& callback_info,
+ DOMWindow* impl) {
+ V8SetReturnValue(callback_info, ToV8(impl, callback_info.Holder(),
+ callback_info.GetIsolate()));
+}
+
+template <typename CallbackInfo>
+inline void V8SetReturnValue(const CallbackInfo& callback_info,
+ EventTarget* impl) {
+ V8SetReturnValue(callback_info, ToV8(impl, callback_info.Holder(),
+ callback_info.GetIsolate()));
+}
+
+template <typename CallbackInfo>
+inline void V8SetReturnValue(const CallbackInfo& callback_info, Node* impl) {
+ V8SetReturnValue(callback_info, static_cast<ScriptWrappable*>(impl));
+}
+
+template <typename CallbackInfo>
+inline void V8SetReturnValueForMainWorld(const CallbackInfo& callback_info,
+ DOMWindow* impl) {
+ V8SetReturnValue(callback_info, ToV8(impl, callback_info.Holder(),
+ callback_info.GetIsolate()));
+}
+
+template <typename CallbackInfo>
+inline void V8SetReturnValueForMainWorld(const CallbackInfo& callback_info,
+ EventTarget* impl) {
+ V8SetReturnValue(callback_info, ToV8(impl, callback_info.Holder(),
+ callback_info.GetIsolate()));
+}
+
+template <typename CallbackInfo>
+inline void V8SetReturnValueForMainWorld(const CallbackInfo& callback_info,
+ Node* impl) {
+ // Since EventTarget has a special version of ToV8 and V8EventTarget.h
+ // defines its own v8SetReturnValue family, which are slow, we need to
+ // override them with optimized versions for Node and its subclasses.
+ // Without this overload, V8SetReturnValueForMainWorld for Node would be
+ // very slow.
+ //
+ // class hierarchy:
+ // ScriptWrappable <-- EventTarget <--+-- Node <-- ...
+ // +-- Window
+ // overloads:
+ // V8SetReturnValueForMainWorld(ScriptWrappable*)
+ // Optimized and very fast.
+ // V8SetReturnValueForMainWorld(EventTarget*)
+ // Uses custom ToV8 function and slow.
+ // V8SetReturnValueForMainWorld(Node*)
+ // Optimized and very fast.
+ // V8SetReturnValueForMainWorld(Window*)
+ // Uses custom ToV8 function and slow.
+ V8SetReturnValueForMainWorld(callback_info,
+ static_cast<ScriptWrappable*>(impl));
+}
+
+template <typename CallbackInfo>
+inline void V8SetReturnValueFast(const CallbackInfo& callback_info,
+ DOMWindow* impl,
+ const ScriptWrappable*) {
+ V8SetReturnValue(callback_info, ToV8(impl, callback_info.Holder(),
+ callback_info.GetIsolate()));
+}
+
+template <typename CallbackInfo>
+inline void V8SetReturnValueFast(const CallbackInfo& callback_info,
+ EventTarget* impl,
+ const ScriptWrappable*) {
+ V8SetReturnValue(callback_info, ToV8(impl, callback_info.Holder(),
+ callback_info.GetIsolate()));
+}
+
+template <typename CallbackInfo>
+inline void V8SetReturnValueFast(const CallbackInfo& callback_info,
+ Node* impl,
+ const ScriptWrappable* wrappable) {
+ V8SetReturnValueFast(callback_info, static_cast<ScriptWrappable*>(impl),
+ wrappable);
+}
+
template <typename CallbackInfo, typename T>
inline void V8SetReturnValue(const CallbackInfo& callbackInfo,
NotShared<T> notShared) {
diff --git a/third_party/WebKit/Source/core/frame/DOMWindow.cpp b/third_party/WebKit/Source/core/frame/DOMWindow.cpp
index b3ce2ee..308c99c1 100644
--- a/third_party/WebKit/Source/core/frame/DOMWindow.cpp
+++ b/third_party/WebKit/Source/core/frame/DOMWindow.cpp
@@ -40,30 +40,12 @@
DOMWindow::~DOMWindow() {
// The frame must be disconnected before finalization.
DCHECK(!frame_);
-
- // Because the wrapper object of a DOMWindow is the global proxy, which may
- // live much longer than the DOMWindow, we need to dissociate all wrappers
- // for this instance.
- DOMWrapperWorld::DissociateDOMWindowWrappersInAllWorlds(this);
}
v8::Local<v8::Object> DOMWindow::Wrap(v8::Isolate* isolate,
v8::Local<v8::Object> creation_context) {
- // Notice that we explicitly ignore |creation_context| because the DOMWindow
- // has its own creation context.
-
- // TODO(yukishiino): Get understanding of why it's possible to initialize
- // the context after the frame is detached. And then, remove the following
- // lines. See also https://crbug.com/712638 .
- if (!GetFrame())
- return v8::Local<v8::Object>();
-
- // 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.
- return window_proxy_manager_
- ->GetWindowProxy(DOMWrapperWorld::Current(isolate))
- ->GlobalProxyIfNotDetached();
+ NOTREACHED();
+ return v8::Local<v8::Object>();
}
v8::Local<v8::Object> DOMWindow::AssociateWithWrapper(
