Detect cyclic object references when marshaling objects in evaluate.toJSON

RESOLVED FIXED in Firefox 59

Status

defect
P1
normal
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: whimboo, Assigned: ato)

Tracking

(Blocks 1 bug)

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(3 attachments)

Yesterday I noticed that a call to execute_script() can cause an infinite recursion combined with an OOM by simply passing in an retrieved DOM node as parameter and calling .ownerDocument.defaultView on it. In the following you can find the two examples:

Example 1 will raise an out of memory JavascriptException for trying to retrieve the owner document for DOM node:

        element = self.marionette.find_element('css selector', ':root')
        print self.marionette.execute_script("""
            return arguments[0].ownerDocument;
        """, script_args=[element])

Example 2 might be related here, and simply adds another property. But it fails differently with a "InternalError: too much recursion" JavascriptException:

        element = self.marionette.find_element('css selector', ':root')
        print self.marionette.execute_script("""
            return arguments[0].ownerDocument.defaultView;
        """, script_args=[element])
Returning a `document` will cause issues because it is a complex object and not something that we can stringify into a JSON object. What is it that you trying to do?
Flags: needinfo?(hskupin)
Yes, you are right! That's something I totally missed when I created this bug. So indeed the document is not serializable. 

I wonder if we should throw a specific exception in such a case to let people know about it. Right now it's kinda hard to see.
Flags: needinfo?(hskupin)
I totally agree that we should be throwing a more meaningful message when we get into this situation.
Summary: OOM and infinite recursion in execute_script() by trying to retrieve the owner document and default view → Return meaningful error message when trying to serialize complex objects from server to client

Comment 4

4 years ago
could you please tell the path of the file where you found it?
Flags: needinfo?(hskupin)
The code in comment 0 was an example I put together. So you would have to create a simple test for Marionette and put this in. Best would be to work together with David if you are interested to take this bug.
Flags: needinfo?(hskupin)

Comment 6

4 years ago
As this bug is related to marionette component do i need to do some extra build up for this?
what i mean is i have built mozilla-central and fixed 3 bugs, but this bug may be my first marionette related.So could you please tell do i need to pull/build anything extra to work on this bug?
Also like in comment 0 you said to create a simple test. Test for what?
Flags: needinfo?(hskupin)
As mentioned in my last comment I would kindly refer to David here.
Flags: needinfo?(hskupin) → needinfo?(dburns)
this appears to be a Spidermonkey bug in all honesty since we are just calling JSON.stringify() on the complex object and then never getting anything back
Flags: needinfo?(dburns)
(In reply to David Burns :automatedtester from comment #8)
> this appears to be a Spidermonkey bug in all honesty since we are just
> calling JSON.stringify() on the complex object and then never getting
> anything back

No, it's not. The problem here is that we really have an infinite loop, and never finish serializing the data. This is because we do not follow the WindowProxy serialization steps as pointed out in the spec:

https://w3c.github.io/webdriver/webdriver-spec.html#dfn-json-serialization-of-the-windowproxy-object

It means in case of type [object Window] the following has to be returned:

In case of a:

* top-browsing context: {"window-fcc6-11e5-b4f8-330a88ab9d7f", handle}
* browsing context: {"frame-075b-4da1-b6ba-e579c2d3230a", handle}

Andreas, given that you did most of the refactoring lately, would you mind to take this bug?
Flags: needinfo?(ato)
Blocks: webdriver
Summary: Return meaningful error message when trying to serialize complex objects from server to client → Make serialization of WindowProxy object webdriver compliant
(In reply to Henrik Skupin (:whimboo) from comment #9)
> (In reply to David Burns :automatedtester from comment #8)
> > this appears to be a Spidermonkey bug in all honesty since we are just
> > calling JSON.stringify() on the complex object and then never getting
> > anything back
> 
> No, it's not. The problem here is that we really have an infinite loop, and
> never finish serializing the data. This is because we do not follow the
> WindowProxy serialization steps as pointed out in the spec:


Yes it's a spidermonkey bug. We had to change the webdriver spec to get around this problem.
(In reply to David Burns :automatedtester from comment #10)
> Yes it's a spidermonkey bug. We had to change the webdriver spec to get
> around this problem.

Can you please explain further? Should JSON.stringify() take into account cyclic references? I also cannot find a bug about that problem raised anywhere. Thanks.
Flags: needinfo?(dburns)
Assignee

Comment 12

2 years ago
This issue seems to coalesce two different problems:

  1. Marionette does not yet support web window and web frame
     serialisation.

  2. Attempting to return objects with cyclic references from
     Execute Script/Execute Async Script causes infinite recursion.

(In reply to David Burns :automatedtester from comment #8)

> this appears to be a Spidermonkey bug in all honesty since we are
> just calling JSON.stringify() on the complex object and then never
> getting anything back

You are assuming here that we use JSON.stringify to serialise the
return value from the evaluated script.  In fact we reuse the
evaluate.toJSON [1] function because there are cases JSON.stringify
doesn’t do for us, such as expanding element collections (Array,
NodeList, HTMLCollection, et al.), converting Elements to web
element representations, and converting the value of <input
type=file> to a string.

  [1] https://searchfox.org/mozilla-central/rev/a5d613086ab4d0578510aabe8653e58dc8d7e3e2/testing/marionette/evaluate.js#245-307
Flags: needinfo?(ato)
Assignee

Comment 13

2 years ago
I will reuse this bug to track the original issue whimboo reported,
which is that Marionette currently ends up in an infinite recursion
loop when trying to marshal objects with cyclic references.

The work on making Marionette marshal web windows and web frames
depends on https://bugzil.la/marionette-window-tracking.  I’ve
filed https://bugzil.la/1420461 specifically about this.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Flags: needinfo?(dburns)
Priority: -- → P1
Assignee

Updated

2 years ago
Summary: Make serialization of WindowProxy object webdriver compliant → Detect cyclic object references when marshaling objects in evaluate.toJSON
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Duplicate of this bug: 1398094
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Reporter

Comment 24

2 years ago
mozreview-review
Comment on attachment 8931747 [details]
Bug 1106913 - Send errors from sendResponse/sendError back to chrome.

https://reviewboard.mozilla.org/r/202874/#review208644

::: commit-message-c15f1:6
(Diff revision 3)
> +Bug 1106913 - Send errors from sendResponse/sendError back to chrome. r?whimboo
> +
> +Errors that arise from inside sendResponse or sendError are currently
> +only logged to stdout.  Now that we have a pretty safe error.wrap
> +implementation we should serialise them and return them to the main
> +process so that they are not "lost" in stdout.

This is great and soooo needed. It's a pity to have to search the log for such failures. 

I noticed that we also have instances of error.report at least in server.js. I think we should make sure to reduce calling this method to only a single place, which is the method for sending the response. Or is something blocking us from doing so?
Attachment #8931747 - Flags: review?(hskupin)
Reporter

Comment 25

2 years ago
mozreview-review
Comment on attachment 8931748 [details]
Bug 1106913 - Add assert.acyclic for testing for cyclic objects.

https://reviewboard.mozilla.org/r/202876/#review208646

::: testing/marionette/assert.js:43
(Diff revision 3)
> + *
> + * @param {*} obj
> + *     Object test.  This assertion is only meaningful if passed
> + *     an actual object or array.
> + * @param {Error=} [error=JavaScriptError] error
> + *     Error to throw if assertion fails.

Do we have to make this configurable? I would hard-code that type. Then the @throws line matches too.
Attachment #8931748 - Flags: review?(hskupin)
Reporter

Comment 26

2 years ago
mozreview-review
Comment on attachment 8931749 [details]
Bug 1106913 - Detect cyclic objects when marshaling objects.

https://reviewboard.mozilla.org/r/202878/#review208648

::: commit-message-1e6d4:11
(Diff revision 3)
> +Example of cyclic object:
> +
> +	let obj = {};
> +	obj.cyclic = obj;
> +
> +Passing this through evalaute.toJSON currently causes an infinite

evaluate

::: testing/marionette/evaluate.js:327
(Diff revision 3)
>    }
>  
>    // arbitrary objects + files
>    let rv = {};
>    for (let prop in obj) {
> +    assert.acyclic(obj[prop]);

Please note that this degrades the performance of the method a lot. Specifically when there is a deep-nested hierarchy. Currently this check will be run at each level, and then down to the leaves.

What can we do to improve that? I assume running the check once from the top-level doesn't work?

::: testing/web-platform/meta/MANIFEST.json:373426
(Diff revision 3)
>       {}
>      ]
>     ],
> +   "webdriver/tests/execute_script/cyclic.py": [
> +    [
> +     "/webdriver/tests/execute_script/cyclic.py",

I see `acyclic` vs `cyclic` through those patches. Can we agree on using `cyclic`?

::: testing/web-platform/meta/webdriver/tests/contexts/json_serialize_windowproxy.py.ini
(Diff revision 3)
>  [json_serialize_windowproxy.py]
> -  expected: TIMEOUT

Is that an expected change?
Attachment #8931749 - Flags: review?(hskupin)
Assignee

Comment 27

2 years ago
mozreview-review-reply
Comment on attachment 8931747 [details]
Bug 1106913 - Send errors from sendResponse/sendError back to chrome.

https://reviewboard.mozilla.org/r/202874/#review208644

> This is great and soooo needed. It's a pity to have to search the log for such failures. 
> 
> I noticed that we also have instances of error.report at least in server.js. I think we should make sure to reduce calling this method to only a single place, which is the method for sending the response. Or is something blocking us from doing so?

Filed https://bugzil.la/1421316 to follow up on this.
Assignee

Comment 28

2 years ago
mozreview-review-reply
Comment on attachment 8931748 [details]
Bug 1106913 - Add assert.acyclic for testing for cyclic objects.

https://reviewboard.mozilla.org/r/202876/#review208646

> Do we have to make this configurable? I would hard-code that type. Then the @throws line matches too.

We don’t, this is a good point.
Assignee

Comment 29

2 years ago
mozreview-review-reply
Comment on attachment 8931749 [details]
Bug 1106913 - Detect cyclic objects when marshaling objects.

https://reviewboard.mozilla.org/r/202878/#review208648

> Please note that this degrades the performance of the method a lot. Specifically when there is a deep-nested hierarchy. Currently this check will be run at each level, and then down to the leaves.
> 
> What can we do to improve that? I assume running the check once from the top-level doesn't work?

> Please note that this degrades the performance of the method a
> lot.

[citation needed] I think.  Did you measure it?

> What can we do to improve that? I assume running the check once
> from the top-level doesn't work?

Running assert.acyclic on obj once you know it’s an arbitrary
object will run you into problems if one of the immediate or nested
properties is something you are supposed to serialise.

For example if obj was {foo: <HTMLElement>} it would complain that
HTMLElement is cyclic because it contains references to WindowProxy.

> I see `acyclic` vs `cyclic` through those patches. Can we agree on using `cyclic`?

The assertion is called assert.acyclic because it asserts that the
object isn’t cyclic.  The test is testing cyclic values.  It’s
really only the negatory form that is used for the assertion.  I
don’t really have any idea what would be a better way of phrasing
this.

> Is that an expected change?

It used to be expected to time out because we had an infinite
recursion, now it is expected to just fail because we don’t serialise
web windows.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Reporter

Comment 33

2 years ago
mozreview-review
Comment on attachment 8931747 [details]
Bug 1106913 - Send errors from sendResponse/sendError back to chrome.

https://reviewboard.mozilla.org/r/202874/#review209444
Attachment #8931747 - Flags: review?(hskupin) → review+
Reporter

Comment 34

2 years ago
mozreview-review
Comment on attachment 8931748 [details]
Bug 1106913 - Add assert.acyclic for testing for cyclic objects.

https://reviewboard.mozilla.org/r/202876/#review209446

::: testing/marionette/test_assert.js:46
(Diff revision 4)
> +    let obj = {};
> +    obj.reference = obj;
> +    assert.acyclic([obj]);
> +  }, JavaScriptError);
> +
> +  // custom error and message

`Custom message` is enough
Attachment #8931748 - Flags: review?(hskupin) → review+
Reporter

Comment 35

2 years ago
mozreview-review-reply
Comment on attachment 8931749 [details]
Bug 1106913 - Detect cyclic objects when marshaling objects.

https://reviewboard.mozilla.org/r/202878/#review208648

> > Please note that this degrades the performance of the method a
> > lot.
> 
> [citation needed] I think.  Did you measure it?
> 
> > What can we do to improve that? I assume running the check once
> > from the top-level doesn't work?
> 
> Running assert.acyclic on obj once you know it’s an arbitrary
> object will run you into problems if one of the immediate or nested
> properties is something you are supposed to serialise.
> 
> For example if obj was {foo: <HTMLElement>} it would complain that
> HTMLElement is cyclic because it contains references to WindowProxy.

> For example if obj was {foo: <HTMLElement>} it would complain that
> HTMLElement is cyclic because it contains references to WindowProxy.

This is not an example for a cyclic reference per se. If we would serialize WindowProxy correctly, its reference to the HTMLElement somewhere down the node tree, would be gone.

So maybe I misread how the check works. Lets say we have a collection and enter the `isCollection` case. Which levels do we check with `assert.acyclic` here? It's every element, and everything below it, right? As next step we run `toJSON` for each of the elements in the collection, and run the assert again for data we already have verified?

If that is the case, we should maybe mark a subtree as already successfully tested so that no further calls to `assert.acyclic` are necessary? This parameter could be overwritten for those data below an arbitrary object.
Assignee

Comment 36

2 years ago
mozreview-review-reply
Comment on attachment 8931749 [details]
Bug 1106913 - Detect cyclic objects when marshaling objects.

https://reviewboard.mozilla.org/r/202878/#review208648

> > For example if obj was {foo: <HTMLElement>} it would complain that
> > HTMLElement is cyclic because it contains references to WindowProxy.
> 
> This is not an example for a cyclic reference per se. If we would serialize WindowProxy correctly, its reference to the HTMLElement somewhere down the node tree, would be gone.
> 
> So maybe I misread how the check works. Lets say we have a collection and enter the `isCollection` case. Which levels do we check with `assert.acyclic` here? It's every element, and everything below it, right? As next step we run `toJSON` for each of the elements in the collection, and run the assert again for data we already have verified?
> 
> If that is the case, we should maybe mark a subtree as already successfully tested so that no further calls to `assert.acyclic` are necessary? This parameter could be overwritten for those data below an arbitrary object.

>> For example if obj was {foo: <HTMLElement>} it would complain
>> that HTMLElement is cyclic because it contains references to
>> WindowProxy.
> 
> This is not an example for a cyclic reference per se. If we would
> serialize WindowProxy correctly, its reference to the HTMLElement
> somewhere down the node tree, would be gone.

This is the very definition of a cyclic element, but you may be
right that if we were to have a serialisation for WindowProxy the
el.ownerDocument.defaultView property wouldn’t cause a cyclic
reference.

However I don’t see how this changes the fact that you need to
iterate over the properties to know which child nodes you are meant
to provide special serialisations for.  If assert.acyclic(obj) is
run on the top-level object it will complain that obj contains
cyclic references, even if some of those cyclic references are
things we are meant to provide custom serialisations for.

Also keep in mind that JSON.stringify doesn’t check everything.
For example JSON.stringify({foo: document.documentElement}) returns
"{\"foo\":\"{}\"}", not that I think this matters.

> So maybe I misread how the check works. Lets say we have a
> collection and enter the isCollection case. Which levels do we
> check with assert.acyclic here? It's every element, and everything
> below it, right? As next step we run toJSON for each of the
> elements in the collection, and run the assert again for data we
> already have verified?
> 
> If that is the case, we should maybe mark a subtree as already
> successfully tested so that no further calls to assert.acyclic
> are necessary? This parameter could be overwritten for those data
> below an arbitrary object.

I think that is correct summary.  The JSON.stringify check is
only run on collections and each of the values of an arbitrary
object; not on things we recognise such as primitives, web element
references, elements, and custom JSON representations.

I guess it is possible to add a third argument to evaluate.toJSON
with a choice on whether to re-run assert.acyclic on the child nodes
when it is invoked recursively, but I’m skeptical that this is a
worthwhile performance improvement to make.  My gut feeling is that
we shouldn’t make premature performance optimisations, because it
does come at the cost of complexity.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Reporter

Comment 40

2 years ago
mozreview-review-reply
Comment on attachment 8931749 [details]
Bug 1106913 - Detect cyclic objects when marshaling objects.

https://reviewboard.mozilla.org/r/202878/#review208648

> >> For example if obj was {foo: <HTMLElement>} it would complain
> >> that HTMLElement is cyclic because it contains references to
> >> WindowProxy.
> > 
> > This is not an example for a cyclic reference per se. If we would
> > serialize WindowProxy correctly, its reference to the HTMLElement
> > somewhere down the node tree, would be gone.
> 
> This is the very definition of a cyclic element, but you may be
> right that if we were to have a serialisation for WindowProxy the
> el.ownerDocument.defaultView property wouldn’t cause a cyclic
> reference.
> 
> However I don’t see how this changes the fact that you need to
> iterate over the properties to know which child nodes you are meant
> to provide special serialisations for.  If assert.acyclic(obj) is
> run on the top-level object it will complain that obj contains
> cyclic references, even if some of those cyclic references are
> things we are meant to provide custom serialisations for.
> 
> Also keep in mind that JSON.stringify doesn’t check everything.
> For example JSON.stringify({foo: document.documentElement}) returns
> "{\"foo\":\"{}\"}", not that I think this matters.
> 
> > So maybe I misread how the check works. Lets say we have a
> > collection and enter the isCollection case. Which levels do we
> > check with assert.acyclic here? It's every element, and everything
> > below it, right? As next step we run toJSON for each of the
> > elements in the collection, and run the assert again for data we
> > already have verified?
> > 
> > If that is the case, we should maybe mark a subtree as already
> > successfully tested so that no further calls to assert.acyclic
> > are necessary? This parameter could be overwritten for those data
> > below an arbitrary object.
> 
> I think that is correct summary.  The JSON.stringify check is
> only run on collections and each of the values of an arbitrary
> object; not on things we recognise such as primitives, web element
> references, elements, and custom JSON representations.
> 
> I guess it is possible to add a third argument to evaluate.toJSON
> with a choice on whether to re-run assert.acyclic on the child nodes
> when it is invoked recursively, but I’m skeptical that this is a
> worthwhile performance improvement to make.  My gut feeling is that
> we shouldn’t make premature performance optimisations, because it
> does come at the cost of complexity.

At least we should get notified by perfherder if there are some substantial performance regressions. So lets get this out, and have a look.
Reporter

Comment 41

2 years ago
mozreview-review
Comment on attachment 8931749 [details]
Bug 1106913 - Detect cyclic objects when marshaling objects.

https://reviewboard.mozilla.org/r/202878/#review209790
Attachment #8931749 - Flags: review?(hskupin) → review+
Assignee

Comment 42

2 years ago
(In reply to Henrik Skupin (:whimboo) from comment #40)

> At least we should get notified by perfherder if there are some
> substantial performance regressions. So lets get this out, and
> have a look.

This is true.  For the record I’m not saying I’m opposed to
making the change if it proves to be a problem.  Actually I did try
it out locally and it seems to work fine, even in more complicated
tests than those I wrote for WPT.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 46

2 years ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d18cc7d8df91
Send errors from sendResponse/sendError back to chrome. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/eeed4304f9af
Add assert.acyclic for testing for cyclic objects. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/04b16dc7a9b8
Detect cyclic objects when marshaling objects. r=whimboo
https://hg.mozilla.org/mozilla-central/rev/d18cc7d8df91
https://hg.mozilla.org/mozilla-central/rev/eeed4304f9af
https://hg.mozilla.org/mozilla-central/rev/04b16dc7a9b8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.