Attachment 340600 Details for Bug 185737 – WIP patch

[patch] WIP patch

185737_WindowProxy_RefCounted_wip.patch (text/plain), 15.25 KB, created by Chris Dumez on 2018-05-17 10:55:13 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-05-17 10:55:13 PDT

Size: 15.25 KB

patch

obsolete

>diff --git a/Source/WebCore/bindings/js/JSWindowProxy.cpp b/Source/WebCore/bindings/js/JSWindowProxy.cpp
>index a4b1805c652..2b819345f4a 100644
>--- a/Source/WebCore/bindings/js/JSWindowProxy.cpp
>+++ b/Source/WebCore/bindings/js/JSWindowProxy.cpp
>@@ -146,10 +146,10 @@ AbstractDOMWindow& JSWindowProxy::wrapped() const
> 
> JSValue toJS(ExecState* state, WindowProxy& windowProxy)
> {
>-    return &windowProxy.jsWindowProxy(currentWorld(*state));
>+    return windowProxy.jsWindowProxy(currentWorld(*state));
> }
> 
>-JSWindowProxy& toJSWindowProxy(WindowProxy& windowProxy, DOMWrapperWorld& world)
>+JSWindowProxy* toJSWindowProxy(WindowProxy& windowProxy, DOMWrapperWorld& world)
> {
>     return windowProxy.jsWindowProxy(world);
> }
>diff --git a/Source/WebCore/bindings/js/JSWindowProxy.h b/Source/WebCore/bindings/js/JSWindowProxy.h
>index e6f171c6869..08093dc6e17 100644
>--- a/Source/WebCore/bindings/js/JSWindowProxy.h
>+++ b/Source/WebCore/bindings/js/JSWindowProxy.h
>@@ -77,8 +77,8 @@ inline JSC::JSValue toJS(JSC::ExecState* state, WindowProxy* windowProxy) { retu
> inline JSC::JSValue toJS(JSC::ExecState* state, JSDOMGlobalObject*, WindowProxy& windowProxy) { return toJS(state, windowProxy); }
> inline JSC::JSValue toJS(JSC::ExecState* state, JSDOMGlobalObject* globalObject, WindowProxy* windowProxy) { return windowProxy ? toJS(state, globalObject, *windowProxy) : JSC::jsNull(); }
> 
>-JSWindowProxy& toJSWindowProxy(WindowProxy&, DOMWrapperWorld&);
>-inline JSWindowProxy* toJSWindowProxy(WindowProxy* windowProxy, DOMWrapperWorld& world) { return windowProxy ? &toJSWindowProxy(*windowProxy, world) : nullptr; }
>+JSWindowProxy* toJSWindowProxy(WindowProxy&, DOMWrapperWorld&);
>+inline JSWindowProxy* toJSWindowProxy(WindowProxy* windowProxy, DOMWrapperWorld& world) { return windowProxy ? toJSWindowProxy(*windowProxy, world) : nullptr; }
> 
> 
> template<> struct JSDOMWrapperConverterTraits<WindowProxy> {
>diff --git a/Source/WebCore/bindings/js/ScriptController.cpp b/Source/WebCore/bindings/js/ScriptController.cpp
>index 6e2e408264d..e9a3b69fedd 100644
>--- a/Source/WebCore/bindings/js/ScriptController.cpp
>+++ b/Source/WebCore/bindings/js/ScriptController.cpp
>@@ -117,7 +117,7 @@ JSValue ScriptController::evaluateInWorld(const ScriptSourceCode& sourceCode, DO
>     // and false for <script>doSomething()</script>. Check if it has the
>     // expected value in all cases.
>     // See smart window.open policy for where this is used.
>-    auto& proxy = windowProxy().jsWindowProxy(world);
>+    auto& proxy = jsWindowProxy(world);
>     auto& exec = *proxy.window()->globalExec();
>     const String* savedSourceURL = m_sourceURL;
>     m_sourceURL = &sourceURL;
>@@ -150,7 +150,7 @@ void ScriptController::loadModuleScriptInWorld(LoadableModuleScript& moduleScrip
> {
>     JSLockHolder lock(world.vm());
> 
>-    auto& proxy = windowProxy().jsWindowProxy(world);
>+    auto& proxy = jsWindowProxy(world);
>     auto& state = *proxy.window()->globalExec();
> 
>     auto& promise = JSMainThreadExecState::loadModule(state, moduleName, JSC::JSScriptFetchParameters::create(state.vm(), WTFMove(topLevelFetchParameters)), JSC::JSScriptFetcher::create(state.vm(), { &moduleScript }));
>@@ -166,7 +166,7 @@ void ScriptController::loadModuleScriptInWorld(LoadableModuleScript& moduleScrip
> {
>     JSLockHolder lock(world.vm());
> 
>-    auto& proxy = windowProxy().jsWindowProxy(world);
>+    auto& proxy = jsWindowProxy(world);
>     auto& state = *proxy.window()->globalExec();
> 
>     auto& promise = JSMainThreadExecState::loadModule(state, sourceCode.jsSourceCode(), JSC::JSScriptFetcher::create(state.vm(), { &moduleScript }));
>@@ -182,7 +182,7 @@ JSC::JSValue ScriptController::linkAndEvaluateModuleScriptInWorld(LoadableModule
> {
>     JSLockHolder lock(world.vm());
> 
>-    auto& proxy = windowProxy().jsWindowProxy(world);
>+    auto& proxy = jsWindowProxy(world);
>     auto& state = *proxy.window()->globalExec();
> 
>     // FIXME: Preventing Frame from being destroyed is essentially unnecessary.
>@@ -211,7 +211,7 @@ JSC::JSValue ScriptController::evaluateModule(const URL& sourceURL, JSModuleReco
> 
>     const auto& jsSourceCode = moduleRecord.sourceCode();
> 
>-    auto& proxy = windowProxy().jsWindowProxy(world);
>+    auto& proxy = jsWindowProxy(world);
>     auto& state = *proxy.window()->globalExec();
>     SetForScope<const String*> sourceURLScope(m_sourceURL, &sourceURL.string());
> 
>@@ -268,7 +268,7 @@ static Identifier jsValueToModuleKey(ExecState* exec, JSValue value)
> 
> void ScriptController::setupModuleScriptHandlers(LoadableModuleScript& moduleScriptRef, JSInternalPromise& promise, DOMWrapperWorld& world)
> {
>-    auto& proxy = windowProxy().jsWindowProxy(world);
>+    auto& proxy = jsWindowProxy(world);
>     auto& state = *proxy.window()->globalExec();
> 
>     // It is not guaranteed that either fulfillHandler or rejectHandler is eventually called.
>@@ -325,6 +325,13 @@ WindowProxy& ScriptController::windowProxy()
>     return m_frame.windowProxy();
> }
> 
>+JSWindowProxy& ScriptController::jsWindowProxy(DOMWrapperWorld& world)
>+{
>+    auto* jsWindowProxy = m_frame.windowProxy().jsWindowProxy(world);
>+    ASSERT_WITH_MESSAGE(jsWindowProxy, "The JSWindowProxy can only be null if the frame has been destroyed");
>+    return *jsWindowProxy;
>+}
>+
> TextPosition ScriptController::eventHandlerPosition() const
> {
>     // FIXME: If we are not currently parsing, we should use our current location
>@@ -442,7 +449,7 @@ NPObject* ScriptController::windowScriptNPObject()
>         if (canExecuteScripts(NotAboutToExecuteScript)) {
>             // JavaScript is enabled, so there is a JavaScript window object.
>             // Return an NPObject bound to the window object.
>-            auto* window = windowProxy().jsWindowProxy(pluginWorld()).window();
>+            auto* window = jsWindowProxy(pluginWorld()).window();
>             ASSERT(window);
>             Bindings::RootObject* root = bindingRootObject();
>             m_windowScriptNPObject = _NPN_CreateScriptObject(0, window, root);
>@@ -603,7 +610,7 @@ bool ScriptController::executeIfJavaScriptURL(const URL& url, ShouldReplaceDocum
>         return true;
> 
>     String scriptResult;
>-    if (!result || !result.getString(windowProxy().jsWindowProxy(mainThreadNormalWorld()).window()->globalExec(), scriptResult))
>+    if (!result || !result.getString(jsWindowProxy(mainThreadNormalWorld()).window()->globalExec(), scriptResult))
>         return true;
> 
>     // FIXME: We should always replace the document, but doing so
>diff --git a/Source/WebCore/bindings/js/ScriptController.h b/Source/WebCore/bindings/js/ScriptController.h
>index d6e59001f28..14b562f7631 100644
>--- a/Source/WebCore/bindings/js/ScriptController.h
>+++ b/Source/WebCore/bindings/js/ScriptController.h
>@@ -83,7 +83,11 @@ public:
> 
>     JSDOMWindow* globalObject(DOMWrapperWorld& world)
>     {
>-        return JSC::jsCast<JSDOMWindow*>(windowProxy().jsWindowProxy(world).window());
>+        // If the frame has been detached from its page, then the global object should be null to avoid running script in this frame.
>+        if (!m_frame.page())
>+            return nullptr;
>+
>+        return JSC::jsCast<JSDOMWindow*>(jsWindowProxy(world).window());
>     }
> 
>     static void getAllWorlds(Vector<Ref<DOMWrapperWorld>>&);
>@@ -166,6 +170,7 @@ private:
>     void disconnectPlatformScriptObjects();
> 
>     WEBCORE_EXPORT WindowProxy& windowProxy();
>+    JSWindowProxy& jsWindowProxy(DOMWrapperWorld&);
> 
>     Frame& m_frame;
>     const String* m_sourceURL;
>diff --git a/Source/WebCore/bindings/js/ScriptControllerMac.mm b/Source/WebCore/bindings/js/ScriptControllerMac.mm
>index 3ec416585ba..de7938e9f6a 100644
>--- a/Source/WebCore/bindings/js/ScriptControllerMac.mm
>+++ b/Source/WebCore/bindings/js/ScriptControllerMac.mm
>@@ -103,7 +103,7 @@ WebScriptObject *ScriptController::windowScriptObject()
>     if (!m_windowScriptObject) {
>         JSC::JSLockHolder lock(commonVM());
>         JSC::Bindings::RootObject* root = bindingRootObject();
>-        m_windowScriptObject = [WebScriptObject scriptObjectForJSObject:toRef(&windowProxy().jsWindowProxy(pluginWorld())) originRootObject:root rootObject:root];
>+        m_windowScriptObject = [WebScriptObject scriptObjectForJSObject:toRef(&jsWindowProxy(pluginWorld())) originRootObject:root rootObject:root];
>     }
> 
>     return m_windowScriptObject.get();
>diff --git a/Source/WebCore/bindings/js/ScriptState.cpp b/Source/WebCore/bindings/js/ScriptState.cpp
>index 31b6e784234..eee839cc426 100644
>--- a/Source/WebCore/bindings/js/ScriptState.cpp
>+++ b/Source/WebCore/bindings/js/ScriptState.cpp
>@@ -75,7 +75,7 @@ JSC::ExecState* mainWorldExecState(Frame* frame)
> {
>     if (!frame)
>         return nullptr;
>-    return frame->windowProxy().jsWindowProxy(mainThreadNormalWorld()).window()->globalExec();
>+    return frame->windowProxy().jsWindowProxy(mainThreadNormalWorld())->window()->globalExec();
> }
> 
> JSC::ExecState* execStateFromNode(DOMWrapperWorld& world, Node* node)
>diff --git a/Source/WebCore/bindings/js/WindowProxy.cpp b/Source/WebCore/bindings/js/WindowProxy.cpp
>index 9018d23d23c..dcf1e0e560c 100644
>--- a/Source/WebCore/bindings/js/WindowProxy.cpp
>+++ b/Source/WebCore/bindings/js/WindowProxy.cpp
>@@ -49,12 +49,22 @@ static void collectGarbageAfterWindowProxyDestruction()
> }
> 
> WindowProxy::WindowProxy(AbstractFrame& frame)
>-    : m_frame(frame)
>+    : m_frame(&frame)
> {
> }
> 
> WindowProxy::~WindowProxy()
> {
>+    ASSERT(!m_frame);
>+    ASSERT(m_jsWindowProxies.isEmpty());
>+}
>+
>+void WindowProxy::detachFromFrame()
>+{
>+    ASSERT(m_frame);
>+
>+    m_frame = nullptr;
>+
>     // It's likely that destroying windowProxies will create a lot of garbage.
>     if (!m_jsWindowProxies.isEmpty()) {
>         while (!m_jsWindowProxies.isEmpty()) {
>@@ -75,12 +85,14 @@ void WindowProxy::destroyJSWindowProxy(DOMWrapperWorld& world)
> 
> JSWindowProxy& WindowProxy::createJSWindowProxy(DOMWrapperWorld& world)
> {
>+    ASSERT(m_frame);
>+
>     ASSERT(!m_jsWindowProxies.contains(&world));
>-    ASSERT(m_frame.window());
>+    ASSERT(m_frame->window());
> 
>     VM& vm = world.vm();
> 
>-    Strong<JSWindowProxy> jsWindowProxy(vm, &JSWindowProxy::create(vm, *m_frame.window(), world));
>+    Strong<JSWindowProxy> jsWindowProxy(vm, &JSWindowProxy::create(vm, *m_frame->window(), world));
>     Strong<JSWindowProxy> jsWindowProxy2(jsWindowProxy);
>     m_jsWindowProxies.add(&world, jsWindowProxy);
>     world.didCreateWindowProxy(this);
>@@ -94,15 +106,19 @@ Vector<JSC::Strong<JSWindowProxy>> WindowProxy::jsWindowProxiesAsVector() const
> 
> JSDOMGlobalObject* WindowProxy::globalObject(DOMWrapperWorld& world)
> {
>-    return jsWindowProxy(world).window();
>+    if (auto* windowProxy = jsWindowProxy(world))
>+        return windowProxy->window();
>+    return nullptr;
> }
> 
> JSWindowProxy& WindowProxy::createJSWindowProxyWithInitializedScript(DOMWrapperWorld& world)
> {
>+    ASSERT(m_frame);
>+
>     JSLockHolder lock(world.vm());
>     auto& windowProxy = createJSWindowProxy(world);
>-    if (is<Frame>(m_frame))
>-        downcast<Frame>(m_frame).script().initScriptForWindowProxy(windowProxy);
>+    if (is<Frame>(*m_frame))
>+        downcast<Frame>(*m_frame).script().initScriptForWindowProxy(windowProxy);
>     return windowProxy;
> }
> 
>@@ -137,6 +153,8 @@ void WindowProxy::setDOMWindow(AbstractDOMWindow* newDOMWindow)
>     if (m_jsWindowProxies.isEmpty())
>         return;
> 
>+    ASSERT(m_frame);
>+
>     JSLockHolder lock(commonVM());
> 
>     for (auto& windowProxy : jsWindowProxiesAsVector()) {
>@@ -147,8 +165,8 @@ void WindowProxy::setDOMWindow(AbstractDOMWindow* newDOMWindow)
> 
>         ScriptController* scriptController = nullptr;
>         Page* page = nullptr;
>-        if (is<Frame>(m_frame)) {
>-            auto& frame = downcast<Frame>(m_frame);
>+        if (is<Frame>(*m_frame)) {
>+            auto& frame = downcast<Frame>(*m_frame);
>             scriptController = &frame.script();
>             page = frame.page();
>         }
>@@ -173,17 +191,7 @@ void WindowProxy::attachDebugger(JSC::Debugger* debugger)
> 
> AbstractDOMWindow* WindowProxy::window() const
> {
>-    return m_frame.window();
>-}
>-
>-void WindowProxy::ref()
>-{
>-    m_frame.ref();
>-}
>-
>-void WindowProxy::deref()
>-{
>-    m_frame.deref();
>+    return m_frame ? m_frame->window() : nullptr;
> }
> 
> } // namespace WebCore
>diff --git a/Source/WebCore/bindings/js/WindowProxy.h b/Source/WebCore/bindings/js/WindowProxy.h
>index 11f02248b7a..4722e493b06 100644
>--- a/Source/WebCore/bindings/js/WindowProxy.h
>+++ b/Source/WebCore/bindings/js/WindowProxy.h
>@@ -23,6 +23,7 @@
> #include "DOMWrapperWorld.h"
> #include <JavaScriptCore/Strong.h>
> #include <wtf/HashMap.h>
>+#include <wtf/RefCounted.h>
> 
> namespace JSC {
> class Debugger;
>@@ -35,15 +36,20 @@ class AbstractFrame;
> class JSDOMGlobalObject;
> class JSWindowProxy;
> 
>-class WindowProxy {
>+class WindowProxy : public RefCounted<WindowProxy> {
>     WTF_MAKE_FAST_ALLOCATED;
> public:
>     using ProxyMap = HashMap<RefPtr<DOMWrapperWorld>, JSC::Strong<JSWindowProxy>>;
> 
>-    explicit WindowProxy(AbstractFrame&);
>-    ~WindowProxy();
>+    static Ref<WindowProxy> create(AbstractFrame& frame)
>+    {
>+        return adoptRef(*new WindowProxy(frame));
>+    }
> 
>-    AbstractFrame& frame() const { return m_frame; }
>+    WEBCORE_EXPORT ~WindowProxy();
>+
>+    AbstractFrame* frame() const { return m_frame; }
>+    void detachFromFrame();
> 
>     void destroyJSWindowProxy(DOMWrapperWorld&);
> 
>@@ -53,13 +59,15 @@ public:
>     ProxyMap releaseJSWindowProxies() { return std::exchange(m_jsWindowProxies, ProxyMap()); }
>     void setJSWindowProxies(ProxyMap&& windowProxies) { m_jsWindowProxies = WTFMove(windowProxies); }
> 
>-    JSWindowProxy& jsWindowProxy(DOMWrapperWorld& world)
>+    JSWindowProxy* jsWindowProxy(DOMWrapperWorld& world)
>     {
>-        auto it = m_jsWindowProxies.find(&world);
>-        if (it != m_jsWindowProxies.end())
>-            return *it->value.get();
>+        if (!m_frame)
>+            return nullptr;
> 
>-        return createJSWindowProxyWithInitializedScript(world);
>+        if (auto* existingProxy = existingJSWindowProxy(world))
>+            return existingProxy;
>+
>+        return &createJSWindowProxyWithInitializedScript(world);
>     }
> 
>     JSWindowProxy* existingJSWindowProxy(DOMWrapperWorld& world) const
>@@ -79,14 +87,13 @@ public:
> 
>     WEBCORE_EXPORT AbstractDOMWindow* window() const;
> 
>-    WEBCORE_EXPORT void ref();
>-    WEBCORE_EXPORT void deref();
>-
> private:
>+    explicit WindowProxy(AbstractFrame&);
>+
>     JSWindowProxy& createJSWindowProxy(DOMWrapperWorld&);
>     WEBCORE_EXPORT JSWindowProxy& createJSWindowProxyWithInitializedScript(DOMWrapperWorld&);
> 
>-    AbstractFrame& m_frame;
>+    AbstractFrame* m_frame;
>     ProxyMap m_jsWindowProxies;
> };
> 
>diff --git a/Source/WebCore/page/AbstractFrame.cpp b/Source/WebCore/page/AbstractFrame.cpp
>index 1fded40ea04..594a55e1045 100644
>--- a/Source/WebCore/page/AbstractFrame.cpp
>+++ b/Source/WebCore/page/AbstractFrame.cpp
>@@ -31,12 +31,13 @@
> namespace WebCore {
> 
> AbstractFrame::AbstractFrame()
>-    : m_windowProxy(makeUniqueRef<WindowProxy>(*this))
>+    : m_windowProxy(WindowProxy::create(*this))
> {
> }
> 
> AbstractFrame::~AbstractFrame()
> {
>+    m_windowProxy->detachFromFrame();
> }
> 
> } // namespace WebCore
>diff --git a/Source/WebCore/page/AbstractFrame.h b/Source/WebCore/page/AbstractFrame.h
>index c65b3ac487b..568c6fc1c6d 100644
>--- a/Source/WebCore/page/AbstractFrame.h
>+++ b/Source/WebCore/page/AbstractFrame.h
>@@ -52,7 +52,7 @@ protected:
> private:
>     virtual AbstractDOMWindow* virtualWindow() const = 0;
> 
>-    UniqueRef<WindowProxy> m_windowProxy;
>+    Ref<WindowProxy> m_windowProxy;
> };
> 
> } // namespace WebCore

Actions: View | Formatted Diff | Diff

Attachments on bug 185737: 340598 | 340600 | 340607 | 340612 | 340619 | 340631 | 340647 | 340707