Andreas Tolfsen ❲:ato❳ Assignee
	Description • 11 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>].

	David Burns :automatedtester
	Comment 1 • 11 years ago

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

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 2 • 11 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.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 3 • 10 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.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 4 • 10 years ago

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

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 5 • 10 years ago

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.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 6 • 10 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.

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 7 • 10 years ago

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.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 8 • 10 years ago

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

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 9 • 10 years ago

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!

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 11 • 10 years ago

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