Bug 155221 – Add WebAutomationSessionProxy for WebProcess side automation tasks

• Component: WebKit2
• Status: RESOLVED FIXED

[Timothy Hatcher]

We need to do some work directly on the WebProcess side, so set up a WebAutomationSessionProxy object and messages.

[Radar WebKit Bug Importer]

<rdar://problem/25054868>

[Timothy Hatcher]

Created attachment 273402 [details]
WIP Sketch

This builds but I didn't test it in any way.

[Timothy Hatcher]

Comment on attachment 273402 [details]
WIP Sketch

View in context: https://bugs.webkit.org/attachment.cgi?id=273402&action=review

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.h:53
> +class WebAutomationSession final : public API::ObjectImpl<API::Object::Type::AutomationSession>, public IPC::MessageReceiver

This needs to register as a message receiver with the process pool when WebAutomationSession::setProcessPool is called.

processPool->addMessageReceiver(Messages::WebAutomationSession::messageReceiverName(), *this);

And unregister is the destructor.

[Blaze Burg]

Comment on attachment 273402 [details]
WIP Sketch

View in context: https://bugs.webkit.org/attachment.cgi?id=273402&action=review

> Source/WebKit2/DerivedSources.make:113
> +    WebAutomationSessionProxy \

Need the analogue changes for CMakeLists.txt, too.

> Source/WebKit2/WebProcess/WebProcess.h:355
> +    RefPtr<WebAutomationSessionProxy> m_automationSessionProxy;

Shouldn't this be owned by the WebProcess class? Not seeing the need for RefPtr in my imagination yet..

[Timothy Hatcher]

Comment on attachment 273402 [details]
WIP Sketch

View in context: https://bugs.webkit.org/attachment.cgi?id=273402&action=review

>> Source/WebKit2/WebProcess/WebProcess.h:355
>> +    RefPtr<WebAutomationSessionProxy> m_automationSessionProxy;
> 
> Shouldn't this be owned by the WebProcess class? Not seeing the need for RefPtr in my imagination yet..

You are right. It could be an OwnedPtr that we create and destroy as needed.

[Timothy Hatcher]

Created attachment 273551 [details]
WIP Sketch v2

This adds an injected script to the WebProcess side so it is easy to implement the JS heavy features like element mapping and evaluating scripts.

This patch replaces the refresh end point and calls a test function on the injected script. I verified this worked before passing out.

[Timothy Hatcher]

Comment on attachment 273551 [details]
WIP Sketch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=273551&action=review

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:258
> +    page->process().send(Messages::WebAutomationSessionProxy::EvaluateJavaScript(page->mainFrame()->frameID(), WTF::emptyString()), 0);

Note: mainFrame can be null. It is null when the WebPageProxy is first created. I am not sure on when it is available yet, be we should null check it.

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.messages.in:25
> +messages -> WebAutomationSession {
> +    Test()
> +}

We will likely need these messages to the UIProcess to implement execute_async or any other events.

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:101
> +    JSRetainPtr<JSStringRef> testString(Adopt, JSStringCreateWithUTF8CString("test"));
> +    JSObjectRef testFunction = (JSObjectRef)JSObjectGetProperty(context, scriptObject, testString.get(), &exception);
> +    RELEASE_ASSERT(!exception);
> +    RELEASE_ASSERT(JSObjectIsFunction(context, testFunction));
> +
> +    JSObjectCallAsFunction(context, testFunction, nullptr, 0, nullptr, &exception);
> +    RELEASE_ASSERT(!exception);

Just a test. This would call JSEvaluateScript and pass the result to the injected script for processing (turning elements into handles, etc.)

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:43
> +    test()
> +    {
> +        console.log("Hello World!");
> +    }

This gets called in the end by requesting the refresh endpoint right now.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1584
> +    if (auto automationSessionProxy = WebProcess::singleton().automationSessionProxy())
> +        automationSessionProxy->didClearWindowObjectForFrame(*m_frame);

Needed so we unprotect our script object ref.

[Timothy Hatcher]

Created attachment 274056 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 274056 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274056&action=review

Looks good, I'll let Brian review the Automation bits.

> Source/WebKit2/CMakeLists.txt:799
> +    COMMAND ${CMAKE_COMMAND} -E echo "//# sourceURL=__WebInspectorInjectedScript__" > ${DERIVED_SOURCES_WEBKIT2_DIR}/WebAutomationSessionProxy.min.js

Should we be reusing the sourceURL "__WebInspectorInjectedScript__" here? Perhaps "__WebAutomationInjectedScript__"?

If we want this to be hidden from WebInspector we can update: Utilities.js

    function isWebInspectorDebugScript(url)
    {
        return url && url.startsWith("__WebInspector");
    }

To just check "__Web" or something.

> Source/WebKit2/DerivedSources.make:236
> +	echo "//# sourceURL=__WebInspectorInjectedScript__" > $(basename $(notdir $<)).min.js

Same comment about the name "WebInspector".

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:92
> +    JSObjectRef scriptObjectFunction = const_cast<JSObjectRef>(JSEvaluateScript(context, toJSString(script).get(), nullptr, nullptr, 0, &exception));

I think JSValueToObject be safer then these casts, is there precedent for just casting?

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:26
> +//# sourceURL=__WebInspectorInjectedScript__

Same comment about the use of "WebInspector" here.

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:32
> +var Object = {}.constructor;
> +
> +var AutomationSessionProxy = class AutomationSessionProxy

Use `let` instead of `var? `Object` and evaluate/createUUID aren't used yet, but I expect they will be soon.

[Blaze Burg]

Comment on attachment 274056 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274056&action=review

> Source/WebKit2/CMakeLists.txt:795
> +add_custom_command(

This command should go after the list(APPEND WebKit2_HEADERS for the inspector generated files.

> Source/WebKit2/CMakeLists.txt:802
> +    VERBATIM)

The CMake build failed because you forgot the following:

list(APPEND WebKit2_HEADERS ${DERIVED_SOURCES_WEBKIT2_DIR}/WebauotmationSessionProxyScriptSource.h)

[Timothy Hatcher]

Comment on attachment 274056 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274056&action=review

>> Source/WebKit2/CMakeLists.txt:795
>> +add_custom_command(
> 
> This command should go after the list(APPEND WebKit2_HEADERS for the inspector generated files.

Thanks. I was just throwing crap at the wall here.

>> Source/WebKit2/CMakeLists.txt:799
>> +    COMMAND ${CMAKE_COMMAND} -E echo "//# sourceURL=__WebInspectorInjectedScript__" > ${DERIVED_SOURCES_WEBKIT2_DIR}/WebAutomationSessionProxy.min.js
> 
> Should we be reusing the sourceURL "__WebInspectorInjectedScript__" here? Perhaps "__WebAutomationInjectedScript__"?
> 
> If we want this to be hidden from WebInspector we can update: Utilities.js
> 
>     function isWebInspectorDebugScript(url)
>     {
>         return url && url.startsWith("__WebInspector");
>     }
> 
> To just check "__Web" or something.

Yeah, I will make that change. I will post a separate patch for Utilities.js.

>> Source/WebKit2/CMakeLists.txt:802
>> +    VERBATIM)
> 
> The CMake build failed because you forgot the following:
> 
> list(APPEND WebKit2_HEADERS ${DERIVED_SOURCES_WEBKIT2_DIR}/WebauotmationSessionProxyScriptSource.h)

Thanks.

>> Source/WebKit2/DerivedSources.make:236
>> +	echo "//# sourceURL=__WebInspectorInjectedScript__" > $(basename $(notdir $<)).min.js
> 
> Same comment about the name "WebInspector".

Will do.

>> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:32
>> +var AutomationSessionProxy = class AutomationSessionProxy
> 
> Use `let` instead of `var? `Object` and evaluate/createUUID aren't used yet, but I expect they will be soon.

Will do. Yes a follow up uses them.

[Timothy Hatcher]

Comment on attachment 274056 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274056&action=review

>> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:92
>> +    JSObjectRef scriptObjectFunction = const_cast<JSObjectRef>(JSEvaluateScript(context, toJSString(script).get(), nullptr, nullptr, 0, &exception));
> 
> I think JSValueToObject be safer then these casts, is there precedent for just casting?

I can change it. Some Safari extensions code just casts for values expected to be objects. In this case we control both sides of the code.

[Timothy Hatcher]

Created attachment 274075 [details]
Patch

[Anders Carlsson]

Comment on attachment 274075 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274075&action=review

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.h:71
> +    // Called by WebAutomationSession messages
> +    // FIXME: Add message functions here.
> +    void test() { };

Messages handlers should be private.

[Timothy Hatcher]

Comment on attachment 274075 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274075&action=review

>> Source/WebKit2/UIProcess/Automation/WebAutomationSession.h:71
>> +    void test() { };
> 
> Messages handlers should be private.

Okay, will change.

[Timothy Hatcher]

Created attachment 274136 [details]
Patch

[Timothy Hatcher]

https://trac.webkit.org/r198736

[Joanmarie Diggs]

So... Looks like the reason why the Gtk and Efl bots are red is a typo. Which I can fix with this:

diff --git a/Source/WebKit2/CMakeLists.txt b/Source/WebKit2/CMakeLists.txt
index 85dd3d7..71278d4 100644
--- a/Source/WebKit2/CMakeLists.txt
+++ b/Source/WebKit2/CMakeLists.txt
@@ -815,7 +815,7 @@ add_custom_command(
     VERBATIM)
 
 list(APPEND WebKit2_HEADERS
-    ${DERIVED_SOURCES_WEBKIT2_DIR}/WebauotmationSessionProxyScriptSource.h
+    ${DERIVED_SOURCES_WEBKIT2_DIR}/WebAutomationSessionProxyScriptSource.h
 )

But now the build fails with:

ninja: error: '/xxd.pl', needed by 'DerivedSources/WebKit2/WebAutomationSessionProxyScriptSource.h', missing and no known rule to make it

Suggestions?

[Timothy Hatcher]

(In reply to comment #18)
> So... Looks like the reason why the Gtk and Efl bots are red is a typo.
> Which I can fix with this:
> 
> diff --git a/Source/WebKit2/CMakeLists.txt b/Source/WebKit2/CMakeLists.txt
> index 85dd3d7..71278d4 100644
> --- a/Source/WebKit2/CMakeLists.txt
> +++ b/Source/WebKit2/CMakeLists.txt
> @@ -815,7 +815,7 @@ add_custom_command(
>      VERBATIM)
>  
>  list(APPEND WebKit2_HEADERS
> -    ${DERIVED_SOURCES_WEBKIT2_DIR}/WebauotmationSessionProxyScriptSource.h
> +    ${DERIVED_SOURCES_WEBKIT2_DIR}/WebAutomationSessionProxyScriptSource.h
>  )
> 
> But now the build fails with:
> 
> ninja: error: '/xxd.pl', needed by
> 'DerivedSources/WebKit2/WebAutomationSessionProxyScriptSource.h', missing
> and no known rule to make it
> 
> Suggestions?

xxd.pl should be in the JavaScriptCore_SCRIPTS_DIR directory.

$(PERL) $(JavaScriptCore_SCRIPTS_DIR)/xxd.pl

[Michael Catanzaro]

(In reply to comment #19) 
> xxd.pl should be in the JavaScriptCore_SCRIPTS_DIR directory.
> 
> $(PERL) $(JavaScriptCore_SCRIPTS_DIR)/xxd.pl

Looks like JavaScriptCore_SCRIPTS_DIR is an empty string :)

[Carlos Alberto Lopez Perez]

This fixes the CMake part

--- a/Source/WebKit2/CMakeLists.txt
+++ b/Source/WebKit2/CMakeLists.txt
@@ -750,6 +750,12 @@ set(PluginProcess_LIBRARIES
     WebKit2
 )
 
+if (WIN32 AND INTERNAL_BUILD)
+    set(JavaScriptCore_SCRIPTS_DIR "${CMAKE_BINARY_DIR}/../include/private/JavaScriptCore/Scripts")
+else ()
+    set(JavaScriptCore_SCRIPTS_DIR "${DERIVED_SOURCES_DIR}/ForwardingHeaders/JavaScriptCore/Scripts")
+endif ()
+
 # librt is needed for shm_open on Linux.
 find_library(LIBRT_LIBRARIES NAMES rt)
 mark_as_advanced(LIBRT_LIBRARIES)
@@ -815,7 +821,7 @@ add_custom_command(
     VERBATIM)
 
 list(APPEND WebKit2_HEADERS
-    ${DERIVED_SOURCES_WEBKIT2_DIR}/WebauotmationSessionProxyScriptSource.h
+    ${DERIVED_SOURCES_WEBKIT2_DIR}/WebAutomationSessionProxyScriptSource.h
 )
 




But then the build fails here:


In file included from ../../Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:27:
../../Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.h:68:12: error: field has incomplete type 'WTF::String'
    String m_sessionIdentifier;
           ^
../../Source/WTF/wtf/Forward.h:49:7: note: forward declaration of 'WTF::String'
class String;
      ^
In file included from ../../Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:27:
../../Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.h:46:12: error: incomplete result type 'WTF::String' in function definition
    String sessionIdentifier() const { return m_sessionIdentifier; }
           ^
../../Source/WTF/wtf/Forward.h:49:7: note: forward declaration of 'WTF::String'
class String;
      ^
../../Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:101:29: error: no matching member function for call to 'addMessageReceiver'
    WebProcess::singleton().addMessageReceiver(Messages::WebAutomationSessionProxy::messageReceiverName(), *this);
    ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
../../Source/WebKit2/Shared/ChildProcess.h:61:10: note: candidate function not viable: no known conversion from 'WebKit::WebAutomationSessionProxy' to 'IPC::MessageReceiver &' for 2nd argument
    void addMessageReceiver(IPC::StringReference messageReceiverName, IPC::MessageReceiver&);
         ^
../../Source/WebKit2/Shared/ChildProcess.h:62:10: note: candidate function not viable: requires 3 arguments, but 2 were provided
    void addMessageReceiver(IPC::StringReference messageReceiverName, uint64_t destinationID, IPC::MessageReceiver&);
         ^
3 errors generated.
[107/604] Building CXX object Source/WebKit2/CMakeFiles/WebKit2.dir/WebProcess/WebPage/WebPage.cpp.o

[Carlos Alberto Lopez Perez]

Committed r198757: <http://trac.webkit.org/changeset/198757>

[Michael Catanzaro]

I'm a bit confused why we had to redefine JavaScriptCore_SCRIPTS_DIR in WebKit2/CMakeLists.txt. I wonder if CMake is processing WebKit2/CMakeLists.txt before it processes JavaScriptCore/CMakeLists.txt.

[Carlos Alberto Lopez Perez]

(In reply to comment #23)
> I'm a bit confused why we had to redefine JavaScriptCore_SCRIPTS_DIR in
> WebKit2/CMakeLists.txt. I wonder if CMake is processing
> WebKit2/CMakeLists.txt before it processes JavaScriptCore/CMakeLists.txt.

Is also redefined in Source/WebCore/CMakeLists.txt
Basically I copied the definition from there.

[Blaze Burg]

(In reply to comment #23)
> I'm a bit confused why we had to redefine JavaScriptCore_SCRIPTS_DIR in
> WebKit2/CMakeLists.txt. I wonder if CMake is processing
> WebKit2/CMakeLists.txt before it processes JavaScriptCore/CMakeLists.txt.

Sorry about the breakage folks, Mac CMake just started working again recently or I would have caught it. I am not sure why variable needs to be redefined, but I have run into this before and just redefined it without figuring why. :|