1296597 - Allow Marionette to quit a running Firefox instance

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P3)

Description

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

The method quit_in_app() which has been brought to us via bug 1279243 doesn't look that it is working as expected. When looking at the code I would expect a constant hang due to the call to wait_for_port(). At this time Firefox does not run, and Marionette is blocked launching a new instance.

Also quit_in_app() should not pre-define the eRestart flag, which is only for restarts but not when we want to quit Firefox only.

Comment 1

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

Just to add, I had a Vidyo call with Andre today and we figured out those and other issues.

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Bug 1279243 was a refactor, but I now think quit_in_app should have been called restart_in_app all along. The restart flag is there intentionally.

Comment 3

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

Andre don't want a restart method. The in_app one was already working before. What he needs is a way to really quit firefox from inside the application, which means without the 'eRestart' flag. If you remove it you will see that it hangs in wait_for_port(). 

I should have a working patch on Monday maybe already tests included.

Comment 4

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

Attachment: Bug 1296597 - Allow Marionette to quit a running application instance.

Review commit: https://reviewboard.mozilla.org/r/73882/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73882/

Comment 5

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

Comment on attachment 8784409 [details]
Bug 1296597 - Allow Marionette to quit a running application instance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73882/diff/1-2/

Comment 6

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

Andre, the attached patch should completely unblock you for the telemetry tests. Even we cannot instruct Marionette from not launching Firefox at the beginning yet, quit() can be called as first command in the test. This will allow the modification of the profile afterward. Another call to start_session() will launch a new instance of Firefox.

If there is anything else please let me know and maybe better add it to bug 1294403.

Comment 7

[David Burns :automatedtester]

Comment on attachment 8784409 [details]
Bug 1296597 - Allow Marionette to quit a running application instance.

https://reviewboard.mozilla.org/r/73882/#review72492

::: testing/marionette/client/marionette_driver/marionette.py:1065
(Diff revision 2)
> -            "eRestart",
> -        ]
> -        self._send_message("quitApplication", {"flags": restart_flags})
> -        self.client.close()
>  
> -        try:
> +        self.reset_timeouts()

Why are you resetting the timeouts here?

::: testing/marionette/harness/marionette/tests/unit/unit-tests.ini:111
(Diff revision 2)
>  [test_execute_isolate.py]
>  [test_click_scrolling.py]
>  [test_profile_management.py]
>  skip-if = buildapp == 'b2g'
> +[test_quit_restart.py]
> +skip-if = buildapp == 'b2g'

can you also skip if mobile

Comment 8

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

Comment on attachment 8784409 [details]
Bug 1296597 - Allow Marionette to quit a running application instance.

https://reviewboard.mozilla.org/r/73882/#review72492

> Why are you resetting the timeouts here?

We also reset the timeouts in restart() and that was the case from the very beginning as it looks like. If this is not wanted let me know and I can file a new bug to change that. I feel a bit nervous doing it here in this patch.

> can you also skip if mobile

I don't see why this should be necessary. All of our restart tests as formerly part of test_profile_management.py were not marked as skip by Maja for Fennec: https://hg.mozilla.org/integration/autoland/file/tip/testing/marionette/harness/marionette/tests/unit/unit-tests.ini#l113

Anyway I triggered another try build specifically for Fennec. So lets see how it works for quit:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b136a1db5810

Comment 9

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8784409 [details]
Bug 1296597 - Allow Marionette to quit a running application instance.

https://reviewboard.mozilla.org/r/73882/#review72492

> I don't see why this should be necessary. All of our restart tests as formerly part of test_profile_management.py were not marked as skip by Maja for Fennec: https://hg.mozilla.org/integration/autoland/file/tip/testing/marionette/harness/marionette/tests/unit/unit-tests.ini#l113
> 
> Anyway I triggered another try build specifically for Fennec. So lets see how it works for quit:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b136a1db5810

FWIW, I wasn't sure about the status of test_profile_management.py for Fennec, but it does currently fail. I list this test in Bug 1297394 as something to look into.

Also, re try, keep in mind that Mn runs on Fennec debug builds only -- the opt build in the try push above is unnecessary.

Comment 10

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

Actually we have a failure for restarts and quit on Fennec. And this is for the chunk Mn5 now (Mn4 on autoland):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b136a1db5810&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=26514697

> TEST-UNEXPECTED-ERROR | test_quit_restart.py TestQuitRestart.test_force_quit | OSError: [Errno 2] No such file or directory: '/tmp/tmpZ4Hmpt'

It looks different to the failure from autoland which says "In app initiated quit only supported in Firefox". Not sure why actually, but I will check.

https://treeherder.mozilla.org/logviewer.html#?job_id=2683300&repo=autoland#L1899

Comment 11

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

So quitApplication in driver.js is blocking a request to quit/restart for Fennec:

https://dxr.mozilla.org/mozilla-central/rev/1a5b53a831e5a6c20de1b081c774feb3ff76756c/testing/marionette/driver.js#2568

I don't know why. But as it looks like way more efforts are necessary to get all this working for Fennec. So I would suggest that we move this out to a new bug and I stop running this for Fennec.

Comment 12

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

Comment on attachment 8784409 [details]
Bug 1296597 - Allow Marionette to quit a running application instance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73882/diff/2-3/

Comment 13

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

Comment on attachment 8784409 [details]
Bug 1296597 - Allow Marionette to quit a running application instance.

https://reviewboard.mozilla.org/r/73882/#review72492

> FWIW, I wasn't sure about the status of test_profile_management.py for Fennec, but it does currently fail. I list this test in Bug 1297394 as something to look into.
> 
> Also, re try, keep in mind that Mn runs on Fennec debug builds only -- the opt build in the try push above is unnecessary.

I disabled the new test_quit_restart.py test for Fennec for the time being.

Comment 14

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

David, please let me know what you think about the one outstanding issue. My proposal is to file a new bug to get this behavior changed, which also included restart().

Comment 15

[David Burns :automatedtester]

Comment on attachment 8784409 [details]
Bug 1296597 - Allow Marionette to quit a running application instance.

https://reviewboard.mozilla.org/r/73882/#review73356

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b37159b1121
Allow Marionette to quit a running application instance. r=automatedtester

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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