Closed
Bug 1149618
Opened 9 years ago
Closed 9 years ago
Remove SpecialPowers from Marionette
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
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.
Assignee | ||
Comment 1•9 years ago
|
||
try run for WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68a7a362c7a5 crashtests are the only thing left to fix for emulator tests
Assignee | ||
Comment 2•9 years ago
|
||
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.
Assignee | ||
Comment 3•9 years ago
|
||
firefox-ui-tests don't appear to use SpecialPowers, but cc'ing chmanchester just in case.
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
Current WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bba9f023dc2a
Assignee | ||
Comment 6•9 years ago
|
||
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.
/me cheers!
Comment 8•9 years ago
|
||
(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)
Updated•9 years ago
|
Keywords: ateam-marionette-server
Assignee | ||
Comment 10•9 years ago
|
||
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.
Assignee | ||
Comment 11•9 years ago
|
||
/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/
Assignee | ||
Comment 12•9 years ago
|
||
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.
Assignee | ||
Comment 13•9 years ago
|
||
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/
Assignee | ||
Comment 14•9 years ago
|
||
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)
Assignee | ||
Comment 15•9 years ago
|
||
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•9 years ago
|
||
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+
Assignee | ||
Comment 17•9 years ago
|
||
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•9 years ago
|
||
Assignee | ||
Comment 19•9 years ago
|
||
One more try run just to be safe: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4049e7b99182&exclusion_profile=false
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Updated•9 years ago
|
Attachment #8600618 -
Attachment is obsolete: true
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/73ec15af7075 https://hg.mozilla.org/integration/mozilla-inbound/rev/82a9bf38481b
Comment 21•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/73ec15af7075 https://hg.mozilla.org/mozilla-central/rev/82a9bf38481b
\o/ Great progress here Jonathan, thanks!
Assignee | ||
Comment 23•9 years ago
|
||
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.
Assignee | ||
Comment 25•9 years ago
|
||
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•9 years ago
|
||
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.
Assignee | ||
Comment 27•9 years ago
|
||
(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.
Assignee | ||
Comment 28•9 years ago
|
||
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)
Assignee | ||
Comment 29•9 years ago
|
||
I've reviewed firefox-ui-tests and firefox-media-tests, and neither looks to be impacted by this change.
Comment 30•9 years ago
|
||
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•9 years ago
|
||
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+
Assignee | ||
Comment 32•9 years ago
|
||
Nits addressed; new try run after rebasing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=400f455d0122
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Comment 34•9 years ago
|
||
Backed out for suspected debug emulator bustage in https://hg.mozilla.org/integration/mozilla-inbound/rev/1e983ca9801f
Assignee | ||
Comment 35•9 years ago
|
||
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
Assignee | ||
Comment 36•9 years ago
|
||
trying a fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e93d0b95af2b
Comment 38•9 years ago
|
||
I'm so excited, thanks a lot for this work Jonathan!
Comment 39•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d40c144ae71a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 40•9 years ago
|
||
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?
Updated•9 years ago
|
Flags: needinfo?(jgriffin)
Flags: needinfo?(dburns)
Assignee | ||
Comment 41•9 years ago
|
||
try run with this backed out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=358e5064de58
Flags: needinfo?(jgriffin)
Flags: needinfo?(dburns)
Assignee | ||
Comment 42•9 years ago
|
||
(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
Assignee | ||
Comment 43•9 years ago
|
||
Attachment #8598133 -
Attachment is obsolete: true
Attachment #8619921 -
Flags: review+
Attachment #8619922 -
Flags: review+
Assignee | ||
Comment 44•9 years ago
|
||
Assignee | ||
Comment 45•9 years ago
|
||
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•