Closed Bug 711140 Opened 8 years ago Closed 8 years ago

Use maintenance service if the fallback key is present so tests can run


(Toolkit :: Application Update, defect)

Windows 7
Not set





(Reporter: bbondy, Assigned: bbondy)




(1 file, 1 obsolete file)

If fallback key is present always use the service when pending-service state is written, even if we already have write access to the installation directory.

This is needed before we can land bug 481815 and friends so that Ehsan's tests can run. I plan to do this today.  The patch should be very small.
Attached patch Patch v1. (obsolete) — Splinter Review
This patch fixes tests so they work after the newly added security mechanisms.
Attachment #582171 - Flags: review?(robert.bugzilla)
Comment on attachment 582171 [details] [diff] [review]
Patch v1.

>diff --git a/toolkit/mozapps/update/common/pathhash.h b/toolkit/mozapps/update/common/pathhash.h
>--- a/toolkit/mozapps/update/common/pathhash.h
>+++ b/toolkit/mozapps/update/common/pathhash.h
>@@ -44,9 +44,12 @@
>  * @param  filePath     The input file path to get a registry path from
>  * @param  registryPath A buffer to write the registry path to, must 
>  *                      be of size in WCHARs MAX_PATH + 1
>  * @return TRUE if successful
> */
> BOOL CalculateRegistryPathFromFilePath(const LPCWSTR filePath, 
>                                        LPWSTR registryPath);
>+#define FallbackKey \
>+  L"SOFTWARE\\Mozilla\\MaintenanceService\\3932ecacee736d366d6436db0f55bce4"
You have both FallbackKey and fallbackKey which can be confusing. For this I think FallbackKeyPath or similar would be better

I know I didn't catch this until now so if it is a pain don't bother but it would be better to replace Fallback / fallback with Test / test throughout so it is clear that this isn't used as a fallback for normal updates.

Add a comment describing what this is used for.

> #endif
>diff --git a/toolkit/mozapps/update/test/unit/ b/toolkit/mozapps/update/test/unit/
>--- a/toolkit/mozapps/update/test/unit/
>+++ b/toolkit/mozapps/update/test/unit/
>@@ -542,33 +542,48 @@ function runUpdateUsingService(aInitialS
>-    if (status == STATE_PENDING) {
>+    if (status == STATE_APPLYING) {
Do you think this could race where the status file isn't changed in time to STATE_APPLYING before this check? If it is a possibility it might be best to check for both STATE_PENDING and STATE_APPLYING.

>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
>@@ -1595,20 +1595,34 @@ int NS_main(int argc, NS_tchar **argv)
>   gSourcePath = argv[1];
> #ifdef XP_WIN
>   // Disable every privilege we don't need. Processes started using
>   // CreateProcess will use the same token as this process.
>   UACHelper::DisablePrivileges(NULL);
>   bool useService = false;
>+  bool fallbackKeyExists = false;
>   // We never want the service to be used unless we build with
>   // the maintenance service.
>   IsUpdateStatusPending(useService);
>+  // Our tests run with a different apply directory for each test.
>+  // We use this registry key on our test slaves to store the 
>+  // allowed name/issuers.
>+  HKEY fallbackKey;
>+                                  FallbackKey, 0,
>+                                  KEY_READ | KEY_WOW64_64KEY, &fallbackKey);
The return type for RegOpenKeyExW is LONG let's just go with LONG here and elsewhere. Since I missed this previously could you check the patches for other instances such as registrycertificates.cpp?

Alternatively, you could just do this in the if statement itself since retCode is only used in the if statement here... likely the same in the other uses as well.

>+  if (ERROR_SUCCESS == retCode) {
>+    fallbackKeyExists = true;
>+    RegCloseKey(fallbackKey);
>+  }

I'd like a quick skim over this again before r+'ing though I am tempted to just r+ with nit fixed.
Attachment #582171 - Flags: review?(robert.bugzilla) → review+
Attached patch Patch v2.Splinter Review
Implemented review comments, thanks for the quick review.
Attachment #582171 - Attachment is obsolete: true
Attachment #582254 - Flags: review?(robert.bugzilla)
Comment on attachment 582254 [details] [diff] [review]
Patch v2.

Looks good and thanks.
Attachment #582254 - Flags: review?(robert.bugzilla) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.