938228 - Marshal NodeList according to WebDriver's executeScript algorithm

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Andreas Tolfsen ❲:ato❳]

Attachment: test_execute_script.py

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>].

Comment 1

[David Burns :automatedtester]

(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.

Comment 2

[Andreas Tolfsen ❲:ato❳]

(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.

Comment 3

[Andreas Tolfsen ❲:ato❳]

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.

Comment 4

[Andreas Tolfsen ❲:ato❳]

Attachment: 0001-Bug-938228-Fix-marshaling-of-NodeList-in-executeScri.patch

try: https://tbpl.mozilla.org/?tree=Try&rev=a682a32c3581

Comment 5

[Malini Das [:mdas] - Away, not checking bugmail]

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.

Comment 6

[Andreas Tolfsen ❲:ato❳]

Attachment: 0001-Bug-938228-Fix-marshaling-of-NodeList-in-executeScri.patch

(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.

Comment 7

[Malini Das [:mdas] - Away, not checking bugmail]

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.

Comment 8

[Andreas Tolfsen ❲:ato❳]

Attachment: 0001-Bug-938228-Fix-marshaling-of-NodeList-in-executeScri.patch

try: https://tbpl.mozilla.org/?tree=Try&rev=bafab98aaa24

Comment 9

[Malini Das [:mdas] - Away, not checking bugmail]

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!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdc6fd62d987

Comment 11

[Carsten Book [:Tomcat]]

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