1064523 - Create staging directory outside of the Mac bundle

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

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

This will make it so updates don't invalidate the v2 signature on the bundle

Comment 1

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

Attachment: 1. patch in progress - client changes

Comment 2

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

Attachment: 2. patch - client whitespace cleanup

Comment 3

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

Attachment: 3. patch in progress - test changes

Comment 4

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

Comment on attachment 8486281 [details] [diff] [review]
1. patch in progress - client changes

This patch will need changes due to gonk so obsoleting.

Comment 5

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

Attachment: Client patch

Comment 6

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

Attachment: patch - client whitespace cleanup

Comment 7

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

Note: I think these changes make it so the tests no longer install the new maintenance service. :(

Comment 8

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

Comment on attachment 8487755 [details] [diff] [review]
Client patch

Found a bug so clearing review request

Comment 9

[Brian R. Bondy [:bbondy]]

Comment on attachment 8487756 [details] [diff] [review]
patch - client whitespace cleanup

Review of attachment 8487756 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for clearing this whitespace. I've been meaning to do that at some point.

Comment 10

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #7)
> Note: I think these changes make it so the tests no longer install the new
> maintenance service. :(

Perhaps around a different registry path lookup being done for CalculateRegistryPathFromFilePath

Comment 11

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

Attachment: Client patch

Comment 12

[Brian R. Bondy [:bbondy]]

Comment on attachment 8488533 [details] [diff] [review]
Client patch

Review of attachment 8488533 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/maintenanceservice/workmonitor.cpp
@@ +102,5 @@
> +IsDigits(WCHAR *str)
> +{
> +  while (*str) {
> +    if (!iswdigit(*str++)) {
> +      return FALSE;

nit: false;

@@ +105,5 @@
> +    if (!iswdigit(*str++)) {
> +      return FALSE;
> +    }
> +  }
> +  return TRUE;

nit: true;

@@ +119,5 @@
> + * @param boolean True if the command line contains just the directory to apply
> + *                the update to
> + */
> +static bool
> +IsOldCommandline(int argc, LPWSTR *argv)

Do we need to have the concept of an old command line at all?
Couldn't we just always use the new command line format?
In what case would we still use the old command line?

Comment 13

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

(In reply to Brian R. Bondy [:bbondy] from comment #12)
> Comment on attachment 8488533 [details] [diff] [review]
> Client patch
> 
> Review of attachment 8488533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/maintenanceservice/workmonitor.cpp
> @@ +102,5 @@
> > +IsDigits(WCHAR *str)
> > +{
> > +  while (*str) {
> > +    if (!iswdigit(*str++)) {
> > +      return FALSE;
> 
> nit: false;
> 
> @@ +105,5 @@
> > +    if (!iswdigit(*str++)) {
> > +      return FALSE;
> > +    }
> > +  }
> > +  return TRUE;
> 
> nit: true;
I did this due to all of the other cases where it used FALSE and TRUE for return values. Do you mind if I change all of the other instances too for consistency?

> @@ +119,5 @@
> > + * @param boolean True if the command line contains just the directory to apply
> > + *                the update to
> > + */
> > +static bool
> > +IsOldCommandline(int argc, LPWSTR *argv)
> 
> Do we need to have the concept of an old command line at all?
> Couldn't we just always use the new command line format?
> In what case would we still use the old command line?
Old versions of Firefox.

Comment 14

[Brian R. Bondy [:bbondy]]

> I did this due to all of the other cases where it used FALSE and TRUE for return values. Do you mind if 
> I change all of the other instances too for consistency?

I'm fine with that but the uses I think you are talking about are to be consistent with return values of BOOL returned from various win32 api.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #13)
> (In reply to Brian R. Bondy [:bbondy] from comment #12)
> > Do we need to have the concept of an old command line at all?
> > Couldn't we just always use the new command line format?
> > In what case would we still use the old command line?
> Old versions of Firefox.

The old version of firefox will be using the old version of update.exe to update though right? Maybe I'm missing the use case.

Comment 15

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

static BOOL vs static bool and the functions returning TRUE vs true and FALSE vs false.

The old version of firefox will be using the old version of updater.exe and sending a command line without the extra parameter. So, IsUpdateBeingStaged, GetInstallationDir, etc. will be using the wrong index of argv.

Comment 16

[Brian R. Bondy [:bbondy]]

Sorry if this brings frustration, but I don't feel comfortable accepting as is until I understand the reasoning more.  Which you'll agree is the correct way to act when performing reviews.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #15)
> static BOOL vs static bool and the functions returning TRUE vs true and
> FALSE vs false.
> 
> The old version of firefox will be using the old version of updater.exe

Ya that was my point which led me to not understand why we need the extra complexity of detecting IsOldCommandline.

> and sending a command line without the extra parameter. 

Right the old version sends the command line without the extra parameter to the old updater.exe.

> So, IsUpdateBeingStaged,
> GetInstallationDir, etc. will be using the wrong index of argv.

Only the new firefox will send the new args to the new updater.exe. 
Only the old firefox will send the old args to the old updater.exe.
So why would it be the wrong index?

It seems like the patch is assuming that the old firefox will be sending args to the new updater. Which will never happen.

Comment 17

[Brian R. Bondy [:bbondy]]

Comment on attachment 8488533 [details] [diff] [review]
Client patch

Review of attachment 8488533 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed on IRC the IsOldCommandline isn't needed (as far as I can still tell) for a single installation on a single system. 
But it is needed because you can have multiple installs on a single system, and in that case there's only 1 maintenance service (the newest).

I'd suggest doing some extra testing to make 100% sure the maintenance service gets updated before landing in the various edge cases.
I.e. that probably is easiest accomplished by pushing to oak first and then trying things like disabling staged updates, doing an update without the service, etc.
Perhaps Kamil can help do that after you push the patches there if you are swamped with other v2 signing stuff.

Comment 18

[Brian R. Bondy [:bbondy]]

btw I only mention that extra testing because it seems like there could be some major problems if there is some edge case where the maintenance service is using an older one than the installed firefox. And previously there would have been nothing critically bad that would happen if that was the case.

Comment 19

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

Agreed regarding the testing. I have also ran the xpcshell service tests individually using the old service. They passed and upgraded the service to the new service so it should be good to go.

Comment 20

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #19)
> Agreed regarding the testing. I have also ran the xpcshell service tests
> individually using the old service. They passed and upgraded the service to
> the new service so it should be good to go.
Sorry, I meant to say that I have also ran the old xpcshell service tests individually using the new service. They passed and upgraded the service to the old service since they are the version.

Comment 21

[Stephen A Pohl [:spohl]]

Comment on attachment 8488533 [details] [diff] [review]
Client patch

Review of attachment 8488533 [details] [diff] [review]:
-----------------------------------------------------------------

Robert, I've looked at the changes to nsUpdateDriver.cpp and it all seems correct. Not sure if you wanted me to run any tests as well. Just let me know.

Comment 22

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

Attachment: Combined patch

Additional changes in this patch.

--- ../../_1-temp/mozilla/toolkit/components/maintenanceservice/workmonitor.cpp	2014-09-16 11:31:49 -0700
+++ toolkit/components/maintenanceservice/workmonitor.cpp	2014-09-16 11:33:20 -0700
@@ -140,22 +140,32 @@
   int index = 3;
   if (IsOldCommandline(argcTmp, argvTmp)) {
     index = 2;
   }
 
   if (argcTmp < index) {
     return FALSE;
   }
-  wcsncpy(aResultDir, argvTmp[index], MAX_PATH);
+
+  wcsncpy(aResultDir, argvTmp[2], MAX_PATH);
   WCHAR* backSlash = wcsrchr(aResultDir, L'\\');
   // Make sure that the path does not include trailing backslashes
   if (backSlash && (backSlash[1] == L'\0')) {
     *backSlash = L'\0';
   }
+
+  // The new command line's argv[2] is always the installation directory.
+  if (index == 2) {
+    bool backgroundUpdate = IsUpdateBeingStaged(argcTmp, argvTmp);
+    bool replaceRequest = (argcTmp >= 4 && wcsstr(argvTmp[3], L"/replace"));
+    if (backgroundUpdate || replaceRequest) {
+      return PathRemoveFileSpecW(aResultDir);
+    }
+  }
   return TRUE;
 }
 
 /**
  * Runs an update process as the service using the SYSTEM account.
  *
  * @param  argc           The number of arguments in argv
  * @param  argv           The arguments normally passed to updater.exe
@@ -174,21 +184,26 @@
   si.cb = sizeof(STARTUPINFO);
   si.lpDesktop = L"winsta0\\Default";
   PROCESS_INFORMATION pi = {0};
 
   // The updater command line is of the form:
   // updater.exe update-dir apply [wait-pid [callback-dir callback-path args]]
   LPWSTR cmdLine = MakeCommandLine(argc, argv);
 
+  int index = 3;
+  if (IsOldCommandline(argc, argv)) {
+    index = 2;
+  }
+
   // If we're about to start the update process from session 0,
   // then we should not show a GUI.  This only really needs to be done
   // on Vista and higher, but it's better to keep everything consistent
   // across all OS if it's of no harm.
-  if (argc >= 2 ) {
+  if (argc >= index) {
     // Setting the desktop to blank will ensure no GUI is displayed
     si.lpDesktop = L"";
     si.dwFlags |= STARTF_USESHOWWINDOW;
     si.wShowWindow = SW_HIDE;
   }
 
   // We move the updater.ini file out of the way because we will handle
   // executing PostUpdate through the service.  We handle PostUpdate from
@@ -276,31 +291,31 @@
 
   // Now that we're done with the update, restore back the updater.ini file
   // We use it ourselves, and also we want it back in case we had any type
   // of error so that the normal update process can use it.
   if (selfHandlePostUpdate) {
     MoveFileExW(updaterINITemp, updaterINI, MOVEFILE_REPLACE_EXISTING);
 
     // Only run the PostUpdate if the update was successful
-    if (updateWasSuccessful && argc > 2) {
+    if (updateWasSuccessful && argc > index) {
       LPCWSTR updateInfoDir = argv[1];
-      bool backgroundUpdate = IsUpdateBeingStaged(argc, argv);
+      bool stagingUpdate = IsUpdateBeingStaged(argc, argv);
 
       // Launch the PostProcess with admin access in session 0.  This is
       // actually launching the post update process but it takes in the
       // callback app path to figure out where to apply to.
       // The PostUpdate process with user only access will be done inside
       // the unelevated updater.exe after the update process is complete
       // from the service.  We don't know here which session to start
       // the user PostUpdate process from.
       // Note that we don't need to do this if we're just staging the
       // update in the background, as the PostUpdate step runs when
       // performing the replacing in that case.
-      if (!backgroundUpdate) {
+      if (!stagingUpdate) {
         LOG(("Launching post update process as the service in session 0."));
         if (!LaunchWinPostProcess(installDir, updateInfoDir, true, nullptr)) {
           LOG_WARN(("The post update process could not be launched."
                     " installDir: %ls, updateInfoDir: %ls",
                     installDir, updateInfoDir));
         }
       }
     }
@@ -374,21 +389,17 @@
     }
     return FALSE;
   }
 
   // Verify that the updater.exe that we are executing is the same
   // as the one in the installation directory which we are updating.
   // The installation dir that we are installing to is installDir.
   WCHAR installDirUpdater[MAX_PATH + 1] = { L'\0' };
-  if (IsOldCommandline(argc, argv)) {
-    wcsncpy(installDirUpdater, installDir, MAX_PATH);
-  } else {
-    wcsncpy(installDirUpdater, argv[2], MAX_PATH);
-  }
+  wcsncpy(installDirUpdater, installDir, MAX_PATH);
   if (!PathAppendSafe(installDirUpdater, L"updater.exe")) {
     LOG_WARN(("Install directory updater could not be determined."));
     result = FALSE;
   }
 
   BOOL updaterIsCorrect;
   if (result && !VerifySameFiles(argv[0], installDirUpdater,
                                  updaterIsCorrect)) {
diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
--- a/toolkit/mozapps/update/updater/updater.cpp
+++ b/toolkit/mozapps/update/updater/updater.cpp
@@ -1925,17 +1925,17 @@ ProcessReplaceRequest()
   // The replacement algorithm is like this:
   // 1. Move destDir to tmpDir.  In case of failure, abort.
   // 2. Move newDir to destDir.  In case of failure, revert step 1 and abort.
   // 3. Delete tmpDir (or defer it to the next reboot).
 
 #ifdef XP_MACOSX
   NS_tchar destDir[MAXPATHLEN];
   NS_tsnprintf(destDir, sizeof(destDir)/sizeof(destDir[0]),
-               NS_T("%s"), gInstallDirPath);
+               NS_T("%s/Contents"), gInstallDirPath);
 #elif XP_WIN
   // Windows preserves the case of the file/directory names.  We use the
   // GetLongPathName API in order to get the correct case for the directory
   // name, so that if the user has used a different case when launching the
   // application, the installation directory's name does not change.
   NS_tchar destDir[MAXPATHLEN];
   if (!GetLongPathNameW(gInstallDirPath, destDir, sizeof(destDir)/sizeof(destDir[0]))) {
     return NO_INSTALLDIR_ERROR;
@@ -1946,17 +1946,17 @@ ProcessReplaceRequest()
 
   NS_tchar tmpDir[MAXPATHLEN];
   NS_tsnprintf(tmpDir, sizeof(tmpDir)/sizeof(tmpDir[0]),
                NS_T("%s.bak"), destDir);
 
   NS_tchar newDir[MAXPATHLEN];
   NS_tsnprintf(newDir, sizeof(newDir)/sizeof(newDir[0]),
 #ifdef XP_MACOSX
-               NS_T("%s"),
+               NS_T("%s/Contents"),
                gWorkingDirPath);
 #else
                NS_T("%s.bak/updated"),
                gInstallDirPath);
 #endif
 
   // First try to remove the possibly existing temp directory, because if this
   // directory exists, we will fail to rename destDir.
@@ -2013,49 +2013,21 @@ ProcessReplaceRequest()
     } else {
       LOG(("Failed to schedule OS reboot removal of directory: " LOG_S,
            tmpDir));
     }
 #endif
   }
 
 #ifdef XP_MACOSX
-  // On OS X, we need to copy anything else left over inside the Updated.app
-  // directory, and then we need to get rid of it as it's no longer going to
-  // be useful.
+  // On OS X, we we need to remove the staging directory after its Contents
+  // directory has been moved.
   NS_tchar updatedAppDir[MAXPATHLEN];
   NS_tsnprintf(updatedAppDir, sizeof(updatedAppDir)/sizeof(updatedAppDir[0]),
                NS_T("%s/Updated.app"), gPatchDirPath);
-  NS_tDIR *dir = NS_topendir(updatedAppDir);
-  if (dir) {
-    NS_tdirent *entry;
-    while ((entry = NS_treaddir(dir)) != 0) {
-      if (NS_tstrcmp(entry->d_name, NS_T(".")) &&
-          NS_tstrcmp(entry->d_name, NS_T(".."))) {
-        NS_tchar childSrcPath[MAXPATHLEN];
-        NS_tsnprintf(childSrcPath, sizeof(childSrcPath)/sizeof(childSrcPath[0]),
-                     NS_T("%s/%s"), updatedAppDir, entry->d_name);
-        NS_tchar childDstPath[MAXPATHLEN];
-        NS_tsnprintf(childDstPath, sizeof(childDstPath)/sizeof(childDstPath[0]),
-                     NS_T("%s/%s"), gInstallDirPath, entry->d_name);
-        ensure_remove_recursive(childDstPath);
-        rv = rename_file(childSrcPath, childDstPath, true);
-        if (rv) {
-          LOG(("Moving " LOG_S " to " LOG_S " failed, err: %d",
-               childSrcPath, childDstPath, errno));
-        }
-      }
-    }
-
-    NS_tclosedir(dir);
-  } else {
-    LOG(("Updated.app dir can't be found: " LOG_S ", err: %d",
-         updatedAppDir, errno));
-  }
-
   ensure_remove_recursive(updatedAppDir);
 #endif
 
   gSucceeded = true;
 
   return 0;
 }

Comment 23

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

I've run

1. the old tests without the v2 signing changes using the new maintenance service with all of the v2 signing changes. The maintenance service wasn't upgraded and all of the old tests without the v2 signing changes passed.

2. the new tests with the v2 signing changes and the old maintenance service without the v2 signing changes. The maintenance service was upgraded and all of the new tests with the v2 signing changes passed.

Comment 24

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #22)
>...
>  #ifdef XP_MACOSX
> -  // On OS X, we need to copy anything else left over inside the Updated.app
> -  // directory, and then we need to get rid of it as it's no longer going to
> -  // be useful.
> +  // On OS X, we we need to remove the staging directory after its Contents
> +  // directory has been moved.
>    NS_tchar updatedAppDir[MAXPATHLEN];
>    NS_tsnprintf(updatedAppDir,
> sizeof(updatedAppDir)/sizeof(updatedAppDir[0]),
>                 NS_T("%s/Updated.app"), gPatchDirPath);
Note: instead of this gWorkingDirPath should just be used.

Comment 25

[Brian R. Bondy [:bbondy]]

Comment on attachment 8490294 [details] [diff] [review]
Combined patch

Review of attachment 8490294 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

Comment 26

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

Attachment: Combined patch

Fixing bitrot

Comment 27

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/3c2af5883a93

Comment 28

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

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/71611abb9569

Comment 29

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

Attachment: patch for aurora

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/71611abb9569

Comment 31

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

Landed on aurora in the Mac V2 signing combined patch in bug 1047584