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?

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.

(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!

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

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.

(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
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!

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?

(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.

(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.

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.

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.

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.

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.

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

(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?

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.

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.

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.

https://hg.mozilla.org/mozilla-central/rev/3fbf8d627c5c

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!

(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 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 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.

https://hg.mozilla.org/releases/mozilla-esr68/rev/47aae290cc07