Service is sometimes not used after several successful service updates because maintenanceservice.exe is missing

RESOLVED FIXED in mozilla12

Status

()

Toolkit
Application Update
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

unspecified
mozilla12
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
After several successful updates, sid0 reports that the latest update did not attempt to use the service and instead displayed a UAC prompt. 

There are no errors that occurred during the update, just the service wasn't attempted to be used.

Some notes on sid0's case: 

- I looked at his maintenanceservice log files and they all showed success. So for the time it used the UAC prompt, it didn't even attempt to use the service.
- He does have permissions to the service so it seems to be a different issue than this bug.  I'll spin off a new bug ID.  
- The update succeeded with the UAC prompt so there are no errors it just didn't try to use the service for some reason.
- His preferences are set to automatically use updates and use service is still ticked on.
- He mentioned he did not accidentally use an x64 native build for this update nor a different branch such as UX.
(Assignee)

Comment 1

7 years ago
Next time you're about to apply an update, please check this file before applying the update but after the update is downloaded and ready to be installed.

C:\Users\YOURUSERNAME\AppData\Local\Mozilla\Firefox\Nightly\updates\0
Open up the update.status file in a text editor and let me know what's inside.

I only need this info if you happened to reproduce this after getting the contents of that file.  So you may need to do this for the next few updates until you can reproduce if you don't mind.
(Assignee)

Comment 2

7 years ago
There is a similar issue where the service is not attempted to be used when the user does not have permissions to start the service here: Bug 715910, but it is not the same as this bug since sid0 does have access.
(Assignee)

Updated

7 years ago
Summary: Service is sometimes not used after several successful service updates → Service is sometimes not used after several successful service updates because maintenanceservice.exe is missing
(Assignee)

Comment 3

7 years ago
Robert Strong reproduced this. He noticed that maintennaceservice.exe was missing from his maintenance service directory.

This sounds like the same thing as you had. 
If after fixing this issue you still reproduce we can post another one, but it is likely the same.
(Assignee)

Updated

7 years ago
Depends on: 717377
Maybe, but at least after the update it was there.
(Assignee)

Comment 5

7 years ago
That is consistent with Robert Strong as well.
(Assignee)

Comment 6

7 years ago
Created attachment 588145 [details] [diff] [review]
Service sometimes gets deleted

Note: After this lands, the service will be deleted one last time. So it is recommended to reboot before getting the update after this lands to avoid having to have that happen.
Attachment #588145 - Flags: review?(robert.bugzilla)

Comment 7

7 years ago
What was the cause of the missing exe? Some sort of file copy error on install or something?
(Assignee)

Comment 8

7 years ago
We think it was the DeleteFileW, and then right after CopyFileW.  The new code fixes that and also should succeed in a larger amount of cases because if the CopyFile fails we try to move the old file out of the way (which may be possible if the file is open), then copy in the old one. 

When we install the maintenance service we drop an exe maintenanceservice_tmp.exe beside the first one, and then on upgrade we do a MoveFile w/ delete on reboot flag on the _tmp file.  But if you originally dropped it as maintenanceservice.exe because it didn't already exist (because of the first paragraph bug) then we would cal MoveFile w/ delete on maintenanceservice.exe.  This was a second bug only caused if you happened to have the first bug, because on upgrade we should always call the file _tmp.exe because the file should always exist.
(Assignee)

Comment 9

7 years ago
> We think it was the DeleteFileW, and then right after CopyFileW.

This happens on upgrades by the way, to get rid of the old service file and copy in the new one.
Comment on attachment 588145 [details] [diff] [review]
Service sometimes gets deleted

>diff --git a/toolkit/components/maintenanceservice/serviceinstall.cpp b/toolkit/components/maintenanceservice/serviceinstall.cpp
>--- a/toolkit/components/maintenanceservice/serviceinstall.cpp
>+++ b/toolkit/components/maintenanceservice/serviceinstall.cpp
>@@ -194,40 +194,102 @@ SvcInstall(SvcInstallAction action)
>         (existingA == newA && existingB == newB && 
>          existingC < newC) ||
>         (existingA == newA && existingB == newB && 
>          existingC == newC && existingD < newD)) {
>       if (!StopService()) {
>         return FALSE;
>       }
> 
>-      if (!DeleteFileW(serviceConfig.lpBinaryPathName)) {
>-        LOG(("Could not delete old service binary file %ls.  (%d)\n", 
>-             serviceConfig.lpBinaryPathName, GetLastError()));
>-        return FALSE;
>+      if (!wcscmp(newServiceBinaryPath, serviceConfig.lpBinaryPathName)) {
>+        LOG(("File is already in the correct location, no action needed for "
>+             "upgrade.\n"));
>+        return TRUE;
>       }
> 
>+      BOOL result = TRUE;
>+
>+      // Attempt to copy the new binary over top the existing binary.
>+      // If there is an error we try to move it out of the way and then
>+      // copy it in.  First try the safest / easiest way to overwrite the file.
>       if (!CopyFileW(newServiceBinaryPath, 
>-                    serviceConfig.lpBinaryPathName, FALSE)) {
>-        LOG(("Could not overwrite old service binary file. "
>-             "This should never happen, but if it does the next upgrade will fix"
>-             " it, the service is not a critical component that needs to be "
>-             " installed for upgrades to work. (%d)\n", 
>-             GetLastError()));
>-        return FALSE;
>+                     serviceConfig.lpBinaryPathName, FALSE)) {
>+        LOG(("WARNING: Could not overwrite old service binary file."
>+             " (%d)\n", GetLastError()));
>+
>+        // The code below makes the assumption that the length is at least 3.
>+        // This will always be the case, but just in case we check anyway.
>+        const size_t len = wcslen(serviceConfig.lpBinaryPathName);
>+        if (len > 3) {
I hate coming up with the "best" number for these types of checks and in this instance all it gaurantees is that you can rename the last 3 characters of the file name. At the very least it should require '.', since this takes a full path at least 3 more characters to account for the drive (e.g. c:\), and I wouldn't be adverse to requiring it to be under a directory as well (e.g. a\) though that is a bit much. Please update the comment accordingly.

>+          // Calculate the temp file path that we're moving the file to. This 
>+          // is the same as the proper service path but with a .old extension.
>+          LPWSTR oldServiceBinaryTempPath = 
>+            new WCHAR[wcslen(serviceConfig.lpBinaryPathName) + 1];
>+          wcscpy(oldServiceBinaryTempPath, serviceConfig.lpBinaryPathName);
>+          // Rename the extension to .old
nit: // Rename the last 3 characters to old

>+          wcscpy(oldServiceBinaryTempPath + len - 3, L"old");
>+
>+          // Move the current (old) service file to the temp path.
>+          if (MoveFileExW(serviceConfig.lpBinaryPathName, 
>+                          oldServiceBinaryTempPath, 
>+                          MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH)) {
>+            // The old binary is moved out of the way, copy in the new one.
>+            if (!CopyFileW(newServiceBinaryPath, 
>+                           serviceConfig.lpBinaryPathName, FALSE)) {
>+              // It is best to leave the old service binary in this condition.
>+              LOG(("ERROR: The new service binary could not be copied in."
>+                   " The service will not be upgraded.\n"));
>+              result = FALSE;
>+            } else {
>+              LOG(("The new service binary was copied in by first moving the"
>+                   " old one out of the way.\n"));
>+            }
>+
>+            // Attempt to get rid of the old service temp path.
>+            if (DeleteFileW(oldServiceBinaryTempPath)) {
>+              LOG(("The old temp service path was deleted: %ls.\n", 
>+                   oldServiceBinaryTempPath));
>+            } else {
>+              // The old temp path could not be removed.  It will be removed
>+              // the next time the user can't copy the binary in or on uninstall.
>+              LOG(("WARNING: The old temp service path was not deleted.\n"));
>+            }
>+          } else {
>+            // It is best to leave the old service binary in this condition.
>+            LOG(("ERROR: Could not move old service file out of the way from:"
>+                 ": \"%ls\" to \"%ls\". Service will not be upgraded. (%d)\n", 
nit: previous line to this on ends with ':' and this line starts with ':'.

>+                 serviceConfig.lpBinaryPathName, 
>+                 oldServiceBinaryTempPath, GetLastError()));
>+            result = FALSE;
Do you think it makes sense to schedule the replacement on next OS reboot for this case?

>+          }
>+          delete[] oldServiceBinaryTempPath;
>+        } else {
>+            // It is best to leave the old service binary in this condition.
>+            LOG(("ERROR: Service binary path was less than 3, service will"
>+                 " not be updated.  This should never happen.\n"));
>+            result = FALSE;
>+        }
>+      } else {
>+        LOG(("The new service binary was copied in.\n"));
>       }
> 
>       // We made a copy of ourselves to the existing location.
>       // The tmp file (the process of which we are executing right now) will be
>       // left over.  Attempt to delete the file on the next reboot.
>-      MoveFileExW(newServiceBinaryPath, NULL, MOVEFILE_DELAY_UNTIL_REBOOT);
>+      if (MoveFileExW(newServiceBinaryPath, NULL, MOVEFILE_DELAY_UNTIL_REBOOT)) {
>+        LOG(("Deleting the old file path on the next reboot: %ls.\n", 
>+             newServiceBinaryPath));
>+      } else {
>+        LOG(("Call to delete the old file path failed: %ls.\n", 
>+             newServiceBinaryPath));
>+      }
>       
>       // Setup the new module path
>       wcsncpy(newServiceBinaryPath, serviceConfig.lpBinaryPathName, MAX_PATH);
>-      // Fall through so we replace the service
>+      return result;
>     } else {
>       // We don't need to copy ourselves to the existing location.
>       // The tmp file (the process of which we are executing right now) will be
>       // left over.  Attempt to delete the file on the next reboot.
>       MoveFileExW(newServiceBinaryPath, NULL, MOVEFILE_DELAY_UNTIL_REBOOT);
>       
>       return TRUE; // nothing to do, we already have a newer service installed
>     }

I think the logic used could be cleaned up a bit but this suffices.
Attachment #588145 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 11

7 years ago
> Do you think it makes sense to schedule the replacement 
> on next OS reboot for this case?

I don't think this condition will ever be hit, to test it I had to manually fall through to it through code.  I prefer not to add more complexity there and there may be edge cases like if an uninstall and then install happens again before a reboot that could lead to further problems.
(Assignee)

Comment 13

7 years ago
Created attachment 589282 [details] [diff] [review]
Patch v2.

Implemented review comments.  Left the check at 3 but explained in the comment that this is protection against unsafe code and that otherwise it will fail safely in the call to MoveFileExW if it's not a valid path.
Attachment #588145 - Attachment is obsolete: true
Attachment #589282 - Flags: review+
(Assignee)

Updated

7 years ago
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/3706737bdf48
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Depends on: 766833
You need to log in before you can comment on or make changes to this bug.