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.

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

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…

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

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

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

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.

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

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

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

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.

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.

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

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?

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.

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.

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.

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.

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.

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

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

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.

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.

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.

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

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

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.

Comment on attachment 8991053 [details] Bug 1447977 - Move evaluate module API docs to RST. https://reviewboard.mozilla.org/r/256042/#review262970

Comment on attachment 8991054 [details] Bug 1447977 - Move cyclic object test function to evaluate. https://reviewboard.mozilla.org/r/256044/#review262972

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

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.

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

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

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

Upstream PR merged

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

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

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

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

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

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.