WebDriver:ExecuteScript causes cyclic reference exception on attempt to convert element collection in evaluate.toJSON

RESOLVED FIXED in Firefox 62

Status

defect
P1
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: barancev, Assigned: ato)

Tracking

(Blocks 1 bug)

61 Branch
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed, firefox63 fixed)

Details

()

Attachments

(5 attachments)

Reporter

Description

a year ago
Posted file geckodriver.log
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.
Reporter

Comment 1

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

Updated

a year ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → All
Priority: -- → P2
Hardware: Unspecified → All
Summary: Cyclic reference exception on attempt to convert element collection in evaluate.toJSON → WebDriver:ExecuteScript causes cyclic reference exception on attempt to convert element collection in evaluate.toJSON
Assignee

Comment 2

a year 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…
Assignee

Comment 3

a year ago
I’ve ported the Marionette collection tests to WPT in bug 1453009.
Depends on: 1453009

Comment 4

a year ago
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.

Comment 6

a year 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.

Comment 7

a year 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
(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.
Flags: needinfo?(barancev)
Flags: needinfo?(artur)
Reporter

Comment 9

11 months ago
It's still actual for me in the current Nightly (63), see the new trace log attached.
Flags: needinfo?(barancev)
Reporter

Comment 10

11 months 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.
Flags: needinfo?(artur) → needinfo?(ato)
Assignee

Comment 12

11 months 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.
Flags: needinfo?(ato)
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()`.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P2 → P1
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?
Flags: needinfo?(ato)
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.
Flags: needinfo?(ato)
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.
Flags: needinfo?(ato)
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
Assignee

Comment 22

11 months 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.
Flags: needinfo?(ato)
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.
Assignee

Comment 25

11 months 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?
Assignee

Updated

11 months ago
Assignee: hskupin → ato
Assignee

Comment 26

11 months ago
Reproducible TC from whimboo:
https://irccloud.mozilla.com/pastebin/6iSpyOR0/
Assignee

Comment 27

11 months 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 32

11 months ago
mozreview-review
Comment on attachment 8991053 [details]
Bug 1447977 - Move evaluate module API docs to RST.

https://reviewboard.mozilla.org/r/256042/#review262970
Attachment #8991053 - Flags: review?(dburns) → review+

Comment 33

11 months ago
mozreview-review
Comment on attachment 8991054 [details]
Bug 1447977 - Move cyclic object test function to evaluate.

https://reviewboard.mozilla.org/r/256044/#review262972
Attachment #8991054 - Flags: review?(dburns) → review+

Comment 34

11 months 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
Attachment #8991055 - Flags: review?(dburns) → review+
Assignee

Comment 35

11 months 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.

Comment 36

11 months 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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11912 for changes under testing/web-platform/tests

Comment 38

11 months 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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Upstream PR merged
Assignee

Comment 40

10 months ago
I want to consider uplifting this to beta because it unblocks
Protractor locators.  Do you have any concerns?
Flags: needinfo?(dburns)
Assignee

Comment 41

10 months 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
Flags: needinfo?(dburns)
Assignee

Comment 42

10 months ago
Let’s uplift this to beta now that we know it didn’t cause the
intermittents.
Whiteboard: [checkin-needed-beta]

Comment 44

8 months 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
Assignee

Comment 45

8 months 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.
You need to log in before you can comment on or make changes to this bug.