906074 - Mozmill fails to preserve the whole persisted object after a restart if it contains non-serializable data

Status: RESOLVED WONTFIX

(Testing Graveyard :: Mozmill, defect)

Description

[Andrei Eftimie]

We use the persisted object to store information between different tests.

If we store a more complex object (I am not sure of the trigger yet, might seem unserializable references) all persisted items are destroyed.

Relevant testcase coming up shortly.

Comment 1

[Andrei Eftimie]

Attachment: testcase.js

Run this with mozmill 2.0

I've left some dump statements in there.
Notice how when persisted.two is referencing the window object, after a restart persisted.one is lost.

If persisted.two is a regular string, we correctly maintain the value of persisted.one

=====

Bottom line: an undesired element stored in persisted should *not* influence (destroy) other persisted entries.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

And that's fully correct. You can not include any kind of object in the persisted data, if it cannot be converted to a JSON string. There is nothing Mozmill can do here as forgetting all of it.

The question here is do we fail with an exception or not? If we do all is fine. Otherwise we should fix that.

Comment 3

[Andrei Eftimie]

We should fail graciously.

Lets take the following example:
> persisted.one = "one";
> persisted.two = window;
> persisted.one; // undefined

persisted.one should retain its value.

We should not invalidate the *whole* persisted object if one of its attributes is faulty.

Comment 4

[Andrei Eftimie]

Attachment: patch v1

Here is a potential approach.

The problem is that we send the whole persisted object to be serialized:
https://github.com/mozilla/mozmill/blob/master/jsbridge/jsbridge/extension/resource/modules/Server.jsm#L56

To make sure that we persist good objects, and discard only rotten ones, as I am exemplifying in this patch, we can try to serialize each persisted object before sending it further, and nullifying its contents if that fails.

As Henrik pointed out in comment 2, should we throw an exception?
I'd say we should only throw a warning.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 792056 [details] [diff] [review]
patch v1

Review of attachment 792056 [details] [diff] [review]:
-----------------------------------------------------------------

Any checks like that have to be part of jsbridge, which indeed might already have a special handling if we fail in using JSON.stringify(). That given code will have to be updated. I don't think it's helpful to have it at two separate locations.

Comment 6

[Andrei Eftimie]

This is not an easy problem to solve.

If we reference DOM elements we fail with:
> TypeError: cyclic object value
because they contain cyclic references.


I've tried a few approaches:
1) to "decycle" the problematic object if the serialization fails, with Douglas Crockford's cycle.js https://github.com/douglascrockford/JSON-js/blob/master/cycle.js

This is failing to on some "protected" objects, would fail with:
> [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDOMWindow.sessionStorage]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: resource://jsbridge/modules/Server.jsm :: derez :: line 280"  data: no]

I've patched the cycle code to ignore these objects, but we failed with:
> CRITICAL | Framework Failure | {
>   "message": "[JavaScript Error: \"invalid property id\" {file: \"resource://jsbridge/modules/Server.jsm\" line: 32 column: 60 source: \"ache\": {}, \"toolbar\": {}, \"onprogress\": null, \"scrollX\": 0, \"scro\"}]"
> }
Which is looks like an encoding problem or maybe the resulting string is to long and gets trimmed. Maybe something else entirely.

2) I've tried writing my own small function to either break these cyclic references or nullify them.
But this is problematic, I haven't been able to come up with a good & reliable solution.

In theory this would improve our stringify calls, but at this point might be overkill for our particular problem.

After a good day and a half trying to fix this in a generalised and abstract way in jsbridge, fixing it locally for our persisted object, similar to attachement 792056 looks like a more viable option.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is nothing new and the behavior exists since the 1.5 branch. So it should not block 2.0. We can try to find a nice solution for 2.1. For now the tests should ensure that they clean-up the persisted object (if used) from non-supported data.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This bug will not be taken for version 2.1. We might want to re-evaluate for 2.2.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We lived with that for years, so I do not think it is a blocker for the latest releases of Mozmill.

Also Mozmill will reach its end of life soon. We are currently working on getting all the tests for Firefox ported to Marionette. For status updates please see bug 1080766.