1149618 - Remove SpecialPowers from Marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Jonathan Griffin (:jgriffin)]

We don't really need or want SpecialPowers integrated into Marionette.  Instead, individual harnesses should be able to load it on-demand.

This bug will track the work needed to rip SpecialPowers out of Marionette (which is easy) and the work needed to adapt current users to either use chrome scripts or to load SpecialPowers dynamically.

Comment 1

[Jonathan Griffin (:jgriffin)]

try run for WIP:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=68a7a362c7a5

crashtests are the only thing left to fix for emulator tests

Comment 2

[Jonathan Griffin (:jgriffin)]

emulator tests are green now:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e49a0df2016

b2gdesktop, not so much: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35351ca17444

Fixing this will require coordinated landings in several repos.  yippee.

Comment 3

[Jonathan Griffin (:jgriffin)]

firefox-ui-tests don't appear to use SpecialPowers, but cc'ing chmanchester just in case.

Comment 4

[Jonathan Griffin (:jgriffin)]

A complication:  some tests need to be able to execute script in the chrome space of a content process.  Currently, all such cases rely on SpecialPowers, e.g., 

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/tests/unit/test_switch_remote_frame.py#51

In order to provide a way for this to be done without SpecialPowers, we'll need to introduce a new mechanism to allow content scripts to access their chrome space.  The question is, how?

We could:
* introduce a new context, something like 'content-system'
* add a new parameter to execute_script, like (system_principal=True/False)
* do something else hackier, like parse the script for some special string and set up the sandbox accordingly

The actual implementation isn't hard, it's just a matter of deciding how to expose this to tests.

David, Andreas, any opinions?

Comment 5

[Jonathan Griffin (:jgriffin)]

Current WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bba9f023dc2a

Comment 6

[Jonathan Griffin (:jgriffin)]

Current WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdf99f89ad0e&exclusion_profile=false

Everything is green except Gip, which is going to be lots of work.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

/me cheers!

Comment 8

[David Burns :automatedtester]

(In reply to Jonathan Griffin (:jgriffin) (not reading bugmail until April 20) from comment #4)
> 
> We could:
> * introduce a new context, something like 'content-system'

Maybe, but probably not

> * add a new parameter to execute_script, like (system_principal=True/False)

maybe. My initial thought would be allow you to specify the sandbox that you want to execute in and then work from that. The default would be content but would allow people to specify the sandbox name that they want. This might solve bug 1122583 at same time

> * do something else hackier, like parse the script for some special string
> and set up the sandbox accordingly

No, just no

Comment 10

[Jonathan Griffin (:jgriffin)]

Significant Gip progress:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb4e71e8d932

Landing this is going to be complicated, my plan is:

1. Land a patch that introduces new sandbox semantics from comment #8 and release a new Marionette client.
2. Update Gip to use the new sandbox semantics in lieu of SpecialPowers, and land.
3. Land another gecko patch that removes SpecialPowers from Marionette and in-tree users.

Comment 11

[Jonathan Griffin (:jgriffin)]

Attachment: MozReview Request: bz://1149618/jgriffin

/r/7699 - Bug 1149618 - Add a sandbox parameter to execute, r=dburns

Pull down this commit:

hg pull -r 5e633bec2a3ad9a15151e4923aea8aaa386d1012 https://reviewboard-hg.mozilla.org/gecko/

Comment 12

[Jonathan Griffin (:jgriffin)]

try:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e85f8d140fa8

a few notes:

This is backwards compatible with older clients that don't know about the new 'sandbox' parameter.  The 'system' sandbox is special; it uses the system principal and so has elevated privileges, even on content context.

If you use attempt to reuse a sandbox in a different environment (i.e., after a switch_to_frame) the sandbox will be silently reset.  This is the same as earlier behavior, but I wonder if we should throw an error in this case?

It's sort of tempting to try and fold the new_sandbox=True/False behavior into the new sandbox parameter as well; i.e., if you don't specify a sandbox, it will automatically create and use a new one, and destroy the former (unless it was a named box created specifically by the user).  But, this would break backwards compatibility, and I didn't want to tackle it in the context of this bug...maybe we can do something like that as a follow-up if you think it's a good idea.

Comment 13

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8598133 [details]
MozReview Request: bz://1149618/jgriffin

/r/7699 - Bug 1149618 - Implement push_permission in Marionette, r=dburns

Pull down this commit:

hg pull -r eba8282bd88b0f3b2b53d9cfc322f87cc81e16c2 https://reviewboard-hg.mozilla.org/gecko/

Comment 14

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8598133 [details]
MozReview Request: bz://1149618/jgriffin

/r/7699 - Bug 1149618 - Add a sandbox parameter to execute, r=dburns
/r/7899 - Bug 1149618 - Implement push_permission in Marionette, r=dburns

Pull down these commits:

hg pull -r 6140d3b5fae3b6fa1c064ed4c433dabdb943921d https://reviewboard-hg.mozilla.org/gecko/

Comment 15

[Jonathan Griffin (:jgriffin)]

https://reviewboard.mozilla.org/r/7697/#review6655

::: testing/marionette/client/marionette/runner/base.py:496
(Diff revision 3)
> +    driverclass = Marionette

I'm doing this so I can make a subclass of Marionette for use in gaiatest, which defaults to sandbox='system'.

::: testing/marionette/driver/marionette_driver/marionette.py:795
(Diff revision 3)
> +    def push_permission(self, perm_type, allow):

push_permission replaces some functionality currently found in SpecialPowers.  It doesn't absolutely need to be added to marionette.py; we could add it as a file in the client and import where needed.  But it seems consistent with the other geckoisms in this file.

Comment 16

[David Burns :automatedtester]

Comment on attachment 8598133 [details]
MozReview Request: bz://1149618/jgriffin

https://reviewboard.mozilla.org/r/7697/#review6791

Ship It!

::: testing/marionette/driver.js:1198
(Diff revision 3)
> -      this.sandbox = this.createExecuteSandbox(win, marionette, specialPowers);
> +      this.createExecuteSandbox(win, marionette, 

nit: white space after the `,`

::: testing/marionette/driver/marionette_driver/marionette.py:834
(Diff revision 3)
> +                let principal = secMan.getAppCodebasePrincipal(Services.io.newURI(perm.url, null, null), 

nit: white space after the `,`

::: testing/marionette/driver/marionette_driver/marionette.py:799
(Diff revision 3)
> +                if (allow)

please add `{}` to conditionals

Comment 17

[Jonathan Griffin (:jgriffin)]

Here's a green Gip try run: the last try run missed b2gdesktop builds due to some TC churn.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=300a308db174

Comment 18

[GitHub Autolander Bot]

Attachment: [gaia] jonallengriffin:removeSpecialPowers > mozilla-b2g:master

Comment 19

[Jonathan Griffin (:jgriffin)]

One more try run just to be safe:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=4049e7b99182&exclusion_profile=false

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73ec15af7075
https://hg.mozilla.org/integration/mozilla-inbound/rev/82a9bf38481b

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/73ec15af7075
https://hg.mozilla.org/mozilla-central/rev/82a9bf38481b

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

\o/

Great progress here Jonathan, thanks!

Comment 23

[Jonathan Griffin (:jgriffin)]

The last step is to strip all the remaining bits of SpecialPowers out of Marionette.  Here's the latest try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=09558fea8afa

The only unhappy tests are the file upload ones, which I expected, since ato left a note that element.mozSetFileArray() isn't e10s-compatible.

There are some hints in bug 1149998 about how to address this.

Comment 24

[David Burns :automatedtester]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0486b30ae807

Comment 25

[Jonathan Griffin (:jgriffin)]

Mn unit tests green, just a couple of WebAPI oranges to fix:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e91707a0ecf4&exclusion_profile=false

Comment 26

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Hi Jonathan,

I've refactor the caret tests under layout/base/tests/marionette/ in bug 1110039 part 5. Not sure how soon you're going to land patch to remove SpecialPower from marionette completely. So providing bug 1110039 is landed before yours, I could provide a patch to remove SpecialPower for those refactored caret tests.

Comment 27

[Jonathan Griffin (:jgriffin)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #26)
> Hi Jonathan,
> 
> I've refactor the caret tests under layout/base/tests/marionette/ in bug
> 1110039 part 5. Not sure how soon you're going to land patch to remove
> SpecialPower from marionette completely. So providing bug 1110039 is landed
> before yours, I could provide a patch to remove SpecialPower for those
> refactored caret tests.

Your tests are in the tree, so I've been adapting them myself.  They'll be part of the patch which lands here.

Comment 28

[Jonathan Griffin (:jgriffin)]

Attachment: Remove SpecialPowers from Marionette,

green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e08081475ab1

A couple of notes:  some suites still need SpecialPowers, so SP is enabled on-the-fly (from the Python side) for WebAPI, mochitests, and reftests.  However, SP is completely removed from the server/driver/listener; it requires deliberate activation, and is not required (or allowed) for standard Marionette tests.

As a separate follow-up to this, we could consider having the runner install the SP extension for B2G as it currently does for desktop, so we wouldn't have to initialize it in the WebAPI/mochitest/reftest runners explicitly.

Comment 29

[Jonathan Griffin (:jgriffin)]

I've reviewed firefox-ui-tests and firefox-media-tests, and neither looks to be impacted by this change.

Comment 30

[David Burns :automatedtester]

Comment on attachment 8604757 [details] [diff] [review]
Remove SpecialPowers from Marionette,

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

Looks great! Double check the commit message before shipping.

::: build/mobile/b2gautomation.py
@@ +289,5 @@
> +                testUtils);
> +              testUtils.specialPowersObserver = new testUtils.SpecialPowersObserver();
> +              testUtils.specialPowersObserver.init();
> +              testUtils.specialPowersObserver._loadFrameScript();
> +            }            

nit: whitespace

::: testing/mochitest/runtestsb2g.py
@@ +232,5 @@
> +                    testUtils);
> +                  testUtils.specialPowersObserver = new testUtils.SpecialPowersObserver();
> +                  testUtils.specialPowersObserver.init();
> +                  testUtils.specialPowersObserver._loadFrameScript();
> +                }            

nit: whitespace

Comment 31

[David Burns :automatedtester]

Comment on attachment 8604757 [details] [diff] [review]
Remove SpecialPowers from Marionette,

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

Looks great! Double check the commit message before shipping.

::: build/mobile/b2gautomation.py
@@ +289,5 @@
> +                testUtils);
> +              testUtils.specialPowersObserver = new testUtils.SpecialPowersObserver();
> +              testUtils.specialPowersObserver.init();
> +              testUtils.specialPowersObserver._loadFrameScript();
> +            }            

nit: whitespace

::: testing/mochitest/runtestsb2g.py
@@ +232,5 @@
> +                    testUtils);
> +                  testUtils.specialPowersObserver = new testUtils.SpecialPowersObserver();
> +                  testUtils.specialPowersObserver.init();
> +                  testUtils.specialPowersObserver._loadFrameScript();
> +                }            

nit: whitespace

Comment 32

[Jonathan Griffin (:jgriffin)]

Nits addressed; new try run after rebasing:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=400f455d0122

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc15c275b1e3

Comment 34

[Jonathan Griffin (:jgriffin)]

Backed out for suspected debug emulator bustage in https://hg.mozilla.org/integration/mozilla-inbound/rev/1e983ca9801f

Comment 35

[Jonathan Griffin (:jgriffin)]

18:45:44 INFO - Traceback (most recent call last):
18:45:44 INFO - File "runtestsb2g.py", line 237, in run_tests
18:45:44 INFO - """)
18:45:44 INFO - File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette_driver/marionette.py", line 1497, in execute_script
18:45:44 INFO - filename=os.path.basename(frame[0]))
18:45:44 INFO - File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette_driver/decorators.py", line 36, in _
18:45:44 INFO - return func(*args, **kwargs)
18:45:44 INFO - File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette_driver/marionette.py", line 711, in _send_message
18:45:44 INFO - self._handle_error(response)
18:45:44 INFO - File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette_driver/marionette.py", line 747, in _handle_error
18:45:44 INFO - raise errors.lookup(status)(message, stacktrace=stacktrace)
18:45:44 ERROR - JavascriptException: JavascriptException: TypeError: redeclaration of var Cc

Comment 36

[Jonathan Griffin (:jgriffin)]

trying a fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e93d0b95af2b

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d40c144ae71a

Comment 38

[Julien Wajsberg [:julienw]]

I'm so excited, thanks a lot for this work Jonathan!

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d40c144ae71a

Comment 40

[Gareth Aye [:gaye] (back from PTO)]

Could this explain why we're suddenly seeing Gij errors like https://treeherder.mozilla.org/logviewer.html#?job_id=10042595&repo=mozilla-inbound when trying to fetch something from window.wrappedJSObject using chrome context?

Comment 41

[Jonathan Griffin (:jgriffin)]

try run with this backed out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=358e5064de58

Comment 42

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #41)
> try run with this backed out:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=358e5064de58

this is still broken, so doesn't appear to be the culprit

Comment 44

[Jonathan Griffin (:jgriffin)]

Attachment: MozReview Request: Bug 1149618 - Add a sandbox parameter to execute, r=dburns

Comment 45

[Jonathan Griffin (:jgriffin)]

Attachment: MozReview Request: Bug 1149618 - Implement push_permission in Marionette, r=dburns