Closed Bug 715489 Opened 13 years ago Closed 12 years ago

MozillaMaintenance service is missing a description in Service Manager

Categories

(Toolkit :: Application Update, defect)

Product:
Toolkit
Component:
Application Update
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla13

People

(Reporter: jimm, Assigned: bbondy)

References

Details

Attachments

(1 file, 4 obsolete files)

Patch v5.
14.46 KB, patch
bbondy
: review+
Details | Diff | Splinter Review 
This might be filed already or related to i10s or something, but I thought I'd file it to be sure. In the Service Control Manager, the new maintenance service lacks a human readable description, which means users will have no idea what it's for and are more likely to disable it. For example, the Google update service description reads:

"Keeps your Google software up to date. If this service is disabled or stopped, your Google software will not be kept up to date, meaning security vulnerabilities that may arise cannot be fixed and features may not work. This service uninstalls itself when there is no Google software using it."
Assignee: nobody → netzen
Thanks for posting Jim.

Possible text:
The Mozilla Maintenance Service performs tasks that improve your browsing experience including updating software silently.  This service is only run on demand and will stop as soon as the operation is completed.

Other suggestions?
This is not related to a problem with i10s,  the service just doesn't specify a description.  It will likely need to go through i10s though.
I'd go with something the average joe can grep and not get to specific:

"Mozilla Maintenance Service performs tasks that improve your browsing experience including applying important security updates to installed software. The Maintenance Service is a 'run on demand' service and will normally be in a 'stopped' state."

I think though we might want to mention Firefox somehow, I wonder if people will make the connection between "Mozilla" and "Firefox"?
(In reply to Jim Mathies [:jimm] from comment #3)
> "Mozilla Maintenance Service performs tasks that improve your browsing
> experience including applying important security updates to installed
> software. The Maintenance Service is a 'run on demand' service and will
> normally be in a 'stopped' state."

Actually that seems even more specific than your run at it now that I read it back. :)
I think the first version in development had Firefox in the name itself but we generalized it because it will be used for other products in the future.  We could say in the description that it is for Firefox and update it later when other products are supported? Not sure what is best there.
(In reply to Brian R. Bondy [:bbondy] from comment #5)
> I think the first version in development had Firefox in the name itself but
> we generalized it because it will be used for other products in the future. 
> We could say in the description that it is for Firefox and update it later
> when other products are supported? Not sure what is best there.

I think you have to mention Firefox because nobody knows anything about what Mozilla is. In fact, I think it should be called the "Firefox Update Service" and and that we should NOT update other products with it. So, I would write:

"The Mozilla Firefox Update Service helps ensure you have the latest, most secure, version of Mozilla Firefox on your computer. Keeping Firefox up to date is very important for the security of your computer, and Mozilla strongly recommends that you keep this service enabled. The service only runs during Firefox updates, and it will be uninstalled if Firefox is uninstalled."

If we really must not name the service "Firefox Update" then I still strongly suggest mentioning Firefox somewhere in the description.

You should get feedback from marketing on the exact wording.
I'm not opposed to 'Firefox' in the name/description but saying update service is a bit deceptive since it is intended to do other things as well soon.
For example see bug 692255 already.
(In reply to Brian Smith (:bsmith) from comment #6)
> "The Mozilla Firefox Update Service helps ensure you have the latest, most
> secure, version of Mozilla Firefox on your computer. Keeping Firefox up to
> date is very important for the security of your computer, and Mozilla
> strongly recommends that you keep this service enabled. The service only
> runs during Firefox updates, and it will be uninstalled if Firefox is
> uninstalled."

This sounds really good to me.  

> You should get feedback from marketing on the exact wording.

I don't know anyone in marketing, so cc'ing a couple people who might know who to cc in.
"…for updating Mozilla applications like the world-acclaimed Firefox(TM)…"
OK, I was being a little bit silly but what about also giving a short description of Mozilla by which the average user can make the connection to this thingie that connects him to the internets?
(In reply to Brian R. Bondy [:bbondy] from comment #7)
> I'm not opposed to 'Firefox' in the name/description but saying update
> service is a bit deceptive since it is intended to do other things as well
> soon.

My suggestion is that if/when it does other things, we change the description to take those other things into account.
It will also be doing system restore points, possibly defragging profiles, and other things.
Comment 6 text looks fine to me, and this isn't really user visible so I'd rather not do too many iterations on it. Let's get a patch together for review with Brian's text and I'll CC laura mesa for a marketing check-in, but comment 6 is not bad, and better than what we have now, so let's not bury it in wordsmithy - easy enough to change later.
A few tweaks to comment 6 copy but ok by me otherwise:

"The Mozilla Firefox Update Service helps ensure that you have the latest and most secure version of Mozilla Firefox on your computer. Keeping Firefox up to date is very important for your online security, and Mozilla strongly recommends that you keep this service enabled. The service only runs during Firefox updates, and will be uninstalled if Firefox is ever uninstalled."
Ship it.

(Though, you know, if I were given to wordsmithy, I'd s/helps ensure/ensures/)

*scampers off to disrupt some other bug*
I don't think we want to change the wording to Comment 14 at this point, especially since the new name implies 3 design changes we have not agreed to that contradicts the current reality of the service. Namely the new name implies:

1. There will eventually be multiple services per computer.
2. The service is only used for updates.
3. The service gets uninstalled with Firefox

I suggest the same text as Comment 14, minus the last line, and with the name the same as it is currently. So we'd have:

"The Mozilla Maintenance Service ensures that you have the latest and most secure version of Mozilla Firefox on your computer. Keeping Firefox up to date is very important for your online security, and Mozilla strongly recommends that you keep this service enabled."

If we want to make a decision of multiple services per computer that should be done in another bug with another discussion.
Ditto regarding combining the Mozilla Maintenance Service uninstaller with the Firefox uninstaller. If anyone has strong objections to any of the 3 design decisions please feel free to post another bug where discussion can be had and link it up to this bug.

I'll wait for rs' feedback before implementing this though.
(In reply to Brian R. Bondy [:bbondy] from comment #16)
> I don't think we want to change the wording to Comment 14 at this point,
> especially since the new name implies 3 design changes we have not agreed to
> that contradicts the current reality of the service. Namely the new name
> implies:
> 
> 1. There will eventually be multiple services per computer.

I filed bug 720527, "Firefox maintenance service should be separate from the services used for maintenance of other Gecko-based products".

> 2. The service is only used for updates.

If we are going to have the service do some of the other things that are planned for it, then we shouldn't use the wording I proposed, and instead we should make it clear to users that it will (eventually) do those other things and that they should disable the service if they don't want it to do those other things.

> 3. The service gets uninstalled with Firefox

I filed bug 720527.
(In reply to Brian Smith (:bsmith) from comment #17)
> > 3. The service gets uninstalled with Firefox
> 
> I filed bug 720527.

I meant bug 720521.
updater.ini seems like a good place to put this so it can be localized alongside the other updater strings.
OK so use that and store it in registry and read it out when creating the service from the serviceinstall.cpp code?
I forgot that installing the service isn't entirely contained inside the maintenance service installer. I'm ok with using updater.ini for this.
OK cool, then I can just re-use readstrings as I'm doing with reading in the MAR channel file.
Attached patch Patch v1. (obsolete) — — Splinter Review 
Implemented service description filling. It loads the description from updater.ini which gets copied into the maintenance service directory from the installation directory.  The file gets removed on uninstall.

The service description gets updated both on service installs and on every successful upgrade (not on upgrade attempts where the service is newer than what is attempted to be installed), so we can change it on any update.  The service install log file gets updated on every error condition and on success.

If no maintenance service description string is specified in updater.ini, or it is left blank, or updater.ini does not exist, then the description will not get updated.

I went with the text from Comment 16 but can update the patch to have any string you wish.
Attachment #591887 - Flags: review?(robert.bugzilla)
I forgot to mention, I think this review should be treated as higher priority than the others because of the implied localization demands.
Comment on attachment 591887 [details] [diff] [review]
Patch v1.

># HG changeset patch
># Parent 6e664cc01ffa22d825515f2a37403d2dcb70c58d
># User Brian R. Bondy <netzen@gmail.com>
>Bug 715489 - MozillaMaintenance service is missing a description in Service Manager
>
>diff --git a/browser/installer/windows/nsis/maintenanceservice_installer.nsi b/browser/installer/windows/nsis/maintenanceservice_installer.nsi
>--- a/browser/installer/windows/nsis/maintenanceservice_installer.nsi
>+++ b/browser/installer/windows/nsis/maintenanceservice_installer.nsi
>@@ -176,16 +176,23 @@ Section "MaintenanceService"
>   IfFileExists "$INSTDIR\maintenanceservice.exe" 0 skipAlreadyExists
>     StrCpy $TempMaintServiceName "maintenanceservice_tmp.exe"
>   skipAlreadyExists:
> 
>   ; We always write out a copy and then decide whether to install it or 
>   ; not via calling its 'install' cmdline which works by version comparison.
>   CopyFiles "$EXEDIR\maintenanceservice.exe" "$INSTDIR\$TempMaintServiceName"
> 
>+  ; The updater.ini file is only used when performing an install or upgrade,
>+  ; and only if that install or upgrade is successful.  If an old updater.ini
>+  ; happened to be copied into the maintenance service installation directory
>+  ; but the service was not newer, the updater.ini file would be unused.
>+  ; It is used to fill the description of the service on success.
>+  CopyFiles "$EXEDIR\updater.ini" "$INSTDIR\updater.ini"
>+
>   ; Install the application maintenance service.
>   ; If a service already exists, the command line parameter will stop the
>   ; service and only install itself if it is newer than the already installed
>   ; service.  If successful it will remove the old maintenanceservice.exe
>   ; and replace it with maintenanceservice_tmp.exe.
>   ClearErrors
>   ${GetParameters} $0
>   ${GetOptions} "$0" "/Upgrade" $0
>@@ -247,16 +254,18 @@ Function un.RenameDelete
>   ${EndIf}
>   ClearErrors
> FunctionEnd
> 
> Section "Uninstall"
>   ; Delete the service so that no updates will be attempted
>   nsExec::Exec '"$INSTDIR\maintenanceservice.exe" uninstall'
> 
>+  Push "$INSTDIR\updater.ini"
>+  Call un.RenameDelete
>   Push "$INSTDIR\maintenanceservice.exe"
>   Call un.RenameDelete
>   Push "$INSTDIR\maintenanceservice_tmp.exe"
>   Call un.RenameDelete
>   Push "$INSTDIR\maintenanceservice.old"
>   Call un.RenameDelete
>   Push "$INSTDIR\Uninstall.exe"
>   Call un.RenameDelete
>diff --git a/browser/locales/en-US/updater/updater.ini b/browser/locales/en-US/updater/updater.ini
>--- a/browser/locales/en-US/updater/updater.ini
>+++ b/browser/locales/en-US/updater/updater.ini
>@@ -1,4 +1,5 @@
> ; This file is in the UTF-8 encoding
> [Strings]
> TitleText=%MOZ_APP_DISPLAYNAME% Update
> InfoText=%MOZ_APP_DISPLAYNAME% is installing your updates and will start in a few momentsâ¦
What do you think about this having its own section so it isn't loaded when the updater UI strings are loaded? I *think* I prefer that but could likely be convinced otherwise.
As for the code to read it in a different section here is an example
http://mxr.mozilla.org/mozilla2.0/source/toolkit/mozapps/installer/wince/nsSetupStrings.cpp#74

>+MaintenanceServiceDescription=The Mozilla Maintenance Service ensures that you have the latest and most secure version of Mozilla Firefox on your computer. Keeping Firefox up to date is very important for your online security, and Mozilla strongly recommends that you keep this service enabled.
>diff --git a/toolkit/components/maintenanceservice/Makefile.in b/toolkit/components/maintenanceservice/Makefile.in
>--- a/toolkit/components/maintenanceservice/Makefile.in
>+++ b/toolkit/components/maintenanceservice/Makefile.in
>@@ -58,16 +58,17 @@ PROGRAM = maintenanceservice$(BIN_SUFFIX
> DIST_PROGRAM = maintenanceservice$(BIN_SUFFIX)
> 
> # Don't link the maintenanceservice against libmozutils. See bug 687139
> MOZ_UTILS_LDFLAGS =
> MOZ_UTILS_PROGRAM_LDFLAGS =
> 
> LIBS += \
>   ../../mozapps/update/common/$(LIB_PREFIX)updatecommon.$(LIB_SUFFIX) \
>+  ../../mozapps/readstrings/$(LIB_PREFIX)readstrings.$(LIB_SUFFIX) \
I'm kind of torn on this. On the one hand I like the consistency of always using readstrings though it currently isn't used everywhere but on the other hand you could remove the dependency and just use GetPrivateProfileStringW. I think this is fine and if we choose to use GetPrivateProfileStringW in the future we can just make readstrings use it if it is available. Sound good?

>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
>@@ -33,28 +33,30 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include <windows.h>
> #include <aclapi.h>
> #include <stdlib.h>
>+#include <shlwapi.h>
> 
> // Used for DNLEN and UNLEN
> #include <lm.h>
> 
> #include <nsAutoPtr.h>
> #include <nsWindowsHelpers.h>
> #include <nsMemory.h>
> 
> #include "serviceinstall.h"
> #include "servicebase.h"
> #include "updatehelper.h"
> #include "shellapi.h"
>+#include "readstrings.h"
> 
> #pragma comment(lib, "version.lib")
> 
> /**
>  * Obtains the version number from the specified PE file's version information
>  * Version Format: A.B.C.D (Example 10.0.0.300)
>  *  
>  * @param  path The path of the file to check the version on
>@@ -90,16 +92,86 @@ GetVersionNumberFromPath(LPWSTR path, DW
>   A = HIWORD(fixedFileInfo->dwFileVersionMS);
>   B = LOWORD(fixedFileInfo->dwFileVersionMS);
>   C = HIWORD(fixedFileInfo->dwFileVersionLS);
>   D = LOWORD(fixedFileInfo->dwFileVersionLS);
>   return TRUE;
> }
> 
> /**
>+ * Updates the service description with what is stored in the updater.ini
>+ * at the same path as the currently executing module binary.
>+ *
>+ * @param serviceHandle A handle to an opened service with 
>+ *                      SERVICE_CHANGE_CONFIG access right
>+ * @param TRUE on succcess.
>+*/
>+BOOL
>+UpdateServiceDescription(SC_HANDLE serviceHandle)
>+{
>+  WCHAR updaterINIPath[MAX_PATH + 1];
>+  if (!GetModuleFileNameW(NULL, updaterINIPath, 
>+                          sizeof(updaterINIPath) / 
>+                          sizeof(updaterINIPath[0]))) {
nit: I am fairly certain that the above can be on the previous line and not go over 80

>+    LOG(("Could not obtain module filename when attempting to "
>+         "modify service description. (%d)\n", GetLastError()));
>+    return FALSE;
>+  }
>+
>+  if (!PathRemoveFileSpecW(updaterINIPath)) {
>+    LOG(("Could not remove file spec when attempting to "
>+         "modify service description. (%d)\n", GetLastError()));
>+    return FALSE;
>+  }
>+
>+  if (!PathAppendSafe(updaterINIPath, L"updater.ini")) {
>+    LOG(("Could not append updater.ini filename when attempting to "
>+         "modify service description. (%d)\n", GetLastError()));
>+    return FALSE;
>+  }
>+
>+  if (GetFileAttributesW(updaterINIPath) == INVALID_FILE_ATTRIBUTES) {
>+    LOG(("updater.ini file does not exist, will not modify "
>+         "service description. (%d)\n", GetLastError()));
>+    return FALSE;
>+  }
>+  
>+  UpdaterStringTable uiStrings;
nit: I think uiStrings would be better named as regStrings
Attachment #591887 - Flags: review?(robert.bugzilla) → review-
Note to Brian and myself, since readstrings is only used by updater code once again we might decide to move it back under toolkit/mozapps/update/ so the error codes the updater uses are alongside the update code (bug 711054).
> What do you think about this having its own section so it isn't loaded 
> when the updater UI strings are loaded? I *think* I prefer that but 
> could likely be convinced otherwise.

I almost did it with a diff section but then I wondered if for localization it would be easier to have it under the Strings section.  I'll change to a diff section.

> I think this is fine and if we choose to use GetPrivateProfileStringW 
> in the future we can just make readstrings use it if it is available.

Sounds good.

> Note to Brian and myself, since readstrings is only used by updater 
> code once again we might decide to move it back under toolkit/mozapps/update/ 
> so the error codes the updater uses are alongside the update code (bug 711054).

This is described already in bug 711054. I think it would be best to put it in mozapps/update/common since the maintenance service will be using it as of this patch.  Also maintenance service has its own error codes it keeps separate in workmonitor.cpp that could go inside there as well.
Attached patch Patch v2. (obsolete) — — Splinter Review 
Implemented review comments.
Attachment #591887 - Attachment is obsolete: true
Attachment #593912 - Flags: review?(robert.bugzilla)
Comment on attachment 593912 [details] [diff] [review]
Patch v2.

I'd prefer it if readstrings didn't have the wrapper functions. The existing one for the updater strings is there due to the initial implementation and I think it would be better to just have readstrings be generic.
Attachment #593912 - Flags: review?(robert.bugzilla) → review-
Do you mind if I just move the whole "ReadMaintenanceServiceStrings" into serviceinstall.cpp? 

I think having this function helps with code clarity instead of having the several extra lines of code into the core logic of serviceinstall.cpp.
Not at all and I agree... I just don't think it is right to have these consumer specific wrappers in readstrings when the consumers could just as easily add their own wrappers and keep readstrings generic.
Agreed, thanks for clarifying.
Attached patch Patch v3. (obsolete) — — Splinter Review 
- Made ReadStrings have a template based parameter for different buffer sizes
- Moved Maintenance Service related functions for ReadStrings inside serviceinstall.cpp
Attachment #593912 - Attachment is obsolete: true
Attachment #594000 - Flags: review?(robert.bugzilla)
Attachment #594000 - Flags: review?(robert.bugzilla)
Attached patch Patch v4. (obsolete) — — Splinter Review 
Would have had to move too much to the header file so just used the same constants for max length in both files instead of making a template parameter.
Attachment #594000 - Attachment is obsolete: true
Attachment #594048 - Flags: review?(robert.bugzilla)
Attachment #594048 - Flags: review?(robert.bugzilla) → review+
I should not request aurora on this one after it lands right? (Since there are string changes)
Correct.

I also forgot to put the nit we discussed in this bug to add a comment to updater.ini regarding maximum length for the strings.
Attached patch Patch v5. — — Splinter Review 
Added in comment regarding less than 600 chars to the INI file.  Pushed to try as well.  Will push to inbound once that succeeds.
Attachment #594048 - Attachment is obsolete: true
Attachment #594249 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/15fe29a17221
Status: NEW → RESOLVED
Closed: 12 years ago
Hardware: x86_64 → x86
Resolution: --- → FIXED
Comment on attachment 594249 [details] [diff] [review]
Patch v5.

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

::: browser/locales/en-US/updater/updater.ini
@@ +4,5 @@
>  TitleText=%MOZ_APP_DISPLAYNAME% Update
>  InfoText=%MOZ_APP_DISPLAYNAME% is installing your updates and will start in a few moments…
> +
> +[MaintenanceServiceStrings]
> +ServiceDescription=The Mozilla Maintenance Service ensures that you have the latest and most secure version of Mozilla Firefox on your computer. Keeping Firefox up to date is very important for your online security, and Mozilla strongly recommends that you keep this service enabled.

Can we move this up to [Strings]?

Sadly, compare-locales isn't built out to support more than one section per ini file, see also bug 725064.

Fixing this on the compare-locales side is tough as the version we're using on our build infra is already way behind what's current in public version.
Blocks: 725064
Depends on: 725155
I'll do this work in Bug 725155 since Bug 725064 seems more like an infrastructure improvement bug.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: