Bug 196847 - WebDriver: evaluating javascript shouldn't fail if a dialog is shown
Summary: WebDriver: evaluating javascript shouldn't fail if a dialog is shown
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebDriver (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
Keywords: InRadar
Depends on:
Reported: 2019-04-11 19:48 PDT by Devin Rousso
Modified: 2019-05-07 03:59 PDT (History)
5 users (show)

See Also:

Patch (1.71 KB, patch)
2019-04-11 19:50 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-04-11 19:48:51 PDT
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.

Comment 1 Devin Rousso 2019-04-11 19:49:01 PDT
Comment 2 Devin Rousso 2019-04-11 19:50:01 PDT
Created attachment 367279 [details]
Comment 3 BJ Burg 2019-04-12 13:20:09 PDT
Comment on attachment 367279 [details]

Comment 4 WebKit Commit Bot 2019-04-12 13:47:01 PDT
Comment on attachment 367279 [details]

Clearing flags on attachment: 367279

Committed r244230: <https://trac.webkit.org/changeset/244230>
Comment 5 WebKit Commit Bot 2019-04-12 13:47:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Carlos Garcia Campos 2019-05-07 01:58:37 PDT
Comment on attachment 367279 [details]

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";)