Closed Bug 1480612 Opened Last year Closed Last year

Restart manager uplift for 61.0.2

Categories

(Core :: Widget: Win32, defect)

62 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox61 blocking verified
firefox62 --- unaffected
firefox63 --- unaffected

People

(Reporter: agashlin, Assigned: agashlin)

References

Details

Attachments

(4 files, 5 obsolete files)

9.61 KB, patch
jimm
: review+
Details | Diff | Splinter Review
50.51 KB, patch
agashlin
: review+
Details | Diff | Splinter Review
12.24 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
32.41 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → agashlin
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.
Attachment #8997219 - Flags: review?(jmathies)
Attachment #8997219 - Attachment description: Add Register Application Restart → 1. Add Register Application Restart
This constitutes parts 2 & 3 of bug 603903 and all of the other bugs mentioned above. All applied cleanly.
Attachment #8997239 - Flags: review+
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.
Attachment #8997243 - Flags: review?(hskupin)
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?
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.
Here's the new tests applying cleanly on mozilla-central (just had to run dos2unix on the patch).
Attachment #8997965 - Flags: review?(hskupin)
Attachment #8997243 - Flags: review?(hskupin)
Attachment #8997219 - Flags: review?(jmathies) → review+
Attachment #8997965 - Attachment is obsolete: true
Attachment #8997965 - Flags: review?(hskupin)
Attachment #8998013 - Flags: review?(hskupin)
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.
Attachment #8998013 - Flags: review?(hskupin) → review-
Attachment #8998013 - Attachment is obsolete: true
Attachment #8998020 - Flags: review?(hskupin)
Attachment #8998020 - Attachment is obsolete: true
Attachment #8998020 - Flags: review?(hskupin)
Attachment #8998023 - Flags: review?(hskupin)
Attachment #8998023 - Attachment is obsolete: true
Attachment #8998023 - Flags: review?(hskupin)
Attachment #8998027 - Flags: review?(hskupin)
Hopefully this is the last of the futzing around with whitespace.
Attachment #8997243 - Attachment is obsolete: true
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
Attachment #8997219 - Flags: approval-mozilla-release?
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
Attachment #8997239 - Flags: approval-mozilla-release?
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.
Attachment #8998028 - Flags: approval-mozilla-release?
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 `"""`.
Attachment #8998027 - Flags: review?(hskupin) → review+
Comment on attachment 8997219 [details] [diff] [review]
1. Add Register Application Restart

Driver for the 61.0.2 release. Approved for uplift.
Attachment #8997219 - Flags: approval-mozilla-release? → approval-mozilla-release+
Attachment #8997239 - Flags: approval-mozilla-release? → approval-mozilla-release+
Attachment #8998028 - Flags: approval-mozilla-release? → approval-mozilla-release+
Flags: qe-verify+
Pushed by agashlin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e669c95818bd
Test Register Application Restart more thoroughly. r=whimboo
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
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
Flags: qe-verify+
Flags: in-testsuite+
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/e669c95818bd
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Sorry, the mozilla-central commit should have been "leave-open"
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Adam, can we resolve this bug? The feature was uplifted to 61.0.2 on mozilla-release in comment 19.
Flags: needinfo?(agashlin)
Yes, I don't recall why I wanted this left open.
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Flags: needinfo?(agashlin)
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.