Alexei Barantsev Reporter
	Description • 6 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.186 Safari/537.36

Steps to reproduce:

Execute this sample using Selenium Java binding:

driver.get("http://todomvc.com/examples/scalajs-react/#/");
driver.executeScript("return document.getElementsByTagName('input')");


Actual results:

JavascriptException: TypeError: cyclic object value

See attached trace level log for more details.

	Alexei Barantsev Reporter
	Comment 1 • 6 years ago

Original report in Selenium issue tracker: https://github.com/SeleniumHQ/selenium/issues/5621

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 2 • 6 years ago

We have tests for different types of collections: Arguments, Array,
FileList, HTMLAllCollection, HTMLCollection, HTMLFormControlsCollection,
HTMLOptionsCollection, and NodeList.  I verified that these are all
passing.

Unless this has something to do specifically with React…

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 3 • 6 years ago

I’ve ported the Marionette collection tests to WPT in bug 1453009.

	artur
	Comment 4 • 6 years ago

Have you seen the test case in https://github.com/Artur-/selenium-shadydom ?

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 5 • 6 years ago

(In reply to artur from comment #4)
> Have you seen the test case in https://github.com/Artur-/selenium-shadydom ?

This example is using shadow DOM which we currently not yet support.

	artur
	Comment 6 • 6 years ago

It uses the ShadyDOM polyfill because Firefox does not support shadow DOM. If you run it in Firefox, you see the cyclic reference issue because of how the polyfill works and the properties it adds.

	artur
	Comment 7 • 6 years ago

If you want a simpler test case, see the `no-shady` branch or https://github.com/Artur-/selenium-shadydom/blob/no-shady/src/main/webapp/testpage.html

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 8 • 6 years ago

(In reply to artur from comment #7)
> If you want a simpler test case, see the `no-shady` branch or
> https://github.com/Artur-/selenium-shadydom/blob/no-shady/src/main/webapp/
> testpage.html

Tested this for both the `div` and `span` tag via https://output.jsbin.com/zusogijulo/2 and it works just fine with Firefox 61 Beta and Firefox 62 Nightly. Firefox 61 will be released today, so please check if that works for you.

(In reply to Alexei Barantsev from comment #0)
> driver.get("http://todomvc.com/examples/scalajs-react/#/");
> driver.executeScript("return document.getElementsByTagName('input')");

This works just fine too.

I don't think that there is a remaining problem with Marionette here. Can you both please verify? Thanks.

	Alexei Barantsev Reporter
	Comment 9 • 6 years ago

It's still actual for me in the current Nightly (63), see the new trace log attached.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 11 • 6 years ago

Interesting. So I have always tested this script with Marionette and as you can see here it works perfectly:

> 1530016135724	Marionette	TRACE	3 -> [0,9,"WebDriver:Navigate",{"url":"http://todomvc.com/examples/scalajs-react/#/"}]
> 1530016135730	Marionette	DEBUG	[4294967297] Received DOM event beforeunload for about:blank
> 1530016136144	Marionette	DEBUG	[4294967297] Received DOM event pagehide for about:blank
> 1530016137468	Marionette	DEBUG	[4294967297] Received DOM event DOMContentLoaded for http://todomvc.com/examples/scalajs-react/#/
> 1530016137474	Marionette	DEBUG	[4294967297] Received DOM event pageshow for http://todomvc.com/examples/scalajs-react/#/
> 1530016137475	Marionette	TRACE	3 <- [1,9,null,{"value":null}]
> 1530016137498	Marionette	TRACE	3 -> [0,10,"WebDriver:ExecuteScript",{"scriptTimeout":null,"newSandbox":true,"args":[],"filename":"_a/test_minimized.py","script":"return document.getElementsByTagName('input')","sandbox":"default","line":58}]
> [<marionette_driver.marionette.HTMLElement object at 0x1078a84d0>]
> 1530016137513	Marionette	TRACE	3 <- [1,10,null,{"value":[{"element-6066-11e4-a52e-4f735466cecf":"f64a2dd5-ae1e-1b41-ba6b-d642ab5dae33","ELEMENT":"f64a2dd5-ae1e-1b41-ba6b-d642ab5dae33"}]}]

Now the excerpt when running it via geckodriver:

> 1530016200228	webdriver::server	DEBUG	-> POST /session/27dbee0c-4541-6f44-ad1a-16dc052c1b58/url {"url": "http://todomvc.com/examples/scalajs-react/#/"}
> 1530016200237	Marionette	TRACE	0 -> [0,2,"WebDriver:Navigate",{"url":"http://todomvc.com/examples/scalajs-react/#/"}]
> 1530016200259	Marionette	DEBUG	[2147483649] Received DOM event beforeunload for about:blank
> 1530016200700	Marionette	DEBUG	[2147483649] Received DOM event pagehide for about:blank
> 1530016202528	Marionette	DEBUG	[2147483649] Received DOM event DOMContentLoaded for http://todomvc.com/examples/scalajs-react/#/
> 1530016202536	Marionette	DEBUG	[2147483649] Received DOM event pageshow for http://todomvc.com/examples/scalajs-react/#/
> 1530016202541	Marionette	TRACE	0 <- [1,2,null,{"value":null}]
> 1530016202568	webdriver::server	DEBUG	<- 200 OK {"value": null}
> 1530016202570	webdriver::server	DEBUG	-> POST /session/27dbee0c-4541-6f44-ad1a-16dc052c1b58/execute/sync {"args": [], "script": "return document.getElementsByTagName('input')"}
> 1530016202590	Marionette	TRACE	0 -> [0,3,"WebDriver:ExecuteScript",{"args":[],"newSandbox":false,"script":"return document.getElementsByTagName('input')","scriptTimeout":null,"specialPowers":false}]
> 1530016202606	Marionette	TRACE	0 <- [1,3,{"error":"javascript error","message":"TypeError: cyclic object value","stacktrace":"assert.acyclic@chrome://marionette/ ... :529:3\nregisterSelf@chrome://marionette/content/listener.js:459:5\n@chrome://marionette/content/listener.js:1687:1\n"},null]

I thought maybe it's related to `newSandbox=False` or `specialPowers=False`, but it isn't. Marionette is still working:

> 1530016975971	Marionette	TRACE	3 -> [0,10,"WebDriver:ExecuteScript",{"scriptTimeout":null,"sandbox":"default","script":"return document.getElementsByTagName('input')","newSandbox":false,"line":59,"args":[],"specialPowers":false,"filename":"_a/test_minimized.py"}]
> 1530016975993	Marionette	TRACE	3 <- [1,10,null,{"value":[{"element-6066-11e4-a52e-4f735466cecf":"6aa9e100-e06e-5842-970d-1846e3b37d57","ELEMENT":"6aa9e100-e06e-5842-970d-1846e3b37d57"}]}]

But finally I figured out that this is the extra `sandbox="default"` argument Marionette specifies and what geckodriver doesnt, and which makes it actually failing.

I haven't done anything with sandboxes yet, so I would like to hear the feedback from Andreas what will be the correct approach to get it fixed.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 12 • 6 years ago

sandbox set to "default" uses an immutable sandbox, whereas not
specifying it uses a mutable one.  I don’t know why the serialisation
would fail because both script’s return values get serialises in
the same location in driver.js:
https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/testing/marionette/driver.js#1032

You should verify that the return value from this.listener.execute
is equal to this.listener.executeInSandbox and that both res’es are
passed to evaluate.toJSON.  It is also a possibility that the
serialisation issue is in the frame script, but without a full
stacktrace it is difficult to tell.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 13 • 6 years ago

So the problem with serialization only happens when there is no sandbox in use. Which means that `listener.execute()` is returning something different than `listener.executeInSandbox()`.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 14 • 6 years ago

So I figured out what's going on here...

Basically when executing any Javascript in a Sandbox the custom properties which have been added by the code on an element won't be present anymore when we are going to serialize the data inside of proxy.js. Only the properties as defined by the prototype will be present. As such it just works.

But now with running the Javascript code without a sandbox, any custom property will remain and as such causes a cyclic reference during serialization. 

When returning elements we only need a reference id to identify the element later. As such it doesn't matter if custom properties have been set, and so those should be simply ignored when serializing the element. Means we should skip all properties which are not part of the element's prototype.

Maybe we are doing so already, but the following assert fails:
https://dxr.mozilla.org/mozilla-release/rev/e9f06fd63988182f9f79f223a295b57acf81cee8/testing/marionette/evaluate.js#272

I assume that this assert is a left-over from before we were able to successfully detect/serialize cyclic data (bug 1106913). Removing it for testing purposes let the testcase pass. So we might be able to remove it? Andreas, do you have any objections?

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 15 • 6 years ago

Thinking more about it, this assert indeed seems to be wrong at this place, and would need to be removed. We should only use it for arbitrary objects and files, because for all other types we have custom `toJSON()` methods.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 16 • 6 years ago

Well, maybe not. Currently we treat `Array` as a collection, and as such would fail for cyclic elements in the array. As such the `assert.acyclic()` is necessary. But maybe we shouldn't treat `Array` as collection, because it is something different, and can contain arbitrary elements whereby the defined collections only return known types?

If the above can't be done we would need a way to get rid of all custom properties of elements when calling `JSON.stringify()` in `assert.acycle`.

Andreas, given that this is an area you did most of the work I would like to hear your feedback.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 17 • 6 years ago

Please note that the assertion we do here (`assert.acyclic`) causes a chicken and egg problem.

We actually want to check if an object isn't acyclic, but that method uses `JSON.stringify` which actually doesn't take care of our custom `toJSON` implementation for WebElement instances. And as such it fails.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 18 • 6 years ago

Ok, while reading https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify I noticed that `stringify` indeed should call our `toJSON` method. I will check if this really gets done, and if not why it doesn't work.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 19 • 6 years ago

Here the difference for elements in the collection which fails to serialize:

sandbox = None
--------------
*** element: let elements = document.getElementsByTagName('div');
            return elements;
*** element:
*** element: [object Object]
*** element: [object HTMLDivElement]

sandbox = "foo"
---------------
*** element: let elements = document.getElementsByTagName('div');
            return elements;
*** element:
*** element: [object Object]
*** element: [object HTMLDivElement]
*** element: [object Object]
*** element: [object ContentWebElement uuid=bfa31d2d-fc69-8446-abf2-587ab24814f3]

So with a sandbox we have two more elements in the HTMLCollection. The problematic `div` element is present in both cases, and only contains the circular property in the first case. 

Btw I thought that we would only serialize the return value of the script, but looks like we do way more.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 20 • 6 years ago

So it's basically the `wantXrays = false` setting in `sandbox.createMutable()` which causes this. But it's actually needed.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 21 • 6 years ago

I feel there needs to be some discussion in how those expando properties have to be handled. I will propose that for our next WebDriver meeting on Monday. Until then here something to read:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Xray_vision

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 22 • 6 years ago

There is so much contradictory information here it is hard to get
the essence of the problem we are facing.  I’ve replied to your
various comments, but it would be great with a summary of where we
stand.

(In reply to Henrik Skupin (:whimboo) from comment #16)

> But maybe we shouldn't treat `Array` as collection, because it is
> something different, and can contain arbitrary elements whereby
> the defined collections only return known types?

Arrays are the definition of what a collection is!  We also have a
couple of other specialised collections that fundamentally behave
more or less as arrays.

However the fundamental problem here is that you can have cyclic
references, e.g.:

> let a = [];
> let b = [];
> a[0] = b;
> b[0] = a;

(In reply to Henrik Skupin (:whimboo) from comment #17)

> Please note that the assertion we do here (`assert.acyclic`)
> causes a chicken and egg problem.
> 
> We actually want to check if an object isn't acyclic, but that
> method uses `JSON.stringify` which actually doesn't take care of
> our custom `toJSON` implementation for WebElement instances. And
> as such it fails.

What do you mean by “take care of”?

JSON.stringify implicitly calls callable toJSON properties.  We
should explicitly _not_ trust what that returns.  If that returns
something cyclic we should error.

(In reply to Henrik Skupin (:whimboo) from comment #20)

> So it's basically the `wantXrays = false` setting in
> `sandbox.createMutable()` which causes this. But it's actually
> needed.

(In reply to Henrik Skupin (:whimboo) from comment #21)

> I feel there needs to be some discussion in how those expando
> properties have to be handled.

I won’t pretend I actually understand how sandboxes and principals
work.  This is beyond my area of knowledge.

The alternative here is to evaluate the script directly within the
frame script without the use of sandboxes.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 23 • 6 years ago

Something I would like to add before we discuss that is that the example script works when I use `getElementsById`, but fails when using `getElementsByTagName`.

It means a single mapping from HTMLElement -> WebElement is fine and simply ignores this expando property.

But for a collection it fails, most likely because there is this extra HTMLElement in the returned data as visible in comment 19.

I don't know how and where we map the data and if we should replace those native HTMLElement instances with our own WebElement instance. But at least with this we should get rid of the cyclic problem.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 24 • 6 years ago

I checked again and here my finding:

1) When a single HTML element is returned from `evaluate.sandbox()` the `evaluate.toJSON()` method correctly retrieves the known WebElement from the store:

https://dxr.mozilla.org/mozilla-release/rev/e9f06fd63988182f9f79f223a295b57acf81cee8/testing/marionette/evaluate.js#279-282

As such any expando property isn't taken into account for serialization at all.

2) Now when a HTMLCollection object is getting returned, we directly call the `assert.acyclic()` check, which internally runs `JSON.stringify()`. Here the HtmlDivElement.toJSON() method is internally called, and it fails because of the expando property.

Here possible solutions:

1) We filter any collection before running the `assert.acyclic()` check against it, and convert any known elements to WebElements (and frame/window elements?). At the same time we ignore any custom objects, which could have a cyclic reference. By that method we could eliminate cyclic references for HTMLElement objects.

2) We enable `wantXrays` for mutable sandboxes. Here it is not clear to me yet which implications this change would have. The benefit here is clearly that it is more safe to run JS code. 

I won't have the time to continue here until I'm back from PTO.

Andreas, if you have a bit of time feel free to pick it up.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 25 • 6 years ago

(In reply to Henrik Skupin (:whimboo) [away 07/08 - 07/22] from
comment #24)

> 1) We filter any collection before running the `assert.acyclic()`
> check against it, and convert any known elements to WebElements
> (and frame/window elements?). At the same time we ignore any
> custom objects, which could have a cyclic reference. By that
> method we could eliminate cyclic references for HTMLElement
> objects.

More precisely we will only want to run assert.acyclic on array
entires that are not elements.

You can do this by filtering/iterating over the array and check
each entry individually, or perhaps assert.acyclic can be
made element-aware, so that it would ignore elements found in
collections?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 26 • 6 years ago

Reproducible TC from whimboo:
https://irccloud.mozilla.com/pastebin/6iSpyOR0/

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 27 • 6 years ago

Just to clarify one point above: JSON.stringify does not actually
run toJSON on the node element.  It calls the toJSON property if
it is callable, but the Element prototype does not provide this.
JSON.stringify enumerates an object’s own properties and serialises
these, which explains why it tries to serialise body.foo = div.

	David Burns :automatedtester
	Comment 32 • 6 years ago
mozreview-review

Comment on attachment 8991053 [details]
Bug 1447977 - Move evaluate module API docs to RST.

https://reviewboard.mozilla.org/r/256042/#review262970

	David Burns :automatedtester
	Comment 33 • 6 years ago
mozreview-review

Comment on attachment 8991054 [details]
Bug 1447977 - Move cyclic object test function to evaluate.

https://reviewboard.mozilla.org/r/256044/#review262972

	David Burns :automatedtester
	Comment 34 • 6 years ago
mozreview-review

Comment on attachment 8991055 [details]
Bug 1447977 - Handle cyclic references in element prototypes.

https://reviewboard.mozilla.org/r/256046/#review262974

::: testing/web-platform/meta/MANIFEST.json:612425
(Diff revision 2)
>    "shadow-dom/untriaged/events/event-dispatch/test-002.html": [
>     "d3538e40ac8cc4749d69cc0dcd35d42fd3ef778a",
>     "testharness"
>    ],
>    "shadow-dom/untriaged/events/event-dispatch/test-003.html": [
> -   "94ed940ccca11f9abc37a940ba5f5fc194ea2317",
> +   "47b84ac9527ad9e51bdba5b0c0b3ecbdba2e3696",

The changes in this file don't seem related to this patch

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 35 • 6 years ago
mozreview-review-reply

Comment on attachment 8991055 [details]
Bug 1447977 - Handle cyclic references in element prototypes.

https://reviewboard.mozilla.org/r/256046/#review262974

> The changes in this file don't seem related to this patch

You are of course right, but "./mach wpt-manifest-update" does not
update SHAs only of the changed files in the repository.

	Pulsebot
	Comment 36 • 6 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dccb4ac6468a
Move evaluate module API docs to RST. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/cbb958819d32
Move cyclic object test function to evaluate. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/b4cd5a640564
Handle cyclic references in element prototypes. r=automatedtester

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 37 • 6 years ago

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11912 for changes under testing/web-platform/tests

	Stefan Hindli [:stefan_hindli]
	Comment 38 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/dccb4ac6468a
https://hg.mozilla.org/mozilla-central/rev/cbb958819d32
https://hg.mozilla.org/mozilla-central/rev/b4cd5a640564

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 39 • 6 years ago

Upstream PR merged

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 40 • 6 years ago

I want to consider uplifting this to beta because it unblocks
Protractor locators.  Do you have any concerns?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 41 • 6 years ago

Cancel that.  We have a recent spike in intermittents that could
be caused by this:
https://bugzilla.mozilla.org/show_bug.cgi?id=1414495#c211

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 42 • 6 years ago

Let’s uplift this to beta now that we know it didn’t cause the
intermittents.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 43 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/332084df6651
https://hg.mozilla.org/releases/mozilla-beta/rev/43f6112e9578
https://hg.mozilla.org/releases/mozilla-beta/rev/20c023fb254c

	Jozef Marko
	Comment 44 • 6 years ago

Hello Everyone, I would like to find out, which firefox esr release will contain fix of this issue, looking on this table [1] I think it will be included in 60.3.esr, am I right?

[1]
https://wiki.mozilla.org/Release_Management/Calendar

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 45 • 6 years ago

This change will not be included in the current ESR, but the _next_
ESR release.  I don’t know what version number that will get.