1122583 - Add support for multiple sandboxes

Status: RESOLVED DUPLICATE of bug 1149618

(Remote Protocol :: Marionette, defect)

Description

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

Right now there is only a single sandbox available to use for Marionette tests. This can cause problems if one part of the tests has to keep some states or listeners active during a test, but another part is using execute_script with new_sandbox=TRUE. With that the formerly setup sandbox gets replaced, and all data is lost.

Given that we cannot have new_sandbox=False as default, I would propose that we add multiple sandbox support. Identification of the right sandbox could happen with an uuid, so its very unlikely that other code is re-using the same sandbox. This is somewhat important if you have split out the modules for UI elements and API from your tests, so any usage of the sandbox is unknown for each party.

Here an example:

# module (maybe even in another repository)
def register(value):
    self.marionette.execute_script("""
        this.value = arguments[0];
    """, script_args=[value])

def check():
    return self.marionette.execute_script("""
        return this.value;
    """, new_sandbox=False)

# test 1 - pass, because the same sandbox is re-used
register(3)
self.assertEqual(check(), 3)

# test 2 - fail, because the test doesn't know about the module sandbox
register(4)
self.marionette.execute_script("return window.document.title")
self.assertEqual(check(), 4)

This is a huge problem if tests have to use events and observers which need to be registered before an action, and unregistered afterward. If the action is also done via execute_script, the registered event listener or observer topic is gone.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

I noticed there's a namespace-like object called "testUtils" tacked on to every sandbox that has some utility functions available to each sandbox, and persists between sandboxes. Maybe this could be used for our case on implementing a registry of observers/listeners that need to be queried for state and later un-registered. For instance, this works right now (new sandbox for each call):

        self.marionette.execute_script("""
          this.testUtils.ffuiTestRegistry = {};
          this.testUtils.ffuiTestRegistry['key'] = {'topic': 123};
        """)
        keyed_value = self.marionette.execute_script("""
          return this.testUtils.ffuiTestRegistry['key'];
        """)
        self.assertEqual(keyed_value, {'topic': 123})

execute_script seems to really be for tests that need to run ad-hoc scripts, so discouraging communication between globals in a sandbox or multiple sandboxes seems desirable in general. We're building libraries that need to know a lot about browser state and frequently interact with chrome apis, using testUtils or some other specific facility for this purpose might be more appropriate than tacking things on to execute_script.

Comment 2

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

Sounds like an alternative. David, what do you say? Can this object be used?

Comment 3

[David Burns :automatedtester]

Not sure where that came in so redirecting to Jonathan as he might know if that is a good idea.

Comment 4

[Jonathan Griffin (:jgriffin)]

testUtils is just used to load several JS files into the test's context:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-server.js#22

It's a singleton, and I don't see any reason we couldn't use to store state between execute calls.

There's a slight risk of error in that someone could inadvertently overwrite one of the globals that is imported that way; we could also create a new 'globals' object or something that persists between calls and is used similarly but without this risk.

Comment 5

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

(In reply to Jonathan Griffin (:jgriffin) from comment #4)
> testUtils is just used to load several JS files into the test's context:
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-
> server.js#22

I don't think that we should mess with that object then. 

> There's a slight risk of error in that someone could inadvertently overwrite
> one of the globals that is imported that way; we could also create a new
> 'globals' object or something that persists between calls and is used
> similarly but without this risk.

If that would work I'm totally in favor of this. While reading this I also got the impression that something like `global.foobar` should also work. I haven't tested this initially but did now. But it has the same problem as `this`. So the new `globals` object (or how it would be named) you are proposing here, would be different from the usage of `this` and `global`? Also it would only survive the life-time of a test, and will be reset for the next one, right?

Comment 6

[Jonathan Griffin (:jgriffin)]

(In reply to Henrik Skupin (:whimboo) from comment #5)

> If that would work I'm totally in favor of this. While reading this I also
> got the impression that something like `global.foobar` should also work. I
> haven't tested this initially but did now. But it has the same problem as
> `this`. So the new `globals` object (or how it would be named) you are
> proposing here, would be different from the usage of `this` and `global`?
> Also it would only survive the life-time of a test, and will be reset for
> the next one, right?

No, it could be implemented to persist for the lifetime of the browser.  The server doesn't have a concept of a test, only a session, so if you need something that persists exactly for the duration of a test, you'll need to implement a persistent object and then reset it in the runner when a test starts/ends.

Comment 7

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

The maximum lifetime is not that important at the Marionette level and you are right that we can tweak that in our testcase class. Otherwise this proposal sounds great then!

Comment 8

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

Jonathan, can we assume that this bug is properly fixed by the landing of the patch on bug 1149618?

Comment 9

[Jonathan Griffin (:jgriffin)]

Yes, this should work now, within a given session.  There is a caveat: don't try to persist a sandbox between different frames or windows; that will cause the sandbox to be silently reset.  Since it's now possible to name sandboxes, this should be avoidable.