Closed Bug 876399 Opened 11 years ago Closed 11 years ago

Mozmill module loader should prevent leaking of symbols through the global object by using a sandbox

Categories

(Testing Graveyard :: Mozmill, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: tessarakt, Assigned: tessarakt)

References

Details

(Keywords: memory-leak, Whiteboard: [ateamtrack: p=mozmill q=2013q2 m=3][mentor=whimboo][lang=js][mozmill-2.0+][mozmill-1.5.22+])

Attachments

(2 files, 12 obsolete files)

943 bytes, patch
whimboo
: review+
cmtalbert
: review+
Details | Diff | Splinter Review
6.05 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
Bug 874690 exposed two related problems with Mozmill tests:

1. The order in which the files in RELATIVE_ROOT (=shared-modules), and probably the test files in a folder as well, are loaded, is not specified, see https://bugzilla.mozilla.org/show_bug.cgi?id=874690#c21

By itself, this is not a problem, because loaded scripts should only expect in their global object the symbols placed there by the module system, and import everything else using the mechanisms provided.

2. The loading mechanism of Mozmill allows leaking of symbols through the global object of the calling script. It uses the loadSubScript() method of mozIJSSubScriptLoader, see https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/mozIJSSubScriptLoader. As explained in http://stackoverflow.com/questions/15179265/how-do-i-use-the-targetobj-parameter-to-loadsubscript-in-firefox-extensions (2nd answer), "undeclared variables will still be created in the outer scope and the outer scope will be searched for variables that cannot be resolved in the subscript scope."

It should be possible to detect and prevent such accidental leaking of symbols, which may lead to scripts working only by accident when the loading order is "nice", but not working in other cases where the loading order is "not so nice".

This is possible by using a sandbox instead of a plain JavaScript object as the scope object for loading scripts. This patch: https://bugzilla.mozilla.org/attachment.cgi?id=754080&action=diff shows this in the c-c copy of Mozmill. (Bug 876089 until now is only about fixing this in Thunderbird tests. If the problem is fixed for Mozmill in general, that bug will still be about adding explicit imports that are not detected today because the respective symbols leak in).

Please tell me where to find the authoritative Mozmill source. I will then port the patch.
Please don't use the same summary for different bugs. For some clients (like Gmail) it will mix both bugs in the same thread.

Can you also please attach the patch for Mozmill to this bug? The other bug is not appropriate for it. Well, only if we decide that we will not release another 1.5 release of Mozmill. Lets get this fixed in Mozmill 2.0.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Mozmill module loader should prevent leaking of symbols through the global object → Mozmill module loader should prevent leaking of symbols through the global object by using a sandbox
Whiteboard: [mozmill-2.0?]
(In reply to Henrik Skupin (:whimboo) from comment #1)
> Can you also please attach the patch for Mozmill to this bug? The other bug
> is not appropriate for it. Well, only if we decide that we will not release
> another 1.5 release of Mozmill. Lets get this fixed in Mozmill 2.0.

As stated above:

> Please tell me where to find the authoritative Mozmill source. I will then 
> port the patch.
Sorry, I missed that. You can find the source here:
https://github.com/mozilla/mozmill
Whiteboard: [mozmill-2.0?] → [mozmill-2.0?][mozmill-1.5?]
Here is the ported patch - the differences were minimal.

I didn't find an explanation how to build or run automatic tests.
On https://github.com/mozilla/mozmill/tree/master/mozmill, the link "the installation page" points to https://github.com/mozilla/mozmill/blob/master/mozmill/Installation, which results in a 404.

So unfortunately, I have not yet been able to test this.
Attachment #754576 - Flags: review?(hskupin)
Attachment #754576 - Attachment description: Use sandbox instead of plan JS object for loading scripts → Use sandbox instead of plain JS object for loading scripts
Comment on attachment 754576 [details] [diff] [review]
Use sandbox instead of plain JS object for loading scripts

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

I haven't known that we are leaking our scope that drastically. So that might be the reason why we are leaking that much with debug builds. Thanks for the details information and the patch Jens! I really appreciate your investigation and other work here! It's indeed something we have to fix. I even would spin another Mozmill 1.5 release for it.

Before I can give a full review we will have to run all of our existing tests for Mozmill to ensure that everything works as expected and we do not introduce any regressions due to side-effects of this change. There are two types of tests:

1. Our 'unit' tests for Mozmill done via Mutt: Those tests can be found under mutt/mutt/tests and can be executed with 'mutt -b %path_to_firefox_binary% -m mutt/mutt/tests/all-tests.ini'.

2. Our Firefox desktop tests: Those tests can be run via the testrun_* scripts of our mozmill-automation scripts. For Mozmill 2.0 those can be found here: https://github.com/whimboo/mozmill-automation

Both type of tests we should also run in opt vs. debug builds with leak checks enabled. While this patch should cut down a lot of current leaks we might still face some others we can fix in a follow-up bug for Mozmill 2.0 or following.

Due to missing testing I can't give an review. I will do it for the next version of the patch with the mentioned comments fixed.

::: mozmill/mozmill/extension/resource/modules/frame.js
@@ +31,4 @@
>                        .getService(Ci.mozIJSSubScriptLoader);
>  var uuidgen = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
>  
> +Cu.import("resource://gre/modules/Services.jsm");

Please move this up right before the import of httpd.js and separate by a blank line.

@@ +31,5 @@
>                        .getService(Ci.mozIJSSubScriptLoader);
>  var uuidgen = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
>  
> +Cu.import("resource://gre/modules/Services.jsm");
> +var systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal();

It's only used in loadFile() so please move it right next to its usage.

@@ +76,4 @@
>  
>    loadTestResources();
>  
> +  var module = new Components.utils.Sandbox(systemPrincipal); 

nit: trailing white-space.

@@ +89,5 @@
> +  module.findElement = mozelement;
> +  module.log = log;
> +  module.mozmill = mozmill;
> +  module.persisted = persisted;
> +  

nit: trailing white-space.
Attachment #754576 - Flags: review?(hskupin) → feedback+
I cannot test this as long as bug 872237 is not fixed.
Depends on: 872237
Ok, I tested the patch with Mozmill 2.0 now and it works fantastic. We do no longer leak objects defined as globals in the tests. Jens, would you mind to do the updates and also add a test for that to mutt/mutt/tests/js/frame/sandbox/?
Assignee: nobody → blog
Status: NEW → ASSIGNED
Whiteboard: [mozmill-2.0?][mozmill-1.5?] → [mentor=whimboo][lang=js][mozmill-2.0?][mozmill-1.5?]
Attachment #754576 - Attachment is obsolete: true
(In reply to Henrik Skupin (:whimboo) from comment #7)
> Ok, I tested the patch with Mozmill 2.0 now and it works fantastic. We do no
> longer leak objects defined as globals in the tests. Jens, would you mind to
> do the updates and also add a test for that to
> mutt/mutt/tests/js/frame/sandbox/?

OK, I have a basic idea how to test this.

This would involve a test script that resembles, e.g., < http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-widget/test-message-filters.js >. It would set RELATIVE_ROOT to a subdirectory.

The subdir would contain three modules - two which are imported by the main test script using MODULE_REQUIRES, and one that is not.

Using this structure, I will check whether symbols leak into the test scripts, out of them, and between them.

@Henrik: Can you please give me an advice how to set this up? I.e., what do I have to write into the .ini file? Just mention the main test script there? Does its name have to start with "test"? 

Actually, I am a bit confused now. I just saw that the frame.js (function Collector.prototype.initTestModule) of the Mozmill packaged in comm-central looks for RELATIVE_ROOT, MODULE_REQUIRES, and MODULE_NAME. I grep'ed for all these symbols in the Mozmill from Github, and found nothing. So are are module dependencies expressed there? And where does this stuff in comm-centrals copy come from? Or did it once exist in the main Mozmill and has been replaced by some magic I do not understand?

Best regards, Jens
(In reply to Jens Müller (:tessarakt) from comment #9)
> Actually, I am a bit confused now. I just saw that the frame.js (function
> Collector.prototype.initTestModule) of the Mozmill packaged in comm-central
> looks for RELATIVE_ROOT, MODULE_REQUIRES, and MODULE_NAME. I grep'ed for all
> these symbols in the Mozmill from Github, and found nothing. So are are
> module dependencies expressed there? And where does this stuff in
> comm-centrals copy come from? Or did it once exist in the main Mozmill and
> has been replaced by some magic I do not understand?

According to the setup.py over there it is apparently version 1.5.16 from https://github.com/mozautomation/mozmill (which no longer is available).
(In reply to Jens Müller (:tessarakt) from comment #9)
> This would involve a test script that resembles, e.g., <
> http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-widget/
> test-message-filters.js >. It would set RELATIVE_ROOT to a subdirectory.
> 
> The subdir would contain three modules - two which are imported by the main
> test script using MODULE_REQUIRES, and one that is not.

The leak here does not only involve how we load sub modules. There are even two ways. What you describe is the old module system we know usually don't use anymore. State of the art is now require. See our tests in the mozmill-tests repository.

You can even reproduce this problem when you have two simple tests. In the first test file add the following line into the global scope:

> GLOBAL = { 'asdfsafasf': 'sdf', 'sadfsafsasdfsadfsfsadfsdafsadfsd': 1234567};

In the second test file which has to be executed right after check if that GLOBAL variable is present. Without your patch that's the case. With your patch it will be undefined. You can use expect.ok() to test for it's existence.

> @Henrik: Can you please give me an advice how to set this up? I.e., what do
> I have to write into the .ini file? Just mention the main test script there?
> Does its name have to start with "test"? 

We currently have a problem with naming which we should finally solve. So yes, please start the files with the test prefix.

> Actually, I am a bit confused now. I just saw that the frame.js (function
> Collector.prototype.initTestModule) of the Mozmill packaged in comm-central
> looks for RELATIVE_ROOT, MODULE_REQUIRES, and MODULE_NAME. I grep'ed for all
> these symbols in the Mozmill from Github, and found nothing. So are are
> module dependencies expressed there? And where does this stuff in
> comm-centrals copy come from? Or did it once exist in the main Mozmill and
> has been replaced by some magic I do not understand?

I have no idea where this comes from. I thought this code is up2date on Mozmill 1.5.21? As mentioned above we don't use the old module system anymore. But it's still in the code for backward compatibility.

(In reply to Jens Müller (:tessarakt) from comment #10)
> According to the setup.py over there it is apparently version 1.5.16 from
> https://github.com/mozautomation/mozmill (which no longer is available).

Which setup.py are you referring to? All Mozmill code is located now at https://github.com/mozilla/mozmill
I kinda would like to have this fixed in the next Mozmill 2.0rc4. Adding whiteboard tag.
Whiteboard: [mentor=whimboo][lang=js][mozmill-2.0?][mozmill-1.5?] → [ateamtrack: p=mozmill q=2013q2 m=3][mentor=whimboo][lang=js][mozmill-2.0?][mozmill-1.5?]
(In reply to Henrik Skupin (:whimboo) from comment #11)
> (In reply to Jens Müller (:tessarakt) from comment #9)
> > This would involve a test script that resembles, e.g., <
> > http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-widget/
> > test-message-filters.js >. It would set RELATIVE_ROOT to a subdirectory.
> > 
> > The subdir would contain three modules - two which are imported by the main
> > test script using MODULE_REQUIRES, and one that is not.
> 
> The leak here does not only involve how we load sub modules. There are even
> two ways. What you describe is the old module system we know usually don't
> use anymore. State of the art is now require. See our tests in the
> mozmill-tests repository.

Ah, http://hg.mozilla.org/qa/mozmill-tests/ ? I will have a look.

> You can even reproduce this problem when you have two simple tests. 

Yes, that was happened in the Thunderbird tests as well.

> In the
> first test file add the following line into the global scope:
> 
> > GLOBAL = { 'asdfsafasf': 'sdf', 'sadfsafsasdfsadfsfsadfsdafsadfsd': 1234567};
> 
> In the second test file which has to be executed right after check if that
> GLOBAL variable is present. Without your patch that's the case. With your
> patch it will be undefined. You can use expect.ok() to test for it's
> existence.

Test for it's NON-existence, right?

[...]

> > Actually, I am a bit confused now. I just saw that the frame.js (function
> > Collector.prototype.initTestModule) of the Mozmill packaged in comm-central
> > looks for RELATIVE_ROOT, MODULE_REQUIRES, and MODULE_NAME. I grep'ed for all
> > these symbols in the Mozmill from Github, and found nothing. So are are
> > module dependencies expressed there? And where does this stuff in
> > comm-centrals copy come from? Or did it once exist in the main Mozmill and
> > has been replaced by some magic I do not understand?
> 
> I have no idea where this comes from. I thought this code is up2date on
> Mozmill 1.5.21? As mentioned above we don't use the old module system
> anymore. But it's still in the code for backward compatibility.

But where? As said, I grep'ed for those names and could not find them anywhere ...


> (In reply to Jens Müller (:tessarakt) from comment #10)
> > According to the setup.py over there it is apparently version 1.5.16 from
> > https://github.com/mozautomation/mozmill (which no longer is available).
> 
> Which setup.py are you referring to? All Mozmill code is located now at
> https://github.com/mozilla/mozmill

http://hg.mozilla.org/comm-central/file/b63bf044ddcc/mail/test/resources/mozmill/setup.py
(In reply to Jens Müller (:tessarakt) from comment #13)
> > The leak here does not only involve how we load sub modules. There are even
> > two ways. What you describe is the old module system we know usually don't
> > use anymore. State of the art is now require. See our tests in the
> > mozmill-tests repository.
> 
> Ah, http://hg.mozilla.org/qa/mozmill-tests/ ? I will have a look.

Yes, sorry that I missed to hand out the URL.

> > In the second test file which has to be executed right after check if that
> > GLOBAL variable is present. Without your patch that's the case. With your
> > patch it will be undefined. You can use expect.ok() to test for it's
> > existence.
> 
> Test for it's NON-existence, right?

Exactly.

> > I have no idea where this comes from. I thought this code is up2date on
> > Mozmill 1.5.21? As mentioned above we don't use the old module system
> > anymore. But it's still in the code for backward compatibility.
> 
> But where? As said, I grep'ed for those names and could not find them
> anywhere ...

I would suggest you ask Mark or mconley where you base on for thunderbird tests. I have no idea about that. Sorry.

> > Which setup.py are you referring to? All Mozmill code is located now at
> > https://github.com/mozilla/mozmill
> 
> http://hg.mozilla.org/comm-central/file/b63bf044ddcc/mail/test/resources/
> mozmill/setup.py

That's all thunderbird specific. I wouldn't cover any of this code in this bug.
(In reply to Henrik Skupin (:whimboo) from comment #14)
> > > In the second test file which has to be executed right after check if that
> > > GLOBAL variable is present. Without your patch that's the case. With your
> > > patch it will be undefined. You can use expect.ok() to test for it's
> > > existence.
> > 
> > Test for it's NON-existence, right?
> 
> Exactly.

OK. How?

expect.ok(globalB === undefined);

gives me:

TEST-START | test-sandbox-a.js | testNoLeakFromB
ERROR | Test Failure | {
  "exception": {
    "stack": [
      "testNoLeakFromB@resource://mozmill/modules/frame.js -> file:///home/jens/devel/mozmill/mutt/mutt/tests/js/frame/sandbox/test-sandbox-a.js:16", 
      "Runner.prototype.wrapper@resource://mozmill/modules/frame.js:636", 
      "Runner.prototype.runTestModule@resource://mozmill/modules/frame.js:706", 
      "Runner.prototype.runTestFile@resource://mozmill/modules/frame.js:604", 
      "runTestFile@resource://mozmill/modules/frame.js:751", 
      "Bridge.prototype._execFunction@resource://jsbridge/modules/Bridge.jsm:140", 
      "Bridge.prototype.execFunction@resource://jsbridge/modules/Bridge.jsm:147", 
      "@resource://jsbridge/modules/Server.jsm:33", 
      ""
    ], 
    "message": "globalB is not defined", 
    "fileName": "resource://mozmill/modules/frame.js -> file:///home/jens/devel/mozmill/mutt/mutt/tests/js/frame/sandbox/test-sandbox-a.js", 
    "name": "ReferenceError", 
    "lineNumber": 16
  }
}
TEST-UNEXPECTED-FAIL | test-sandbox-a.js | testNoLeakFromB

Or how would you checked for undefinedness?
expect.ok(typeof globalB == 'undefined') i think
Oh, yes. In case if the variable does not exist expect.ok will not work. So you should use expect.throws() and check for the ReferenceError which gets thrown when accessing the variable. typeof will also not work because it checks the value but the variable doesn't exist.
Attached patch Tests for leaking symbols (obsolete) — Splinter Review
This does not yet work as expected ...

Those tests also pass without the first patch applied.
Attachment #756414 - Flags: feedback?(hskupin)
Comment on attachment 756414 [details] [diff] [review]
Tests for leaking symbols

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

Good start on the tests. But please check my inline comments in how to be able to check the right stuff.

::: mutt/mutt/tests/js/frame/sandbox/test-sandbox-a.js
@@ +7,5 @@
> + * Make sure no symbols leak through the global scope from other tests or
> + * required modules
> + **/
> +
> +var subModule = require("sub/sub");

There is no need to require submodules here. As I have stated earlier on this bug it's not related to that feature. So you can remove sub.js.

@@ +9,5 @@
> + **/
> +
> +var subModule = require("sub/sub");
> +
> +var globalA = "a";

Remove the 'var' keyword from that line.

@@ +14,5 @@
> +
> +var testNoLeakFromB = function(){
> +  expect.throws(function(){
> +    var local = globalB;
> +  }, "ReferenceError", "Global variable from a different script should not be visible here.");

You can't do any of those checks in the first test. Move all of them in the second one.

::: mutt/mutt/tests/js/frame/sandbox/test-sandbox-b.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var globalB = "b";

There is no need to define another global variable here. We only want to check the one from the previous test.
Attachment #756414 - Flags: feedback?(hskupin) → feedback+
Attached patch Test for leaking symbols (obsolete) — Splinter Review
Attachment #756414 - Attachment is obsolete: true
Attachment #756423 - Flags: review?(hskupin)
Attachment #755739 - Flags: review?(hskupin)
Comment on attachment 755739 [details] [diff] [review]
Use sandbox instead of plain JS object for loading scripts

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

Except the nit it looks fine. For the next version please combine both patches into a single one.

::: mozmill/mozmill/extension/resource/modules/frame.js
@@ +74,5 @@
>  
>    loadTestResources();
>  
> +  var systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
> +  var module = new Components.utils.Sandbox(systemPrincipal); 

nit: there is still a trailing white-space at the end.
Attachment #755739 - Flags: review?(hskupin) → review+
Comment on attachment 756423 [details] [diff] [review]
Test for leaking symbols

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

As said in my last review, please combine both. Once done we can get it landed. After that you should work on the backport for Mozmill 1.5.

::: mutt/mutt/tests/js/frame/sandbox/test-sandbox-b.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var testNoLeakFromA = function(){
> +  expect.throws(function(){

nit: please fix the white-spaces. That means a blank between function and the round opening bracket, and one more after the closing round bracket.

@@ +4,5 @@
> +
> +var testNoLeakFromA = function(){
> +  expect.throws(function(){
> +    var local = globalA;
> +  }, "ReferenceError", "Global variable from a different script should not be visible here.");

Please rename to: '... is not accessible.' That's better understandable as two negative forms.
Attachment #756423 - Flags: review?(hskupin) → review+
Attachment #755739 - Attachment is obsolete: true
Attachment #756423 - Attachment is obsolete: true
(In reply to Henrik Skupin (:whimboo) from comment #22)
> After that you should work on the backport for Mozmill 1.5.

That is the Git branch "mozmill-1.5"?
Comment on attachment 756561 [details] [diff] [review]
Use a sandbox for loading scripts to prevent leaking of symbols; test

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

The patch looks fine but running the mutt tests reveals that at least one other test is failing now given that mozelement cannot be found anymore. That are good news, that we no longer leak. Please see tests/js/elementslib/mozelement_windows.js. Please update that line and check if even other tests have been regressed. I think it might be also good to add one more check to your second test which exactly tests that you can no longer use objects defined in frame.js like mozelement.
(In reply to Jens Müller (:tessarakt) from comment #24)
> That is the Git branch "mozmill-1.5"?

Yes, it is. And do not wonder, we don't have any tests there.
(In reply to Henrik Skupin (:whimboo) from comment #25)
> The patch looks fine but running the mutt tests reveals that at least one
> other test is failing now given that mozelement cannot be found anymore.

Yes, even 3 other tests. The one you mention, mutt/mutt/tests/js/frame/user_shotdown/multiple/test5.js (fix: put "var controller;" before the first function), and mutt/mutt/tests/js/utils/savescreenshot.js. 

The last one is trickier: It wants to use btoa. According to https://developer.mozilla.org/en-US/docs/Web/API/window.btoa, it is a property of the window object, and "btoa() is also available to XPCOM components implemented in JavaScript, even though window is not the global object in components."

Not sure yet how to fix this.
(In reply to Jens Müller (:tessarakt) from comment #27)
> Not sure yet how to fix this.

The easiest way would of course be to insert it into the global scope in loadFile() in frame.js.

Maybe we should do this with all the symbols mentioned in http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/mozJSComponentLoader.cpp#324 and the following lines?
I added dump, btoa, and atob to the scope object. Those are the functions I could find proper documentation for.
Attachment #756561 - Attachment is obsolete: true
Attachment #757042 - Flags: review?(hskupin)
Now added the requested test.
Attachment #757042 - Attachment is obsolete: true
Attachment #757042 - Flags: review?(hskupin)
Attachment #757046 - Flags: review?(hskupin)
Whiteboard: [ateamtrack: p=mozmill q=2013q2 m=3][mentor=whimboo][lang=js][mozmill-2.0?][mozmill-1.5?] → [ateamtrack: p=mozmill q=2013q2 m=3][mentor=whimboo][lang=js][mozmill-2.0+][mozmill-1.5?]
Comment on attachment 757046 [details] [diff] [review]
Use a sandbox for loading scripts to prevent leaking of symbols; test; followup fixes

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

::: mozmill/mozmill/extension/resource/modules/frame.js
@@ +88,5 @@
> +  module.findElement = mozelement;
> +  module.log = log;
> +  module.mozmill = mozmill;
> +  module.persisted = persisted;
> +  module.dump = dump;

When we wouldn't add this line here, will the tests not being able to call dump()? Or what happens?

::: mutt/mutt/tests/js/frame/user_shutdown/multiple/test5.js
@@ +7,2 @@
>  var setupModule = function () {
>    controller = mozmill.getBrowserController();

Please add the aModule parameter to the setupModule function and attach the controller to it. That's how it should work.
Attachment #757046 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #31)
> Comment on attachment 757046 [details] [diff] [review]
> Use a sandbox for loading scripts to prevent leaking of symbols; test;
> followup fixes
> 
> Review of attachment 757046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozmill/mozmill/extension/resource/modules/frame.js
> @@ +88,5 @@
> > +  module.findElement = mozelement;
> > +  module.log = log;
> > +  module.mozmill = mozmill;
> > +  module.persisted = persisted;
> > +  module.dump = dump;
> 
> When we wouldn't add this line here, will the tests not being able to call
> dump()? Or what happens?

Strangely enough, it _is_ available. I don't know whether this is by accident or by design. Documentation is here: https://developer.mozilla.org/en-US/docs/Web/API/window.dump

"dump is also available to XPCOM components implemented in JavaScript, even though window is not the global object in components." - but a script loaded into a sandbox is not a XPCOM component ...

I also noted another thing: atob and btoa should also be in the list of global symbols for scripts loaded using require (some lines below in frame.js), right?

> ::: mutt/mutt/tests/js/frame/user_shutdown/multiple/test5.js
> @@ +7,2 @@
> >  var setupModule = function () {
> >    controller = mozmill.getBrowserController();
> 
> Please add the aModule parameter to the setupModule function and attach the
> controller to it. That's how it should work.

Will do. And the same for teardownModule(), right?
(In reply to Jens Müller (:tessarakt) from comment #32)
> (In reply to Henrik Skupin (:whimboo) from comment #31)
> > Comment on attachment 757046 [details] [diff] [review]
> > > +  module.dump = dump;
> > 
> > When we wouldn't add this line here, will the tests not being able to call
> > dump()? Or what happens?
> 
> Strangely enough, it _is_ available. I don't know whether this is by
> accident or by design. Documentation is here:
> https://developer.mozilla.org/en-US/docs/Web/API/window.dump
> 
> "dump is also available to XPCOM components implemented in JavaScript, even
> though window is not the global object in components." - but a script loaded
> into a sandbox is not a XPCOM component ...

Found the documentation:

https://developer.mozilla.org/en/docs/Components.utils.Sandbox#Methods_available_on_the_Sandbox_object

So I can remove it, yes.
Attachment #757046 - Attachment is obsolete: true
Attachment #758213 - Flags: review?(hskupin)
(In reply to Jens Müller (:tessarakt) from comment #32)
> > When we wouldn't add this line here, will the tests not being able to call
> > dump()? Or what happens?
> 
> Strangely enough, it _is_ available. I don't know whether this is by
> accident or by design. Documentation is here:
> https://developer.mozilla.org/en-US/docs/Web/API/window.dump
> 
> "dump is also available to XPCOM components implemented in JavaScript, even
> though window is not the global object in components." - but a script loaded
> into a sandbox is not a XPCOM component ...

Well, it's not about the script which gets loaded but more about the base of the sandbox which is getting used. Given that we make use of getSystemPrincipal() we get an instance of an XPCOM object. I think that this would explain why those default methods are available to us.

> I also noted another thing: atob and btoa should also be in the list of
> global symbols for scripts loaded using require (some lines below in
> frame.js), right?

See above. I believe the same applies to those methods, and that we do not have to attach them to our created sandbox'ed module.

> > ::: mutt/mutt/tests/js/frame/user_shutdown/multiple/test5.js
> > @@ +7,2 @@
> > >  var setupModule = function () {
> > >    controller = mozmill.getBrowserController();
> > 
> > Please add the aModule parameter to the setupModule function and attach the
> > controller to it. That's how it should work.
> 
> Will do. And the same for teardownModule(), right?

If we need a teardownModule then it should follow that style. That's correct.
Attachment #758213 - Attachment is obsolete: true
Attachment #758213 - Flags: review?(hskupin)
Attachment #758252 - Flags: review?(hskupin)
Inaccurate and superfluous comment removed.
Attachment #758252 - Attachment is obsolete: true
Attachment #758252 - Flags: review?(hskupin)
Attachment #758261 - Flags: review?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #22)
> As said in my last review, please combine both. Once done we can get it
> landed. After that you should work on the backport for Mozmill 1.5.

(In reply to Henrik Skupin (:whimboo) from comment #26)
> (In reply to Jens Müller (:tessarakt) from comment #24)
> > That is the Git branch "mozmill-1.5"?
> 
> Yes, it is. And do not wonder, we don't have any tests there.

So I did not any tests myself and did not have to fix any existing tests ...
Comment on attachment 758261 [details] [diff] [review]
Use a sandbox for loading scripts to prevent leaking of symbols; test; followup fixes

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

That looks great now. Only two nits to fix.

Jens, for the updated patch please ask review from Clint. I also want to get his feedback before the patch gets landed. Thanks!

::: mutt/mutt/tests/js/frame/sandbox/test-sandbox-b.js
@@ +8,5 @@
> +  }, "ReferenceError", "Global variable from a different script is not accessible.");
> +};
> +
> +var testNoLeakFromFrameJS = function() {
> +  expect.throws( function() {

nit: three white-spaces are wrong here. After an anonymous function we always have a blank before the opening bracket, and when calling a method in this case throws there is no white space after the opening bracket. The first test function in this file does this correctly.

::: mutt/mutt/tests/js/utils/savescreenshot.js
@@ +49,4 @@
>      var binaryStream = Cc["@mozilla.org/binaryinputstream;1"]
>                         .createInstance(Ci.nsIBinaryInputStream);
>      binaryStream.setInputStream(aInputStream);
> +    var controller = mozmill.getBrowserController();

We should have a setupModule() method for that.
Attachment #758261 - Flags: review?(hskupin) → review-
Comment on attachment 758284 [details] [diff] [review]
Use a sandbox for loading scripts to prevent leaking of symbols (backport to Mozmill 1.5)

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

Looks good to me. I also want to see a review from Clint here. Before we get this landed I would like to land the 2.0 patch and get 1 or 2 days to verify that everything works as expected.
Attachment #758284 - Flags: review?(hskupin)
Attachment #758284 - Flags: review?(ctalbert)
Attachment #758284 - Flags: review+
Attachment #758261 - Attachment is obsolete: true
Attachment #758396 - Flags: review?(hskupin)
Comment on attachment 758396 [details] [diff] [review]
Use a sandbox for loading scripts to prevent leaking of symbols; test; followup fixes

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

::: mutt/mutt/tests/js/elementslib/mozelement_window.js
@@ +11,2 @@
>  var setupModule = function () {
>    controller = mozmill.getBrowserController();

Oh, I missed that. Shouldn't we also use aModule in all of those cases? Or doesn't it harm us when we don't make use of it.
Attachment #758284 - Flags: review?(ctalbert) → review+
(In reply to Henrik Skupin (:whimboo) from comment #43)
> Comment on attachment 758396 [details] [diff] [review]
> Use a sandbox for loading scripts to prevent leaking of symbols; test;
> followup fixes
> 
> Review of attachment 758396 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mutt/mutt/tests/js/elementslib/mozelement_window.js
> @@ +11,2 @@
> >  var setupModule = function () {
> >    controller = mozmill.getBrowserController();
> 
> Oh, I missed that. Shouldn't we also use aModule in all of those cases? Or
> doesn't it harm us when we don't make use of it.

Well, it was already there ...

I grep'ed for setupModule in the tests. In the first two tests that have a setupModule() function, exactly the same controller = mozmill.getBrowserController(); is used.

I guess controller will end up as a property of the global object, which is now the sandbox. So less harm done than before.

What exactly do you mean by "when we don't make use of it"? When we don't make which use of what?

Anyway, I don't think this needs to be part of this bug. IMO, we can do this (for all tests ...) in a separate bug.
Well, for new tests and additions should use the right way. There is already a bug open which will change all existing tests to that aModule format. But given that you have added the win variable in setupModule it will require that change too.
Attachment #758396 - Attachment is obsolete: true
Attachment #758396 - Flags: review?(hskupin)
Attachment #759976 - Flags: review?(hskupin)
And aModule.win instead of just win ...
Attachment #759976 - Attachment is obsolete: true
Attachment #759976 - Flags: review?(hskupin)
Attachment #759994 - Flags: review?(hskupin)
Comment on attachment 759994 [details] [diff] [review]
Use a sandbox for loading scripts to prevent leaking of symbols; test; followup fixes

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

Looks fine to me and makes things working as expected. I will get it landed.
Attachment #759994 - Flags: review?(hskupin) → review+
Landed on master:
https://github.com/mozilla/mozmill/commit/750668350cdb8eecd02b170d6003589796b901e5

Landed on hotfix-1.5:
https://github.com/mozilla/mozmill/commit/447ffd0721a7fc1d751d7c4132d7e032a46bacc6

Thanks for the patches. There are some other ones we most likely want to get integrated for a 1.5.22 release. Lets see how fast those can be done. This bug is fixed!
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [ateamtrack: p=mozmill q=2013q2 m=3][mentor=whimboo][lang=js][mozmill-2.0+][mozmill-1.5?] → [ateamtrack: p=mozmill q=2013q2 m=3][mentor=whimboo][lang=js][mozmill-2.0+][mozmill-1.5.22]
Whiteboard: [ateamtrack: p=mozmill q=2013q2 m=3][mentor=whimboo][lang=js][mozmill-2.0+][mozmill-1.5.22] → [ateamtrack: p=mozmill q=2013q2 m=3][mentor=whimboo][lang=js][mozmill-2.0+][mozmill-1.5.22+]
Bug 876089 is facing issues with plugins and this patch applied. So we will wait for a solution before releasing the next version of Mozmill 1.5.

As of now I'm not sure if it is a defect in general or if something is broken for the Thunderbird tests.
I think I now gained some insight into this problem. Both the plugin code and the test access the document object through an XrayWrapper. The plugin code thus sets the mozNoPluginCrashedNotification on the wrapper, not the underlying object.

This is "no problem" when the test does not run in a sandbox, because both pieces of code share the same XrayWrapper. But sandboxed code always gets its own XrayWrapper, so then it suddenly becomes a problem.

IMO, looking for some internal flags of other code is wrong anyway. Rather, it should look for real UI changes ... The error message for the code, btw, seems to indicate exactly that - than an expected UI change did not occur.

(In reply to Henrik Skupin (:whimboo) [away 07/29 - 08/11] from comment #50)
> As of now I'm not sure if it is a defect in general or if something is
> broken for the Thunderbird tests.

There will be a problem for tests which rely on accessing expando properties set on wrappers of DOM objects. The reason is that the wrappers are no longer shared when the test code runs in a sandbox. We will see if there are more such cases ...
(In reply to Henrik Skupin (:whimboo) [away 07/29 - 08/11] from comment #50)
> Bug 876089 is facing issues with plugins and this patch applied. So we will
> wait for a solution before releasing the next version of Mozmill 1.5.

The fix for the one problem in bug 876089 just got review.
Thank you for your feedback. It's good to see that it was not a problem in Mozmill itself.
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.