Marionette:Quit only works on Firefox
Categories
(Remote Protocol :: Marionette, defect, P1)
Tracking
(firefox-esr68 fixed, firefox68 wontfix, firefox70 wontfix, firefox71 fixed)
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
(Whiteboard: [geckoview:p3])
Attachments
(1 file, 1 obsolete file)
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
Updated•7 years ago
|
Comment 1•6 years ago
|
||
This needs to be resolved for Bug 1525126. I have no knowledge of the history here, but #c0 suggests that this Just Needs Work and that there's no reason to not allow DELETE /session/...
to tear down a session (in Fennec or in a GV-consuming App). It's possible that geckodriver
needs to play along to ensure that the App is really killed, but we can cross that bridge as we need to.
ato or whimboo: can you confirm?
Assignee | ||
Comment 2•6 years ago
|
||
Nick, can you please explain why you need that command implemented for Fennec/GeckoView? This command is used to quit the application, but not to delete a session which would be WebDriver:DeleteSession
.
Comment 3•6 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #2)
Nick, can you please explain why you need that command implemented for Fennec/GeckoView? This command is used to quit the application, but not to delete a session which would be
WebDriver:DeleteSession
.
Absolutely! I have a Web Driver script like https://github.com/SeleniumHQ/selenium/blob/master/javascript/node/selenium-webdriver/example/chrome_android.js, but lightly modified to work with GeckoDriver targeting GVE:
'use strict';
const {Builder, By, Key, promise, until} = require('..');
const {Options, ServiceBuilder} = require('../firefox');
promise.consume(function* () {
let driver;
try {
driver = yield new Builder()
.forBrowser('firefox')
.setFirefoxOptions(
new Options()
.androidPackage('org.mozilla.geckoview_example')
.androidActivity('.GeckoViewActivity'))
.build();
yield driver.get('http://www.google.com/ncr');
yield driver.findElement(By.name('q')).sendKeys('webdriver', Key.RETURN);
yield driver.wait(until.titleIs('webdriver - Google Search'), 1000);
} finally {
yield driver && driver.quit();
}
}).then(_ => console.log('SUCCESS'), err => console.error('ERROR: ' + err));
(This depends on much work in progress, so please don't try it locally.) That fails with:
1550030514492 webdriver::server DEBUG -> DELETE /session/08f7a7ea-e0b4-4aae-96ea-cc4e8ac5e8dc
1550030514497 webdriver::server DEBUG <- 500 Internal Server Error {"value":{"error":"unsupported operation","message":"Only supported in Firefox","stacktrace":"WebDriverError@chrome://marionette/content/error.js:179:5\nUnsupportedOperationError@chrome://marionette/content/error.js:495:5\nassert.that/<@chrome://marionette/content/assert.js:417:13\nassert.firefox@chrome://marionette/content/assert.js:92:3\nGeckoDriver.prototype.quit@chrome://marionette/content/driver.js:3336:3\ndespatch@chrome://marionette/content/server.js:289:20\nexecute@chrome://marionette/content/server.js:262:11\nonPacket/<@chrome://marionette/content/server.js:235:15\nonPacket@chrome://marionette/content/server.js:234:8\n_onJSONObjectReady/<@chrome://marionette/content/transport.js:492:9\n"}}
ERROR: UnsupportedOperationError: Only supported in Firefox
Simply because the Marionette:Quit
call is unsupported. If I remove the assertion, then Gecko tries to quit, and indeed it does quit -- but it doesn't bubble out to the surrounding App (GVE, in this case), so you end up with this zombie-like App shell. That might be a thing that we should make GV accommodate, or it might be that when targeting Android we should make geckodriver
force-stop the App from the outside, rather than through Marionette:Quit
from the inside.
Generally GV doesn't anticipate Gecko quitting -- see all the shenanigans around roboextender and Robocop:Quit
-- so I tend to think that we shouldn't do this from the inside. Hence the question!
Comment 4•6 years ago
|
||
snorp: do you have an opinion on whether GV should be able to be quit "from the inside", like via Services.startup.quit
? https://bugzilla.mozilla.org/show_bug.cgi?id=1298921#c3 should provide some context.
(In reply to Nick Alexander :nalexander [he/him] from comment #4)
snorp: do you have an opinion on whether GV should be able to be quit "from the inside", like via
Services.startup.quit
? https://bugzilla.mozilla.org/show_bug.cgi?id=1298921#c3 should provide some context.
Apps can detect that a GeckoRuntime
has shut down by registering a delegate[0]. We do this in TestRunnerActivity
[1], which is used to run mochitest.
[0] https://mozilla.github.io/geckoview/javadoc/mozilla-central/org/mozilla/geckoview/GeckoRuntime.html#setDelegate-org.mozilla.geckoview.GeckoRuntime.Delegate-
[1] https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TestRunnerActivity.java#183
Assignee | ||
Comment 6•6 years ago
|
||
I'm not against for removing this assertion, as long as the currently supported apps are all supporting the shutdown triggered via Services.app.quit()
. If not we might have to adjust the assertion to exclude those specific apps.
If GV hosted apps need updates please move this out to their own bugs, so that we can keep this Marionette only.
Comment 7•6 years ago
|
||
(In reply to Nick Alexander :nalexander [he/him] from comment #3)
Simply because the
Marionette:Quit
call is unsupported. If I
remove the assertion, then Gecko tries to quit, and indeed it
does quit -- but it doesn't bubble out to the surrounding App
(GVE, in this case), so you end up with this zombie-like App
shell. That might be a thing that we should make GV accommodate,
or it might be that when targeting Android we should make
geckodriver
force-stop the App from the outside, rather than
throughMarionette:Quit
from the inside.Generally GV doesn't anticipate Gecko quitting -- see all the
shenanigans around roboextender andRobocop:Quit
-- so I tend
to think that we shouldn't do this from the inside. Hence the
question!
The reason behind Marionette:Quit
and triggering the application
to shut down internally, is that Firefox doesn’t respect kill(1)
signals. If geckodriver were to send SIGINT to Firefox it fails
to generate leak logs.
If this isn’t a concern for GeckoView, geckodriver can simply be
made application-aware and kill the outer application GV is embedded
in from the outside, and avoid calling Marionette:Quit
altogether.
In other words, it sounds me like the assertion should not be
removed?
Assignee | ||
Comment 8•6 years ago
|
||
(In reply to Andreas Tolfsen ⦗:ato⦘ from comment #7)
The reason behind
Marionette:Quit
and triggering the application
to shut down internally, is that Firefox doesn’t respect kill(1)
signals. If geckodriver were to send SIGINT to Firefox it fails
to generate leak logs.
This is not only due to this reason, but also because in some cases you want to restart the application. And those need to be triggered inside of eg. Firefox. If for Fennec and GV we have a similar case, then it would indeed make sense to enable that feature for other products than Firefox.
Comment 9•6 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #8)
This is not only due to this reason, but also because in some
cases you want to restart the application. And those need to be
triggered inside of eg. Firefox. If for Fennec and GV we have
a similar case, then it would indeed make sense to enable that
feature for other products than Firefox.
AIUI it doesn’t really make sense for GeckoView to handle restarting,
since it does not itself control the lifetime of the application
it is embedded in.
I’m assuming here there is no hook in GeckoView that allows the
application to trigger a shutdown based on an in-browser event?
(In reply to Andreas Tolfsen ⦗:ato⦘ from comment #9)
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #8)
This is not only due to this reason, but also because in some
cases you want to restart the application. And those need to be
triggered inside of eg. Firefox. If for Fennec and GV we have
a similar case, then it would indeed make sense to enable that
feature for other products than Firefox.AIUI it doesn’t really make sense for GeckoView to handle restarting,
since it does not itself control the lifetime of the application
it is embedded in.I’m assuming here there is no hook in GeckoView that allows the
application to trigger a shutdown based on an in-browser event?
See comment #5. The app can detect when Gecko has shut down and follow suit if it wishes.
Comment 11•6 years ago
|
||
I believe this is a blocker for Android PGO as well (bug 632954). I'm using Marionette to drive the profile generation step in Android PGO, and ideally Fennec would shutdown on driver.quit(in_app=True) so the profile data can be written out during a clean shutdown.
Comment 12•6 years ago
|
||
And FWIW, simply removing the assert.firefox() makes this work for my purposes, but I don't know what ramifications that has for GV or other clients.
Assignee | ||
Comment 13•6 years ago
|
||
I will have a look at it by early next week. As long as tests for GV don't call quit, there shouldn't be any side-effects.
Assignee | ||
Comment 14•6 years ago
|
||
Turns out this is not easily doable. When the assert is removed nearly all the tests in test_quit_restart.py fail due to profile issues. So to be able to allow quitting outside of Firefox more work needs to be done. And we won't have the time in the next weeks. Sorry.
I provided Michael with some workarounds which might hopefully work.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 15•6 years ago
|
||
FWIW, I ran into this today as well doing automated testing on Fenix. Similar to the PGO use case but for telmetry, I am clearing internal state, persisting cleared state, and quitting so that the next invocation is "clean" and ready for testing in reproducible way.
Can work around but would prefer Desktop FF and Android FF to match behaviors
Comment 16•6 years ago
|
||
(In reply to Benjamin De Kosnik [:bdekoz] from comment #15)
FWIW, I ran into this today as well doing automated testing on Fenix. Similar to the PGO use case but for telmetry, I am clearing internal state, persisting cleared state, and quitting so that the next invocation is "clean" and ready for testing in reproducible way.
Can work around but would prefer Desktop FF and Android FF to match behaviors
For Android PGO we ended up using Marionette instead of the quitter extension because of this bug. That code lives here: https://dxr.mozilla.org/mozilla-central/rev/d9d0399a6baf2f0677586b79f3195d39b2119f97/testing/mozharness/scripts/android_emulator_pgo.py#213
snorp had a patch in bug 1524992 to move desktop PGO over to Marionette as well, but that didn't make it in the final landing of the bug (https://phabricator.services.mozilla.com/D21609)
Perhaps either of those serve as a template?
Assignee | ||
Comment 17•5 years ago
|
||
As we discussed for geckodriver on bug 1525126 yesterday, it doesn't make sense to use an in-application quit request for a mobile browser on Android because it is not clear what the behavior should be. Instead the application needs to be force-killed via adb. If you have some PGO scripts running which are using Marionette driver only, it is the way to go. Your script even starts the browser and as such should control its shutdown.
Let me know if there would be problems with this approach. We should figure out something else in such a case.
Comment 18•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 19•5 years ago
|
||
Now that Fennec is no longer supported, and we don't have to spend the time figuring out how to make it work, I would suggest that we simply remove the line.
For GeckoView and any vehicle the quit command wouldn't work, but also doesn't cause an exception. At least not for now when I tried with geckodriver and the geckoview example app.
In general for Android the browser should always be force stopped by geckodriver. I will have a patch shortly.
Assignee | ||
Comment 20•5 years ago
|
||
Formerly this assertion for Firefox was added because it doesn't work for Fennec,
and other mobile apps. While thinking more about it we shouldn't have this hard
restriction anymore, given that no exception is thrown by the code when running
with GeckoView based apps.
As we know this method doesn't quit the GeckoView app, and geckodriver itself
has to ensure to force stop the process on Android.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 21•5 years ago
|
||
Comment 22•5 years ago
|
||
bugherder |
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 23•5 years ago
|
||
Nick, would you mind to have a check if the Android specific change in your patch would be ok to get uplifted to esr68? Thanks!
Assignee | ||
Updated•5 years ago
|
Comment 24•5 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #23)
Nick, would you mind to have a check if the Android specific change in your patch would be ok to get uplifted to esr68? Thanks!
Yes, it should be fine to uplift, and I think it's a good idea to do so. Setting the flag.
Comment 25•5 years ago
|
||
Comment on attachment 9081499 [details]
Bug 1298921 - [marionette] Don't restrict Marionette:Quit to Firefox only.
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is an irritation that will bite us, and folks testing against Firefox for Android, for however many years that product hangs around in testing configurations.
- User impact if declined: End users? Almost none. Folks testing against Firefox for Android? Almost certainly will irritate them.
- Fix Landed on Version: 71
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): It's a tiny change to Marionette, so it's not enabled by default. Even then, very few folks are using Marionette to test Firefox for Android, since this is all very, very recent: Bug 1525126.
- String or UUID changes made by this patch:
Comment 26•5 years ago
|
||
Comment on attachment 9081499 [details]
Bug 1298921 - [marionette] Don't restrict Marionette:Quit to Firefox only.
Makes life easier for people testing against Fennec. Approved for 68.5b4.
Comment 27•5 years ago
|
||
bugherder uplift |
Updated•2 years ago
|
Description
•