1370871 - Remove "global" global from evaluate script context

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement)

Description

[Andreas Tolfsen ❲:ato❳]

For scripts that are executed in sandboxes, we expose a global named "global".  I’m not quite sure why we do that, but I suspect it may have had something to do with the way evaluating scripts worked in Marionette prior to the rewrite of testing/marionette/evaluate.js.

In any case, I see no harm in removing this now.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1370871 - Remove "global" global from evaluate script context;

Marionette exposes a global called "global" to scripts that are
evaluated in sandboxes.  This is no longer needed after the rewrite of
the testing/marionette/evaluate.js module.

Review commit: https://reviewboard.mozilla.org/r/146682/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146682/

Comment 2

[David Burns :automatedtester]

Comment on attachment 8875278 [details]
Bug 1370871 - Remove "global" global from evaluate script context;

https://reviewboard.mozilla.org/r/146682/#review150736

Comment 3

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1370871 - Remove use of global from Firefox refresh test;

The previous commit removes the "global" global exposed n Marionette
script evaluation environments.  This patch makes use of named sandboxes
that are maintained for the lifetime of the test.

Review commit: https://reviewboard.mozilla.org/r/147134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147134/

Comment 4

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8875278 [details]
Bug 1370871 - Remove "global" global from evaluate script context;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146682/diff/1-2/

Comment 5

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8875700 [details]
Bug 1370871 - Remove use of global from Firefox refresh test;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147134/diff/1-2/

Comment 6

[:Gijs (he/him)]

Comment on attachment 8875700 [details]
Bug 1370871 - Remove use of global from Firefox refresh test;

https://reviewboard.mozilla.org/r/147134/#review151290

r=me in the sense that this is a refactor and, well, meh. But we should declare variables properly and avoid overwriting existing things. :-)

::: browser/components/migration/tests/marionette/test_refresh_firefox.py:315
(Diff revision 2)
> -          global.LoginInfo = Components.Constructor("@mozilla.org/login-manager/loginInfo;1", "nsILoginInfo", "init");
> -          global.profSvc = Cc["@mozilla.org/toolkit/profile-service;1"].getService(Ci.nsIToolkitProfileService);
> -          global.Preferences = Cu.import("resource://gre/modules/Preferences.jsm", {}).Preferences;
> -          global.FormHistory = Cu.import("resource://gre/modules/FormHistory.jsm", {}).FormHistory;
> +          LoginInfo = Components.Constructor("@mozilla.org/login-manager/loginInfo;1", "nsILoginInfo", "init");
> +          profSvc = Cc["@mozilla.org/toolkit/profile-service;1"].getService(Ci.nsIToolkitProfileService);
> +          Preferences = Cu.import("resource://gre/modules/Preferences.jsm", {}).Preferences;
> +          FormHistory = Cu.import("resource://gre/modules/FormHistory.jsm", {}).FormHistory;

I'm not excited about not declaring these variables, and then assigning them onto... something... (maybe window?) where it might conflict.

In particular, I'd expect browser.xul to have a Preferences ref already.

I think this should use `let`, or make explicit where it's assigning, and really ideally would be scoped to our own evaluation context (where, as you noted on IRC, it might be easier to just use `window.global = {}`).

Really, as a consumer, what I want is for the global evaluation object to be a transparent proxy on `window`, so that I both don't interfere with `window` but can use global variables without jumping through hoops. But I guess that's not what evaluate.js does right now?

Comment 7

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8875278 [details]
Bug 1370871 - Remove "global" global from evaluate script context;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146682/diff/2-3/

Comment 8

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8875700 [details]
Bug 1370871 - Remove use of global from Firefox refresh test;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147134/diff/2-3/

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8875700 [details]
Bug 1370871 - Remove use of global from Firefox refresh test;

https://reviewboard.mozilla.org/r/147134/#review151290

> I'm not excited about not declaring these variables, and then assigning them onto... something... (maybe window?) where it might conflict.
> 
> In particular, I'd expect browser.xul to have a Preferences ref already.
> 
> I think this should use `let`, or make explicit where it's assigning, and really ideally would be scoped to our own evaluation context (where, as you noted on IRC, it might be easier to just use `window.global = {}`).
> 
> Really, as a consumer, what I want is for the global evaluation object to be a transparent proxy on `window`, so that I both don't interfere with `window` but can use global variables without jumping through hoops. But I guess that's not what evaluate.js does right now?

Thanks for your quick review, Gijs!  Sadly I’m not equally good at following up, but I blame that mainly on a startup crash bug on Linux (now fixed).

All good points, and I’ve changed the patch to define a window.global = {} in the "firefox-refresh" sandbox now used throughout the test.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9a85535d607
Remove "global" global from evaluate script context; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/ba02f6a155b8
Remove use of global from Firefox refresh test; r=Gijs

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c9a85535d607
https://hg.mozilla.org/mozilla-central/rev/ba02f6a155b8