1827651 - macOS resume session support doesn't seem to work

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P3)

Description

[Sam Johnson]

After bug 639707, it is my understanding that Firefox should automatically restart itself if it was open when the OS restarted for an update. I use Nightly, and for both the macOS 13.3 update, and the 13.3.1 updates, I have purposefully left Nightly open thinking it would auto restart after the update. However, that has not happened in my experience. Other apps, such as Slack, do re-open themselves.

Comment 1

[Stephen A Pohl [:spohl]]

Telemetry seems to indicate that this works at least "sometimes". Now we need to figure out when this does not work and fix those situations. Could you tell us what you have set for the following settings in about:preferences?

"Open previous windows and tabs"
"Confirm before closing multiple tabs"
"Confirm before quitting with ⌘Q"

Comment 2

[Sam Johnson]

(In reply to Stephen A Pohl [:spohl] from comment #1)

Telemetry seems to indicate that this works at least "sometimes". Now we need to figure out when this does not work and fix those situations. Could you tell us what you have set for the following settings in about:preferences?

Sure!

"Open previous windows and tabs": Enabled
"Confirm before closing multiple tabs": Disabled
"Confirm before quitting with ⌘Q": Disabled

Comment 3

[Stephen A Pohl [:spohl]]

I'm going to try and gather more info about the systems that seem to successfully use this feature using telemetry. However, I need to wrap up a few other tasks first. Setting n-i to keep this on my radar.

Comment 4

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

(In reply to Sam Johnson from comment #2)

"Open previous windows and tabs": Enabled
"Confirm before closing multiple tabs": Disabled
"Confirm before quitting with ⌘Q": Disabled

I've seen the same today and my settings are the following:

• "Open previous windows and tabs": Enabled
• "Confirm before closing multiple tabs": Disabled
• "Confirm before quitting with ⌘Q": Enabled

Comment 5

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

I've restarted again with a build as started via mach run so that it has all the default preferences. And even here it is not working. The instance doesn't get reopened after a restart.

Comment 6

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

Btw. note that Thunderbird actually perfectly gets reopened after the restart, and that already for some time.

Comment 7

[Stephen A Pohl [:spohl]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #5)

I've restarted again with a build as started via mach run so that it has all the default preferences. And even here it is not working. The instance doesn't get reopened after a restart.

I don't believe that mach run will be a reliable way to test this feature. The application should be launched from the dock before testing.

Comment 8

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

I see. But creating a dock icon for the eg. artifact build and starting it by clicking on that icon should make it work, right?

Comment 9

[Stephen A Pohl [:spohl]]

We don't know in what ways application identifiers may or may not interfere with this feature at the moment. It is possible that only officially branded builds work. It is also possible that only builds with the "firefox" identifier might work, rather than "nightly". Telemetry seems to indicate that it works for both, but we haven't been able to narrow down what parameters make this work and when it doesn't.

Comment 10

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

I tried with a release build of Firefox 113 and it doesn't work as well. It's not getting reopened after the startup. Note that Thunderbird works fine for already quite some time...

Comment 11

[Sam Johnson]

I just noticed this again with the 13.4 update. Is there some logging I can produce before initiating the next macOS update that would be useful for identifying the issue?

Comment 12

[Sam Johnson]

Reporting back in that Nightly did not resume after the 13.5 update either.

Comment 13

[Haik Aftandilian [:haik]]

This might be a factor. @Sam and @Henrik, do you launch the browser to the profile manager to start it? i.e., you do not have a default profile that gets used by default when starting the browser.

It's unofficially documented that you can see the list of apps which will be restarted by running

$ defaults -currentHost read com.apple.loginwindow TALAppsToRelaunchAtLogin

or by looking at the loginwindow's plist file with $ plutil -p ~/Library/Preferences/ByHost/com.apple.loginwindow.<UUID>.plist

And I noticed that Firefox doesn't get put in the list if I launch it from the dock and select my profile. If I launch it and provide the profile on the command line using the open command, it does get added to the list. Note, the open command is a way to launch/open things from the command line that is meant to be equivalent to launching/opening through the finder. You can do this with a Firefox repo using $ ./mach run --macos-open.

So this $ open /Applications/Firefox.app/ --args -P MyReleaseProfile populates the file with Firefox.

But this $ open /Applications/Firefox.app/ does not. I have several profiles and none of them are set to be the default.

Comment 14

[Sam Johnson]

I have a default profile and do not launch to the profile manager. However, since I use Nightly, ~every launch of the browser is from the update notification, so I am not using the dock icon either.

When I ran defaults -currentHost read com.apple.loginwindow TALAppsToRelaunchAtLogin, Nightly was not in the list of apps, and it was last started via the update notification. Quitting Nightly and then opening it via the dock icon caused it to appear in the list.

Comment 15

[Haik Aftandilian [:haik]]

Thanks, Sam.

After digging into this a bit more, I've found that the method we use to relaunch the browser after an update [NSTask launchedTaskWithLaunchPath:<path to the firefox executable> ...] does not result in Firefox being added to the resume list described in comment 13. This method is deprecated as of macOS 14. The call site in updater code is here and there may be other sites.

Using the newer [[NSWorkspace sharedWorkspace] openApplicationAtURL:<path to Firefox.app> ...] does result in the browser being added to the resume list. This method is available on 10.15+ which is now the minimum supported OS version for Release. With openApplicationAtURL, we shouldn't need to use the arch utility to launch because openApplicationAtURL uses the application's preferred architecture by default.

Assuming adopting openApplicationAtURL is the fix we use, we will want to find all the call sites where we launch the browser and change them to use the new method. Launches after updates and after profile-selection are the two that seem highest priority.

Comment 16

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

I'm always opening Firefox via the dock and yes the profile manager is always in use. This also explains comment 10 given that I have only a single profile for Thunderbird.

Comment 17

[Haik Aftandilian [:haik]]

Attachment: Sample Code For Launching App

Comment 18

[Haik Aftandilian [:haik]]

Additional note for anyone testing this: if an update is pending, launching from the Finder/dock will not result in the resume list being populated because the browser will be restarted at some point before becoming usable.

Comment 19

[Kris Wright :KrisWright]

Attachment: Bug 1827651 - Update launch code to use nondeprecated APIs.

While this patch updates the launch code, it cannot be run on sdk versions below 10.15. In our code we still have a minimum requirement of 10.12, so to make this work we check for our sdk version using the existing @available objc call. Some changes have to be made to the way we launch to accommodate the new method, noted below.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8ef3d7a6294
Update launch code to use nondeprecated APIs. r=spohl

Comment 21

[Noemi Erli[:noemi_erli]]

Backed out changeset f8ef3d7a6294 (Bug 1827651) for causing NSWorkspace related assertions CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=f8ef3d7a629425f2db02f9c97e69c1b775881b73&selectedTaskRun=YMy3lTpbS8yxIRuQEj97Bg.0

Log: https://treeherder.mozilla.org/logviewer?job_id=429834317&repo=autoland&lineNumber=19075

Backout: https://hg.mozilla.org/integration/autoland/rev/c10a06cd7599f92aa4c8d3521b45f17906810138

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:KrisWright, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 23

[Stephen A Pohl [:spohl]]

Henrik and I briefly discussed try failures on Matrix. Adding n-i to follow up next week.

Comment 24

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

I had a look at the test TestQuitRestart.test_in_app_restart_component_prevents_shutdown as pointed out by Stephen on Matrix. When I compared the log output by running mach marionette-test -vv --gecko-log - --setpref="remote.log.truncate=true" testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py with just this test enabled I can see that the registered quit-application-requested observer is no longer able to abort a quit request (returning true), but instead Firefox just quits:

New behavior:

1709726194145	Marionette	DEBUG	2 -> [0,15,"WebDriver:ExecuteScript",{"script":"Services.obs.addObserver(subject => {\n                  let cancelQuit = subject.QueryInterface(Ci.nsISupportsPRBool);\n                  cancelQuit.data = true;\n                }, \"quit-application-requested\");","args":[],"newSandbox":true,"sandbox":"default","line":231,"filename":"testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py"}]
1709726194146	Marionette	DEBUG	2 <- [1,15,null,{"value":null}]
1709726194146	Marionette	DEBUG	2 -> [0,16,"Marionette:SetContext",{"value":"content"}]
1709726194146	Marionette	DEBUG	2 <- [1,16,null,{"value":null}]
1709726194146	Marionette	DEBUG	2 -> [0,17,"Marionette:GetContext",{}]
1709726194147	Marionette	DEBUG	2 <- [1,17,null,{"value":"content"}]
1709726194147	Marionette	DEBUG	2 -> [0,18,"Marionette:AcceptConnections",{"value":false}]
1709726194147	Marionette	INFO	Stopped listening on port 2828
1709726194147	Marionette	DEBUG	2 <- [1,18,null,{"value":null}]
1709726194147	Marionette	DEBUG	2 -> [0,19,"Marionette:Quit",{"flags":["eRestart"]}]
1709726194425	Marionette	TRACE	[2] MarionetteCommands actor destroyed for window id 4
1709726194462	Marionette	TRACE	Received observer notification quit-application
1709726194466	Marionette	TRACE	Received observer notification quit-application
1709726194468	Marionette	DEBUG	Marionette stopped listening
1709726194468	Marionette	DEBUG	2 <- [1,19,null,{"cause":"restart","forced":true,"in_app":true}]

Current behavior:

1709726323700	Marionette	DEBUG	2 -> [0,15,"WebDriver:ExecuteScript",{"script":"Services.obs.addObserver(subject => {\n                  let cancelQuit = subject.QueryInterface(Ci.nsISupportsPRBool);\n                  cancelQuit.data = true;\n                }, \"quit-application-requested\");","args":[],"newSandbox":true,"sandbox":"default","line":231,"filename":"testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py"}]
1709726323700	Marionette	DEBUG	2 <- [1,15,null,{"value":null}]
1709726323701	Marionette	DEBUG	2 -> [0,16,"Marionette:SetContext",{"value":"content"}]
1709726323701	Marionette	DEBUG	2 <- [1,16,null,{"value":null}]
1709726323701	Marionette	DEBUG	2 -> [0,17,"Marionette:GetContext",{}]
1709726323701	Marionette	DEBUG	2 <- [1,17,null,{"value":"content"}]
1709726323701	Marionette	DEBUG	2 -> [0,18,"Marionette:AcceptConnections",{"value":false}]
1709726323701	Marionette	INFO	Stopped listening on port 2828
1709726323701	Marionette	DEBUG	2 <- [1,18,null,{"value":null}]
1709726323701	Marionette	DEBUG	2 -> [0,19,"Marionette:Quit",{"flags":["eRestart"]}]
1709726323915	Marionette	TRACE	[2] MarionetteCommands actor destroyed for window id 4
1709726323930	Marionette	TRACE	Received observer notification quit-application
1709726323933	Marionette	TRACE	Received observer notification quit-application
1709726323934	Marionette	DEBUG	Marionette stopped listening
1709726323934	Marionette	DEBUG	2 <- [1,19,null,{"cause":"restart","forced":true,"in_app":true}]

So it looks like that we no longer check the return value of the observer and cancel a quit request?

Comment 25

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

I was probably wrong with the above statement. The difference that I can see is that with the new behavior the process might be differently forked and we maybe fail to track the correct process? Only for builds with the patches applied I can see the following output:

Child process with id "53717" has been marked as detached because it is no longer in the managed process group. Keeping reference to the process id "53715" which is the new child process.

Let me take a further look.

Comment 26

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

The actual problem is that the process mozrunner is tracking ends with an exit code of 0 and not stays running and reports None here:
https://searchfox.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#1359-1360

As such while a restarted process of Firefox is present we assume no such process is running and start another process.

Not sure if a restart could be done so that the original process still stays around and we can successfully check for the exit code of None, or if Marionette, mozrunner, or even mozprocess needs to be updated.

Comment 27

[Kris Wright :KrisWright]

Attachment: Bug 1827651 - Disable resume session supported API in marionette testing

Disables use of LaunchApplicationAtUrl(...) for marionette testing to limit risk of bug 1884401.

Comment 28

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

(In reply to Kris Wright :KrisWright from comment #27)

Created attachment 9391468 [details]
Bug 1827651 - Disable resume session supported API in marionette testing

Disables use of LaunchApplicationAtUrl(...) for marionette testing to limit risk of bug 1884401.

That bug is fixed now. So please let us know if something else is needed like not loosing the stdout / stderr output from Firefox.

Comment 29

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 30

[Stephen A Pohl [:spohl]]

Attachment: Bug 1827651: Switch from NSTask to NSWorkspace APIs to launch our callback apps to get macOS session resume support. Based on an initial patch by :kriswright.

Comment 31

[Stephen A Pohl [:spohl]]

Attachment: Bug 1827651: Make update tests compatible with the NSWorkspace API for launching callback apps on macOS.

Depends on D217954

Comment 32

[Stephen A Pohl [:spohl]]

We're going to want QA test coverage of this before rolling it out, due to the risk of breaking updates. Robin, do we still use one of our development branches for this? Pine, Oak or similar?

A few things that come to mind that are worth testing (this list is preliminary and shouldn't be viewed as final):

1. Updates from before this change to a build with this change.
2. Updates from a build with this change to another build with this change.
3. Elevated updates.
4. DMG installs.
5. Elevated DMG installs.
6. .pkg installs & subsequent updates.
7. Ensure that macOS session resume now works, both when Firefox is started from the Dock AND after Firefox was restarted after an update.

Comment 33

[Stephen A Pohl [:spohl]]

Green try run: https://treeherder.mozilla.org/jobs?repo=try&revision=e396b90ec0b4d4de1be62c8146c6b549cd9ef7f9

Comment 34

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

Stephen, given the former issues with Marionette and restart tests, how does it work now with your recent changes? I wonder if the psutils approach is still needed.

Comment 35

[Stephen A Pohl [:spohl]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #34)

Stephen, given the former issues with Marionette and restart tests, how does it work now with your recent changes? I wonder if the psutils approach is still needed.

All tests are now passing with the current state of Marionette. I am not sure whether or not the psutils is still needed. Is there a way for me to check? (I would have need-info'd you, but you have n-i requests blocked).

Comment 36

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

(In reply to Stephen A Pohl [:spohl] from comment #35)

All tests are now passing with the current state of Marionette. I am not sure whether or not the psutils is still needed. Is there a way for me to check?

Yeah, you can test by applying these steps:

1. Comment out the usage of the custom process handler
2. Comment out most of the code for updating the process after restart

What specifically fixed the usage of the NSWorkspace API which was failing before even with the psutil based process handler?

(I would have need-info'd you, but you have n-i requests blocked).

Oh thanks. Totally forgot to remove that earlier today now that I'm back.

Comment 37

[Stephen A Pohl [:spohl]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #36)

1. Comment out the usage of the custom process handler
2. Comment out most of the code for updating the process after restart

I have kicked off a try build with these changes commented out to see where we stand: https://treeherder.mozilla.org/jobs?repo=try&revision=6c677f8ff8b7b650219b0c0f6aa3697a9dab2702

What specifically fixed the usage of the NSWorkspace API which was failing before even with the psutil based process handler?

As you can see from the patches (and how long it took to write them), the number of changes, fixes and test updates were innumerable and I can't specifically point to one change in particular that fixed this.

Comment 38

[Robin Steuber (she/her) [:bytesized]]

(In reply to Stephen A Pohl [:spohl] from comment #32)

We're going to want QA test coverage of this before rolling it out, due to the risk of breaking updates. Robin, do we still use one of our development branches for this? Pine, Oak or similar?

The Pine branch is reserved for testing update related things. Generally you should ask in #desktop-integrations on Slack if anyone else is using it, though I don't think anyone is currently.

Comment 39

[Stephen A Pohl [:spohl]]

(In reply to Robin Steuber (they/them) [:bytesized] from comment #38)

(In reply to Stephen A Pohl [:spohl] from comment #32)

We're going to want QA test coverage of this before rolling it out, due to the risk of breaking updates. Robin, do we still use one of our development branches for this? Pine, Oak or similar?

The Pine branch is reserved for testing update related things. Generally you should ask in #desktop-integrations on Slack if anyone else is using it, though I don't think anyone is currently.

Thank you! I'll do so once we have r+'s on the patches and I'll request QA coverage at the same time.

Comment 40

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

(In reply to Stephen A Pohl [:spohl] from comment #37)

I have kicked off a try build with these changes commented out to see where we stand: https://treeherder.mozilla.org/jobs?repo=try&revision=6c677f8ff8b7b650219b0c0f6aa3697a9dab2702

As it can be seen the related Marionette tests involving restarts are failing. So it is most likely required to keep using the psutil approach which is good to see that is wasn't unnecessary work on my side.

As you can see from the patches (and how long it took to write them), the number of changes, fixes and test updates were innumerable and I can't specifically point to one change in particular that fixed this.

I haven't checked the patches so no worries if you cannot tell the details. I thought that I would just ask in case it is easy to explain. But good to see that you got the NSWorkspace API working!

Comment 41

[Stephen A Pohl [:spohl]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #40)

(In reply to Stephen A Pohl [:spohl] from comment #37)

I have kicked off a try build with these changes commented out to see where we stand: https://treeherder.mozilla.org/jobs?repo=try&revision=6c677f8ff8b7b650219b0c0f6aa3697a9dab2702

As it can be seen the related Marionette tests involving restarts are failing. So it is most likely required to keep using the psutil approach which is good to see that is wasn't unnecessary work on my side.

Let me confirm this, because this might be failing due to a test that I tried to reenable in bug 1910401.

Even if the switch to psutils does not end up being necessary anymore, it did allow me to debug these tests and isolate all the issues that prevented the test runs to succeed, so I appreciate you helping us out.

Comment 42

[Stephen A Pohl [:spohl]]

(In reply to Stephen A Pohl [:spohl] from comment #41)

Let me confirm this, because this might be failing due to a test that I tried to reenable in bug 1910401.

This does indeed look like it can be reverted: https://treeherder.mozilla.org/jobs?repo=try&revision=eb7a0ec80f66aff2b2afdc7f83f7df39aebbb29f

Again, thank you for your help. Without the switch to psutils, even just temporarily, this would have been difficult to isolate.

Comment 43

[Stephen A Pohl [:spohl]]

I'm going to land this on Pine first to do some preliminary testing. I have also filed a QA request to get proper test coverage (https://mozilla-hub.atlassian.net/browse/QA-2567).

Comment 44

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

(In reply to Stephen A Pohl [:spohl] from comment #42)

This does indeed look like it can be reverted: https://treeherder.mozilla.org/jobs?repo=try&revision=eb7a0ec80f66aff2b2afdc7f83f7df39aebbb29f

I finally have taken a look and I don't think that we can remove the usage of psutil. There are permanent test failures for Mn jobs which are testing the restart feature.

Comment 45

[Stephen A Pohl [:spohl]]

The sanity testing on Pine by QA was successful. Queued for landing to m-c.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a006e9c93f96
Switch from NSTask to NSWorkspace APIs to launch our callback apps to get macOS session resume support. Based on an initial patch by :kriswright. r=application-update-reviewers,bytesized,nalexander,bradwerth
https://hg.mozilla.org/integration/autoland/rev/3dd77a128905
Make update tests compatible with the NSWorkspace API for launching callback apps on macOS. r=application-update-reviewers,bytesized

Comment 47

[Serban Stanca [:SerbanS]]

Backed out for causing TestAUSHelper related bustages.

• Backout link
• Push with failures
• Failure Log

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6020af95de1
Switch from NSTask to NSWorkspace APIs to launch our callback apps to get macOS session resume support. Based on an initial patch by :kriswright. r=application-update-reviewers,bytesized,nalexander,bradwerth
https://hg.mozilla.org/integration/autoland/rev/e6a9e84f78d9
Make update tests compatible with the NSWorkspace API for launching callback apps on macOS. r=application-update-reviewers,bytesized

Comment 49

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/c6020af95de1
https://hg.mozilla.org/mozilla-central/rev/e6a9e84f78d9

Comment 50

[Norisz Fay [:noriszfay]]

Backed out as requested by developer

Backout link

Comment 51

[Stephen A Pohl [:spohl]]

Attachment: Bug 1827651: Prevent duplicate Dock icons when restarting applications on macOS. r=#mac-reviewers,bytesized

Comment 52

[Phabricator Automation]

Comment on attachment 9425077 [details]
Bug 1827651: Prevent duplicate Dock icons when restarting applications on macOS. r=#mac-reviewers,bytesized

Revision D222326 was moved to bug 1914807. Setting attachment 9425077 [details] to obsolete.

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65767b4ca97d
Switch from NSTask to NSWorkspace APIs to launch our callback apps to get macOS session resume support. Based on an initial patch by :kriswright. r=application-update-reviewers,bytesized,nalexander,bradwerth
https://hg.mozilla.org/integration/autoland/rev/fa673b914505
Make update tests compatible with the NSWorkspace API for launching callback apps on macOS. r=application-update-reviewers,bytesized

Comment 54

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/65767b4ca97d
https://hg.mozilla.org/mozilla-central/rev/fa673b914505

Comment 55

[Stephen A Pohl [:spohl]]

Release Note Request (optional, but appreciated)
[Why is this notable]:

This addresses a long-standing inconsistency in behavior of Firefox on macOS. The expectation is for Firefox to restart after an OS restart (for example due to an OS update). However, Firefox would frequently fail to restart. This was due to the fact that if a Firefox update was applied, the updater used to restart Firefox using an API that was not supported by macOS session resume.

[Suggested wording]:

We have improved our support for macOS session resume. More specifically, this means that Firefox will now restart as expected if it was running before macOS restarted, for example due to an OS update.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

Added to the Fx132 relnotes.