Changeset 254328 in webkit

Timestamp:

Jan 10, 2020 12:42:11 AM (4 years ago)

Author:

Carlos Garcia Campos

Message:

Automation: scripts are executed in the wrong js context after a history navigation
​https://bugs.webkit.org/show_bug.cgi?id=204880
<rdar://problem/58413615>

Reviewed by Brian Burg.

After a history navigation we use the script object from the previous frame js context because
didClearWindowObjectForFrame() is not called in that case. We are caching the script object for every frame ID,
and after a history navigation the frame ID is the same, but the frame js context isn't. That also means we might
be leaking the script objects in those cases, because we end up calling JSValueUnprotect with the wrong
context. It would be easier to set the script object as a property of the global object and let JSC handle the
lifetime. Instead of caching the script object and protect/unprotect it, we just check if the global object of
the current js context has the property or not to get or create it. We use a private symbol as the key of the
global object property to ensure it's not visible.

• WebProcess/Automation/WebAutomationSessionProxy.cpp:

(WebKit::WebAutomationSessionProxy::WebAutomationSessionProxy): Initialize m_scriptObjectIdentifier.
(WebKit::WebAutomationSessionProxy::scriptObject): Helper function to get the script object for the given
JavaScript context.
(WebKit::WebAutomationSessionProxy::setScriptObject): Helper function to set the script object for the given
JavaScript context.
(WebKit::WebAutomationSessionProxy::scriptObjectForFrame): Get or create the script object.
(WebKit::WebAutomationSessionProxy::elementForNodeHandle): Get the script object from global object.
(WebKit::WebAutomationSessionProxy::didClearWindowObjectForFrame): Remove the code to unprotect script objects
of the frame.

• WebProcess/Automation/WebAutomationSessionProxy.h: Add m_scriptObjectIdentifier and remove m_webFrameScriptObjectMap.

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• WebProcess/Automation/WebAutomationSessionProxy.cpp (modified) (5 diffs)
• WebProcess/Automation/WebAutomationSessionProxy.h (modified) (3 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2020-01-10  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        Automation: scripts are executed in the wrong js context after a history navigation
+        https://bugs.webkit.org/show_bug.cgi?id=204880
+        <rdar://problem/58413615>
+
+        Reviewed by Brian Burg.
+
+        After a history navigation we use the script object from the previous frame js context because
+        didClearWindowObjectForFrame() is not called in that case. We are caching the script object for every frame ID,
+        and after a history navigation the frame ID is the same, but the frame js context isn't. That also means we might
+        be leaking the script objects in those cases, because we end up calling JSValueUnprotect with the wrong
+        context. It would be easier to set the script object as a property of the global object and let JSC handle the
+        lifetime. Instead of caching the script object and protect/unprotect it, we just check if the global object of
+        the current js context has the property or not to get or create it. We use a private symbol as the key of the
+        global object property to ensure it's not visible.
+
+        * WebProcess/Automation/WebAutomationSessionProxy.cpp:
+        (WebKit::WebAutomationSessionProxy::WebAutomationSessionProxy): Initialize m_scriptObjectIdentifier.
+        (WebKit::WebAutomationSessionProxy::scriptObject): Helper function to get the script object for the given
+        JavaScript context.
+        (WebKit::WebAutomationSessionProxy::setScriptObject): Helper function to set the script object for the given
+        JavaScript context.
+        (WebKit::WebAutomationSessionProxy::scriptObjectForFrame): Get or create the script object.
+        (WebKit::WebAutomationSessionProxy::elementForNodeHandle): Get the script object from global object.
+        (WebKit::WebAutomationSessionProxy::didClearWindowObjectForFrame): Remove the code to unprotect script objects
+        of the frame.
+        * WebProcess/Automation/WebAutomationSessionProxy.h: Add m_scriptObjectIdentifier and remove m_webFrameScriptObjectMap.
+
 2020-01-09  Ross Kirsling  <ross.kirsling@sony.com>
 

trunk/Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp

@@ -109 +109 @@
 WebAutomationSessionProxy::WebAutomationSessionProxy(const String& sessionIdentifier)
     : m_sessionIdentifier(sessionIdentifier)
+    , m_scriptObjectIdentifier(JSC::PrivateName::Description, "automationSessionProxy"_s)
 {
     WebProcess::singleton().addMessageReceiver(Messages::WebAutomationSessionProxy::messageReceiverName(), *this);
@@ -212 +213 @@
 }
 
+JSObjectRef WebAutomationSessionProxy::scriptObject(JSGlobalContextRef context)
+{
+    JSC::JSGlobalObject* globalObject = toJS(context);
+    JSC::VM& vm = globalObject->vm();
+    JSC::JSLockHolder locker(vm);
+    auto scriptObjectID = JSC::Identifier::fromUid(m_scriptObjectIdentifier);
+    if (!globalObject->hasProperty(globalObject, scriptObjectID))
+        return nullptr;
+
+    return const_cast<JSObjectRef>(toRef(globalObject, globalObject->get(globalObject, scriptObjectID)));
+}
+
+void WebAutomationSessionProxy::setScriptObject(JSGlobalContextRef context, JSObjectRef object)
+{
+    JSC::JSGlobalObject* globalObject = toJS(context);
+    JSC::VM& vm = globalObject->vm();
+    JSC::JSLockHolder locker(vm);
+    auto scriptObjectID = JSC::Identifier::fromUid(m_scriptObjectIdentifier);
+    PutPropertySlot slot(globalObject);
+    globalObject->methodTable(vm)->put(globalObject, globalObject, scriptObjectID, toJS(globalObject, object), slot);
+}
+
 JSObjectRef WebAutomationSessionProxy::scriptObjectForFrame(WebFrame& frame)
 {
-    if (JSObjectRef scriptObject = m_webFrameScriptObjectMap.get(frame.frameID()))
+    JSGlobalContextRef context = frame.jsContext();
+    if (auto* scriptObject = this->scriptObject(context))
         return scriptObject;
 
     JSValueRef exception = nullptr;
-    JSGlobalContextRef context = frame.jsContext();
+    String script = StringImpl::createWithoutCopying(WebAutomationSessionProxyScriptSource, sizeof(WebAutomationSessionProxyScriptSource));
+    JSObjectRef scriptObjectFunction = const_cast<JSObjectRef>(JSEvaluateScript(context, OpaqueJSString::tryCreate(script).get(), nullptr, nullptr, 0, &exception));
+    ASSERT(JSValueIsObject(context, scriptObjectFunction));
 
     JSValueRef sessionIdentifier = toJSValue(context, m_sessionIdentifier);
@@ -224 +250 @@
     JSObjectRef createUUIDFunction = JSObjectMakeFunctionWithCallback(context, nullptr, createUUID);
     JSObjectRef isValidNodeIdentifierFunction = JSObjectMakeFunctionWithCallback(context, nullptr, isValidNodeIdentifier);
-
-    String script = StringImpl::createWithoutCopying(WebAutomationSessionProxyScriptSource, sizeof(WebAutomationSessionProxyScriptSource));
-
-    JSObjectRef scriptObjectFunction = const_cast<JSObjectRef>(JSEvaluateScript(context, OpaqueJSString::tryCreate(script).get(), nullptr, nullptr, 0, &exception));
-    ASSERT(JSValueIsObject(context, scriptObjectFunction));
-
     JSValueRef arguments[] = { sessionIdentifier, evaluateFunction, createUUIDFunction, isValidNodeIdentifierFunction };
     JSObjectRef scriptObject = const_cast<JSObjectRef>(JSObjectCallAsFunction(context, scriptObjectFunction, nullptr, WTF_ARRAY_LENGTH(arguments), arguments, &exception));
     ASSERT(JSValueIsObject(context, scriptObject));
 
-    JSValueProtect(context, scriptObject);
-    m_webFrameScriptObjectMap.add(frame.frameID(), scriptObject);
-
+    setScriptObject(context, scriptObject);
     return scriptObject;
 }
@@ -245 +263 @@
     // does not exist, there are no nodes mapped to handles. Using scriptObjectForFrame()
     // will make a new script object if it can't find one, preventing us from returning fast.
-    JSObjectRef scriptObject = m_webFrameScriptObjectMap.get(frame.frameID());
+    JSGlobalContextRef context = frame.jsContext();
+    auto* scriptObject = this->scriptObject(context);
     if (!scriptObject)
         return nullptr;
-
-    JSGlobalContextRef context = frame.jsContext();
 
     JSValueRef functionArguments[] = {
@@ -269 +286 @@
 void WebAutomationSessionProxy::didClearWindowObjectForFrame(WebFrame& frame)
 {
-    WebCore::FrameIdentifier frameID = frame.frameID();
-    if (JSObjectRef scriptObject = m_webFrameScriptObjectMap.take(frameID))
-        JSValueUnprotect(frame.jsContext(), scriptObject);
-
     String errorMessage = "Callback was not called before the unload event."_s;
     String errorType = Inspector::Protocol::AutomationHelpers::getEnumConstantValue(Inspector::Protocol::Automation::ErrorMessage::JavaScriptError);
 
-    auto pendingFrameCallbacks = m_webFramePendingEvaluateJavaScriptCallbacksMap.take(frameID);
+    auto pendingFrameCallbacks = m_webFramePendingEvaluateJavaScriptCallbacksMap.take(frame.frameID());
     for (uint64_t callbackID : pendingFrameCallbacks)
         WebProcess::singleton().parentProcessConnection()->send(Messages::WebAutomationSession::DidEvaluateJavaScriptFunction(callbackID, errorMessage, errorType), 0);

trunk/Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.h

@@ -29 +29 @@
 #include "CoordinateSystem.h"
 #include <JavaScriptCore/JSBase.h>
+#include <JavaScriptCore/PrivateName.h>
 #include <WebCore/FrameIdentifier.h>
 #include <WebCore/IntRect.h>
@@ -57 +58 @@
 
 private:
+    JSObjectRef scriptObject(JSGlobalContextRef);
+    void setScriptObject(JSGlobalContextRef, JSObjectRef);
     JSObjectRef scriptObjectForFrame(WebFrame&);
     WebCore::Element* elementForNodeHandle(WebFrame&, const String&);
@@ -78 +81 @@
 
     String m_sessionIdentifier;
+    JSC::PrivateName m_scriptObjectIdentifier;
 
-    HashMap<WebCore::FrameIdentifier, JSObjectRef> m_webFrameScriptObjectMap;
     HashMap<WebCore::FrameIdentifier, Vector<uint64_t>> m_webFramePendingEvaluateJavaScriptCallbacksMap;
 };