1370850 - Evaluating script fails to return value field when user prompt appears

Assignee

Description

•

7 years ago

When a user prompt, such as window.alert, is made to appear when a script is being evaluated by the GeckoDriver#execute_ function used by Execute Script and Execute Async Script, the returned JSON Object is missing the ‘value’ field.

This is because the dialogueObserver in testing/marionette/proxy.js interrupts the synchronous call to the content frame script, causing it to return with an undefined value.

This was reported in https://github.com/mozilla/geckodriver/issues/431.

Andreas Tolfsen ❲:ato❳

Assignee

Updated

•

7 years ago

Assignee: nobody → ato

Blocks: webdriver

Status: NEW → ASSIGNED

Comment hidden (mozreview-request)

When a call through the content frame proxy is interrupted by the
dialogueObserver, the synchronous promise that is meant to wait for a
response from the frame script is resolved immediately with an undefined
return value.

When an undefined value is assigned to the response body, it gets dropped
during JSON serialisation.  To ensure the "value" field expected from
the Execute Script and Execute Async Script commands is populated,
we need to assign a null value to resp.body.value.

We can treat undefined as null by calling evaluate.toJSON again on the
return value from the proxied frame script call.  This effectively means
we serialise it twice, since it first needs to be serialised to cross
the IPC border, though the second computation only looks at primitives
and no known web element store is required.

It would be nicer if the content frame script itself would be able to
return early with null by installing a user prompt notification event,
but this is not possible because the tabmodal dialogue that appears
blocks script execution.  This means we need to rely on the
dialogueObserver in testing/marionette/proxy.js to take care of the
dialogue for us.

Review commit: https://reviewboard.mozilla.org/r/146656/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146656/

Comment hidden (mozreview-request)

Comment on attachment 8875255 [details]
Bug 1370850 - Serialise undefined script evaluation return value to null;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146656/diff/1-2/

Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)

Comment 3

•

7 years ago

mozreview-review

Comment on attachment 8875255 [details]
Bug 1370850 - Serialise undefined script evaluation return value to null;

https://reviewboard.mozilla.org/r/146656/#review151366

::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_script.py:375
(Diff revision 2)
>              sandbox=None)
>          self.assert_is_web_element(el)
>  
> +    def test_return_value_on_alert(self):
> +        res = self.marionette._send_message("executeScript", {"script": "alert()"})
> +        self.assertIn("value", res)

Suggestion: also check that the value is None.

Attachment #8875255 - Flags: review?(mjzffr) → review+

Andreas Tolfsen ❲:ato❳

Assignee

Comment 4

•

7 years ago

mozreview-review-reply

Comment on attachment 8875255 [details]
Bug 1370850 - Serialise undefined script evaluation return value to null;

https://reviewboard.mozilla.org/r/146656/#review151366

> Suggestion: also check that the value is None.

Good point.  This was an omission from my side.

Comment hidden (mozreview-request)

Comment on attachment 8875255 [details]
Bug 1370850 - Serialise undefined script evaluation return value to null;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146656/diff/2-3/

Pulsebot

Comment 6

•

7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eaef7cd5e288
Serialise undefined script evaluation return value to null; r=maja_zf

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 7

•

7 years ago

Backed out for failing modified Marionette test test_execute_script.py TestExecuteContent.test_return_value_on_alert:

https://hg.mozilla.org/integration/autoland/rev/00c3f75ed9a00df54df729135cbd12651cd736b8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=eaef7cd5e2884314fb81c70d1b9576772a38e415&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=105556932&repo=autoland

[task 2017-06-08T17:31:59.054356Z] 17:31:59     INFO - TEST-START | test_execute_script.py TestExecuteContent.test_return_value_on_alert
[task 2017-06-08T17:31:59.222102Z] 17:31:59  WARNING - Failed to gather test failure debug: 
[task 2017-06-08T17:31:59.223308Z] 17:31:59  WARNING - stacktrace:
[task 2017-06-08T17:31:59.223891Z] 17:31:59  WARNING - 	WebDriverError@chrome://marionette/content/error.js:222:5
[task 2017-06-08T17:31:59.224952Z] 17:31:59  WARNING - 	UnexpectedAlertOpenError@chrome://marionette/content/error.js:486:5
[task 2017-06-08T17:31:59.225945Z] 17:31:59  WARNING - 	assert.that/<@chrome://marionette/content/assert.js:355:13
[task 2017-06-08T17:31:59.226909Z] 17:31:59  WARNING - 	assert.noUserPrompt@chrome://marionette/content/assert.js:129:3
[task 2017-06-08T17:31:59.228003Z] 17:31:59  WARNING - 	GeckoDriver.prototype.getPageSource@chrome://marionette/content/driver.js:1036:3
[task 2017-06-08T17:31:59.228885Z] 17:31:59  WARNING - 	TaskImpl_run@resource://gre/modules/Task.jsm:321:42
[task 2017-06-08T17:31:59.229460Z] 17:31:59  WARNING - 	TaskImpl@resource://gre/modules/Task.jsm:279:3
[task 2017-06-08T17:31:59.230175Z] 17:31:59  WARNING - 	asyncFunction@resource://gre/modules/Task.jsm:254:14
[task 2017-06-08T17:31:59.230715Z] 17:31:59  WARNING - 	Task_spawn@resource://gre/modules/Task.jsm:168:12
[task 2017-06-08T17:31:59.231244Z] 17:31:59  WARNING - 	TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:391:16
[task 2017-06-08T17:31:59.231818Z] 17:31:59  WARNING - 	TaskImpl_run@resource://gre/modules/Task.jsm:329:15
[task 2017-06-08T17:31:59.232291Z] 17:31:59  WARNING - 	TaskImpl@resource://gre/modules/Task.jsm:279:3
[task 2017-06-08T17:31:59.232816Z] 17:31:59  WARNING - 	asyncFunction@resource://gre/modules/Task.jsm:254:14
[task 2017-06-08T17:31:59.233521Z] 17:31:59  WARNING - 	Task_spawn@resource://gre/modules/Task.jsm:168:12
[task 2017-06-08T17:31:59.234230Z] 17:31:59  WARNING - 	execute@chrome://marionette/content/server.js:510:15
[task 2017-06-08T17:31:59.235517Z] 17:31:59  WARNING - 	onPacket@chrome://marionette/content/server.js:481:7
[task 2017-06-08T17:31:59.236236Z] 17:31:59  WARNING - 	_onJSONObjectReady/<@chrome://marionette/content/transport.js:480:9
[task 2017-06-08T17:31:59.236972Z] 17:31:59  WARNING - 	openTabPrompt@jar:file:///home/worker/workspace/build/application/firefox/omni.ja!/components/nsPrompter.js:420:13
[task 2017-06-08T17:31:59.237699Z] 17:31:59  WARNING - 	openPrompt@jar:file:///home/worker/workspace/build/application/firefox/omni.ja!/components/nsPrompter.js:535:17
[task 2017-06-08T17:31:59.238451Z] 17:31:59  WARNING - 	alert@jar:file:///home/worker/workspace/build/application/firefox/omni.ja!/components/nsPrompter.js:602:9
[task 2017-06-08T17:31:59.239081Z] 17:31:59  WARNING - 	@dummy file:0:15
[task 2017-06-08T17:31:59.239224Z] 17:31:59  WARNING - 	@dummy file:0:2
[task 2017-06-08T17:31:59.240655Z] 17:31:59  WARNING - 	evaluate.sandbox/promise<@chrome://marionette/content/evaluate.js:154:13
[task 2017-06-08T17:31:59.241653Z] 17:31:59  WARNING - 	evaluate.sandbox@chrome://marionette/content/evaluate.js:105:17
[task 2017-06-08T17:31:59.241771Z] 17:31:59  WARNING - 	execute@chrome://marionette/content/listener.js:733:19
[task 2017-06-08T17:31:59.241899Z] 17:31:59  WARNING - 	TaskImpl_run@resource://gre/modules/Task.jsm:321:42
[task 2017-06-08T17:31:59.242525Z] 17:31:59  WARNING - 	TaskImpl@resource://gre/modules/Task.jsm:279:3
[task 2017-06-08T17:31:59.242660Z] 17:31:59  WARNING - 	asyncFunction@resource://gre/modules/Task.jsm:254:14
[task 2017-06-08T17:31:59.243199Z] 17:31:59  WARNING - 	Task_spawn@resource://gre/modules/Task.jsm:168:12
[task 2017-06-08T17:31:59.243361Z] 17:31:59  WARNING - 	TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:391:16
[task 2017-06-08T17:31:59.243511Z] 17:31:59  WARNING - 	TaskImpl_run@resource://gre/modules/Task.jsm:329:15
[task 2017-06-08T17:31:59.244481Z] 17:31:59  WARNING - 	TaskImpl@resource://gre/modules/Task.jsm:279:3
[task 2017-06-08T17:31:59.244577Z] 17:31:59  WARNING - 	asyncFunction@resource://gre/modules/Task.jsm:254:14
[task 2017-06-08T17:31:59.244706Z] 17:31:59  WARNING - 	Task_spawn@resource://gre/modules/Task.jsm:168:12
[task 2017-06-08T17:31:59.245186Z] 17:31:59  WARNING - 	dispatch/<@chrome://marionette/content/listener.js:427:15
[task 2017-06-08T17:31:59.245239Z] 17:31:59  WARNING - 
[task 2017-06-08T17:31:59.419109Z] 17:31:59     INFO - TEST-UNEXPECTED-FAIL | test_execute_script.py TestExecuteContent.test_return_value_on_alert | AssertionError: {u'value': None} is not None
[task 2017-06-08T17:31:59.423178Z] 17:31:59     INFO - Traceback (most recent call last):
[task 2017-06-08T17:31:59.423275Z] 17:31:59     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_harness/marionette_test/testcases.py", line 157, in run
[task 2017-06-08T17:31:59.423717Z] 17:31:59     INFO -     testMethod()
[task 2017-06-08T17:31:59.423818Z] 17:31:59     INFO -   File "/home/worker/workspace/build/tests/marionette/tests/testing/marionette/harness/marionette_harness/tests/unit/test_execute_script.py", line 363, in test_return_value_on_alert
[task 2017-06-08T17:31:59.423868Z] 17:31:59     INFO -     self.assertIsNone(res)
[task 2017-06-08T17:31:59.423909Z] 17:31:59     INFO - TEST-INFO took 168ms

Flags: needinfo?(ato)

Andreas Tolfsen ❲:ato❳

Assignee

Comment 8

•

7 years ago

The assertion should be

> self.assertIsNone(res["value"])

Flags: needinfo?(ato)

Comment hidden (mozreview-request)

Comment on attachment 8875255 [details]
Bug 1370850 - Serialise undefined script evaluation return value to null;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146656/diff/3-4/

Pulsebot

Comment 10

•

7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb3cbcd197fe
Serialise undefined script evaluation return value to null; r=maja_zf

Carsten Book [:Tomcat]

Comment 11

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/fb3cbcd197fe

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

status-firefox55: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla55

Henrik Skupin [:whimboo][⌚️UTC+1]

Updated

•

7 years ago

Depends on: 1371905

Henrik Skupin [:whimboo][⌚️UTC+1]

Updated

•

7 years ago

Depends on: 1371924

BMO Automation

Updated

•

1 year ago

Product: Testing → Remote Protocol

Bugzilla

Quick Search

Evaluating script fails to return value field when user prompt appears

Categories

(Remote Protocol :: Marionette, defect)

Tracking

(firefox55 fixed)

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Crash Data

Security

(public)

User Story

Attachments

(1 file)

Description

Updated

Comment 1

Comment 2

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Comment 11

Updated

Updated

Updated

Attachment

General

Description

File Name

Content Type