WebDriver: evaluating javascript shouldn't fail if a dialog is shown

• Component: WebDriver
• Status: RESOLVED FIXED

[Devin Rousso]

We currently send an "unexpected alert open error" if a dialog is shown while evaluating javascript, but we really should be silently failing with no data.

<https://github.com/w3c/webdriver/issues/1253>

[Devin Rousso]

<rdar://problem/49609396>

[Devin Rousso]

Created attachment 367279 [details]
Patch

[BJ Burg]

Comment on attachment 367279 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 367279 [details]
Patch

Clearing flags on attachment: 367279

Committed r244230: <https://trac.webkit.org/changeset/244230>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

Comment on attachment 367279 [details]
Patch

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

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:657
> -                callback->sendFailure(unexpectedAlertOpenError);
> +                callback->sendSuccess(emptyString());

This is not correct, because emptyString() is not a valid JSON serialized string. JSON::Value::parseJSON() will fail when trying to handle this result. I'm not even sure this is the right fix, because evaluateJavaScriptFunction message is used for other commands than executeString and actions. We handle the executeScript case in WebDriver itself, considering a success when unexpected alert open error is received. We could do the same for the actions command. This commit has caused all user_prompts test to fail, because create_dialog() fixture always fails. It executes a script to create the dialog itself (ignoring NoSuchAlertException, indeed), that is always failing now because the response from evaluateJavaScriptFunction is not a valid JSON serialized string. So, we have two ways to fix this: 

1) Roll out this patch and handle unexpected alert open in the driver for executeScript and actions commands
2) Keep handling it here, but using "null" instead of emptyString(). This is what we do in the web process when the result of the script can't be json stringfied. (return JSON.stringify(original, (key, value) => this._replaceJSONValue(key, value)) || "null";)