Open Bug 1827651 Opened 1 year ago Updated 10 days ago

macOS resume session support doesn't seem to work

Categories

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

Product:
Core
Component:
Widget: Cocoa
Version:
Firefox 114
Platform:
Desktop
macOS
Type:
defect

Tracking

()

Status:
ASSIGNED

People

(Reporter: sam, Assigned: KrisWright)

References

Details

Attachments

(3 files)

Sample Code For Launching App
2.35 KB, application/octet-stream
Details
Bug 1827651 - Update launch code to use nondeprecated APIs.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1827651 - Disable resume session supported API in marionette testing
48 bytes, text/x-phabricator-request
Details | Review

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.

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"

Flags: needinfo?(sam)
Severity: -- → S3
Priority: -- → P3

(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

Flags: needinfo?(sam)

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.

Flags: needinfo?(spohl.mozilla.bugs)

(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

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.

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

(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.

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?

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.

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...

Depends on: 639707
Status: UNCONFIRMED → NEW
Ever confirmed: true

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?

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

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.

Flags: needinfo?(sam)
Flags: needinfo?(hskupin)

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.

Flags: needinfo?(sam)

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.

Flags: needinfo?(hskupin)
See Also: → 1250901

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.

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.

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.

Assignee: nobody → kwright
Status: NEW → ASSIGNED
Attachment #9349975 - Attachment description: Bug 1827651 - Update launch code to use nondeprecated libraries. → Bug 1827651 - Update launch code to use nondeprecated APIs.
Pushed by kwright@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8ef3d7a6294
Update launch code to use nondeprecated APIs. r=spohl

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.

Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(kwright)
Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(kwright)
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #9349975 - Attachment description: Bug 1827651 - Update launch code to use nondeprecated APIs. → Bug 1827651 - Update launch code to use OpenApplicationAtURL where applicable
Attachment #9349975 - Attachment description: Bug 1827651 - Update launch code to use OpenApplicationAtURL where applicable → Bug 1827651 - Update launch code to use nondeprecated APIs.
No longer blocks: 1847347

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

Flags: needinfo?(hskupin)

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?

Flags: needinfo?(hskupin)

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.

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.

Depends on: 1884401

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

(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.

Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: