Closed Bug 1149618 Opened 5 years ago Closed 5 years ago

Remove SpecialPowers from Marionette

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jgriffin, Assigned: jgriffin)

References

Details

(Keywords: pi-marionette-server)

Attachments

(3 files, 2 obsolete files)

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.
try run for WIP:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=68a7a362c7a5

crashtests are the only thing left to fix for emulator tests
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.
Depends on: 1150753
Depends on: 1150755
firefox-ui-tests don't appear to use SpecialPowers, but cc'ing chmanchester just in case.
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?
Flags: needinfo?(dburns)
Flags: needinfo?(ato)
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.
(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
Flags: needinfo?(dburns)
Flags: needinfo?(ato)
Duplicate of this bug: 1088263
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.
/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/
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 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 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/
Attachment #8598133 - Flags: review?(dburns)
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 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
Attachment #8598133 - Flags: review?(dburns) → review+
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
Keywords: leave-open
Attachment #8600618 - Attachment is obsolete: true
Blocks: 1161209
\o/

Great progress here Jonathan, thanks!
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.
Mn unit tests green, just a couple of WebAPI oranges to fix:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e91707a0ecf4&exclusion_profile=false
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.
(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.
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.
Attachment #8604757 - Flags: review?(dburns)
I've reviewed firefox-ui-tests and firefox-media-tests, and neither looks to be impacted by this change.
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 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
Attachment #8604757 - Flags: review?(dburns) → review+
Keywords: leave-open
Backed out for suspected debug emulator bustage in https://hg.mozilla.org/integration/mozilla-inbound/rev/1e983ca9801f
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
I'm so excited, thanks a lot for this work Jonathan!
https://hg.mozilla.org/mozilla-central/rev/d40c144ae71a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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?
Flags: needinfo?(jgriffin)
Flags: needinfo?(dburns)
try run with this backed out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=358e5064de58
Flags: needinfo?(jgriffin)
Flags: needinfo?(dburns)
(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
Attachment #8598133 - Attachment is obsolete: true
Attachment #8619921 - Flags: review+
Attachment #8619922 - Flags: review+
Duplicate of this bug: 1122583
You need to log in before you can comment on or make changes to this bug.