1480612 - Restart manager uplift for 61.0.2

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

This is an uplift to release for bug 603903, as well as follow-up fixes from bug 1463560, and session store fixes from bug 1458119 and bug 1466071. Small changes were made in toolkit/xre/nsAppRunner.cpp to avoid pulling in anything else. Additional tests were also added.

I will be requesting reviews for the test and app runner parts, the others are sufficiently unchanged that they should be fine as-is.

Comment 1

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: 1. Add Register Application Restart

This is the introduction of the Register Application Restart feature, with the modifications listed in the commit message.

Note that here it is preffed off by default beyond Nightly.

Comment 2

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: 2. Session restore, fixes for screen listing and args, session store fixes and tests

This constitutes parts 2 & 3 of bug 603903 and all of the other bugs mentioned above. All applied cleanly.

Comment 3

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: 3. Additional Register Application Restart tests

The Register Application Research mechanism has been fairly well tested, but the automated tests left something to be desired, I'm addressing that here.

To apply this directly to mozilla-central you would have to run unix2dos on testing/firefox-ui/tests/functional/sessionstore/session_store_test_case.py first. Someone fixed the line endings, but I didn't want to do that here (and on test_restore_windows_after_windows_shutdown.py) with this patch, to reduce noise and merge trouble down the road.

Comment 4

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Try running:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7d5e4d24680963e794ca75bd34840d93957eaee

Comment 5

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

Comment on attachment 8997243 [details] [diff] [review]
3. Additional Register Application Restart tests

Review of attachment 8997243 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py
@@ +22,3 @@
>  
> +
> +def shutdown_with_variety(test_case, restart_by_os, expect_restore):

Is there a reason why you don't make it a member of the SessionStoreTestCase, so it can be shared?

Comment 6

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Thanks for taking a quick look at this!

My thought was that is is specific to this set of test cases, I don't think it will be useful more generally.

Comment 7

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Additional Register Application Restart tests (for mozilla-central)

Here's the new tests applying cleanly on mozilla-central (just had to run dos2unix on the patch).

Comment 8

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Additional Register Application Restart tests (for mozilla-central) 2

Comment 9

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

Comment on attachment 8998013 [details] [diff] [review]
Additional Register Application Restart tests (for mozilla-central) 2

Review of attachment 8998013 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, but not restoring the original app_args for Marionette in case of failures needs to be fixed.

::: testing/firefox-ui/tests/functional/sessionstore/session_store_test_case.py
@@ +319,5 @@
> +            self.marionette.instance.app_args = ['-os-restarted']
> +
> +        self.marionette.start_session()
> +        self.marionette.set_context('chrome')
> +        self.marionette.instance.app_args = saved_args

If there is something going wrong above we never restore the original `app_args`. Please use `try` and `finally`.

::: testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_windows_shutdown.py
@@ +29,5 @@
>  
>      def test_with_variety(self):
> +        """Test session restore selected by user."""
> +
> +        self.windows_shutdown_with_variety(restart_by_os=False, expect_restore=True)

nit: no need for an empty line above. Similar to other changed code.

Comment 10

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Additional Register Application Restart tests (for mozilla-central) 4

Comment 11

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Additional Register Application Restart tests (for mozilla-central) 5

Comment 12

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Additional Register Application Restart tests (for mozilla-central) 6

Comment 13

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: 3. Additional Register Application Restart tests 2

Hopefully this is the last of the futzing around with whitespace.

Comment 14

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8997219 [details] [diff] [review]
1. Add Register Application Restart

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Automatic restart will not be available until at least 62
[Is this code covered by automated tests?]: Yes, preexisting and some new ones included
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: There are known risks, but it will be pref'd off by default and rolled out slowly
[Why is the change risky/not risky?]: On Windows 10 media can begin playing before the user has logged in (see bug 1462123), this may surprise and/or irritate some users. The functionality has been working on Nightly for a few months with no issues reported that are not covered by the patches in this bug.
[String changes made/needed]: None

Comment 15

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8997239 [details] [diff] [review]
2. Session restore, fixes for screen listing and args, session store fixes and tests

Approval Request Comment
see comment 14

Comment 16

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8998028 [details] [diff] [review]
3. Additional Register Application Restart tests 2

See comment 14, this greatly expands the range of automated testing available for the automated restart feature.

Comment 17

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

Comment on attachment 8998027 [details] [diff] [review]
Additional Register Application Restart tests (for mozilla-central) 6

Review of attachment 8998027 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/firefox-ui/tests/functional/sessionstore/session_store_test_case.py
@@ +285,5 @@
>              RmEndSession(dwSessionHandle)
> +
> +    def windows_shutdown_with_variety(self, restart_by_os, expect_restore):
> +        """
> +        Test restoring windows after Windows shutdown.

Just a hint, ideally the summery should be placed right after `"""`.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8997219 [details] [diff] [review]
1. Add Register Application Restart

Driver for the 61.0.2 release. Approved for uplift.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/1b3c76905dab
https://hg.mozilla.org/releases/mozilla-release/rev/c7b8ddc5a878
https://hg.mozilla.org/releases/mozilla-release/rev/338feb0782ff

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e669c95818bd
Test Register Application Restart more thoroughly. r=whimboo

Comment 21

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8998027 [details] [diff] [review]
Additional Register Application Restart tests (for mozilla-central) 6

Thanks whimboo!

https://hg.mozilla.org/integration/mozilla-inbound/rev/e669c95818bdd82332bcfe99f8a1d7a73e116639

Comment 22

[Abe - QA (:Abe_LV)]

We conducted a smoke test and Normandy rollout on Windows Restart Manager using Windows 10 x86, x64 preview,7 x86 and 8.1 x64 and RC 61.0.2. We used a Normandy staging server recipe to get a 100% sample rate for testing purposes.

All our test runs are green. 

Test runs are here:  https://testrail.stage.mozaws.net/index.php?/plans/view/11557

Comment 23

[Natalia Csoregi [:nataliaCs]]

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

Comment 24

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Sorry, the mozilla-central commit should have been "leave-open"

Comment 25

[Chris Peterson [:cpeterson]]

Adam, can we resolve this bug? The feature was uplifted to 61.0.2 on mozilla-release in comment 19.

Comment 26

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Yes, I don't recall why I wanted this left open.