Closed Bug 1106913 Opened 10 years ago Closed 7 years ago

Detect cyclic object references when marshaling objects in evaluate.toJSON

Categories

(Remote Protocol :: Marionette, defect, P1)

Product:
Remote Protocol
Component:
Marionette
Type:
defect

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: whimboo, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1106913 - Send errors from sendResponse/sendError back to chrome.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1106913 - Add assert.acyclic for testing for cyclic objects.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1106913 - Detect cyclic objects when marshaling objects.
59 bytes, text/x-review-board-request
whimboo
: review+
Details 
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
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)
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)
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)
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
Summary: Make serialization of WindowProxy object webdriver compliant → Detect cyclic object references when marshaling objects in evaluate.toJSON
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)
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)
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)
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.
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.
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 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+
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+
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.
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 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.
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+
(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.
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
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: