1141335 - quit() and restart() have to shutdown the application sanely by default and not by specifying `in_app=True`

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Bug 1137388 implemented a safe shutdown of the application but sadly that is not the default. So the browser is still hard-killed. That's why each test, which want to restart the browser sanely has to set `in_app=True` as parameter for the restart() method. This will be the case for nearly every invoke of this method.

Personally I do not think that this parameter is necessary at all. We should get it removed, or at least default to True.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

While working on a testcase for bug 1228390 I have seen that we do not correctly restore the former session after a restart of Firefox. When browser.startup.page=3 is set sessionstore should restore exactly the opened windows and tabs from the last session. With the attachment 8693720 [details] it does not happen when it gets run via:

marionette --binary /mozilla/bin/nightly/firefox test_marionette.py --pref browser.startup.page:3

So I still think that the way how we shutdown Firefox via the in_app flag is not correct or at least misses some pieces.

Initially (for the first restart related bug) I already gave all the information about Mozmill related code which was working without any issues. Sadly most of that hasn't been obeyed yet. So I will give it again. Also I talked with Mike on IRC to ensure the code we had in Mozmill was doing the right thing.

Code for Marionette:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/marionette.py#1123

Code for Mozmill:
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/frame.js#L40

1. As of now we force Firefox to exit! This can cause dataloss if components waiting for a shutdown request wont get it. See eAttemptQuit vs. eForceQuit here. Maybe best would be to allow a parameter for flags to be passed in to trigger a shutdown or restart.

2. Something else helpful might be to fire a quit-application-requested observer notification to announce a shutdown of Firefox. Components should pick this request up, run their finalizing code and return.

As it looks right now this bug actually blocks us from writing sane sessionrestore restart tests.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

The flags are effectively the same for marionette and mozmill -- the beginning of mozmill's "shutdownApplication" function linked means eForceQuit is always present. My recollection from implementing this was that there was really no difference between specifying both eAttemptQuit and eForceQuit and specifying just eForceQuit.

We found working on one of the mozmill tests that broadcasting quit-application-requested was definitely necessary to session restore. Bug 1148220 was filed a while back to get it added to marionette.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Chris Manchester [:chmanchester] from comment #2)
> The flags are effectively the same for marionette and mozmill -- the
> beginning of mozmill's "shutdownApplication" function linked means
> eForceQuit is always present. My recollection from implementing this was
> that there was really no difference between specifying both eAttemptQuit and
> eForceQuit and specifying just eForceQuit.

Indeed. Interesting, cause this was not planned. But given that it was working fine in Mozmill it would mean eForceQuit is fine, or aAttemptQuit gets checked first. I don't really know the backend code. But anyway, lets discuss that on the other bug.

> We found working on one of the mozmill tests that broadcasting
> quit-application-requested was definitely necessary to session restore. Bug
> 1148220 was filed a while back to get it added to marionette.

Oh! That was right before I left for a month. I totally forgot about this bug. So indeed that part is covered by the other bug then.

FYI, as Mike told me he is moving some code of sessionstore out of "quit-application-requested" so sending this message might only be necessary for other components, which would still fail to save a proper state.

Comment 4

[Andreas Tolfsen ❲:ato❳]

Still an issue we care to fix?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yes, at some point we have to make in_app=True the default, to prevent the dataloss which happens when just calling `quit()` or `restart()`.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Enabling the in_app restart feature by default should actually be blocked on bug 1258982.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This safe shutdown logic would actually apply to both the quit() and restart() commands. As such lets extend this bug to cover both at the same time.

Also given that we are pretty stable with in-application quits and restarts over the last months and more tests outside of our unit tests are getting added, I would propose to finally make this switch and flip the default from False to True.

Here a try build to see if that actually works fine:
https://treeherder.mozilla.org/jobs?repo=try&revision=c5392f953c490488412d37812bfdda28798de10c

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1141335 - [marionette] Use in_app by default for quit() and restart().

Hereby the "in_app" argument is required to be specified in case some
other argument eg. "clean" requires a termination of the application.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e220bf19d7b
[marionette] Use in_app by default for quit() and restart(). r=webdriver-reviewers,jdescottes

Comment 10

[Atila Butkovits]

Backed out for causing failures at awsy/test_memory_usage.py.

Backout link: https://hg.mozilla.org/integration/autoland/rev/2339498ee23029401e4e0fe6d36cbb2b23684c28

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=3e220bf19d7b69232b6aa7e702c0c2b0bc569f2c&selectedTaskRun=UfukuLMKTvCrxbD59_JwTQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=391826853&repo=autoland&lineNumber=1640

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The awsy harness calls self.marionette.restart(clean=False) which formerly used in_app=False to restart Firefox. To stay in sync I explicitly added the argument now and pushed to try:

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1232c012f99
[marionette] Use in_app by default for quit() and restart(). r=webdriver-reviewers,jdescottes,perftest-reviewers,AlexandruIonescu

Comment 13

[Atila Butkovits]

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