1000473 - Nightly is called "Updated" in OS X Notifications

Status: RESOLVED WORKSFORME

(Toolkit :: Application Update, defect)

Description

[Ryan Feeley [:rfeeley]]

Attachment: updated.png

According to rstrong, OS X detecting the update staging directory during an update, and setting the name of Nightly as Updated.

Comment 1

[Robert Strong (they/them - no direct email)]

Thanks!

Hey Stephen, I suspect that at the very least we could just name the Updated.app the same as its parent which in this case would be FirefoxNightly.app.

e.g.
current
FirefoxNightly.app/Updated.app

proposed
FirefoxNightly.app/FirefoxNightly.app

Where the name of the Updated.app is determined by the parent. This would avoid any confusion.

If it is possible it would be better to avoid having the updated app listed at all in Notifications and it isn't a lot of work that would be an even better approach.

Comment 2

[Robert Strong (they/them - no direct email)]

Note: as far as I can tell this doesn't cause any problems beyond listing the entry. If there are other issues please let us know. Thanks!

Comment 3

[Robert Strong (they/them - no direct email)]

Ryan, can you confirm that the text in Notifications self corrects after applying the update?

Comment 4

[Ryan Feeley [:rfeeley]]

Robert, it definitely fixed itself.

Comment 5

[Stephen A Pohl [:spohl]]

Attachment: Patch (wip)

Completely untested patch.

Comment 6

[Robert Strong (they/them - no direct email)]

Stephen, another possibility is updating the app in place instead of using the updated.app directory. My main concerns revolve around whether Firefox or Mac OS X would get confused if the files are changed out while the app is running.

Comment 7

[Stephen A Pohl [:spohl]]

Right, updating in place crossed my mind as well. I can't think of any particular problem right now, but I guess we won't know for sure until we give it a shot.

Comment 8

[Stephen A Pohl [:spohl]]

Attachment: Patch (wip)

Here is an improved patch. A try run showed xpcshell tests failing, but the logs weren't very helpful:
https://tbpl.mozilla.org/?tree=Try&rev=ba2c56c36745

However, once I ran ./mach xpcshell-test toolkit/mozapps/update/tests/unit_base_updater/, the update.log file for the failing test toolkit/mozapps/update/tests/unit_base_updater/marStageSuccessPartial.js showed this:

 4:07.15 Performing a staged update
 4:07.15 SOURCE DIRECTORY /Users/Stephen/Documents/objdir-ff-debug/_tests/xpcshell/toolkit/mozapps/update/tests/unit_base_updater/marStageSuccessPartial/dir.app/Contents/MacOS/updates/0
 4:07.15 DESTINATION DIRECTORY /Users/Stephen/Documents/objdir-ff-debug/_tests/xpcshell/toolkit/mozapps/update/tests/unit_base_updater/marStageSuccessPartial/dir.app/objdir-ff-debug
 4:07.15 ensure_copy_recursive: path is not a directory: /Users/Stephen/Documents/objdir-ff-debug/_tests/xpcshell/toolkit/mozapps/update/tests/unit_base_updater/marStageSuccessPartial/dir.app/objdir-ff-debug/objdir-ff-debug/objdir-ff-debug/objdir-ff-debug/objdir-ff-debug/objdir-ff-debug/objdir-ff-debug/objdir-ff-debug/objdir-ff-debug/objdir-ff-debug/objdir-ff-debug/objdir-ff-debug/objdir-ff-debug/objdir-ff-debug/a/b/9/93, rv: 0, err: 24
 4:07.15 failed: 6
 4:07.15 calling QuitProgressUI

The path in ensure_copy_recursive obviously doesn't look very healthy. Looking into it.

Comment 9

[Stephen A Pohl [:spohl]]

Attachment: Patch

This now passes for me locally, but try is currently closed and I'm unable to confirm (bug 1040308). I'll do so once try reopens.

I'll also file a follow-up bug to remove the need for an "Updated.app" directory entirely and update in place instead.

Comment 10

[Stephen A Pohl [:spohl]]

Comment on attachment 8458243 [details] [diff] [review]
Patch

xpcshell-tests are green on try:
https://tbpl.mozilla.org/?tree=Try&rev=6c61f258bc33

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 8458243 [details] [diff] [review]
Patch

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>@@ -91,17 +91,21 @@ const KEY_UPDATE_ARCHIVE_DIR = "UpdArchD
> #elifdef MOZ_WIDGET_GONK
> // In Gonk, the updater will remount the /system partition to move staged files
> // into place, so we skip the test here to keep things isolated.
> #define CHECK_CAN_USE_SERVICE
> #endif
> 
> const DIR_UPDATES         = "updates";
> #ifdef XP_MACOSX
>-const UPDATED_DIR         = "Updated.app";
>+const UPDATED_DIR         = Services.dirsvc.get(KEY_EXECUTABLE,
>+                                                Ci.nsIFile).parent
>+                                                           .parent
>+                                                           .parent
>+                                                           .leafName;
Please make this into a lazy getter so this code isn't evaluated until it is needed.

I'd like to take a look at the change so r-'ing.

Also, please land this after the merge so it gets extra testing on m-c since this bug doesn't cause any serious problems.

Comment 12

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks, Robert!

Do you think we should run this through some testing on Oak?

Comment 13

[Stephen A Pohl [:spohl]]

Ryan, could you verify in a recent nightly that this issue is actually still occurring? We had to make a number of changes to support v2 signing on OSX and this may have been fixed along the way. Thanks!

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 8459622 [details] [diff] [review]
Patch

Cancelling review until we hear back from the reporter.

Comment 15

[Ryan Feeley [:rfeeley]]

I see FirefoxNightly in Notifications now.

Comment 16

[Stephen A Pohl [:spohl]]

Awesome, thanks!