Marshal NodeList according to WebDriver's executeScript algorithm

RESOLVED FIXED in mozilla30

Status

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ato, Assigned: ato)

Tracking

(Blocks 1 bug)

Trunk
mozilla30
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

6 years ago
Collections returned from executed JavaScript in Marionette are not properly unmarshaled.  According to the webdriver specification the executeScript command should return a list of web element reference objects when the result of an evaluation returns a NodeList object.

The algorithm set forth from above is:

  If result is:
    1. undefined or null, return null.
    2. a number, boolean, or DOMString, return result.
    3. a DOMElement, then return the corresponding WebElement for that DOMElement.
    4. an array or NodeList, then return the result of recursively applying this algorithm to result.
    5. an object, then return the dictionary created by recursively applying this algorithm to each property in result.

(See https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html#synchronous-javascript-execution for reference.)

There are few things that are curious about this part of the spec:

  1. The use of “array” is fairly language specific and should probably be replaced by “list”.

  2. Does HTMLCollection subclass NodeList?  If not it seems an oversight to not include that.

  3. NodeLists/HTMLCollections and lists/array literals are not equivalent if marshalled in a strict fashion.  When objects leave the browser context they loose their associations because Marionette's protocol diminishes JavaScript objects to their primary properties, thus the unmarshaller wouldn't know how to distinguish a NodeList's key in {"1": VALUE} from the string primitive "1".

A simpler approach would be to expect executeScript to return something roughly equivalent to List<Object>, where Object can be one of the simple types or WebElement.

The attached TC can be run by executing `python runtests.py --binary=$MOZILLA_BUILD/dist
/bin/firefox --type=browser ~/test_execute_script.py`.  The actual return value we're seeing from execute_script("document.getElementsByTagName('p')") is along the lines of {"1": <marionette.HTMLElement>, "2": <marionette.HTMLElement>, …}, while we're expecting [<marionette.HTMLelement>, <marionette.HTMLElement>].
Attachment #831648 - Attachment is patch: true
Attachment #831648 - Attachment mime type: text/x-python → text/plain
(In reply to Andreas Tolfsen (:ato) from comment #0)
> 
> The algorithm set forth from above is:
> 
>   If result is:
>     1. undefined or null, return null.
>     2. a number, boolean, or DOMString, return result.
>     3. a DOMElement, then return the corresponding WebElement for that
> DOMElement.
>     4. an array or NodeList, then return the result of recursively applying
> this algorithm to result.
>     5. an object, then return the dictionary created by recursively applying
> this algorithm to each property in result.
> 
> (See
> https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.
> html#synchronous-javascript-execution for reference.)
> 
> There are few things that are curious about this part of the spec:
> 
>   1. The use of “array” is fairly language specific and should probably be
> replaced by “list”.

I am going to remove array, see the comment for 2.

> 
>   2. Does HTMLCollection subclass NodeList?  If not it seems an oversight to
> not include that.

 I am going to change the spec to read Sequence<Node> Nodes

>   3. NodeLists/HTMLCollections and lists/array literals are not equivalent
> if marshalled in a strict fashion.  When objects leave the browser context
> they loose their associations because Marionette's protocol diminishes
> JavaScript objects to their primary properties, thus the unmarshaller
> wouldn't know how to distinguish a NodeList's key in {"1": VALUE} from the
> string primitive "1".
> 

Solved under 2.

> A simpler approach would be to expect executeScript to return something
> roughly equivalent to List<Object>, where Object can be one of the simple
> types or WebElement.
Assignee

Comment 2

6 years ago
(In reply to David Burns :automatedtester from comment #1)
> I am going to remove array, see the comment for 2.

Apparently WebIDL prefers the use of “array” (see http://www.w3.org/TR/WebIDL/#idl-array) so please ignore the comment I made about that.
Assignee

Updated

5 years ago
Summary: Unmarshal JS collections to match webdriver protocol → Marshal NodeList according to WebDriver's executeScript algorithm
Assignee

Updated

5 years ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
Assignee

Comment 3

5 years ago
Narrowing down the scope for this bug by making it only about supporting returned NodeList's (sequence of nodes).  While working on this bug I found a number of other incompatibilities dealing with marshaling data between Marionette and the client that I'll will separate bugs about.
Assignee

Updated

5 years ago
Attachment #8366974 - Flags: review? → review?(mdas)
Comment on attachment 8366974 [details] [diff] [review]
0001-Bug-938228-Fix-marshaling-of-NodeList-in-executeScri.patch

Review of attachment 8366974 [details] [diff] [review]:
-----------------------------------------------------------------

Need to fix test_execute_script.py, other than that, it looks good.

::: testing/marionette/client/marionette/tests/unit/test_execute_script.py
@@ +79,4 @@
>          self.assertTrue(isinstance(result, float))
>          self.assertEqual(result, expected_result)
>  
> +class TestExecuteChrome(MarionetteTestCase):

We inherit from TestExecuteContent so we can run the same test cases in TestExecuteContent, but in chrome context. This should be put back, and if test_unmarshal_element_collection doesn't work in chrome, then override it in this class and add a test that works in chrome space.
Attachment #8366974 - Flags: review?(mdas) → review-
Assignee

Comment 6

5 years ago
(In reply to Malini Das [:mdas] from comment #5)
> We inherit from TestExecuteContent so we can run the same test cases
> in TestExecuteContent, but in chrome context.

Ooh, I see.  My mistake.  I've corrected this in the revised patch
attached.
Attachment #8366974 - Attachment is obsolete: true
Attachment #8368528 - Flags: review?(mdas)
Comment on attachment 8368528 [details] [diff] [review]
0001-Bug-938228-Fix-marshaling-of-NodeList-in-executeScri.patch

Review of attachment 8368528 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/client/marionette/tests/unit/test_execute_script.py
@@ +87,4 @@
>      def test_execute_permission(self):
>          self.assertEqual(1, self.marionette.execute_script("var c = Components.classes;return 1;"))
>  
> +    # Returning sequence of web elements is irrelevant to the chrome

No, we have tests that do this already http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/tests/unit/test_findelement_chrome.py#39 since people can use marionette to automate the chrome and will need to retrieve elements to do so.

If you can write a test for this, that would be ideal.
Attachment #8368528 - Flags: review?(mdas) → review-
Comment on attachment 8369460 [details] [diff] [review]
0001-Bug-938228-Fix-marshaling-of-NodeList-in-executeScri.patch

Review of attachment 8369460 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm, thanks!
Attachment #8369460 - Flags: review?(mdas) → review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cdc6fd62d987
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.