Allow Marionette to quit a running Firefox instance

RESOLVED FIXED in Firefox 51

Status

Testing
Marionette
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
Just to add, I had a Vidyo call with Andre today and we figured out those and other issues.
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.
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 hidden (mozreview-request)
Summary: Fix issues with quit_in_app() → Allow Marionette to quit a running application instance.
Summary: Allow Marionette to quit a running application instance. → Allow Marionette to quit a running application instance
Comment hidden (mozreview-request)
Attachment #8784409 - Flags: review?(dburns)
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

a year ago
mozreview-review
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
Blocks: 1298800
(Assignee)

Comment 8

a year ago
mozreview-review-reply
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

a year ago
mozreview-review-reply
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.
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
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.
Blocks: 1298921
Summary: Allow Marionette to quit a running application instance → Allow Marionette to quit a running Firefox instance
Comment hidden (mozreview-request)
(Assignee)

Comment 13

a year ago
mozreview-review-reply
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.
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().
Flags: needinfo?(dburns)

Comment 15

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

https://reviewboard.mozilla.org/r/73882/#review73356
Attachment #8784409 - Flags: review?(dburns) → review+
Flags: needinfo?(dburns)

Comment 16

a year ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b37159b1121
Allow Marionette to quit a running application instance. r=automatedtester

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3b37159b1121
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.