Closed Bug 1363053 Opened 7 years ago Closed 7 years ago

Run JSON.stringify on return value from Execute (Async) Script

Tracking

(firefox55 fixed)

Status:

RESOLVED FIXED

Milestone:

mozilla55

Tracking Flags:

Tracking

Status

firefox55

---

fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1363053 - Move element.fromJson/toJson to evaluate module 7 years ago Andreas Tolfsen ❲:ato❳ 59 bytes, text/x-review-board-request	impossibus : review+	Details
Bug 1363053 - Return JSON representation of return value 7 years ago Andreas Tolfsen ❲:ato❳ 59 bytes, text/x-review-board-request	impossibus : review+	Details
Bug 1363053 - Use Node.ELEMENT_NODE instead of magic number 7 years ago Andreas Tolfsen ❲:ato❳ 59 bytes, text/x-review-board-request	impossibus : review+	Details

Andreas Tolfsen ❲:ato❳

Assignee

Description

•

7 years ago

The WebDriver specification recently allowed an object’s toJSON to represent the return value from the Execute Script and Execute Async Script commands.

The change was made in https://github.com/w3c/webdriver/commit/1ee4c61c11004f38bb3db0fb6a1d740e185a2196.

We should change Marionette so that it calls JSON.stringify, which implicitly calls obj.toJSON, on the return value from these commands.

Andreas Tolfsen ❲:ato❳

Assignee

Updated

•

7 years ago

Blocks: webdriver

Comment hidden (mozreview-request)

This patch renames element.fromJson and element.toJson to
evaluate.fromJSON and evaluate.toJSON, respectively.

The JSON (de)serialisation code belongs more naturally in the evaluate
module, which did not exist when these functions were written.

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

Comment hidden (mozreview-request)

According to a recent change in the WebDriver specification, we need to
return an object's JSON representation iff it exists.

The relevant specification prose change was made in
https://github.com/w3c/webdriver/commit/1ee4c61c11004f38bb3db0fb6a1d740e185a2196.

This patch causes return values that have a toJSON property that is a
function, to return the value of calling said function.

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

Comment hidden (mozreview-request)

This patch removes the magic number and instead uses the ELEMENT_NODE
constant which exists on all objects of the Node prototype chain.
We already test that obj has a nodeType own property.

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

Andreas Tolfsen ❲:ato❳

Assignee

Updated

•

7 years ago

Assignee: nobody → ato

Status: NEW → ASSIGNED

Comment hidden (mozreview-request)

Comment on attachment 8865545 [details]
Bug 1363053 - Move element.fromJson/toJson to evaluate module

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

Comment hidden (mozreview-request)

Comment on attachment 8865546 [details]
Bug 1363053 - Return JSON representation of return value

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

Comment hidden (mozreview-request)

Comment on attachment 8865547 [details]
Bug 1363053 - Use Node.ELEMENT_NODE instead of magic number

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

Andreas Tolfsen ❲:ato❳

Assignee

Updated

•

7 years ago

Attachment #8865545 - Flags: review?(mjzffr)

Attachment #8865546 - Flags: review?(mjzffr)

Attachment #8865547 - Flags: review?(mjzffr)

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

Comment 7

•

7 years ago

mozreview-review

Comment on attachment 8865546 [details]
Bug 1363053 - Return JSON representation of return value

https://reviewboard.mozilla.org/r/137168/#review141234

::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_script.py:333
(Diff revision 2)
>  
>      def test_access_global_objects_from_chrome(self):
>          # test inspection of arguments
>          self.marionette.execute_script("__webDriverArguments.toString()")
>  
> +    def test_toJSON(self):

It would be useful to test the "unsafe json" path as well.

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

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

Comment 8

•

7 years ago

mozreview-review

Comment on attachment 8865545 [details]
Bug 1363053 - Move element.fromJson/toJson to evaluate module

https://reviewboard.mozilla.org/r/137166/#review141240

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

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

Comment 9

•

7 years ago

mozreview-review

Comment on attachment 8865547 [details]
Bug 1363053 - Use Node.ELEMENT_NODE instead of magic number

https://reviewboard.mozilla.org/r/137170/#review141244

::: commit-message-089ab:5
(Diff revision 2)
> +Bug 1363053 - Use Node.ELEMENT_NODE instead of magic number
> +
> +This patch removes the magic number and instead uses the ELEMENT_NODE
> +constant which exists on all objects of the Node prototype chain.
> +We already test that obj has a nodeType own property.

I'll take your word for it, but I'm curious: where is this check?

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

Andreas Tolfsen ❲:ato❳

Assignee

Comment 10

•

7 years ago

mozreview-review-reply

Comment on attachment 8865546 [details]
Bug 1363053 - Return JSON representation of return value

https://reviewboard.mozilla.org/r/137168/#review141234

> It would be useful to test the "unsafe json" path as well.

Sure!  That actually caught a bug, so good call.

Comment hidden (mozreview-request)

Comment on attachment 8865545 [details]
Bug 1363053 - Move element.fromJson/toJson to evaluate module

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

Comment hidden (mozreview-request)

Comment on attachment 8865546 [details]
Bug 1363053 - Return JSON representation of return value

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

Comment hidden (mozreview-request)

Comment on attachment 8865547 [details]
Bug 1363053 - Use Node.ELEMENT_NODE instead of magic number

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

Comment hidden (mozreview-request)

Comment on attachment 8865546 [details]
Bug 1363053 - Return JSON representation of return value

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

Comment hidden (mozreview-request)

Comment on attachment 8865547 [details]
Bug 1363053 - Use Node.ELEMENT_NODE instead of magic number

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

Pulsebot

Comment 16

•

7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ffd5e79bff48
Move element.fromJson/toJson to evaluate module r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/dc14ec8d4358
Return JSON representation of return value r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/5cdef0a42ffc
Use Node.ELEMENT_NODE instead of magic number r=maja_zf

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

Comment 17

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/ffd5e79bff48
https://hg.mozilla.org/mozilla-central/rev/dc14ec8d4358
https://hg.mozilla.org/mozilla-central/rev/5cdef0a42ffc

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

status-firefox55: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla55

BMO Automation

Updated

•

1 year ago

Product: Testing → Remote Protocol

You need to log in before you can comment on or make changes to this bug.