Bug 155524 – Web Automation: Add Automation.evaluateJavaScriptFunction

• Component: Web Inspector
• Status: RESOLVED FIXED

[Timothy Hatcher]

Created attachment 274158 [details]
WIP

Add a way to execute JS to the Automation domain.

[Timothy Hatcher]

Comment on attachment 274158 [details]
WIP

Needs some better error handling then it is ready to go.

[Radar WebKit Bug Importer]

<rdar://problem/25181888>

[Joseph Pecoraro]

Comment on attachment 274158 [details]
WIP

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

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:43
> +static JSObjectRef toJSArray(JSContextRef context, const Vector<T>& data, JSValueRef (*converter)(JSContextRef, const T&), JSValueRef* exception = 0)

Only nit I have is this 0 could be nullptr.

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:48
> +        return JSObjectMakeArray(context, 0, 0, exception);

Second 0 here could be nullptr.

[Timothy Hatcher]

Created attachment 274294 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 274294 [details]
Patch

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

r=me

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:271
> +        argumentsVector.append(argumentString);

Since you did a reserveCapacity you can use uncheckedAppend here.

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:277
> +    page->process().send(Messages::WebAutomationSessionProxy::EvaluateJavaScriptFunction(page->mainFrame()->frameID(), function, argumentsVector, expectsImplicitCallbackArgument, callbackID), 0);

This last parameter is 0, should it be page->pageId() or not included alltogether?

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:57
> +        convertedData.append(convertedValue);

Nit: uncheckedAppend here too.

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:83
> +    JSObjectRef evaluateJavaScriptFunction = const_cast<JSObjectRef>(JSObjectGetProperty(context, object, toJSString(propertyName).get(), exception));

The name evaluateJavaScriptFunction here is too specific.Eventually propertyFunction will probably be used for methods other than "evaluateJavaScriptFunction". Perhaps just "function" or "method".

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:175
> +    auto pendingFrameCallbacks = m_webFramePendingEvaluateJavaScriptCallbacksMap.get(frameID);
> +    for (uint64_t callbackID : pendingFrameCallbacks)
> +        didEvaluateJavaScriptFunction(frameID, callbackID, errorMessage, errorType);

This seems dangerous. It may be modifying the vector that it is iterating over (didEvaluateJavaScriptFunction removes the callback for the given callbackID). It might be safe if pendingFrameCallbacks is a copy of the Vector, but then doing a copy here seems poor.

Since here we know it will be an error, and we know we will remove everything, it seems like it would be best to just do a single iteration and remove the entire list instead of using didEvaluateJavaScriptFunction and doing individual removals (which can be expensive in Vectors).

      for (uint64_t callbackID : pendingFrameCallbacks)
          WebProcess::singleton().parentProcessConnection()->send(Messages::WebAutomationSession::DidEvaluateJavaScriptFunction(callbackID, errorMessage, errorType));
      m_webFramePendingEvaluateJavaScriptCallbacksMap.remove(frameID);

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:232
> +    pendingFrameCallbacks.removeAll(callbackID);

There shouldn't be duplicates, right? So removeFirst() should be enough.

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:235
> +    if (!pendingFrameCallbacks.isEmpty())
> +        m_webFramePendingEvaluateJavaScriptCallbacksMap.set(frameID, pendingFrameCallbacks);

Is this necessary? If so, it seems like this would be doing a lot of Vector copying, which seems poor.

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:239
> +    WebProcess::singleton().parentProcessConnection()->send(Messages::WebAutomationSession::DidEvaluateJavaScriptFunction(callbackID, result, errorType), 0);

Is the 0 needed here?

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:31
>  let Object = {}.constructor;

Is there a plan to use this? Seems we might be good without it.

[Blaze Burg]

Comment on attachment 274294 [details]
Patch

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

s

> Source/WebKit2/UIProcess/Automation/Automation.json:105
> +                { "name": "handle", "$ref": "BrowsingContextHandle", "description": "The handle for the browsing context the script should be evaluated." },

I think this should be a frame handle, but this is OK until frame handles are added.

EDIT: Ah, I see you put a FIXME in the appropriate place. Thanks.

> Source/WebKit2/UIProcess/Automation/Automation.json:106
> +                { "name": "function", "type": "string", "description": "The script to evaluate in the browsing context. It must be a function result." },

What does 'function result' mean here? is the string the body of a function, or does it evaluate to a function that is then called? I think the latter is what this says, but it's not clear.

> Source/WebKit2/UIProcess/Automation/Automation.json:107
> +                { "name": "arguments", "type": "array", "items": { "type": "string" }, "description": "The arguments to pass to the function when called. They will be parsed as JSON before calling the function." },

We really need to add JSON and base64 primitive types to the protocol. I filed some placeholder bugs for later.

>> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:175
>> +        didEvaluateJavaScriptFunction(frameID, callbackID, errorMessage, errorType);
> 
> This seems dangerous. It may be modifying the vector that it is iterating over (didEvaluateJavaScriptFunction removes the callback for the given callbackID). It might be safe if pendingFrameCallbacks is a copy of the Vector, but then doing a copy here seems poor.
> 
> Since here we know it will be an error, and we know we will remove everything, it seems like it would be best to just do a single iteration and remove the entire list instead of using didEvaluateJavaScriptFunction and doing individual removals (which can be expensive in Vectors).
> 
>       for (uint64_t callbackID : pendingFrameCallbacks)
>           WebProcess::singleton().parentProcessConnection()->send(Messages::WebAutomationSession::DidEvaluateJavaScriptFunction(callbackID, errorMessage, errorType));
>       m_webFramePendingEvaluateJavaScriptCallbacksMap.remove(frameID);

I think it would be simpler to use HashMap::take(), this is what it's designed for.

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:237
> +        m_webFramePendingEvaluateJavaScriptCallbacksMap.remove(frameID);

Nit: adding 'to' to this member's name would be nice: m_webFrameToPendingEvaluateJavaScriptCallbacksMap

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:47
> +        // The script is expected to be a function declaration. Evaluate it inside parenthesis to get the function value.

EDIT: ok, this is clearer than the protocol description. Please update to match this better.

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:50
> +            throw new TypeError("Script did not evaluate to a function.");

I'm trying to think of a way to test this in WebKit directly. It's kind of difficult given how LayoutTests are initiated from WebProcess, though.

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:52
> +        let argumentValues = argumentStrings.map(this._jsonParse, this);

Consider doing all JSON parsing related stuff here inside its own try/catch scope, so we can report the error correctly without the page knowing it happened.

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:59
> +            callback(functionValue.apply(null, argumentValues));

We don't want a try-catch scope here because the injected snippet could throw an exception on purpose to test how the page reacts. The call stack could look a bit weird though...

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:98
> +        throw {name: "NodeNotFound", message: "Node with identifier '" + identifier + "' was not found"};

Usually it's bad style to throw anything that's not an Error instance since it breaks promise chaining. Probably not relevant here. You could make an error instance and set these properties on it.

Also, throwing an error here without a surrounding try-catch at looks like a bug to me. Client code could have a top-level error handler, and details from this injected script could show up in 'user space'. See above.

[Joseph Pecoraro]

Comment on attachment 274294 [details]
Patch

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

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:84
> +        if (value instanceof Node)
> +            return this._createNodeHandle(this._identifierForNode(value));

Do we need to handle NodeList and HTMLCollection? For those we can just Array.from() and loop through the nodes in the list.
Also, `document instanceof Node` is true, but `document isntanceof Element` is false. Should `document` be allowed here?

[Timothy Hatcher]

Comment on attachment 274294 [details]
Patch

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

>> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:98
>> +        throw {name: "NodeNotFound", message: "Node with identifier '" + identifier + "' was not found"};
> 
> Usually it's bad style to throw anything that's not an Error instance since it breaks promise chaining. Probably not relevant here. You could make an error instance and set these properties on it.
> 
> Also, throwing an error here without a surrounding try-catch at looks like a bug to me. Client code could have a top-level error handler, and details from this injected script could show up in 'user space'. See above.

Exceptions thrown during a JSC API calls (which is what we use in this patch) are not reported to the page's "error" event listeners.

[Timothy Hatcher]

https://trac.webkit.org/r198737