Open Bug 1141335 Opened 5 years ago Updated Last year

marionette.restart() has to shutdown the application sanely by default and not by specifying `in_app=True`

Categories

(Testing :: Marionette, defect, P3)

defect

Tracking

(Not tracked)

People

(Reporter: whimboo, Unassigned)

References

(Depends on 1 open bug)

Details

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.
Depends on: 1130220
No longer depends on: 1130220
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.
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.
(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.
Depends on: 1148220
Still an issue we care to fix?
Flags: needinfo?(hskupin)
Priority: -- → P3
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()`.
Flags: needinfo?(hskupin)
Depends on: 1440577
Depends on: 1433873
Depends on: 1467583
Enabling the in_app restart feature by default should actually be blocked on bug 1258982.
No longer blocks: 1258982
Depends on: 1258982
You need to log in before you can comment on or make changes to this bug.