603903 - Register with Windows Restart Manager (re-launch Firefox after Windows reboot)

Status: RESOLVED FIXED

(Core :: Widget: Win32, enhancement, P1)

Description

[James May [:fowl2]]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101012 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101010 Firefox/4.0b8pre

When Windows Update (and other installers) restart Windows systems, they can ask nicely for the apps to save their state, and also to restart those apps when done.

I'm mainly concerned about the Windows Update case (sometimes they are un-veto-able by group policy), but other un/installations (such as uninstalling a plugin) could benefit too.

Reproducible: Always

Steps to Reproduce:
Firefo 
Actual Results:  
Firefox is either forcibly terminated and brings up the "Well this is embarrasing" screen when it is manually restarted, the shutdown hangs waiting  on the "Do you want Minefield to save your tabs and windows for the next time it starts?" dialog, or even worse, if that prompt has been turned off, the session is lost.

Expected Results:  
Firefox saves it's session, is orderly shut down, and then restarted exactly where it left off when the system comes back.

In the WM_QUERYENDSESSION handler, check for ENDSESSION_CLOSEAPP and then call RegisterApplicationRestart.
Then, in WM_ENDSESSION save the session without confimation. This should probably happen anyway.

http://msdn.microsoft.com/en-us/library/aa373651%28v=VS.85%29.aspx
http://msdn.microsoft.com/en-us/library/aa376890.aspx
http://msdn.microsoft.com/en-us/library/aa373347.aspx

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

We might need something on the session restore end, but mostly this would be handled by windows widget code.

Comment 2

[Steven]

Is there any chance of this being implemented? Chrome already does this, and restores open tabs. Internet Explorer will re-open as well, but does not restore tabs. I have not tested how Edge behaves.

Comment 3

[Chris Peterson [:cpeterson]]

Chrome and IE11 restart after any Windows reboot, not just Windows Update's forced reboots. Edge doesn't restart. I'm surprised Microsoft changed this behavior between IE11 and Edge.

Romain had some Firefox telemetry that reported the various reasons why Firefox exits and "because Windows is shutting down" was a very small number. I can't find the numbers right now, but we decided that it wasn't enough people to warrant prioritizing this feature. That said, I don't see why we wouldn't accept a patch if someone wanted to work on this. :)

Comment 5

[Romain Testard [:RT]]

My previous look at this on tmo showed 6M restarts per month.
I now discussed with chutten and updated one of his queries for our specific use case and it looks like:
- 4.7% of clients restart Windows with Firefox opened every day: https://sql.telemetry.mozilla.org/queries/52383/source#139604
- We have no data on Mac but per chat on slack with mconnor there is a similar mechanism on mac (separate bug?)

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1399556#c2 Adam has a preliminary implementation.
Could we implement this behind a pref (for any OS shutdown when Firefox was opened), pref it off and test this with Shield when on release? We'd measure engagement, retention and potentially general user sentiment impact of the change to understand if this has a good overall impact on users.

Comment 6

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

> In the WM_QUERYENDSESSION handler, check for ENDSESSION_CLOSEAPP and then call RegisterApplicationRestart.

I've tried this, but it seems that doing RegisterApplicationRestart in WM_QUERYENDSESSION doesn't actually work, despite what the docs say. It's possible that "For a Windows application that is being updated, the last opportunity to call this function is while processing the WM_QUERYENDSESSION message" just means that we can update the command line if we're already registered. If that's the case we might have a "don't actually run" command line option, but that seems like a bad idea.

Neither does doing UnregisterApplicationRestart in WM_QUERYENDSESSION seem to successfully unregister us.

So it looks like we're stuck, as of Fall Creator's update, with this working across all OS shutdowns.
I haven't worked on shield studies before, but I can easily put this behind a pref, defaulted to off.

Comment 7

[Chris Peterson [:cpeterson]]

Here is Chrome's RegisterApplicationRestart implementation. The code comments say "Restart Chrome if the computer is restarted as the result of an update", but in my testing, Chrome (and IE11) restart after all restarts.

https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main_win.cc?q=RegisterApplicationRestart&dr=CSs&l=604-637

Chrome calls RegisterApplicationRestart() on app launch, not in WM_QUERYENDSESSION, perhaps because it doesn't work then?

https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?type=cs&q=WM_QUERYENDSESSION&l=1838-1843

Comment 8

[Chris Peterson [:cpeterson]]

If we are going to run an experiment, I think we should compare three different pref values:

* Don't restart Firefox after reboot. (Firefox's current behavior)
* Restart Firefox after any reboot. (Chrome and IE11's current behavior)
* Restart Firefox only after OS update reboot. (The behavior described in Chrome's code comments, but not implemented as such, AFAICT.)

Comment 9

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

The behavior of RegisterApplicationRestart used to be (and still is described in the docs) that it would only restart registered apps on restart for updates or installs. However as of Windows 10 Fall Creator's Update this now applies to *all* restarts. See this thread, the post from PeterSey... is about the only documentation I know of this change:
https://answers.microsoft.com/en-us/windows/forum/windows_10-other_settings/apps-reopening-on-start-up-after-shutdownrestart/b6d6b6b1-dc08-4ecd-aad7-088eb7cb723e

Comment 10

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

Sorry, the name you want to search for in that thread is PaulSey...

It was earlier mentioned here (search for the post from Jason[MS]) when it hit the insider release:
https://answers.microsoft.com/en-us/insider/forum/insider_wintp-insider_desktop/programs-autostart-after-boot-in-windows-10/09dd8d3e-7b36-45d1-9181-6587dd5d53ab?page=2

Comment 11

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

Attachment: Bug 603903: use RegisterApplicationRestart behind a pref

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

Comment 12

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

The patch from comment 11 uses the pref "toolkit.winRegisterApplicationRestart", just true or false. I didn't add that to browser/app/profile/firefox.js, not sure if I should yet, I use a default of false when reading the pref.

There is some weird behavior, though, when Windows automatically starts up Firefox. First, it starts up before the user has actually logged in if you wait at the login screen long enough. If started this way, it can get into a few weird states:

- appearing to be focused but not taking keyboard input
Alt-Tab or a click in the window fixes this

- being completely frozen but not triggering Windows' "not responding" detection
I've only seen this once so far, oddly it got out of this state when I hit Alt-F4

- if started maximized, restoring makes the window disappear
The Alt-Space menu still appears for it and can be used to resize it.

---

This is probably going to require some more testing...

Comment 13

[Romain Testard [:RT]]

Are we able to set the Firefox to re-open in the background and not foreground?
It sounds most valuable since it would load Firefox but not force the user on our window.

Comment 14

[Chris Peterson [:cpeterson]]

When Firefox is restarted automatically, do we want to restore the previous session's tabs (regardless of the user's "When Firefox starts" setting)?

Chrome and IE11 restore their previous sessions' tabs when I reboot Windows, even though they don't restore tabs (by default) after manually quitting and restarting the browser. Since the user didn't manually quit their browser session, I think Firefox should restore the previous tabs like Chrome and IE11 do.

I had a Windows update pending, so I selected the "Restart and Update" start menu item to compare what happens after an update reboot versus a normal reboot. After the Windows update was replied, to my surprise, neither Chrome nor IE11 were restarted. Maybe because Windows had to reboot more than once when applying the update? And Edge *was* restarted with the previous session's tabs restored, even though it is not restarted after a normal reboot. Edge was started because Windows opens a "What's new in Windows 10" page (which is expected), but I was surprised to see Edge also restore the previous session's tabs.

Comment 15

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

Attachment: Bug 603903: register with restart service, restore session if forced to shutdown

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

Comment 16

[Chris Peterson [:cpeterson]]

Comment on attachment 8966716 [details]
Bug 603903: register with restart service, restore session if forced to shutdown

https://reviewboard.mozilla.org/r/235422/#review241186

::: toolkit/xre/nsAppRunner.cpp:1914
(Diff revision 1)
> +
> +    if (restartCommandLine) {
> +      // Flags RESTART_NO_PATCH and RESTART_NO_REBOOT are not set, so we
> +      // should be restarted if terminated by an update or restart.
> +      ::RegisterApplicationRestart(restartCommandLine, RESTART_NO_CRASH |
> +                                                       RESTART_NO_HANG);

Maybe assert that RegisterApplicationRestart returns success? It would be good to know if the API fails in a debug build, even if failing to register is not that big a problem in opt builds.

::: widget/windows/nsWindow.cpp:5184
(Diff revision 1)
>        DwmDefWindowProc(mWnd, msg, wParam, lParam, &dwmHitResult)) {
>      *aRetValue = dwmHitResult;
>      return true;
>    }
>  
> +  static const char kRestartPrefName[] = "toolkit.winRegisterApplicationRestart";

There are two pref name definitions: this static in nsWindow.cpp and a macro in nsAppRunner.cpp. Can we consolidate the pref definition to a macro in some toolkit header file that can be included in nsWindow.cpp?

::: widget/windows/nsWindow.cpp:5237
(Diff revision 1)
> +        const char16_t* quitType = nullptr;
> +        if (Preferences::GetBool(kRestartPrefName, true)) {
> +          quitType = u"restart";
> +        }

This quitType code is repeated twice in this ProcessMessage() function, which is already pretty long. Maybe extract it into a helper function like GetQuitType()?

Comment 17

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

Comment on attachment 8966716 [details]
Bug 603903: register with restart service, restore session if forced to shutdown

https://reviewboard.mozilla.org/r/235422/#review241186

> Maybe assert that RegisterApplicationRestart returns success? It would be good to know if the API fails in a debug build, even if failing to register is not that big a problem in opt builds.

The biggest issue with this is that RegisterApplicationRestart can fail with E_INVALIDARG if the command line is longer than RESTART_MAX_CMD_LINE characters, which I think is 1k. This might happen in some testing scenarios, I imagine, I know in Chromium there are a lot of reports of them hitting that error in tests.

I could just make sure we don't call RegisterApplicationRestart at all if the command line is too long, I guess.

> This quitType code is repeated twice in this ProcessMessage() function, which is already pretty long. Maybe extract it into a helper function like GetQuitType()?

Good point about the duplication here and in the pref name above.

Comment 18

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

Mike, do you think I'm using the right approach to force a session restore here? (Or could you point me to someone else to ask?)

All I'm doing is passing in "restart" to the "quit-application-requested" and "quit-application" notifications in the synchronous shutdown path for WM_QUERYENDSESSION and WM_ENDSESSION. This mostly seems to work but I've seen it fail to restore (just starting up a fresh session) at least once.

Comment 19

[Chris Peterson [:cpeterson]]

(In reply to Adam Gashlin [:agashlin] from comment #17)
> The biggest issue with this is that RegisterApplicationRestart can fail with
> E_INVALIDARG if the command line is longer than RESTART_MAX_CMD_LINE
> characters, which I think is 1k. This might happen in some testing
> scenarios, I imagine, I know in Chromium there are a lot of reports of them
> hitting that error in tests.
> 
> I could just make sure we don't call RegisterApplicationRestart at all if
> the command line is too long, I guess.

I see. That sounds like it's more trouble than it's worth. Plus, it's possible that RESTART_MAX_CMD_LINE might change some day so the version Windows is using is different than the RESTART_MAX_CMD_LINE defined when Firefox was compiled. Then our length pre-check might be unnecessarily small for no real benefit.

Comment 20

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

Attachment: Bug 603903 - use RegisterApplicationRestart, fix pre-login issues

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

Comment 21

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

Patch updated to address the issues Chris brought up (thanks!) and to fix a gap in handling login.

There's a spot fix for the default browser popup, but there are other modals that could appear at startup (I started looking for all modal dialogs and ran out of steam at 70 [1]), and there has to be a better general solution to handling new windows before we have screens. Popups shouldn't try to limit their size to the screen size when no screen is available, and the lack of a screen to center on shouldn't result in centering at (0,0).

One possibility is that we can fall back to some other API that works better than EnumDisplayMonitors when logged out (if one exists). Or we could move offscreen windows onscreen when encountering a change. I don't think failing to create windows is a good solution, there would be too much work to expect failure everywhere.

Other known issues left to deal with:
- saving environment variables
- restore from maximized size still is very small

[1] https://docs.google.com/spreadsheets/d/1ju-H11HdRcsoEOrzmwebcSbv8moTikMZuHVAmjDZLzg/edit#gid=0

Comment 22

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

A few other issues that slipped my mind:

- if RegisterApplicationRestart fails, we shouldn't force session restore (or anything else which will affect the next startup) when shutting down

- conversely, shouldn't do UnRegisterApplicationRestart if there wasn't a successful RAR previously (probably harmless, but ugly)

- I need to check how this is going to interact with Firefox updates

Comment 23

[Mike de Boer [:mikedeboer]]

(In reply to Adam Gashlin [:agashlin] from comment #18)
> Mike, do you think I'm using the right approach to force a session restore
> here? (Or could you point me to someone else to ask?)
> 
> All I'm doing is passing in "restart" to the "quit-application-requested"
> and "quit-application" notifications in the synchronous shutdown path for
> WM_QUERYENDSESSION and WM_ENDSESSION. This mostly seems to work but I've
> seen it fail to restore (just starting up a fresh session) at least once.

Hi Adam, thanks for working on this and asking about the approach here!

So this looks like the right thing to do indeed. Adding 'restart' to 'quit-application' runs https://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#1718, which is what you want.
The pref that controls the restore-on-startup is set on line 1721 in that file.

If the session fails to restore - as you've seen before, apparently - then there's a bug in sessionstore that we can take a look at together if you can find a way to reliably reproduce it.

Comment 24

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

Attachment: Bug 603903 - register for restart, restore session, fix pre-login issues

Three main things are in the patch:
- Use RegisterApplicationRestart behind the preference
toolkit.winRegisterApplicationRestart

- A workaround in ScreenHelperWin to get the info for all monitors even when
run before login (a Windows 10 "feature" when automatically restarted),
EnumDisplayMonitors with the "DISPLAY" DC doesn't work.

- If the pref is set, force restoring session state (act as if it was a
restart)

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

Comment 25

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

It turns out that EnumDisplayMonitors works fine pre-login as long as you give it nullptr instead of a DC for the whole display. However this means that the callback doesn't get a per-monitor DC (used for getting pixel depth), so I manually create one for each device. With the patch in comment 24 popups should just work. This also fixes the restore size for the main window.

I'm going to leave off trying to find a way to save environment variables, at least as part of this bug.

One obnoxious issue I'd like to fix is that the active tab restores and runs before the user logs in. If an autoplaying video was in the tab on shutdown, it will play audibly behind the login screen. I think it shouldn't be too hard (famous last words) to delay loading the tab if we detect that we're locked/logged out/detached.

Sorry that I haven't been submitting these patches in an interdiff-friendly way, I'll try to fix that with subsequent submissions.

Comment 26

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

Attachment: Bug 603903 - Part 1: add RegisterApplicationRestart

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

Comment 27

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

Attachment: Bug 603903 - Part 2: restore session when restarted by Windows

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

Comment 28

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

Attachment: Bug 603903 - Part 3: fix screen enumeration when not logged in

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

Comment 29

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

Attachment: Bug 603903 - Part 4: don't restore tabs until session unlock

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

Comment 30

[Mike de Boer [:mikedeboer]]

(In reply to Adam Gashlin [:agashlin] from comment #25)
> One obnoxious issue I'd like to fix is that the active tab restores and runs
> before the user logs in. If an autoplaying video was in the tab on shutdown,
> it will play audibly behind the login screen. I think it shouldn't be too
> hard (famous last words) to delay loading the tab if we detect that we're
> locked/logged out/detached.

... which inadvertently may get fixed with the updated autoplay policy that cpearce is implementing right now. If you intend to work on this after all, please be sure to give him a ping!

Comment 31

[Mike de Boer [:mikedeboer]]

Comment on attachment 8971071 [details]
Bug 603903 - Part 2: restore session when restarted by Windows

https://reviewboard.mozilla.org/r/239840/#review246366

Please see comments below. A propos: would you be interested in giving OSX the same treatment? ;-P

::: browser/app/profile/firefox.js:861
(Diff revision 1)
>  pref("browser.rights.override", true);
>  #endif
>  
>  pref("browser.sessionstore.resume_from_crash", true);
>  pref("browser.sessionstore.resume_session_once", false);
> +#if defined(XP_WIN)

There's a very similar (if not identical) mechanism on OSX that I'd like to see implemented as well in the - hopefully - near future!
So that's why I think it'd be a good idea to not put this inside an ifdef block.
Also, let's try a bit more general purpose naming, like 'browser.sessionstore.resume_after_os_restart' (with to 'os' being superfluous, perhaps)?

::: toolkit/xre/nsAppRunner.cpp:4780
(Diff revision 1)
>    if (!mShuttingDown) {
>      rv = appStartup->CreateHiddenWindow();
>      NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
>  
>  #ifdef XP_WIN
> +    if (Preferences::GetBool("browser.sessionstore.resumed_for_rar", false)) {

I'd prefer to keep all this logic inside nsBrowserGlue.js, nsSessionStartup.js and SessionStore.jsm.
If you need help to find the correct place for this, please let me know!

::: widget/windows/nsWindow.cpp:5257
(Diff revision 1)
>          nsCOMPtr<nsIObserverService> obsServ =
>            mozilla::services::GetObserverService();
>          const char16_t* context = u"shutdown-persist";
>          const char16_t* syncShutdown = u"syncShutdown";
> +        const char16_t* quitType = GetQuitType();
> +        if (quitType &&

Same here, really: I'd like this logic to leave in the appropriate JS components, not here.
I'd say nsSessionStartup.js is the perfect spot.

Comment 32

[Mike de Boer [:mikedeboer]]

Comment on attachment 8971073 [details]
Bug 603903 - Part 4: don't restore tabs until session unlock

https://reviewboard.mozilla.org/r/239844/#review246368

I'd like to avoid this entirely, because it will lead to perceived performance issues: we actually have a nice window of opportunity to do work whilst the lockscreen is up. So to do this only because of the potential autoplay annoyance probably isn't worth it.
See also comment 30 - cpearce might be interested in using this property to enforce no-autoplay when the screen is locked. Because I think that would be a neat way to fix this in one fell swoop!

Comment 33

[Jim Mathies [:jimm]]

Comment on attachment 8971070 [details]
Bug 603903 - Part 1: add RegisterApplicationRestart

https://reviewboard.mozilla.org/r/239838/#review246436

::: toolkit/xre/nsAppRunner.cpp:3611
(Diff revision 1)
>  #endif
>  
>    SaveToEnv("MOZ_LAUNCHED_CHILD=");
>  
> +#ifdef XP_WIN
> +  if (CheckArg("rar-restarted", false, nullptr, true) == ARG_FOUND) {

nit - comment explaing this check. you should add something to the global up top as well.

Comment 34

[Jim Mathies [:jimm]]

Comment on attachment 8971072 [details]
Bug 603903 - Part 3: fix screen enumeration when not logged in

https://reviewboard.mozilla.org/r/239842/#review246442

::: widget/windows/ScreenHelperWin.cpp:52
(Diff revision 1)
> +  if (!hDC) {
> +    MOZ_LOG(sScreenLog, LogLevel::Error, ("CollectMonitors CreateDC failed"));
> +    return TRUE;
> +  }
> +  uint32_t pixelDepth = ::GetDeviceCaps(hDC, BITSPIXEL);
> +  DeleteDC(hDC);

Nit - we have win helper templates for managing some windows resources.

https://searchfox.org/mozilla-central/source/xpcom/base/nsWindowsHelpers.h

Comment 35

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

Thanks Mike, I'll get into the Part 2 fixes soon. I think we might want to take a closer look at part 4, besides video I imagine there's other behavior we might want to avoid doing "behind the user's back", so to speak. I understand why MS did it for apps in general but it seems like it'll be creepy when a browser starts doing this with dynamic content.

Comment 36

[Jim Mathies [:jimm]]

(In reply to Mike de Boer [:mikedeboer] from comment #32)
> Comment on attachment 8971073 [details]
> Bug 603903 - Part 4: don't restore tabs until session unlock
> 
> https://reviewboard.mozilla.org/r/239844/#review246368
> 
> I'd like to avoid this entirely, because it will lead to perceived
> performance issues: we actually have a nice window of opportunity to do work
> whilst the lockscreen is up. So to do this only because of the potential
> autoplay annoyance probably isn't worth it.
> See also comment 30 - cpearce might be interested in using this property to
> enforce no-autoplay when the screen is locked. Because I think that would be
> a neat way to fix this in one fell swoop!

Hey Daniel, we're trying to make a security decision here on how to handle the foreground tab during a forced restart. In newer versions of Windows, Windows will launch Firefox before the user logs in (but under the user account that logged off last). The foreground tab will load remote content (web extensions will also load, this is a normal session). This can result is remote content executing behind the login screen (script, audio, video, plugins, etc..).

Mike makes a good point that having this tab loaded provides a perceived perf boost on startup. But I'm concerned about remote content executing when the user does not have control over the browser. Do you have an opinion on this or suggestions on who could make a call like this? (Note this will impact 6% of our release users.)

Comment 37

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

Comment on attachment 8971071 [details]
Bug 603903 - Part 2: restore session when restarted by Windows

https://reviewboard.mozilla.org/r/239840/#review246366

> There's a very similar (if not identical) mechanism on OSX that I'd like to see implemented as well in the - hopefully - near future!
> So that's why I think it'd be a good idea to not put this inside an ifdef block.
> Also, let's try a bit more general purpose naming, like 'browser.sessionstore.resume_after_os_restart' (with to 'os' being superfluous, perhaps)?

Here's Chromium's way of detecting if it was started up via the "Reopen windows" mechanism [1].
Unlike Windows there isn't a chance to explicitly register for and pass in a unique switch so it has to fake add the switch early in startup [2].

This is a little much to include here, but I'll plan on it for the future and so use more OS-agnostic terms. Thanks!

[1] https://cs.chromium.org/chromium/src/base/mac/mac_util.mm?rcl=f4168d99006da476aa0aef04472bb463aa19b925&l=322
[2] https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main_mac.mm?rcl=f4168d99006da476aa0aef04472bb463aa19b925&l=77

Comment 38

[Jim Mathies [:jimm]]

Hey Mike,

I had a discussion with Daniel and few other security folks. The consensus was the loading of a tab behind the login screen wasn't really different from first load on the desktop. If the page is hostile the user is may have a bad time regardless.

However the idea of trying to prevent auto-play was widely panned as there no good controls for this. Also, it's anticipated that Chrome (which suffers from this) will fix it. Hence I see this as a competitive issue.

In addition, I'm very leary of having remote content running behind the login screen generally. At least with the desktop the user has the opportunity to control things, behind the login screen this is impossible.

So I would like to land the disable foreground tab loading, even though it may impact the perception of performance. This only happens during login when everything is running slower.

If perceived performance is significantly worse during testing we can reconsider, or if we ever get good media controls we could do something with those. In the mean time I would like to take the safest path here, and I think that means loading the foreground tab after the desktop session is created.

Thoughts?

Comment 39

[Daniel Veditz [:dveditz]]

(In reply to Jim Mathies [:jimm] from comment #36)
> Hey Daniel, we're trying to make a security decision here on how to handle
> the foreground tab during a forced restart. [...] This can result
> in remote content executing behind the login screen (script, audio, video,
> plugins, etc..).

I don't have a "security" concern per se: whatever gets loaded was already safe enough to load, or it wasn't and we already have a problem to fix!

My concerns would be more about user experience along the lines of what you've listed:
* annoying sounds coming from your computer
* unwanted resource consumption (coin miners churning away? can get expensive!)
* unexpected and potentially expensive bandwith wasting. Even in the US some people pay for bandwidth over a particular cap (or they get throttled) and would get annoyed if some 4K movie were streaming unwatched in the background.

Users often consider these kinds of abusive experiences to be "security" problems. At the very least we should suppress video and audio from _EVER_ starting in a window that does not have focus (as these could never have had if they're behind the lock screen).

Comment 40

[Mike de Boer [:mikedeboer]]

Alright, I don't want to block this work on the issue, since it's already quite minor and something we could tweak with relatively little effort later.
My reasoning was more focused on reducing the complexity of the code (not having to introduce the 'we're behind a lockscreen' state) and having to maintain it.

I'll re-review the patch with all this in mind. Thanks for your input & thoughts!

Comment 41

[Mike de Boer [:mikedeboer]]

Comment on attachment 8971071 [details]
Bug 603903 - Part 2: restore session when restarted by Windows

https://reviewboard.mozilla.org/r/239840/#review247484

Ah I see this patch doesn't really need adjusting, apart from the things I mentioned before of course.

::: widget/windows/nsWindow.cpp:5173
(Diff revision 1)
> +    if (rc == S_OK) {
> +      return u"restart";
> +    } else {
> +      return nullptr;
> +    }
> +  } else {

nit: you can also skip the `} else {}` and keep only `return nullptr;`.

Comment 42

[Mike de Boer [:mikedeboer]]

Comment on attachment 8971073 [details]
Bug 603903 - Part 4: don't restore tabs until session unlock

https://reviewboard.mozilla.org/r/239844/#review247488

::: browser/components/sessionstore/SessionStore.jsm:632
(Diff revision 1)
>      }, this);
>  
>      this._initPrefs();
>      this._initialized = true;
>  
> +    if (Services.appinfo.OS == "WINNT" &&



::: browser/components/sessionstore/SessionStore.jsm:632
(Diff revision 1)
>      }, this);
>  
>      this._initPrefs();
>      this._initialized = true;
>  
> +    if (Services.appinfo.OS == "WINNT" &&

For legibility and maintainability and in general for other contributors who will be reading this code later, can you make the following changes:


1. Create a getter for waitingForUnlock that you can use in other places as well:
```js
get _waitingForUnlock() {
  return AppConstants.platform == "win" &&
    Services.appinfo.QueryInterface(Ci.nsIWinAppHelper).waitingForUnlock;
}
```
2. Aways initialize 'restoreWhenUnlocked' to an array, along with renaming it:
```js
this._deferredContentRestores = [];
```

You can also cache the value of `waitingForUnlock` if it's `true` and set the cached value to `false` in the observer handler.

::: browser/components/sessionstore/SessionStore.jsm:819
(Diff revision 1)
>            userContextId = JSON.parse(aData).userContextId;
>          } catch (e) {}
>          if (userContextId)
>            this._forgetTabsWithUserContextId(userContextId);
> +        break;
> +      case "windows-session-unlocked":

nit: please put the opening brace on this line.

::: browser/components/sessionstore/SessionStore.jsm:821
(Diff revision 1)
>          if (userContextId)
>            this._forgetTabsWithUserContextId(userContextId);
> +        break;
> +      case "windows-session-unlocked":
> +      {
> +        if (this._restoreWhenUnlocked === null) {



::: browser/components/sessionstore/SessionStore.jsm:821
(Diff revision 1)
>          if (userContextId)
>            this._forgetTabsWithUserContextId(userContextId);
> +        break;
> +      case "windows-session-unlocked":
> +      {
> +        if (this._restoreWhenUnlocked === null) {

Please move this code to a dedicated method, `_triggerDeferredContentRestores` or something similar.

I'd recommend to put it below `restoreTabContent()`.

::: browser/components/sessionstore/SessionStore.jsm:824
(Diff revision 1)
> +      case "windows-session-unlocked":
> +      {
> +        if (this._restoreWhenUnlocked === null) {
> +          break;
> +        }
> +        for (let i = 0; i < this._restoreWhenUnlocked.length; i++) {

Please use a for...of loop here:
```js
for (let [browser, restoreMessage] of this._deferredContentRestores) {
  restoreMessage.requestTime = Services.telemetry.msSystemNow();
  browser.messageManager.sendAsyncMessage("SessionStore:restoreTabContent", restoreMessage);
}
```

(As you can see I don't strictly adhere to the 80ch limit... we're keep a pragmatic range between 80 and 120ch to optimize legibility.)

::: browser/components/sessionstore/SessionStore.jsm:3481
(Diff revision 1)
>        }
>  
>        // Add a new tab if needed.
>        if (!tab) {
>          let createLazyBrowser = restoreTabsLazily && !select && !tabData.pinned;
> +        let noInitialRestoreContent = false;

What if you don't do all this and move the deferred tab content restore logic to `restoreTab()`:

```js
if (willRestoreImmediately) {
  if (this._waitingForUnlock) {
    this._deferredContentRestores.push({tab, options});
  } else {
    this.restoreTabContent(tab, options);
  }
} else {
  //...
}
```
?

What do you think?

This'd change the for..of loop I suggested above to:
```js
for (let {tab, options} of this._deferredContentRestores) {
  this.restoreTabContent(tab, options);
}
```

Comment 43

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

Thanks very much for the review Mike! I'll address your concerns next week.

I finally located the official Microsoft release notes for the automatic restart feature:
https://blogs.windows.com/windowsexperience/2017/07/26/announcing-windows-10-insider-preview-build-16251-pc-build-15235-mobile/
(see heading "Improved Boot Up Experience")

Though it doesn't offer any more information than the more informal sources.

Comment 44

[Chris Peterson [:cpeterson]]

Adam, do you have an ETA for landing the Restart Manager patches? We still have over a month before Nightly 62 merges to Beta, but we'd like QA to test the feature soon because there are a lot of corner cases to cover.

Comment 45

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

I plan to have the new revision ready for review by EOB tomorrow.

Comment 46

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

As I was finishing up with rebasing and rewriting, I discovered that the OpenInputDesktop trick I used to detect pre-login no longer works as of the Windows 10 April 2018 Update [0]. So Part 4 is going to have to be reworked somehow.

The update also fixes the display enumeration issue I'm working around in Part 2, which is good [1], but that means I can't use it to detect pre-login either.

P.S. EnumDesktops has a fun bug, if you give it NULL as a Window Station handle, it will enumerate Window Stations, instead of enumerating Desktops on the current Window Station as the docs say.

[0] It does work to detect normal screen locking still, when locked the input desktop is still "Winlogon" so OpenInputDesktop fails, but pre-login it's the normal "Default" which we can get normal access to.

[1] I think I'll leave the workaround in place for users passing through Fall Creators Update, I think it's a better approach than the old way anyway.

Comment 47

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

(In reply to Adam Gashlin [:agashlin] from comment #46)
> P.S. EnumDesktops has a fun bug, if you give it NULL as a Window Station
> handle, it will enumerate Window Stations, instead of enumerating Desktops
> on the current Window Station as the docs say.

According to [1] this is actually enumerating the Session 0 Desktops.

[1] https://stackoverflow.com/a/13323175/3444805

Comment 48

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

Comment on attachment 8971070 [details]
Bug 603903 - Part 1: add RegisterApplicationRestart

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

Comment 49

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

Comment on attachment 8971071 [details]
Bug 603903 - Part 2: restore session when restarted by Windows

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

Comment 50

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

Comment on attachment 8971072 [details]
Bug 603903 - Part 3: fix screen enumeration when not logged in

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

Comment 51

[Mike de Boer [:mikedeboer]]

Comment on attachment 8971071 [details]
Bug 603903 - Part 2: restore session when restarted by Windows

https://reviewboard.mozilla.org/r/239840/#review250528

Superb, I'm really happy with this change! Thanks again for working on this :-)

Comment 52

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

Only seeing known intermittents on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=794f81184cbb770307dda7d50c853a5708d5f525

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3a1988dcf34
Part 1: add RegisterApplicationRestart r=jimm
https://hg.mozilla.org/integration/autoland/rev/6aa6a2fbe90b
Part 2: restore session when restarted by Windows r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/913e7b5bafed
Part 3: fix screen enumeration when not logged in r=jimm

Comment 54

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/e3a1988dcf34
https://hg.mozilla.org/mozilla-central/rev/6aa6a2fbe90b
https://hg.mozilla.org/mozilla-central/rev/913e7b5bafed

Comment 55

[Pascal Chevrel:pascalc (PTO until April 26)]

Is that something release notes worthy?

Comment 56

[Chris Peterson [:cpeterson]]

(In reply to Pascal Chevrel:pascalc from comment #55)
> Is that something release notes worthy?

Yes! However, this feature will ship pref'd off in 62 (so we can run an A/B study) and will then be enabled in 63. When we file a bug to enable this feature in 63, I'll tag that bug for relnote-firefox.

Comment 57

[Pascal Chevrel:pascalc (PTO until April 26)]

(In reply to Chris Peterson [:cpeterson] from comment #56)
> (In reply to Pascal Chevrel:pascalc from comment #55)
> > Is that something release notes worthy?
> 
> Yes! However, this feature will ship pref'd off in 62 (so we can run an A/B
> study) and will then be enabled in 63. When we file a bug to enable this
> feature in 63, I'll tag that bug for relnote-firefox.

Will it be preffed off on pre-release channels as well? (I am thinking of a potential addition to Nightly and Beta release notes)

Comment 58

[Chris Peterson [:cpeterson]]

(In reply to Pascal Chevrel:pascalc from comment #57)
> Will it be preffed off on pre-release channels as well? (I am thinking of a
> potential addition to Nightly and Beta release notes)

The feature is currently preffed on for Nightly (only) by the following "toolkit.winRegisterApplicationRestart" pref. If we have Nightly-only release notes, then this feature would be relevant!

Comment 59

[Pascal Chevrel:pascalc (PTO until April 26)]

(In reply to Chris Peterson [:cpeterson] from comment #58)
> (In reply to Pascal Chevrel:pascalc from comment #57)
> > Will it be preffed off on pre-release channels as well? (I am thinking of a
> > potential addition to Nightly and Beta release notes)
> 
> The feature is currently preffed on for Nightly (only) by the following
> "toolkit.winRegisterApplicationRestart" pref. If we have Nightly-only
> release notes, then this feature would be relevant!

Yes we have nightly-only notes (and the notes for Nightly are not used automatically to beta notes, release managers only copy the relevant ones).

I checked and I don't have this key in Nightly so I guess that this is on but not visible yet in about:config.

Do you have a wording suggestion for the Nightly notes for this feature?

Comment 60

[Chris Peterson [:cpeterson]]

(In reply to Pascal Chevrel:pascalc from comment #59)
> I checked and I don't have this key in Nightly so I guess that this is on
> but not visible yet in about:config.

This pref landed last week ago, so it should be visible in recent Windows Nightly builds.

> Do you have a wording suggestion for the Nightly notes for this feature?

Release Note Request (optional, but appreciated)

[Why is this notable]: Restarting Firefox after Windows reboot should help users resume their work more quickly and prevent their session from being lost by automatic Windows Update reboot.

[Affects Firefox for Android]: No. This feature is currently limited to Firefox 62+ Nightly only on Windows.

[Suggested wording]:

If Firefox Nightly is running when Windows shuts down (by manual user shutdown or automatic Windows Update reboot), your Firefox browser session will be automatically restored when you login after reboot. You can disable this behavior by setting the about:config pref "toolkit.winRegisterApplicationRestart" to false.

[Links (documentation, blog post, etc)]:

Comment 61

[Pascal Chevrel:pascalc (PTO until April 26)]

Thanks, note added to Nightly notes with this wording:
If Firefox Nightly is running when Windows shuts down (by manual user shutdown or automatic Windows Update reboot), your browser session will be automatically restored when you login after reboot

I am leaving the relnote-firefox? flag so as to keep that on our radar for the 63 cycle.

Comment 62

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

(In reply to Pascal Chevrel:pascalc from comment #61)
> If Firefox Nightly is running when Windows shuts down (by manual user
> shutdown or automatic Windows Update reboot), your browser session will be
> automatically restored when you login after reboot

Minor note: This only applies to manual user shutdowns since Windows 10 Fall Creators Update.

Thanks!

Comment 63

[Chris Peterson [:cpeterson]]

(In reply to Adam Gashlin [:agashlin] from comment #62)
> (In reply to Pascal Chevrel:pascalc from comment #61)
> > If Firefox Nightly is running when Windows shuts down (by manual user
> > shutdown or automatic Windows Update reboot), your browser session will be
> > automatically restored when you login after reboot
> 
> Minor note: This only applies to manual user shutdowns since Windows 10 Fall
> Creators Update.

Adam, is this a behavior change since comment 9 about *all* restarts?

(In reply to Adam Gashlin [:agashlin] from comment #9)
> The behavior of RegisterApplicationRestart used to be (and still is
> described in the docs) that it would only restart registered apps on restart
> for updates or installs. However as of Windows 10 Fall Creator's Update this
> now applies to *all* restarts.

This Microsoft Answers link from comment 10 seems to imply that *all* applications will be restarted automatically, not just those that called RegisterApplicationRestart:

> - When shutting down your PC, any open apps are "bookmarked" (for lack of a better word)
> - After reboot/restart, these apps will re-open automatically

https://answers.microsoft.com/en-us/insider/forum/insider_wintp-insider_desktop/programs-autostart-after-boot-in-windows-10/09dd8d3e-7b36-45d1-9181-6587dd5d53ab?page=2

It might be helpful if we created a spreadsheet with a matrix of what happens:

* in different Windows versions
* for different types of shutdowns or update reboots
* for applications that did or did not call RegisterApplicationRestart

Comment 64

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

(In reply to Chris Peterson [:cpeterson] from comment #63)
> (In reply to Adam Gashlin [:agashlin] from comment #62)
> > (In reply to Pascal Chevrel:pascalc from comment #61)
> > > If Firefox Nightly is running when Windows shuts down (by manual user
> > > shutdown or automatic Windows Update reboot), your browser session will be
> > > automatically restored when you login after reboot
> > 
> > Minor note: This only applies to manual user shutdowns since Windows 10 Fall
> > Creators Update.
> 
> Adam, is this a behavior change since comment 9 about *all* restarts?

Sorry, the "only" was unclear:
- Before Win10 FCU this applied only to automatic update reboots (and some non-Windows installers)
- As of Win10 FCU it now applies to all reboots (automatic update/install and manual)

> (In reply to Adam Gashlin [:agashlin] from comment #9)
> > The behavior of RegisterApplicationRestart used to be (and still is
> > described in the docs) that it would only restart registered apps on restart
> > for updates or installs. However as of Windows 10 Fall Creator's Update this
> > now applies to *all* restarts.
> 
> This Microsoft Answers link from comment 10 seems to imply that *all*
> applications will be restarted automatically, not just those that called
> RegisterApplicationRestart:
> 
> > - When shutting down your PC, any open apps are "bookmarked" (for lack of a better word)
> > - After reboot/restart, these apps will re-open automatically
> 
> https://answers.microsoft.com/en-us/insider/forum/insider_wintp-
> insider_desktop/programs-autostart-after-boot-in-windows-10/09dd8d3e-7b36-
> 45d1-9181-6587dd5d53ab?page=2

I believe that the author is misinformed or he is being imprecise to simplify things. In all other sources [1] [2] RegisterApplicationRestart is explicitly mentioned. This is consistent with the behavior I've observed, without registering there is no automatic restart.

> It might be helpful if we created a spreadsheet with a matrix of what
> happens:
> 
> * in different Windows versions
> * for different types of shutdowns or update reboots
> * for applications that did or did not call RegisterApplicationRestart

My best understanding of this is in a glorious ASCII art matrix in bug 1464146 comment 1, again this only applies to applications registered with RegisterApplicationRestart.

[1] https://answers.microsoft.com/en-us/windows/forum/windows_10-other_settings/apps-reopening-on-start-up-after-shutdownrestart/b6d6b6b1-dc08-4ecd-aad7-088eb7cb723e (search "PaulSey")
[2] https://blogs.windows.com/windowsexperience/2017/07/26/announcing-windows-10-insider-preview-build-16251-pc-build-15235-mobile/ (search "Improved Boot Up Experience")

Comment 66

[Liz Henry (:lizzard) (relman/hg->git project)]

As I understand it, we aren't shipping this to beta 62, but there will be shield studies to test it. Is that correct?  If so, I'm not including it in release notes for 62. Thanks!

Comment 67

[Chris Peterson [:cpeterson]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #66)
> As I understand it, we aren't shipping this to beta 62, but there will be
> shield studies to test it. Is that correct?  If so, I'm not including it in
> release notes for 62. Thanks!

Correct. We should list this in the release notes for 63, not 62. This code will ship pref'd off in 62. We will run some Shield studies in Release 62 and then pref on in Nightly 63 to ship.

Comment 68

[Chris Peterson [:cpeterson]]

We plan to let this feature ride the trains with 63, so we should add a real relnote for 63 release. This feature landed in 62 Nightly and we mentioned it in the nightly relnotes here:

https://www.mozilla.org/en-US/firefox/62.0a1/releasenotes/

63 Release Note Request

[Why is this notable]: Restarting Firefox after Windows reboot should help users resume their work more quickly and prevent their session from being lost by automatic Windows Update reboot.

[Affects Firefox for Android]: No. This feature is currently limited to Windows, though it may be extended to macOS later.

[Suggested wording]:

If Firefox Nightly is running when Windows shuts down (by manual user shutdown or automatic Windows Update reboot), your browser session will be automatically restored when you login after reboot.

[Links (documentation, blog post, etc)]: None

Comment 69

[Chris Peterson [:cpeterson]]

Ryan, we should relnote this bug for the 61.0.2 dot release! We had planned to let this feature ride the trains with 63, but we've fast tracked this feature to be rolled out in 61.0.2.

61.0.2 Release Note Request

[Why is this notable]: Restarting Firefox after Windows reboot should help users resume their work more quickly and prevent their session from being lost by automatic Windows Update reboot.

[Affects Firefox for Android]: No. This feature is currently limited to Windows, though it may be extended to macOS later (in bug    1326181).

[Suggested wording]:

Firefox will automatically restore your tabs after Windows OS reboot. To disable this feature, set the "toolkit.winRegisterApplicationRestart" pref to false.

[Links (documentation, blog post, etc)]: None

Comment 70

[Chris Peterson [:cpeterson]]

Ryan, I'm no longer sure if a 61.0.2 relnote is appropriate. This feature will ship pref'd OFF in 61.0.2 and then be unthrottled for all users over a few weeks. So 61.0.2's relnotes are a little too early and 62's are too late. I think 61.0.2 is probably better than no relnote at all.

Comment 71

[Ryan VanderMeulen [:RyanVM]]

Fixed on 61 by way of bug 1480612.

Comment 73

[Mike de Boer [:mikedeboer]]

I just duped bug 1498167 to here and this made me wonder - with my Session Restore module owner hat on - why is this being tested/ rolled out via Normandy at all when - from my point of view - is such a big platform integration and dataloss remediation win?

I kept silent with this concern for a long while, hoping to hear/ read something that resembled progress as a result from the pref-flip study, but that time has passed, I think.

Let me put this quite bluntly: Firefox sucks at integrating with platform relaunch-on-restart handles. This bug was reported eight years ago and so was bug 639707 for OSX. Our story on OSX sucks even more: we're the only app NOT being restarted at all on restart.

Please skip the pref-flipping and ship this to our users asap.

Comment 74

[Mike de Boer [:mikedeboer]]

Or perhaps my concern is invalid and we've shipped with `toolkit.winRegisterApplicationRestart` set to TRUE already for all and the Normandy stuff was there to get it to our users _faster_ instead?

Comment 75

[Winson Wen]

Hi, I'm the reporter of the duped bug. I just checked  and "toolkit.winRegisterApplicationRestart" was already set to true, so it should be shipped already.

That bug was filled for it failing to work for some reason.

Comment 76

[Ryan VanderMeulen [:RyanVM]]

Yep, it shipped enabled by default in Fx62. Only Fx61 had a staged rollout via Normandy.

Comment 77

[Chris Peterson [:cpeterson]]

(In reply to Mike de Boer [:mikedeboer] from comment #73)
> Please skip the pref-flipping and ship this to our users asap.

This feature shipped enabled by default in 62.

(In reply to Mike de Boer [:mikedeboer] from comment #74)
> Or perhaps my concern is invalid and we've shipped with
> `toolkit.winRegisterApplicationRestart` set to TRUE already for all and the
> Normandy stuff was there to get it to our users _faster_ instead?

The Firefox product management team wanted to use the rollout to measure how much (if at all) Windows Restart Manager increased usage. There were some statistical problems with the experiment design in 61, so we just shipped it to everyone without delay in 62.

There is an experiment in the pipeline to measure whether enabling Session Restore by default improves retention and usage. If you'd like to accelerate that experiment, I recommend contacting Romain Testard on the Firefox product management team.

Comment 78

[Mike de Boer [:mikedeboer]]

Thanks for the info, Chris - I think you already guessed, but my agitated tone setting here was based on misconception, my apology for that.

I think that there's value in that experiment, BUT I think there's perhaps even more value in a systematic 'rethinking' to our session restore behavior across platforms. I'll try to reach out to :RT about this, but I'm also still collecting my thoughts here.
One aspect here, for example, is not to test auto-restore-on-startup ON/OFF, but start with _always_ loading the previous session in the background and then, based on the expectation set by the startup context (system restart after update, re-login of user, etc) to decide whether to restore windows and tabs.

But that's a discussion that doesn't belong in a RESOLVED bug. ;-)

Comment 79

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

Comment on attachment 8971072 [details]
Bug 603903 - Part 3: fix screen enumeration when not logged in

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This patch (part 3) fixes bug 1533894, which is a major regression for Citrix since ESR52, caused by bug 1356218.
• User impact if declined: Citrix customers may not be able to update to ESR60, as menus don't work.
• Fix Landed on Version: 61
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Patch has been out for several releases with no issues, it is a small change to specifically to work around such issues with non-logged-in or locked desktops.
• String or UUID changes made by this patch: none

Comment 80

[Ryan VanderMeulen [:RyanVM]]

Can we attach just the part 3 patch to bug 1533894 and request approval there? I really don't like do a cherry-picking uplift from a bug like this because it makes status tracking annoying. Note that it needs rebasing anyway.

Comment 81

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

Rebased patch submitted on bug 1533894.