481815 - Provide a Windows service to update applications without asking Administrator password

Status: RESOLVED FIXED

(Toolkit :: Application Update, enhancement, P1)

Description

[René]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.7) Gecko/2009021910 Firefox/3.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.7) Gecko/2009021910 Firefox/3.0.7

As Vista becomes popular, quite a lot of internet cafes use an unprivileged user for customers to access the web. Therefore Firefox will not be updated unless the administrator logs in from time to time. As most internet cafes are not aware of the importance of regular updates, old Firefox versions will be in use for a long time.

I think it would be a good idea to include a windows service with sufficient privileges checking and installing Firefox updates regularly regardless what user is logged in.

Reproducible: Always

Comment 1

[Matthias Versen [:Matti]]

An User Account doesn't have the permissions to upgrade the files and displaying an alert will annoy users because they can't upgrade.

Comment 2

[fcp]

I second this proposal.

Please change the summary to “Provide a Windows service to update applications without asking Administrator password” or something like that.  The problem which this bug tries to address is not specific to Internet cafes.

Using a Windows service is a well-known technique to update programs without user intervention on Windows.  For example, at least on Windows Vista, a service named “Windows Update” is enabled by default and used in the automatic update feature.  Some of the programs provided by Google also install an apparently similar service named “Google Software Updater.”

I'd like to note that Bug 318855 is related to this bug because both try to address some of the problems arising in the update without privileges.  They are not duplicates.

@Matthias: That is why a Windows service is desirable.

Comment 3

[René]

Summary changed. As Windows provides automatic SW updates (if not disabled) for its core components (read IE), security critical bugs in IE will be automatically fixed. 
As Firefox is not a core component of Windows (yet ;), security fixes will not be automatically downloaded and applied if the user does not have sufficient privileges. Thus, Firefox could be one step behind.

Comment 4

[René]

Google just open-soured its updater, see links and information below:

http://www.h-online.com/open/Google-Update-open-sourced--/news/113064
http://google-opensource.blogspot.com/2009/04/google-update-goes-open-source.html

Comment 5

[Kevin Brosnan]

Released under the Apache License 2.0. As I understand that is incompatible with the MPL.

Comment 6

[René]

Another issue I experienced that would be solved with this enhancement:
1) Admin-Role on XP installed Firefox 3.0.5
2) User-Role used Firefox 3.0.5, eventually an update to 3.0.6 was downloaded. User does not have privilege to install update.
3) Admin-Role updated to Firefox 3.0.8
4) User-Role: Every time (now updated) Firefox 3.0.8 was started, Firefox warned about an old update to 3.0.6 as it was still pending.

=> From my point of view I would expect old updates to be deleted or ignored on startup. I guess this behaviour should be filed as a new bug?

=> How to proceed with this bug? Who decides in general if a windows service is desired to automatically update Firefox? Otherwises this bug will get old...

Comment 7

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

(In reply to comment #6)
>...
> => From my point of view I would expect old updates to be deleted or ignored on
> startup. I guess this behaviour should be filed as a new bug?
That is fixed for the next Firefox release which is 3.5 by bug 485624 and bug 313057.

> => How to proceed with this bug? Who decides in general if a windows service is
> desired to automatically update Firefox? Otherwises this bug will get old...
I've had a couple of discussions this week about whether this bug should be implemented. There is at least one other option which would be to implement MSI's and use its update mechanism since it supports updating for non-admin users. This most likely won't get fixed for Firefox 3.5 or any of the 3.5.x releases but there is a decent chance it will be fixed in one form or another for the next major release after 3.5

Comment 8

[René]

- Thank you for the prompt answer. As a regular and enthusiastic Firefox user I understand if this bug cannot be implemented for 3.5 anymore. On the other hand I also understand if people get a bad impression if SW updates do not run smoothly. 
As with my sister, they cannot handle the problem mentioned in #6 and don't want to bother. In this situation they need assistance. I assume the next major release would probably be in, let's say, 15 months? How do I explain to my sister Firefox 3.5 got a new feature as such and such but her "problem" is not solved?

- A few thoughts I have:
  - Firefox grew steadily in popularity, which is definitively great. But popularity brings also user expectations such as easy and safe upgrade and security fix procedures. Can we afford to postpone this for annother 15 months?

  - As I mentioned earlier, many internet cafes/shops install their computers once with a kiosk solution. Firefox 3.5 or newer may not be installed for a long time after release as they do not renew their computers/kiosk sw. Can we afford to have a lot of internet cafes run very old Firefox releases that are never updated? Of course it is the responsability of this shops, but most of them (at least in South East Asia) are just not aware of updates and their (security) impacts.

I just returned from a 6 month trip in South East Asia and thus visited internet cafes often. I managed to even see them running Firefox 1.5/2.0! If this is the case and Firefox cannot be upgraded manually, I have to use IE... which, in turn, is not good for "spreading the Firefox word".

  - As this enhancement would not solve the kiosk problem immediately, it would definitely solve it for the XP- and Vista Admin-/User Role. Internet Cafes not using kiosk sw tend to install sw with Admin rights only (which is a good thing). And it would solve it for non-tech people like my sister.

What do you think about my concerns/thoughts?

Comment 9

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

Release dates for software products are typically very fluid and I won't comment on when I personally think the next release is.

This bug in one form or another has always existed in Firefox during which time the popularity has grown. As far as app update is concerned chances are the majority of users haven't had a problem with it to the point where they weren't able to update. The problem still should be fixed though the way it is fixed may be different than implementing a service.

I am less concerned about kiosk systems than general user experience since kiosks are typically locked down to prevent users from adding / modifying / updating software. We would also have to provide the ability for the admins of these kiosks to not allow users updating which will likely take the form as an option in the installer. Are there examples of software that allow users on kiosks to update vs. requiring the admin to update it?

What about Firefox 1.5/2.0 being installed forces someone to use IE? This statement doesn't make sense to me.

We are constrained by time and resources to implement this and it simply won't be fixed for Firefox 3.5.

Comment 10

[René]

1) The SW usually used was http://www.tinasoft.com/easycafe. It allows fine control what a user can do. It was setup in general to allow an user to do almost everything - even installing SW - but after logout the machine was rebooted to a reverted, previous, clean and virus free state.

2) The main argument about not using old and maybe unmaintained FF is about not patched security issues (gaining admin-privileges, stealing private data...). Web standard support is also much better in FF 3 thus using older FF versions causes sometimes display problems. If I find an old FF and an IE 7, my choice is pretty clear - I prefer the more or less patched but more current browser.

Comment 12

[[Baboo]]

Is this bug just about Firefox?

Comment 13

[Dave Garrett]

(In reply to [Baboo] from comment #12)
> Is this bug just about Firefox?

The product is listed up top is "Toolkit", thus it could be used by any application using the Mozilla Toolkit. Firefox would of course be the first to do so, but this could also be applied to Thunderbird and SeaMonkey as well as any other application that uses the Mozilla Platform.

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: Intermediate work in progress patch

Just providing a work in progress patch, not close to done yet nor do I need any feedback yet. 

Done so far:
- Created service base code
- Created service cmd line for install and uninstall + wait for stop
- icon & requireAdministrator manfiest
- Created makefile / build process + only build on winnt platform
- Added service to nsis installer and uninstaller.

Comment 15

[Brian R. Bondy [:bbondy]]

Just wanted to provide an intermediate status update here:
- Created service base code (in attached patch)
- Created service cmd line for install and uninstall + wait for stop (in attached patch)
- icon & requireAdministrator manifest (in attached patch)
- Created makefile / build process + only build on winnt platform (in attached patch)
- Added service to nsis installer and uninstaller.  (in attached patch)
- Added code for directory change watching and parsing out values of pending update meta data (not yet uploaded)
- Added code for launching processes in different sessions (not yet uploaded)
- Added code for obtaining linked administrative token of a token of a given session ID to launch as administrator of (not yet uploaded)

Pending: 
- Writing out the update meta data file instead of calling updater.exe directly on startup
- Refactoring and Testing
- Add code for first time updates installing the service (nsis script)
- Create process flow documentation so that I can get a security review
- Anything else I forgot

Comment 16

[Brian R. Bondy [:bbondy]]

Attachment: Patch 1 of 6: Base Windows service code (works but still in progress)

Comment 17

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2 of 6: Service main logic for work items to run updater.exe (works but still in progress)

Comment 18

[Brian R. Bondy [:bbondy]]

Attachment: Patch 3 of 6: /toolkit/xre code launching service work items (works but still in progress)

Comment 19

[Brian R. Bondy [:bbondy]]

Attachment: Patch 4 of 6: Proposed removing UAC elevation prompt from within js (works but still in progress)

Comment 20

[Brian R. Bondy [:bbondy]]

Attachment: Patch 5 of 6: Build procss

Comment 21

[Brian R. Bondy [:bbondy]]

Attachment: Patch 6 of 6: Service installer patch (works but still in progress, first time updates work todo)

Comment 22

[Brian R. Bondy [:bbondy]]

Above I attached the first proof of concept in the form of 6 patches.  There are still a few TODOs for some edge cases I need to do inline in the patches.  Also we are currently too permissive on who can install updates.  So I need to think of a way to fix that.  Also I still need to do some things like check hashes to ensure we're executing the right things.

The way it works is that there is a new Windows service installed:
Display name: Mozilla Application Updater
Service name: MozillaUpdater
File name: updater_svc.exe

The service watches a directory for requested 'work items'. A 'work item' is just a file that contains the info that is usually passed to updater.exe to perform an update.  

Instead of Firefox.exe launching updater.exe on Windows, it will instead write a 'work item' and then shuts down on startup as normal.  If the service is not started, disabled, does not exist, or any other error occurs we will fall back to launching updater.exe directly again.

The service immediately picks up the 'work item' and figures out the correct session, windows station, and desktop to run the update as.  The service gets the user token for this session and then gets its linked token if running on Vista (UAC elevated one).  The service then executes updater.exe with this elevated token.  This is where the magic happens and it bypasses UAC. 

Since I'm not touching the logic of actually applying the updates, I think it is a much lower chance of regressions from what it could be if I tried to do the updates myself inside the service.  I thought it was best to leverage the stable and tested code as much as possible.

The service doesn't need to know about the different installations you currently have since it is just delegates the command line parameters and working directory to updater.exe.

Comment 23

[Robert Kaiser]

The one thing I'm worried about here is how easy is it for a malicious app running as an unprivileged user to put something in the right place and permanently harming Firefox or even gain admin privs through that?

Comment 24

[Brian R. Bondy [:bbondy]]

> The one thing I'm worried about here is how easy is it for a malicious app running as an unprivileged user to put something in the right place 

Right this is mainly what I was talking about at the top of Comment 22.   I'd like to add a hash-check on updater.exe to ensure we're always executing the app which is our code only from the service.  Maybe even code to verify the app is signed by us before executing it (but I'm not sure if we sign aurora and other channels yet).

As for limited account users, I don't think we want to allow them to update though, so I think I need to build some other mechanism there to make sure they can't initiate an update. They will not have a linked elevated token though so I think it would fall back to the way it works currently (which we want to fix by asking for elevated permissions).

Comment 25

[Brian R. Bondy [:bbondy]]

For my reference, code for checking signature of an exe:
http://msdn.microsoft.com/en-us/library/aa382384%28VS.85%29.aspx

Comment 26

[[Baboo]]

To what extend will the Mozilla Updater Service now be a separate product?
What if in the future Thunderbird and Firefox both use this and then the user uninstalls one of them?
What about different versions? (e.g.: user first installs Firefox Beta then installs Thunderbird stable which ships with an older version of the updater service)
Will there be a possibility to uninstall or even install it independently from the Mozilla applications using it?

Comment 27

[Brian R. Bondy [:bbondy]]

Re Baboo: Some of these are still open questions, but below I provided my thoughts.

> To what extend will the Mozilla Updater Service now be a separate product?

It is my understanding that it will be shipped directly inside the installers of each product.  We may have a checkbox for optional install.

> What if in the future Thunderbird and Firefox both use this and then the user uninstalls one of them?

It would stay alive until the last product/channel is alive or my preference would be that we would just have a separate installer/uninstaller that gets executed silent for it from each product.

> What about different versions? [channels]

We would always have at most one service installed.  Initial task were 1 per product (e.g. firefox vs thunderbird) but I see no reason currently to even have it distinct from product to product.  I think the service with the highest core version number would always win and it would have to be always backwards compatible.

> Will there be a possibility to uninstall or even install it independently from the Mozilla applications using it?

If it's not started, disabled or not installed it will fall back to the old way of updating.  So I think we should allow both of these things via some documentation.

Comment 28

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Brian, how do you envision us being able to update the updater service? Presumably updater.exe will need to be taught some new tricks?

Comment 29

[Brian R. Bondy [:bbondy]]

> how do you envision us being able to update the updater service? Presumably updater.exe will need to be taught some new tricks?

I envision the nsis installer which is executed on update stopping the service if it's newer than what is available, then replacing then starting the service.

Comment 30

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

Comment on attachment 562959 [details] [diff] [review]
Patch 1 of 6: Base Windows service code (works but still in progress)

Looks good... please remove the progress bar.

Comment 31

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

(In reply to Robert Strong [:rstrong] (do not email) from comment #30)
> Comment on attachment 562959 [details] [diff] [review] [diff] [details] [review]
> Patch 1 of 6: Base Windows service code (works but still in progress)
> 
> Looks good... please remove the progress bar.
and ui, etc.

Comment 32

[Justin Wood (:Callek)]

Comment on attachment 562961 [details] [diff] [review]
Patch 3 of 6: /toolkit/xre code launching service work items (works but still in progress)

Patch summary says still in progress but has a review request open, so commenting anyway

>+  // Init the update directory path and ensure it exists.
>+  // Example: C:\programData\Mozilla\Firefox\updates
>+  WCHAR programData[MAX_PATH + 1];
>+  HRESULT hr = SHGetFolderPathW(NULL, CSIDL_COMMON_APPDATA, NULL, 
>+    SHGFP_TYPE_CURRENT, programData);
>+  if (FAILED(hr)) {
>+    return FALSE;
>+  }
>+  if (!::PathAppendW(programData, L"Mozilla")) {
>+    return FALSE;
>+  }
>+  ::CreateDirectoryW(programData, NULL);
>+  if (!::PathAppendW(programData, L"Firefox")) {
>+    return FALSE;

More applications than just Firefox exist here and should be accounted for, especially 3'rd party compilations of Firefox (that can't use the trademark).

Comment 33

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

I had a similar comment.

>+BOOL GetUpdateDirectoryPath(WCHAR *path) 
>+{
>+  HRESULT hr = SHGetFolderPathW(NULL, CSIDL_COMMON_APPDATA, NULL, 
>+    SHGFP_TYPE_CURRENT, path);
>+  if (FAILED(hr)) {
>+    return FALSE;
>+  }
>+  if (!::PathAppendW(path, L"Mozilla")) {
>+    return FALSE;
>+  }
>+  ::CreateDirectoryW(path, NULL);
>+  if (!::PathAppendW(path, L"Firefox")) {
>+    return FALSE;
>+  }
>+  ::CreateDirectoryW(path, NULL);
>+
>+  ::CreateDirectoryW(path, NULL);
>+  if (!::PathAppendW(path, L"updates")) {
>+    return FALSE;
>+  }
>+  ::CreateDirectoryW(path, NULL);
>+  return TRUE;
>+}
This won't work for other apps. I'd go with passing this data instead perhaps using the same file used to pass the session id.

Comment 34

[Brian R. Bondy [:bbondy]]

Thanks for the feedback thus far!

> This won't work for other apps. I'd go with passing this data instead perhaps using the same file used to pass the session id.

I'll just monitor the parent directory (is Mozilla/updates ok?) and there is no need to pass the data.  Changes to any subdirectory of that folder would be seen by the service.  I couldn't pass the data anyway because the service needs to know which directory to watch before it watches it.

Comment 35

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

That should be fine with bug 572162 also fixed though it would be possible to use a directory that is writeable by All Users such as ProgramData on Win7.

Comment 36

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

Come to think of it doing that isn't dependent on bug 572162

Comment 37

[[Baboo]]

(In reply to Justin Wood (:Callek) from comment #32)
> especially 3'rd party compilations of Firefox (that can't use the trademark).

Speaking of them, will they be even be able to use this service if it's going to check for (hardcoded?) signatures?

Comment 38

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

At this time we are primarily prototyping and the end result will allow others to use the service.

Comment 39

[Brian R. Bondy [:bbondy]]

Here is the list of things left to do that I'll be tackling this week:

- Make wiki page for the software update process flow.
- Ask to schedule a security review once the wiki page is up
- Limited user accounts should allow updates at all times, if the service is installed.
- Add NSPR logging throughout the base service code and main service logic
- Add NSPR logging into update process inside Firefox
- Make updater code have a separate silent uninstaller that shows up under add/remove programs.
- Make updater code have a checkbox option on install for whether to install it or not.
- If a limited user account installs without elevation, then don't install the service nor show the option in the installer.
- Add an about:config option (or use an existing one) for whether to use the service for updates or not.  
- Default the new about:config option to off, but turn it to on by default for Firefox in particular.
- Possibly add new UI in Firefox preferences for whether or not to use the service.
- Separate out the update and post update process, investigate doing the main update as the system account in session 0 and the post update process as the user's session using his token.
- Fix up directory we are monitoring to not be Firefox specific
- Don't always use the same name for the update meta files so that 2 different updates from 2 different channels or applications can happen at the same time.
- Investigate how to deal with x86 + x64 installs.  (Currently planned to use the newest version regardless of x86 or x64)
- Ensure that only one service gets installed at most.
- Move the service code under /toolkit/components/ as it may later be used for other things than just updating.
- Find a better more generalized name for the service, something like Mozilla Maintenance Service?
- Make updates stop the service, apply the update, and then restart the service so that the service itself can be replaced.  
- Add first time update install of the service
- Add code to only install/update the service if the version number is greater than the last installed version number for the service.
- Possibly add telemetry so we know I) who will be using the service and ii) who isn't using the service explicitly, and iii) who is trying to but falling back to the old way because of some error.
- Possibly add check to make sure we signed the updater.exe if Nightly builds get signed.
- Possibly do a new hash check on the MAR file which is being applied (will likely be done in the context of another bug ID)
- No longer copy the updater.exe outside of Program files to execute it, that way we don't need to do a hash check on the file since it (should be) installed in a location that requires elevation like Program Files.  Use different name in same directory before executing updater.exe from the service.

Comment 40

[Kelley Cook]

One additional thing to check for with the move to a service: a long standing annoyance** exists for Windows computers using Fast User Switch (multiple semi-simultaneous users).  The updater does not realize when there are multiple copies of Firefox running under different accounts.  After an update is downloaded, it prompts you to restart firefox, but since it attempts to overwrite still open files (e.g. DLLs), it fails.

The separate updater service should be able to overcome this problem.  Possibly by only launching the updater process when all the instances of Firefox are down and notifying the current user that other users are still running Firefox.

** I actually don't see this issue in bugzilla, I can do that tonight.

Comment 41

[Brian R. Bondy [:bbondy]]

I believe that we only apply updates currently if we can lock firefox.exe (open without sharing access).  So this would ensure it's not already in use.  But we probably fail to apply in the case you mentioned and the service could help here.  Sure please post a bug for this as the service can shutdown all instances across all windows sessions.  We can plan in the context of that ticket what to do exactly.

Comment 42

[Brian R. Bondy [:bbondy]]

A lot of people in comments of the blog post are asking for this, so adding to the list of things to do:

- Figure out how to allow the service to be started/ stopped from unelevated processes.
- Add ability to start the service only on demand before applying the update and then stop again right after.

Comment 43

[André Ziegler]

Why a service? Run a task with the task scheduler. I use this to workaround the UAC prompts and from what I see google also uses this way to update chrome.

Comment 44

[Brian R. Bondy [:bbondy]]

We have previously discussed windows scheduled tasks but decided to do a service. 

As far as I know there are 2 or 3 different APIs for Windows task scheduler for different Windows operatian elevated administrator process you need to enter the user's username and ng systems.  Also at least in some of those APIs to run as password.   We need a solution that will work on all operating systems Win2k and above consistently. A service seems to be the cleanest way to do that.  As per comment 42 the service will be very low overhead.  Unlike Google we usually install into Program Files which requires elevation to write to.

Comment 45

[Brian R. Bondy [:bbondy]]

Sorry I think that last message got garbled from drag and drop. 

Here it is again (corrected):

We have previously discussed windows scheduled tasks but decided to do a service.

As far as I know there are 2 or 3 different APIs for Windows task scheduler, each for different Windows operating systems.  

Also at least in some of those APIs to run as an elevated administrator process you need to enter the user's username and password.   We need a solution that will work on all operating systems Win2k and above consistently. A service seems to be the cleanest way to do that.  As per comment 42 the service will be very low overhead.  Unlike Google we usually install into Program Files which requires elevation to write to.

Comment 46

[Masatoshi Kimura [:emk]]

(In reply to Brian R. Bondy [:bbondy] from comment #45)
> Unlike Google we usually
> install into Program Files which requires elevation to write to.
Google Updater can update applications installed under Program Files without elevation or password.
Chrome is installed under user's profile by default to avoid UAC dialog on installation (NOT update).

Comment 47

[James Ross]

I would prefer a scheduled task (Vista+ at least) but if a service is the chosen route I'd like back the plan that it should be strictly demand-start. I believe you can do this by using SetServiceObjectSecurity to add the SERVICE_START permission for local authenticated users (DOMAIN_ALIAS_RID_USERS?) to the service's ACL. That will allow any local user to start the service, and the service can shut itself down when it completes the update.

Comment 48

[Brian R. Bondy [:bbondy]]

The ACE work was done earlier today and yes it was done using SetServiceObjectSecurity. The process is described already in the wiki for this service and silent updates here: https://wiki.mozilla.org/Windows_Service_Silent_Update

But that wiki is still heavily in progress, it would be better to read it tomorrow when I'm done with edits.

Comment 49

[André Ziegler]

(In reply to Brian R. Bondy [:bbondy] from comment #44)
> We have previously discussed windows scheduled tasks but decided to do a
> service. 

ok, thanks :)

Comment 50

[Joe Drew (not getting mail)]

I have one feature request, which is that as part of the checking-for-work-to-do, the updater also checks for further updates from whatever source Firefox uses (on a similar schedule so as not to DoS the updater web site); that way, we'll always have the most up-to-date Firefox, even if we haven't run Firefox lately.

Comment 51

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

(In reply to Joe Drew (:JOEDREW!) from comment #50)
That is bug 353804. The patch has a bug in it that I need to find time to investigate.

Comment 52

[Joe Drew (not getting mail)]

Sorry, I was not clear enough. My feature request is for a service that will always keep us up to date without needing to run Firefox, similar to what (IIUC) Chrome has.

Comment 53

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

A couple of things I don't want to tackle in this bug are the security attack vectors that come with having a service or task that has access to the network, how we allow a user to disable updating from within Firefox if there is a service that does the download (keep in mind that a single install can have multiple profiles and multiple users), and the warning when the update will disable add-ons.

So, Firefox product drivers would need to at the very least decide to do the following first:
1. remove the option from within Firefox to disable updates
2. remove the warning when an update will disable add-ons

These two issues are difficult enough to solve that I don't want to hold back the implementation as it stands today for them to be fixed or decided upon in order to have the service perform the download as well.

Comment 54

[Joe Drew (not getting mail)]

All that sounds great. I also wanted to make sure we didn't accidentally make any design decisions that precluded background update from happening.

Comment 55

[Justin Wood (:Callek)]

Brian,

Without reading too much I just want to comment a requirement for Gecko-Dependant Apps here. We can't *fully* rely on code-signing for this, at least not for SeaMonkey.

I am happy however to rely on it by default, pref it, whatever though. I just don't want us baking in a way that prevents SeaMonkey users from [auto-]updating simply because SeaMonkey has no access to a code-signing machine at present.

Comment 56

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

I am ok with there being a way for SeaMonkey to opt out of the signing requirement as long as it isn't so easy that a malicious app could opt out Firefox. I have no idea if this would be simple to implement and I am fairly certain that a pref definitely wouldn't be acceptable since a malicious app in the user session could change the pref.

Comment 57

[Justin Wood (:Callek)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #56)
> I am ok with there being a way for SeaMonkey to opt out of the signing
> requirement as long as it isn't so easy that a malicious app could opt out
> Firefox. I have no idea if this would be simple to implement and I am fairly
> certain that a pref definitely wouldn't be acceptable since a malicious app
> in the user session could change the pref.

Just to elaborate (since you're not on IRC atm)

I'm of the mindset that a malicious app on users computer means we have already lost :-). And we could require the pref to be app-dir only, like some other prefs we use/have relating to this stuff. In-fact the pref for Update Server restriction doesn't even follow that (profile-level prefs are enough)

In the end, I think if app-dir pref is ok, we should be good. Since in win7/Vista the Program Files dir is restricted by default, and if a malicious program can write/modify there as is, it is just as easy for it to also modify Firefox directly.

This said, if its *easier* to do ifdefs in code, I can accept that, but my experience is the more divergent codepaths exist, the easier it is to break them, especially when they are not part of Gecko/Firefox proper.

Comment 58

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

(In reply to Justin Wood (:Callek) from comment #57)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #56)
> > I am ok with there being a way for SeaMonkey to opt out of the signing
> > requirement as long as it isn't so easy that a malicious app could opt out
> > Firefox. I have no idea if this would be simple to implement and I am fairly
> > certain that a pref definitely wouldn't be acceptable since a malicious app
> > in the user session could change the pref.
> 
> Just to elaborate (since you're not on IRC atm)
:rs on irc... I got complaints about another :rsxxx on bugzilla causing multiple matches

> I'm of the mindset that a malicious app on users computer means we have
> already lost :-). And we could require the pref to be app-dir only, like
> some other prefs we use/have relating to this stuff. In-fact the pref for
> Update Server restriction doesn't even follow that (profile-level prefs are
> enough)
I agree when it comes to the data in question being under program files or in HKLM but not in relation to the profile.

If this were to be opt out I think it would be simplest and safest for it to be build time.

Comment 59

[Brian R. Bondy [:bbondy]]

> If this were to be opt out I think it would be simplest and safest for it to be build time.

A compile time #define flag sounds like a great way to disable the requirement for the exe you are launching to check signing.
There is a security risk there though as we are bypassing UAC, so if this is disabled, an alternate check should also be made like a check on the file hash of updater.exe.

> I'm of the mindset that a malicious app on users computer means we have already lost 

If we executed a malicious app instead of our own exe I think we would be blamed here, since we were the one that didn't show the user a UAC prompt.
I'm really only OK with a flag to disable it if an alternate security fix is implemented at the same time.

> In the end, I think if app-dir pref is ok, we should be good. Since in win7/Vista the Program Files dir is restricted
> by default, and if a malicious program can write/modify there as is, it is just as easy for it to also modify Firefox directly.

I was originally justifying it the same way, but I don't think that is enough justification.
The problem here is that WE will install into a non restricted location if the user is a limited user account.
Also the user can specify a non restricted location.  The former is not the user's fault, the later might be, but would in the end be our fault for not warning them or for allowing the service to be installed.

Comment 60

[Nigel]

(In reply to Brian R. Bondy [:bbondy] from comment #41)
> I believe that we only apply updates currently if we can lock firefox.exe
> (open without sharing access).  So this would ensure it's not already in
> use.  But we probably fail to apply in the case you mentioned and the
> service could help here.  Sure please post a bug for this as the service can
> shutdown all instances across all windows sessions.  We can plan in the
> context of that ticket what to do exactly.

Some people use Firefox Preloader applications to speed it working.  Would you not have a rule that if after a period of n hours that you have not been able to get exclusive access to the Firefox.EXE that on starting Firefox the user is advised that an update from a date of Y is still pending - so if at a kiosk, one could see how out of date Firefox was?

Also re the updater working as a service - unless you can 100% guarantee that only genuine Mozilla products will be actioned by the service, then corporates and most users who want security will disable the service

Comment 61

[alanjstr]

Would it be possible for the updater service to be responsible for downloading the updates, instead of the application?  If you're aware of an enhancement request for that, please let me know.

Comment 62

[Brian R. Bondy [:bbondy]]

> Some people use Firefox Preloader applications to speed it working...

I think there is some kind of notification eventually already.  This code is not being changed in any way, it will work the same as it used to.

> Unless you can 100% guarantee that only genuine Mozilla products will be actioned by the service

I think we will be 100% guaranteeing.

> Would it be possible for the updater service to be responsible for downloading the updates

I think this would be desirable, but is probably out of scope for this first release.

Comment 63

[Brian R. Bondy [:bbondy]]

Just wanted to sync up the todo list for this task. 

Remaining items are:
- Investigate how to deal with x86 + x64 installs.  (Currently planned to use the newest version regardless of x86 or x64)
- Separate out the update and post update process, investigate doing the main update as the system account in session 0 and the post update process as the user's session using his token.
- Limited user accounts should allow updates at all times, if the service is installed.
- If a limited user account installs without elevation, then don't install the service nor show the option in the installer.
- Possibly add new UI in Firefox preferences for whether or not to use the service.
- Version number: Need to check all 4 components NSVersionComparitor *maybe switch to it
- Add NSPR logging into update process inside Firefox (already added into the service itself)
- Support installing to a different directory?
- Add check to make sure we signed the updater.exe if Nightly builds get signed.
- Possibly do a new hash check on the MAR file which is being applied (will likely be done in the context of another bug ID)le since it (should be) installed in a location that requires elevation like Program Files.  Use different name in same directory before executing updater.exe from the service.
- Possibly add telemetry so we know I) who will be using the service and ii) who isn't using the service explicitly, and iii) who is trying to but falling back to the old way because of some error.

Recently done:
- Added a debug command line parameter so you can run the service exe normally when you want to debug.
- Removed progress and dialog resource from the service since those are unused and were copied over by accident from the updater.exe project rc file.
- Changed the service to be on demand only at creation time instead of auto start which it used to be 
- Added code so the service can be started by user processes (set the service to have user access DACL)
- Added code for reading version information of executables so that it can be used to see when the service needs to be updated
- Made new nsAutoHandle and nsAutoServiceHandle classes to cleanup code which may one day lead to handle leaks
- Made updates stop the service, apply the update  so that the service itself can be replaced. 
- Now load WTSQueryUser token dynamically so that it will work on Win2k 
- Got rid of several edge cases that had TODO comments in them
- Updated Firefox code to attempt to start the service before attempting to write the service command.
- Added NSPR logging throughout the base service code and main service logic
- Added a new bool preference (app.update.service) in firefox.js defaulted to true
  - Added code inside nsUpdateDriver.cpp to use this pref, if the pref doesn't exist it defaults to false.
  - If the pref is set to false it will not even attempt to use the service for updates
- Make uninstaller have a components page that you can select what to uninstall
- Made new component installer page with required main app component and optional maintenance checkbox.  
  - User can now pick to install the service or not.  
  - Only shows up if you go to Custom setup. 
  - Defaults to on.
- Fix up directory we are monitoring to not be Firefox specific
- Moved the service code under /toolkit/components/maintenanceservice as it may later be used for other things than just updating.
- Renamed everything to Mozilla Maintenance Service, and removed anything specific in filenames to updater
- Added install of the service when updating

Comment 64

[Brian R. Bondy [:bbondy]]

Attachment: Intermediate backup of service related work

This is just a backup of the work for this task, I will split the patch up into sections again once it is ready for review.

Comment 65

[Justin Wood (:Callek)]

(In reply to Brian R. Bondy [:bbondy] from comment #63)
> Just wanted to sync up the todo list for this task. 
> 
> Remaining items are:

> - Add check to make sure we signed the updater.exe if Nightly builds get
> signed.
> - Possibly do a new hash check on the MAR file which is being applied (will
> likely be done in the context of another bug ID)le since it (should be)
> installed in a location that requires elevation like Program Files.  Use
> different name in same directory before executing updater.exe from the
> service.

Just to be clear, SeaMonkey will need _a_ way to still have updates run even though even our releases are currently unsigned. If that is to add a new build-time define, sure. I say this again simply because you mentioned the "hash" thing would be a requirement of that, but you want to split that out.

Comment 66

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

(In reply to Justin Wood (:Callek) from comment #65)
Updates will still fallback to the current method if we aren't able to come up with a decent method for preventing this attack vector without using signing so SeaMonkey will still be able to update using the current method.

Comment 67

[Brian R. Bondy [:bbondy]]

Attachment: Ideas for optional install process

We need to make the installer for Firefox have the ability to optionally install the service.  This UX review is to get feedback on the best way to do that.  This is only a temporary solution that will be shipped until the new installer stub is ready.

I proposed some ideas in the attached screenshot.  Red circles in the attachment indicate each place we could put the new option.  Also acceptable would be a whole new page.

Also I need feedback on the uninstall process.  Whether there should be a separate item in add/remove programs, a components whole installer step, or just a checkbox.

Comment 68

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 566962 [details]
Ideas for optional install process

Let's just install the service, and then potentially allow users to stop it from somewhere in advanced preferences.  The problem with introducing it here is:

-increased complexity to the install process
-there is no way to undo the action later

Comment 69

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

Alex, there has been a decent amount of flack type comments regarding not providing an opt out option in the installer and I would prefer not to alienate these people.

Also, this will be installed by the installer anyways which is what the installer is good at and the increased complexity is actually quite small for this because of this.

There will be a way to uninstall this via Add / Remove Programs and Programs and Features.

Adding an option in advanced preferences on the other hand requires Firefox to launch a separate binary to perform the task of disabling it. This is somewhat complicated.

Comment 70

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

Comment on attachment 566962 [details]
Ideas for optional install process

Alex, please reconsider after reading comment #69. I'm rather dead-set against the approach you suggested.

Comment 71

[Alex Faaborg [:faaborg] (Firefox UX)]

>I would prefer not to alienate these people.

as would I, my premise being that establishing a pref inside of Firefox provides more control because they always have access to where the preference is (uninstalling the entire application by contrast isn't as much control).

>Adding an option in advanced preferences on the other hand requires Firefox to launch
>a separate binary to perform the task of disabling it. This is somewhat complicated.

If it is only somewhat complicated then I think it might be worth it as it provides more user control.  We could also add that level of control later in addition to a check box in the installer.

Comment 72

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 566962 [details]
Ideas for optional install process

let's go with just the third screen in (choose components, which is in the flow after they select custom install).

Comment 73

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

(In reply to Alex Faaborg [:faaborg] (Firefox UX) from comment #71)
> >I would prefer not to alienate these people.
> 
> as would I, my premise being that establishing a pref inside of Firefox
> provides more control because they always have access to where the
> preference is (uninstalling the entire application by contrast isn't as much
> control).
I am ok with that but these people do not want the service installed at all. When the service is already installed whether it is from another Mozilla based application or a second installation of Firefox then the option would not be displayed.

The plan is for the service to only run on demand already and I am fine with having a pref that makes Firefox not use the service when it is installed.

> 
> >Adding an option in advanced preferences on the other hand requires Firefox to launch
> >a separate binary to perform the task of disabling it. This is somewhat complicated.
> 
> If it is only somewhat complicated then I think it might be worth it as it
> provides more user control.  We could also add that level of control later
> in addition to a check box in the installer.
See above. Having Firefox not use the service I am fine with.

Comment 74

[Dave Garrett]

Adding an option to the installer is pretty straightforward, but what about the first installation of the service via Firefox update?

Comment 75

[Brian R. Bondy [:bbondy]]

> but what about the first installation of the service via Firefox update?

The plan is currently to install it via the update for existing users, and people can remove it if they don't want it.

> The plan is for the service to only run on demand already and I am fine with
> having a pref that makes Firefox not use the service when it is installed

I think that pref is independent of the instsall option. I'll put another proposed screenshot for that pref for UX review.
Rob are you OK with the components page on install and a separate uninstall (for the current temporary installer). If not how about a new custom page that just has a checkbox and its own step without the list box?

Comment 76

[Brian R. Bondy [:bbondy]]

Attachment: A proposed new option for whether or not the product should use the silent update service. Only shows up if service is installed.

Comment 77

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

We definitely can't use that NSIS provided ui used in the screenshot unless it has had some fairly major rework done to it sometime in the last few years. We tried to when I implemented the installer for Firefox 2 and had to remove it since it isn't screen reader friendly.

Creating a custom page as is done for the shortcuts page would be a decent alternative. Might be a good thing to include some explanation about why it is a good thing not to uncheck the box on the page as well. Perhaps dynamically show it as is done for the profile removal in the uninstaller so it calls attention to itself if the user unchecks it.

Also, I don't think we want to show the page on install if the service is already installed.

Comment 78

[Brian R. Bondy [:bbondy]]

OK I'll proceed with the new page similar to the component selection page but screen reader friendly.  (i.e. the same idea as the shortcut page). It'll have extra explanation and context inline on the page we can tweak later.

For the uninstaller it will show up as a separate product.

If you have any objections faaborg please let us know.

> Also, I don't think we want to show the page on install if the service is already installed.

Good catch, yup I agree.

Comment 79

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

Comment on attachment 567232 [details]
A proposed new option for whether or not the product should use the silent update service.  Only shows up if service is installed.

Brian, I recall you mentioning plans to bypass the UAC when manually applying an update if the service was present as well. If that is the case then it would make sense to align it with the radio buttons. Also, the ui changed recently in bug 600505.

Comment 80

[Brian R. Bondy [:bbondy]]

Attachment: Patch for service certificate check on updater.exe and related logic

Here is the certificate check code implemented.  It checks to make sure the information on the file we are executing's cert is correct and also checks to make sure it is trusted.

It is currently hard coded to the Mozilla info, but I have a TODO comment in there talking about moving that into HKLM registry.

This is a pretty early patch so I'm pretty sure it will have a security-review-minus for now but I would really appreciate any feedback and help on if I'm checking the certs correctly and if I'm susceptible to any sort of attacks.

When you look at the patch, it's easiest to read it if you start with:
> diff --git a/toolkit/components/maintenanceservice/workmonitor.cpp

Then follow the implementations of CheckCertificateForPEFile and  VerifyCertificateTrustForFile.

On a side note it might be worth also having this check from within Firefox and if it fails don't even try to use the service (fall back to the old method).  The check would be done as well in the service regardless.  If you agree it should be in Firefox as well, please advise on the best place in the tree to put the related PE file certificate check code (certificatecheck.cpp/.h).

Comment 81

[Brian R. Bondy [:bbondy]]

>  If that is the case then it would make sense to align it with the radio buttons

It doesn't do this now but that makes sense.  I'll align it with the radio buttons as you suggested. We can change it later if we want.

Comment 82

[Brian R. Bondy [:bbondy]]

The r? for security is only looking for feedback, I just didn't have a second feedback? field to use.  So please only provide feedback for now then cancel the flag.

Comment 83

[Brian R. Bondy [:bbondy]]

Comment on attachment 567241 [details] [diff] [review]
Patch for service certificate check on updater.exe and related logic

Didn't realize you could do comma separated fields for feedback/reviews, thx for the tip callek.

Comment 84

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 567232 [details]
A proposed new option for whether or not the product should use the silent update service.  Only shows up if service is installed.

this is too detailed, let's leave off silent, which is controversial, UAC, which is jargon, and branding it as Mozilla Maintence, and just say:

"Use a background service to install the update"

Comment 85

[Justin Wood (:Callek)]

(In reply to Alex Faaborg [:faaborg] (Firefox UX) from comment #84)
> Comment on attachment 567232 [details]
> 
> "Use a background service to install the update"

bikeshed: "Use a background service to install updates"

Comment 86

[Brian R. Bondy [:bbondy]]

Attachment: Screenshot: New installer step inside Firefox installer

Comment 87

[Brian R. Bondy [:bbondy]]

Attachment: Screenshot: Program files directory showing output files of Maintenance Service Installer

Comment 88

[Brian R. Bondy [:bbondy]]

Attachment: Screenshot: Showing up separately inside Add/Remve programs (icon pending)

Comment 89

[Brian R. Bondy [:bbondy]]

Attachment: New installer process pretty much completed minus some code cleanup

Attached is the new installer work redone.  

The installer process works by having a completely separate installer for the maintenance service.
You can run this standalone if you want with a GUI, but typically it will be run silently.
This new installer shows up in add/remove programs as Mozilla Maintenance Service.
 
The existing Firefox installer has a new property page the same as the shortcuts page but asking if you want to install the maintenance service.
The new NSIS maintenance service installer gets included and executed silently if the checkbox for Install Maintenance service is specified.

For feedback I'm looking for a general "yup you're in the right direction" or not. 
Also I'd like to know if there's a better way inside toolkit/mozapps/installer/windows/nsis/makensis.mk
from what I'm doing.

In particular in the new NSI I'm using File directives and to get those to work I have to copy the files I want into the installer dir makensis.mk:
$(INSTALL) $(addprefix $(shell pwd)/../../../dist/bin/,maintenanceservice.exe nspr4.dll) $(CONFIG_DIR)
+
I'd like to get rid of that line from makensis.mk for cleaner code.

Comment 90

[Brian R. Bondy [:bbondy]]

Attachment: New installer process pretty much completed minus some code cleanup. v2

Same as last patch w/ minor fix for default install service value and proper checking if it is checked.

Comment 91

[Ian Melven :imelven]

(In reply to Brian R. Bondy [:bbondy] from comment #64)
> Created attachment 566880 [details] [diff] [review] [diff] [details] [review]
> Intermediate backup of service related work
> 
> This is just a backup of the work for this task, I will split the patch up
> into sections again once it is ready for review.

i took a pass through this patch and the earlier iterations of it and sent bbondy a couple of small pieces of feedback. 

my primary findings security wise are that the service ACL allows access from the "Users" group (all authenticated users on the machine), which means any user can start, stop, etc. the service. If the service isn't running and can't be started the installer will fall back to the regular install with a UAC prompt. My chief concern is still the session ID spoofing mechanism, since any user can drop an update file with an arbitrary session ID into the 'pickup' directory, the post install actions can be run by an arbitrary user as an arbitrary user. This means it's crucial an attacker can't influence what actions are run or the parameters to these actions. Additionally, the service is running as SYSTEM so before landing, these needs another more thorough code review to look for input validation/overflow type problems.

Comment 92

[Brian R. Bondy [:bbondy]]

Comment on attachment 567667 [details]
Screenshot: New installer step inside Firefox installer

Adding UX review for new installer step.
Pls + w/ nits if it is just text changes

Comment 93

[Brian R. Bondy [:bbondy]]

Attachment: A proposed new option for whether or not the product should use the silent update service. Only shows up if service is installed. v2

Proposed changes to the preferences dialog. 
This one is not a mockup but actually working.
Please ui-review+ w/ nits if just text changes.

Comment 94

[Ian Melven :imelven]

Comment on attachment 567241 [details] [diff] [review]
Patch for service certificate check on updater.exe and related logic

i took a pass through the patch, the windows-y/c++ part of it looks great.

from my reading, the code takes a hardcoded issuer, cert name, and signer info (program name, publisher and more info link), and uses this to look up a cert with matching details in the OS cert store, it then checks that the retrieved cert matches the hardcoded data it's looking for. if the cert specifies program and publisher info, this is also verified against the hardcoded set we are looking for. the code also verifies the signing certificate on the file chains to a root cert in the OS root store and that the certificate has the ability to sign code. it does not check for cert revocation. 

bbondy, can you elaborate on why in VerifyCertifcateTrustForFile an error TRUST_E_TIME_STAMP ends up saying the file is validated ?

also, your comment questioning the handling of CRYPT_E_SECURITY_SETTINGS is interesting. from searching it appears that AV programs and 3rd party firewalls can often result in this error - it appears that this error results from an admin not trusting the cert and prohibiting the user from trusting the cert - s my inclination is that this should NOT verify the file - however i'm wary of possible support issues if something messes up the cert store, BUT i also found postings saying that this error occurred when trying to update windows itself. so overall, my opinion is that we should fail the verification if this error occurs.

one more question, how does the signer cert initially get into the OS cert store ? additionally, what's the reasoning behind trying to find the match in the OS cert store and using that to compare against the hardcoded info rather than comparing the actual signing cert on the binary itself ? 

adding a feedback? to bsmith to take a look at the crypto specific code/functionality of the patch.

Comment 95

[Brian R. Bondy [:bbondy]]

Thanks for the feedback.

> from my reading...
 
Yup.

> bbondy, can you elaborate on why in VerifyCertifcateTrustForFile an error TRUST_E_TIME_STAMP ends up saying the file is validated ?

What I was concerned with here is an update that is installed after a certificate is expired, or a downgrade. If you think it is safest though I can return an error in that case and it would fall back to the normal update process without the service.

>  so overall, my opinion is that we should fail the verification if this error occurs.

OK I'll change that.  I'd rather only use this service update when we're sure it's safe in all cases. 

> one more question, how does the signer cert initially get into the OS cert store ?

I'm not sure about this, but my understadning is that there are several preloaded trusted authorities.  I have limited authenticode knowledge though.
This is also the first time I write any certificate checking code and I relied on several MSDN code samples that I pulled together.

> what's the reasoning behind trying to find the match in the OS cert store and using that to compare against the hardcoded info rather than comparing the actual signing cert on the binary itself

I think by doing it that way we have access to more information.

Comment 96

[Robert Kaiser]

(In reply to Ian Melven :imelven from comment #94)
> from my reading, the code takes a hardcoded issuer, cert name, and signer
> info (program name, publisher and more info link)

Is there some mechanism to allow e.g. Firefox to switch the cert or issuer in case of expiring or the issuer becoming unused (we recently had a case where SeaMonkey's update service needed to switch issuers because the previous one has been phased out)?

Comment 97

[Brian R. Bondy [:bbondy]]

> Is there some mechanism to allow e.g. Firefox to switch the cert or issuer in case of expiring or the issuer becoming unused

There's a new patch coming later today that will store the info for allowed certs in a trusted location in HKLM.  It's already done actually just needs testing. 

So yes we can update that location as we go, and add new certs for other prodcuts there as well.

Comment 98

[Ian Melven :imelven]

(In reply to Brian R. Bondy [:bbondy] from comment #95)
>
> What I was concerned with here is an update that is installed after a
> certificate is expired, or a downgrade. If you think it is safest though I
> can return an error in that case and it would fall back to the normal update
> process without the service.

right, i think an expired cert should probably fail here. as far as downgrades, i'd be concerned about an arbitrary user on the machine forcing a downgrade, which seems bad. i'm not sure forcing the UAC path on a downgrade necessarily helps here though.

> I'm not sure about this, but my understadning is that there are several
> preloaded trusted authorities.  I have limited authenticode knowledge though.
> This is also the first time I write any certificate checking code and I
> relied on several MSDN code samples that I pulled together.

right, the root cert we use to validate the cert will be preloaded in the OS cert store but our signing cert most likely won't, i would think.
 
> I think by doing it that way we have access to more information.

that was my assumption, that we could get the extended fields like the url and such from the cert itself in the store but not necc. from the cert on the binary itself.

Comment 99

[Brian R. Bondy [:bbondy]]

> Re:imelven cert changes:

Fixed both of these in favor of being more restrictive.

Comment 100

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Why are certs involved at all as opposed to hard-coding the Mozilla code signing public key into the service?

Comment 101

[Ian Melven :imelven]

(In reply to Henri Sivonen (:hsivonen) from comment #100)
> Why are certs involved at all as opposed to hard-coding the Mozilla code
> signing public key into the service?

one reason is that it gives us the ability to revoke or change the cert we use while not requiring a respin of the installers and allowing existing installers in the wild to continue functioning.

Comment 102

[Brian R. Bondy [:bbondy]]

> Why are certs involved at all as opposed to hard-coding the Mozilla code signing public key into the service?

Thanks for the suggestion. 
I am not an expert in this area so I will leave divergence from the current Authenticode certificate sign checks patch for the security team to advise further on.

Comment 103

[Brian R. Bondy [:bbondy]]

The install process is working and tested (just by me) now; works as follows:

Administrative users or Limited Users who are elevated when installing:
The optional service install page will show up unless the service is already installed.
If the service is not already installed the user can chose whether or not to install the service.
If the service is already installed the page will be skipped but the service will be upgraded.
If the user is attempting to install an older build when a newer service is already installed, the service will not be replaced. 

Limited users who are not elevated:
- The optional service page will NOT show up and the service will NOT be installed.

Comment 104

[Brian R. Bondy [:bbondy]]

Attachment: Intermediate backup of service related work

This is a backup of the code.  
The final code is still in progress and once done will be split up end of this week for review.

Ian there is some new code related to registry + authenticode checks.
The main code for this is in registrycertificates.cpp.
The other suggestions were updated inside of certificatecheck.cpp.
And then workmonitor.cpp was changed from hard coded info check to using CheckIfBinaryMatchesAllowedCertificates.

Rob if you are interested you can checkout the installer work early.

Comment 105

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

Comment on attachment 567667 [details]
Screenshot: New installer step inside Firefox installer

Checkboxes are aligned with the text in the installer. Otherwise it looks good though faaborg still needs to approve the text. I personally think that Optional Recommended Components is a tad awkward.

Comment 106

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

Comment on attachment 567740 [details]
A proposed new option for whether or not the product should use the silent update service. Only shows up if service is installed. v2

Please update this since the new UI has already landed on nightly. Also, make sure the checkbox looks a little to the left of where it should be.

Comment 107

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

Comment on attachment 567668 [details]
Screenshot: Program files directory showing output files of Maintenance Service Installer

Though it is unlikely it is possible that another binary for a different app might be named maintenanceservice.exe. Might be a good thing to name it mozmaintenanceservice.exe or mozmaintsvc.exe.

What do you think?

Comment 108

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

Comment on attachment 567669 [details]
Screenshot: Showing up separately inside Add/Remve programs (icon pending)

Looks good. I think it is best to not bother with putting the version in there like we do with Firefox since the only reason we do that with Firefox is so it is easy to differentiate side by side installs which won't be supported for the maintenance service.

Comment 109

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

Comment on attachment 567241 [details] [diff] [review]
Patch for service certificate check on updater.exe and related logic

Instead of feedback reviewing this I am going to try to feedback review "Intermediate backup of service related work" - attachment #568174 [details] [diff] [review]

Comment 110

[Brian R. Bondy [:bbondy]]

Regarding the nsis service installer page feedback:

> Checkboxes are aligned with the text in the installer.

The alignment of the checkbox vs the text is currently consistent with the shortcuts page.

Please let me know your preference:
a) Align the service page checkbox more to the left with the above text, to be inconsistent with the shortcuts page?
b) Change the alignment on both the service page and the shortcuts page to be aligned with the text?
c) Leave as is in that screenshot?

Comment 111

[Brian R. Bondy [:bbondy]]

Attachment: Service checkbox; Preferences page option; v3.

Comment 112

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

Comment on attachment 568253 [details]
Service checkbox; Preferences page option; v3.

Nice! In the past, we've put additional space between elements to denote the different functionality but I don't know if that is the case now so leaving that to faaborg.

Comment 113

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

(In reply to Brian R. Bondy [:bbondy] from comment #110)
> Regarding the nsis service installer page feedback:
> 
> > Checkboxes are aligned with the text in the installer.
> 
> The alignment of the checkbox vs the text is currently consistent with the
> shortcuts page.
> 
> Please let me know your preference:
> a) Align the service page checkbox more to the left with the above text, to
> be inconsistent with the shortcuts page?
> b) Change the alignment on both the service page and the shortcuts page to
> be aligned with the text?
> c) Leave as is in that screenshot?
That's a bumer. :)

The main reason we went with aligning the checkbox with the installer is to allow more space for the locales that need it. I'll leave that to faaborg but I *think* I prefer going with aligning them and just fixing the shortcuts page.

Comment 114

[Brian R. Bondy [:bbondy]]

Attachment: Showing up separately inside Add/Remve programs. v2.

Here's a better screenshot of the add/remove programs showing all fields and the icon.

Comment 115

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

Comment on attachment 568265 [details]
Showing up separately inside Add/Remve programs. v2.

Looks good!

Comment 116

[Ian Melven :imelven]

Comment on attachment 568174 [details] [diff] [review]
Intermediate backup of service related work

in DoesCertificateMatch, it seems a little strange that if no infoToMatch.issuer and infoToMatch.name are passed it, TRUE is returned. from the calling code it looks like this probably can't happen since we error if we can't get the signer info previously. 

the new code looks good to me, it goes through the allowed signing certs we have stored in the registry and returns TRUE on finding the first one that is successfully verified. The registry key used is under HKLM so non-Admin users should not have access to it by default.

Comment 117

[Brian R. Bondy [:bbondy]]

Attachment: All changes in one for easy applying

Comment 118

[Brian R. Bondy [:bbondy]]

Attachment: Patch 1 - Base service code

Comment 119

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2 - Main service logic

Comment 120

[Brian R. Bondy [:bbondy]]

Attachment: Patch 3 - Code for installing the service

Comment 121

[Brian R. Bondy [:bbondy]]

Attachment: Patch 4 - Certificate check code inside service

Comment 122

[Brian R. Bondy [:bbondy]]

Attachment: Patch 5 - XUL Runtime Environment code

Comment 123

[Brian R. Bondy [:bbondy]]

Attachment: Patch 6 - Firefox preferences update page changes

Comment 124

[Brian R. Bondy [:bbondy]]

Attachment: Patch 7 - RAII base helpers

Comment 125

[Brian R. Bondy [:bbondy]]

Attachment: Patch 8 - NSPR wide string logging support on Windows

Comment 126

[Brian R. Bondy [:bbondy]]

Attachment: Patch 9 - Firefox installer/uninstaller changes

Comment 127

[Brian R. Bondy [:bbondy]]

Attachment: Patch 10 - Shared installer code and service upgrade

Comment 128

[Brian R. Bondy [:bbondy]]

Attachment: Patch 11 - New maintenance service installer

Comment 129

[Brian R. Bondy [:bbondy]]

Attachment: Patch 12 - Simple SC plugin inclusion

Comment 130

[Brian R. Bondy [:bbondy]]

Attachment: Patch 13 - Moving the update check after init

Comment 131

[Brian R. Bondy [:bbondy]]

Attachment: Patch 14 - Build process

Comment 132

[Brian R. Bondy [:bbondy]]

All the patches thus far are uploaded.
The task isn't 100% complete, but it is working and ready for review while I finish up minor things in a couple follow up patches. 

I'm building a release config installer right now for testing as well which I'll upload later today.  It will include a signed updater.exe from FF7 so should work out of the box.

Comment 133

[Alex Faaborg [:faaborg] (Firefox UX)]

Some questions as part of the ui-review:

Does this windows service do anything other than update the application, do we have any plans for hang detection, or performance telemetry, or anything else? (trying to get a sense of if we should call it an update service since that is a more literal name).  Does it only update Firefox?

Where in the install wizard does "setup optional components" appear?  You can reference the numbers here: https://bug513414.bugzilla.mozilla.org/attachment.cgi?id=397421

I'm assuming we didn't group "use a background service to install updates" under automatic install because we might still use the service to apply manual installs?  At the very least we should perhaps put it under the button for update history so that it doesn't visually group as one of the top level options (3 radio buttons and a checkbox accidentally parses as 4 radio buttons)

Comment 134

[Ian Melven :imelven]

is the service executable going to be signed also ?

Comment 135

[Brian R. Bondy [:bbondy]]

> is the service executable going to be signed also ?

I think all EXE and DLL get signed, but I'm not sure if I need to add another bug specifically for the service.  Should I add a bug for releng for that Rob? (that's not important for me in nightly but just in general)

Comment 136

[Brian R. Bondy [:bbondy]]

> Does this windows service do anything other than update the application, 
> do we have any plans for hang detection, or performance telemetry, or anything else?

Yes we have plans for other things, but for the initial release it will only be for silent updates without UAC prompts.

> Does it only update Firefox?

Initially it will only be launched for Firefox but it has support initially for other programs as well.  It will be used by more than Firefox.


> Where in the install wizard does "setup optional components" appear?  
> You can reference the numbers here: 
> https://bug513414.bugzilla.mozilla.org/attachment.cgi?id=397421

It would appear as a new step 5.5 just before the shortcuts page (only if the user selects custom).

> I'm assuming we didn't group "use a background service to install updates" 
> under automatic install because we might still use the service to apply manual installs?  

That is correct.

> At the very least we should perhaps put it under the button for update history so that it doesn't visually 
> group as one of the top level options (3 radio buttons and a checkbox accidentally parses as 4 radio buttons)

Sounds good, I was uncomfortable having it right under the radio buttons as well.
I'll move the checkbox under the update history button.

Comment 137

[Brian R. Bondy [:bbondy]]

You can see a working demo on your computer here:
http://people.mozilla.com/~bbondy/InstallerWithService.zip

Instructions:
0. If UAC is off, turn UAC on with default value. Reboot.
1. Install the installer firefox-10.0a1.en-US.win32.installer.exe
2. Stage the update by extracting Nightly.zip into:
C:\Users\<username>\AppData\Local\Mozilla\Firefox
After extracting, your path should look like:
C:\Users\<username>\AppData\Local\Mozilla\Firefox\Nightly\updates\0
3. Do something like mark firefox.exe as hidden so you will know it got replaced.
4. Open Firefox to begin the update.

No UAC prompt should be shown. You should see a progress bar currently but that will probably be disabled later.

If this doesn't work, enable NSPR logging and get back to me with the log file.
For more info on NSPR logging, see the Logging section at: http://www.brianbondy.com/mozilla/cheatsheet/
The NSPR module name is: nsFirefoxService

Comment 138

[Ahmad Saleem]

Does not want to interrupt you developers, but isn't "Mozilla Update" better name than "Maintenance Service"?
Also Keep up the good work!

Comment 139

[alanjstr]

The service has the potential to do more than just update, such as other maintenance tasks.

Comment 140

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 567667 [details]
Screenshot: New installer step inside Firefox installer

Not ideal for us to add an extra step, but we'll be landing the stub installer at some point in the future.  so ui-review+ on the assumption that this is going to eventually get collapsed into a single options dialog in the stub installer.

Comment 141

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 568253 [details]
Service checkbox; Preferences page option; v3.

looks good, just need to move down to below the update history button to break the  visual grouping (gestalt principal of proximity for the cog sci geeks playing along at home)

Comment 142

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 568265 [details]
Showing up separately inside Add/Remve programs. v2.

This is fine for now, we're going to want to roll out a new icon for the service in the future, but we obviously don't need to block on that.

Comment 143

[Brian R. Bondy [:bbondy]]

Attachment: Screenshot: Services checkbox moved under show update history button

Was already +'ed by faaborg so just marking as +.  Including the updated screenshot just in case there is further feedback.

Comment 144

[Brian R. Bondy [:bbondy]]

Attachment: Patch 6 - Firefox preferences update page changes. v2.

Moved checkbox under update history button.

Comment 145

[Brian R. Bondy [:bbondy]]

Re: Ian:
> in DoesCertificateMatch, it seems a little strange that if no infoToMatch.issuer
> and infoToMatch.name are passed it, TRUE is returned. 
> from the calling code it looks like this probably can't happen since we
> error if we can't get the signer info previously

The reasoning was because you can specify any of those 5 criteria for people who want to add cert checking but sign in different ways than we do.  It was more recently updated to NULL or empty string returns TRUE.

Comment 146

[Brian R. Bondy [:bbondy]]

re: rs
> Though it is unlikely it is possible that another binary for a different 
> app might be named maintenanceservice.exe. Might be a good thing to name 
> it mozmaintenanceservice.exe or mozmaintsvc.exe.

The same name wouldn't cause a problem for the exe name.
The svcname is MozillaMaintenance and the friendly name is Mozilla Maintenance Service.  Only the svcname could conflict with other services and not the exe name.  So I think we should just leave as is.  But if you prefer the moz prefix I can add that.

Comment 147

[Ian Melven :imelven]

(In reply to Brian R. Bondy [:bbondy] from comment #145)
> Re: Ian:
> > in DoesCertificateMatch, it seems a little strange that if no infoToMatch.issuer
> > and infoToMatch.name are passed it, TRUE is returned. 
> > from the calling code it looks like this probably can't happen since we
> > error if we can't get the signer info previously
> 
> The reasoning was because you can specify any of those 5 criteria for people
> who want to add cert checking but sign in different ways than we do.  It was
> more recently updated to NULL or empty string returns TRUE.

makes sense, thanks for the explanation.

Comment 148

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2 - Main service logic. v2

- Wasn't stopping the service after the update, fixed.
- Now wait for return code of updater.exe before returning for better error handling and also better logging.
- Added compile time flag DISABLE_SERVICE_AUTHENTICODE_CHECK for when running on try tests
- Some code cleanup / refactoring

Comment 149

[Brian R. Bondy [:bbondy]]

Attachment: Patch 15 - Added support for Limited user account upgrades and improved handling of tokens

- Added new UACHelper class with new code for determining token types and user types
- Added handling for ensuring the token we're using is an elevated token
- Added handling for limited user account upgrades

Comment 150

[Brian R. Bondy [:bbondy]]

Attachment: Patch 9 - Firefox installer/uninstaller changes. v2.

Summary of changes since v1 of the patch:
- Maintenance service page aligned with text to the left instead of having 15px margin 
- Shortcuts page checkboxes aligned with text to the left instead of having 15px margin
- Installation type radio buttons aligned with text to the left instead of having 15px margin.  
  Associated text for each radio button moved from 30px to 15px.

Comment 151

[Brian R. Bondy [:bbondy]]

Attachment: Screenshot of affected pages for installer alignment changes

Radio button and checkbox alignment changes.

For context of why this change was done, please see:
comment 105, comment 110, comment 113, and comment 150.

Comment 152

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

(In reply to Brian R. Bondy [:bbondy] from comment #146)
> re: rs
> > Though it is unlikely it is possible that another binary for a different 
> > app might be named maintenanceservice.exe. Might be a good thing to name 
> > it mozmaintenanceservice.exe or mozmaintsvc.exe.
> 
> The same name wouldn't cause a problem for the exe name.
> The svcname is MozillaMaintenance and the friendly name is Mozilla
> Maintenance Service.  Only the svcname could conflict with other services
> and not the exe name.  So I think we should just leave as is.  But if you
> prefer the moz prefix I can add that.
Understood... the reason I asked for this is *if* we ever need to deal with the process from the installer it would be even more likely to be unique and as a side benefit it is easier to spot and associate to Mozilla in the task manager whereas maintenanceservice.exe is so generic that people would have to investigate to figure out what the process is.

Comment 153

[Brian R. Bondy [:bbondy]]

Gotcha ok, I'll do that change towards the end as it'll need a refresh of most of the patches.

Comment 154

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

Comment on attachment 568676 [details] [diff] [review]
Patch 1 - Base service code

>diff --git a/toolkit/components/maintenanceservice/maintenanceservice.cpp b/toolkit/components/maintenanceservice/maintenanceservice.cpp
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/maintenanceservice/maintenanceservice.cpp
>@@ -0,0 +1,285 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Application Update service.
s/Application Update service/Mozilla Maintenance service/.
Replace here and elsewhere in the license headers

>...
>+int wmain(int argc, WCHAR **argv)
>+{
>+#ifdef PR_LOGGING
>+  if (!gServiceLog) {
>+    gServiceLog = PR_NewLogModule("nsFirefoxService");
s/nsFirefoxService/nsMaintenanceService/
Here and elsewhere

>+  }
>+#endif
>+
>+  // If command-line parameter is "install", install the service
>+  // or upgrade if already installed.
>+  // If command-line parameter is "upgrade", upgrade the service
>+  // but do not install it if it is not already installed.
>+  // If command line parameter is "uninstall", uninstall the service.
>+  // Otherwise, the service is probably being started by the SCM.
>+  if (lstrcmpi(argv[1], L"install") == 0) {
>+    
>+    PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+      ("Installing service..."));
nit: for consistency, start code immediately after conditional here and elsewhere.

>+
>+DWORD WINAPI StartMonitoringThreadProc(LPVOID param) 
>+{
>+  StartDirectoryChangeMonitor();
>+  return 0;
>+}
>+
>+// Entry point for the service
>+void WINAPI SvcMain(DWORD dwArgc, LPWSTR *lpszArgv)
>+{
>+#ifdef PR_LOGGING
>+  if (!gServiceLog) {
>+    gServiceLog = PR_NewLogModule("nsFirefoxService");
s/nsFirefoxService/nsMaintenanceService/

>+  }
>+#endif
>+
>+  // Register the handler function for the service
>+  gSvcStatusHandle = RegisterServiceCtrlHandlerW(SVC_NAME, SvcCtrlHandler);
>+  if (!gSvcStatusHandle) {
>+    PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+      ("RegisterServiceCtrlHandler failed (%d)", ::GetLastError()));
>+    return; 
>+  } 
>+
>+  // These SERVICE_STATUS members remain as set here
>+  gSvcStatus.dwServiceType = SERVICE_WIN32_OWN_PROCESS;
>+  gSvcStatus.dwServiceSpecificExitCode = 0;
>+
>+  // Report initial status to the SCM
>+  ReportSvcStatus(SERVICE_START_PENDING, NO_ERROR, 3000);
>+
>+  // Perform service-specific initialization and work.
>+  SvcInit(dwArgc, lpszArgv);
>+}
>+
>+// Service initialization
>+void SvcInit(DWORD dwArgc, LPWSTR *lpszArgv)
>+{
>+  // Be sure to periodically call ReportSvcStatus() with 
>+  // SERVICE_START_PENDING. If initialization fails, call
>+  // ReportSvcStatus with SERVICE_STOPPED.
This comment is vague especially regarding what periodically actually means.

The second half regarding SERVICE_STOPPED should be before the call to ReportSvcStatus when initialization fails and it should note that initialization has failed.

>+
>+  // Create an event. The control handler function, SvcCtrlHandler,
>+  // signals this event when it receives the stop control code.
>+  ghSvcStopEvent = CreateEvent(
>+    NULL,    // default security attributes
>+    TRUE,    // manual reset event
>+    FALSE,   // not signaled
>+    NULL);   // no name
>+
>+  if (NULL == ghSvcStopEvent) {
>+    ReportSvcStatus(SERVICE_STOPPED, NO_ERROR, 0);
Is NO_ERROR correct for this case?

>+    return;
>+  }
>+
>+  DWORD threadID;
>+  HANDLE thread = ::CreateThread(NULL, 0, StartMonitoringThreadProc, 0, 0, &threadID);
>+
>+  // Report running status when initialization is complete.
>+  ReportSvcStatus(SERVICE_RUNNING, NO_ERROR, 0);
>+
>+
>+  // Perform work until service stops.
>+  for(;;) {
>+    // Check whether to stop the service.
>+    WaitForSingleObject(ghSvcStopEvent, INFINITE);
>+
>+    WCHAR stopFilePath[MAX_PATH +1];
>+    if (!GetUpdateDirectoryPath(stopFilePath)) {
>+      PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+        ("Could not obtain update directory path, terminating thread forcefully."));
>+      TerminateThread(thread, 1);
>+    }
>+
>+    gServiceStopping = true;
>+    if (!::PathAppendSafe(stopFilePath, L"stop")) {
Please add a comment regarding the purpose of the stop file.

>+      TerminateThread(thread, 2);
>+    }
>+    HANDLE stopFile = CreateFile(L"stop", GENERIC_READ, 0, 
>+                                 NULL, CREATE_ALWAYS, 0, NULL);
stopFilePath?

>+    if (stopFile == INVALID_HANDLE_VALUE) {
>+      PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+        ("Could not create stop file, terminating thread forcefully."));
>+      TerminateThread(thread, 3);
>+    } else {
>+      CloseHandle(stopFile);
>+      DeleteFile(stopFilePath);
>+    }
>+
>+    ReportSvcStatus(SERVICE_STOPPED, NO_ERROR, 0);
>+    return;
>+  }
>+}
>+
>+// Sets the current service status and reports it to the SCM.
>+// Parameters:
>+//   dwCurrentState - The current state (see SERVICE_STATUS)
>+//   dwWin32ExitCode - The system error code
>+//   dwWaitHint - Estimated time for pending operation, 
>+//     in milliseconds
Please use a javadoc style comment

>+void ReportSvcStatus(DWORD dwCurrentState, 
>+                     DWORD dwWin32ExitCode, 
>+                     DWORD dwWaitHint)
>+{
>+  static DWORD dwCheckPoint = 1;
>+
>+  // Fill in the SERVICE_STATUS structure.
>+  gSvcStatus.dwCurrentState = dwCurrentState;
>+  gSvcStatus.dwWin32ExitCode = dwWin32ExitCode;
>+  gSvcStatus.dwWaitHint = dwWaitHint;
>+
>+  if (SERVICE_START_PENDING == dwCurrentState) {
>+    gSvcStatus.dwControlsAccepted = 0;
>+  } else {
>+    gSvcStatus.dwControlsAccepted = SERVICE_ACCEPT_STOP;
>+  }
>+
>+  if ((SERVICE_RUNNING == dwCurrentState) ||
>+      (SERVICE_STOPPED == dwCurrentState)) {
>+    gSvcStatus.dwCheckPoint = 0;
>+  } else {
>+    gSvcStatus.dwCheckPoint = dwCheckPoint++;
>+  }
>+
>+  // Report the status of the service to the SCM.
>+  SetServiceStatus(gSvcStatusHandle, &gSvcStatus);
>+}
>+
>+// Called by SCM whenever a control code is sent to the service
>+// using the ControlService function.
>+void WINAPI SvcCtrlHandler(DWORD dwCtrl)
>+{
>+  // Handle the requested control code. 
>+  switch(dwCtrl) {
>+  case SERVICE_CONTROL_STOP: 
>+    ReportSvcStatus(SERVICE_STOP_PENDING, NO_ERROR, 0);
>+
>+    // Signal the service to stop.
>+    SetEvent(ghSvcStopEvent);
>+    ReportSvcStatus(gSvcStatus.dwCurrentState, NO_ERROR, 0);
>+    return;
nit: break;

>+  case SERVICE_CONTROL_INTERROGATE: 
>+    break; 
>+  default: 
>+    break;
>+  } 
>+}

Comment 155

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

Comment on attachment 568676 [details] [diff] [review]
Patch 1 - Base service code

>diff --git a/toolkit/components/maintenanceservice/maintenanceservice.exe.manifest b/toolkit/components/maintenanceservice/maintenanceservice.exe.manifest
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/maintenanceservice/maintenanceservice.exe.manifest
>@@ -0,0 +1,34 @@
>+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
>+<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
>+<assemblyIdentity
>+        version="1.0.0.0"
>+        processorArchitecture="*"
>+        name="Updater"
Please change Updater to something like MaintenanceService

>+        type="win32"
>+/>
>+<description>Updater</description>
>+<dependency>
>+        <dependentAssembly>
>+                <assemblyIdentity
>+                        type="win32"
>+                        name="Microsoft.Windows.Common-Controls"
>+                        version="6.0.0.0"
>+                        processorArchitecture="*"
>+                        publicKeyToken="6595b64144ccf1df"
>+                        language="*"
>+                />
Does this really need common controls? I think this is just a copy / paste inclusion from another manifest.

>+        </dependentAssembly>
>+</dependency>
>+<ms_asmv3:trustInfo xmlns:ms_asmv3="urn:schemas-microsoft-com:asm.v3">
>+  <ms_asmv3:security>
>+    <ms_asmv3:requestedPrivileges>
>+      <ms_asmv3:requestedExecutionLevel level="requireAdministrator" uiAccess="false" />
Out of curiousity, could this safely be asInvoker instead? If so, is there any reason not to just use asInvoker?

>+    </ms_asmv3:requestedPrivileges>
>+  </ms_asmv3:security>
>+</ms_asmv3:trustInfo>
>+  <compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1">
>+    <application>
>+      <supportedOS Id="{35138b9a-5d96-4fbd-8e2d-a2440225f93a}" />
Why isn't Vista included here?

>+    </application>
>+  </compatibility>
>+</assembly>

Comment 156

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

Comment on attachment 568676 [details] [diff] [review]
Patch 1 - Base service code

>diff --git a/toolkit/components/maintenanceservice/servicebase.cpp b/toolkit/components/maintenanceservice/servicebase.cpp
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/maintenanceservice/servicebase.cpp
>...

>+#include "servicebase.h"
>+
>+// Shared code with Firefox and updater.exe
I don't think this comment adds much value but if you want it go with something like
//Shared code between applications and updater.exe

>+#include "nsWindowsRestart.cpp"
>diff --git a/toolkit/components/maintenanceservice/servicebase.h b/toolkit/components/maintenanceservice/servicebase.h
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/maintenanceservice/servicebase.h
>@@ -0,0 +1,45 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Application Update service base code.
Mozilla Maintenance service.

Looks good though I'd like another quick pass at this before r+'ing

Comment 157

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

Comment on attachment 568993 [details] [diff] [review]
Patch 2 - Main service logic. v2

>diff --git a/toolkit/components/maintenanceservice/workmonitor.cpp b/toolkit/components/maintenanceservice/workmonitor.cpp
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/maintenanceservice/workmonitor.cpp
>@@ -0,0 +1,417 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Application Update service fs monitoring.
s/Application Update service fs/Maintenance service file system monitoring/

>...
>+static const int FILE_SHARE_ALL = FILE_SHARE_READ | 
>+                                  FILE_SHARE_WRITE | 
>+                                  FILE_SHARE_DELETE;
>+
>+// Wait 5 minutes for an update operation to run at most.
>+static const int TIME_TO_WAIT_ON_UPDATER = 5 * 60 * 1000;
note: we have had bug reports from people that update an install on a share with a very slow connection. I think this timeout should be sufficient... just calling it out.

>+
>+HANDLE QueryUserToken(DWORD sessionID)
>+{
>+  HMODULE module = LoadLibraryW(L"wtsapi32.dll");
>+  HANDLE token = NULL;
>+  LPWTSQueryUserToken wtsQueryUserToken = (LPWTSQueryUserToken)GetProcAddress(module, "WTSQueryUserToken");
>+  if (wtsQueryUserToken) {
>+    wtsQueryUserToken(sessionID, &token);
>+  }
>+  FreeModule(module);
>+  return token;
>+}
>+
>+HANDLE GetElevatedTokenFromToken(HANDLE token) 
>+{
>+  // Magic below...
>+  // UAC creates 2 tokens.  One is the restricted token which we have.
>+  // the other is the UAC elevated one. Since we are running as a service
>+  // as the system account we have access to both.
nit: this seems like it would be better to have as a javadoc function comment
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html

>+  TOKEN_LINKED_TOKEN tlt;
>+  HANDLE hNewLinkedToken = NULL;
>+  DWORD len;
>+  if(::GetTokenInformation(token, (TOKEN_INFORMATION_CLASS)TokenLinkedToken, 
>+                           &tlt, sizeof(TOKEN_LINKED_TOKEN), &len)) {
>+    token = tlt.LinkedToken;
>+    hNewLinkedToken = token;
>+  }
>+  return hNewLinkedToken;
>+}
>+
>+// Returns TRUE if the process was run and returned 0
nit: always use javadoc style comments for documenting functions

>+BOOL StartElevatedProcessInSession(DWORD sessionID, LPCWSTR appToStart, LPCWSTR workingDir, LPWSTR cmdLine)
>+{
>+  BOOL processStarted;
>+  DWORD myProcessID = GetCurrentProcessId();
>+  DWORD mySessionID;
>+  ProcessIdToSessionId(myProcessID, &mySessionID);
>+
>+  STARTUPINFO si = {0};
>+  si.cb = sizeof(STARTUPINFO);
>+  si.lpDesktop = L"winsta0\\Default";
>+  PROCESS_INFORMATION pi = {0};
>+
>+  PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+    ("Starting process in an elevated session.  Service session ID: %d; Requested session ID: %d", mySessionID, sessionID));
Attempting to start process in an elevated session

There are a few lines greater than 80 that could fairly easily be under 80

>+
>+  // We must be running this from an older OS than Vista or else
>+  // we are running this code as a user process instead of via the service.
>+  // In that case just execute the process in the normal way.
>+  if (mySessionID == sessionID) {
>+    processStarted = ::CreateProcessW(appToStart, cmdLine, 
>+                                      NULL, NULL, FALSE, 
>+                                      CREATE_DEFAULT_ERROR_MODE | 
>+                                      CREATE_UNICODE_ENVIRONMENT, 
>+                                      NULL, workingDir, &si, &pi);
>+    DWORD lastError = ::GetLastError();
>+    PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+      ("Could not create process as current user, last error: %d; appToStart: %S; cmdLine: %S", 
>+       lastError, appToStart, cmdLine));
This is always logged... should be conditional
if (!processStarted) {
...
}

>+
nit: extra blank line

>+  } else {
>+    HANDLE userToken = QueryUserToken(sessionID);
>+
>+    // Check if we are running Vista or later.
>+    OSVERSIONINFO osInfo;
>+    osInfo.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
>+    if (GetVersionEx(&osInfo) && osInfo.dwMajorVersion >= 6) {
>+
>+      PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+        ("Windows Vista or later detected, attempting to find linked token."));
>+
>+      HANDLE elevatedToken = GetElevatedTokenFromToken(userToken);
>+      if (elevatedToken) {
>+        PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+          ("Elevated token was found, using it."));
>+
>+        ::CloseHandle(userToken); 
>+        userToken = elevatedToken;
>+      } else {
>+        PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+          ("An elevated token was not found, will use regular token."));
>+      }
>+    }
>+
>+    // Create an environment block for the process we're about to start using the
>+    // user's token.
>+    LPVOID lpvEnv(NULL);
>+    if (!::CreateEnvironmentBlock(&lpvEnv, userToken, TRUE)) {
>+      
nit: extra blank line here and else where
In case you haven't seen it
https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

>+      PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+        ("Could not create an environment block, setting it to NULL."));
>+
>+      lpvEnv = NULL;
>+    }
>+
>+    // Launch the process in the specified working directory application and command line
>+    processStarted = ::CreateProcessAsUserW(userToken, appToStart, cmdLine, 
>+                                            NULL, NULL, FALSE,
>+                                            CREATE_DEFAULT_ERROR_MODE | 
>+                                            CREATE_UNICODE_ENVIRONMENT, 
>+                                            lpvEnv, workingDir, &si, &pi);
>+    if (!processStarted) {
>+      DWORD lastError = ::GetLastError();
>+      PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+        ("Could not create process with user token, last error: %d; appToStart: %S; cmdLine: %S", 
>+         lastError, appToStart, cmdLine));
>+    }
>+    ::CloseHandle(userToken);
>+  }
>+
>+  if (processStarted) {
>+
>+    PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+      ("Process was started... waiting on result.")); 
>+
>+    // Wait for the updater process to finish
>+    ::WaitForSingleObject(pi.hProcess, TIME_TO_WAIT_ON_UPDATER);
>+
>+    DWORD returnCode;
>+    if (!GetExitCodeProcess(pi.hProcess, &returnCode)) {
nit: consistency :: - please check other usage as well.
if (!::GetExitCodeProcess(pi.hProcess, &returnCode)) {

>+      PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+        ("Process finished but could not obtain return code.")); 
>+      return FALSE;
>+    }
>+
>+    PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+      ("Process finished with return code %d.", returnCode)); 
>+
>+    // updater returns 0 if successful.
>+    return returnCode == 0;
>+  } else {
>+    // The process couldn't be started.
>+    return FALSE;
>+  }
>+}
>+
>+// Returns true if we want the service to stop
>+// For now this is every time we process a .mz file.
nit: javadoc comment - here and else where as applicable

>+bool ProcessWorkItem(LPCWSTR monitoringBasePath, FILE_NOTIFY_INFORMATION &notifyInfo)
>+{
>+  int filenameLength = notifyInfo.FileNameLength / 
>+                       sizeof(notifyInfo.FileName[0]); 
>+
>+  PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+    ("Processing new command meta file: %S", notifyInfo.FileName));
>+
>+  // When the file is ready for processing it will be renamed 
>+  // to have a .mz extension
>+  bool haveWorkItem = notifyInfo.Action == FILE_ACTION_RENAMED_NEW_NAME && 
>+                      notifyInfo.FileNameLength > 3 && 
>+                      notifyInfo.FileName[filenameLength - 3] == L'.' &&
>+                      notifyInfo.FileName[filenameLength - 2] == L'm' &&
>+                      notifyInfo.FileName[filenameLength - 1] == L'z';
>+  if (!haveWorkItem) {
>+    // We don't have a work item, keep looking
>+    return false;
consistency nit: use uppercase FALSE

>+  }
>+
>+  WCHAR fullMetaUpdateFilePath[MAX_PATH+1];
nit: MAX_PATH + 1 here and elsewhere

>+  wcscpy(fullMetaUpdateFilePath, monitoringBasePath);
>+
>+  // We only support file paths in monitoring directories that are MAX_PATH chars or less.
>+  if (!PathAppendSafe(fullMetaUpdateFilePath, notifyInfo.FileName)) {
>+    // Cannot process update, skipfileSize it.
>+    return true;
nit: consistency - uppercase TRUE

>+  }
>+
>+  nsAutoHandle metaUpdateFile = ::CreateFile(fullMetaUpdateFilePath, GENERIC_READ, 
>+                                             FILE_SHARE_ALL, NULL, OPEN_EXISTING,
>+                                             0, NULL);
>+  if (INVALID_HANDLE_VALUE == metaUpdateFile) {
>+
>+    PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+        ("Could not open command meta file: %S", notifyInfo.FileName));
>+    return true;
>+  }
>+
>+  DWORD fileSize = GetFileSize(metaUpdateFile, NULL);
>+  DWORD sessionID = 0;
>+  // The file should be in WIDECHAR's so if it's of odd size it's
s/WIDECHAR's/wide characters/

>+  // an invalid file
>+  const int kSanityCheckFileSize = 1024*64;
nit: 1024 * 64

>+  if (fileSize == INVALID_FILE_SIZE || 
>+      (fileSize %2) != 0 ||
>+      fileSize > kSanityCheckFileSize ||
>+      fileSize < sizeof(DWORD)) {
>+    PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+      ("Could not obtain file size or an improper file size was encountered for command meta file: %S", notifyInfo.FileName));
>+    return true;
>+  }
>+
>+  // The first 4 bytes are for the process ID
>+  DWORD read1;
nit: use descriptive names here and in the other instances of read#

>+  BOOL result1 = ::ReadFile(metaUpdateFile, &sessionID, 
>+                            sizeof(DWORD), &read1, NULL);
>+  fileSize -= sizeof(DWORD);
>+
>+  // The next MAX_PATH wchar's are for the app to start
>+  WCHAR appToStart[MAX_PATH + 1];
>+  ZeroMemory(appToStart, sizeof(appToStart));
>+  DWORD read2;
>+  BOOL result2 = ::ReadFile(metaUpdateFile, appToStart, 
>+                            MAX_PATH * sizeof(WCHAR), &read2, NULL);
How about
BOOL result;
result |= ::ReadFile
result |= ::ReadFile
etc.

>+  fileSize -= read2;
>+
>+  // The next MAX_PATH wchar's are for the app to start
>+  WCHAR workingDirectory[MAX_PATH + 1];
>+  ZeroMemory(workingDirectory, sizeof(workingDirectory));
>+  DWORD read3;
>+  BOOL result3 = ::ReadFile(metaUpdateFile, workingDirectory, 
>+                            MAX_PATH * sizeof(WCHAR), &read3, NULL);
>+  fileSize -= read3;
>+
>+  // + 2 for wide char termination
>+  nsAutoArrayPtr<char> cmdlineBuffer = new char[fileSize + 2];
>+  DWORD cmdLineBufferRead;
>+  BOOL result4 = ::ReadFile(metaUpdateFile, cmdlineBuffer, 
>+                            fileSize, &cmdLineBufferRead, NULL);
>+  fileSize -= cmdLineBufferRead;
>+
>+  if (!result1 || !result2 || !result3 || !result4 ||
>+      read1 != sizeof(DWORD) || 
>+      read2 != MAX_PATH * sizeof(WCHAR) ||
>+      read3 != MAX_PATH * sizeof(WCHAR) ||
>+      fileSize != 0) {
>+    PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+      ("Could not read command data for command meta file: %S", notifyInfo.FileName));
>+    return true;
>+  }
>+  cmdlineBuffer[cmdLineBufferRead] = L'\0';
>+  cmdlineBuffer[cmdLineBufferRead + 1] = '\0';
>+  WCHAR *cmdlineBufferWide = reinterpret_cast<WCHAR*>(cmdlineBuffer.get());
>+
>+  PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+    ("An update command was detected and is being processed for command meta file: %S", notifyInfo.FileName));
>+
>+  // Validate the certificate of the app the user wants us to start.
>+  // Also check to make sure the certificate is trusted.
>+#ifndef DISABLE_SERVICE_AUTHENTICODE_CHECK
>+  if (CheckIfBinaryMatchesAllowedCertificates(appToStart)) {
>+#endif
>+    if (!StartElevatedProcessInSession(sessionID, appToStart, workingDirectory, cmdlineBufferWide)) {
>+      // TODO: Need to tell the app that we couldn't run the update so 
>+      // the app will do the update the old way. rs do you think via update status file?
>+      // I don't want it to keep using the service in case it keeps getting 
>+      // the same error on each launch.
>+      PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+        ("Error running process in session %d.  Last error: %d", sessionID, GetLastError()));
>+    } 
>+#ifndef DISABLE_SERVICE_AUTHENTICODE_CHECK
Seems like this should be for the else branch below

>+    else {
>+      // The update was executed and run successfully
>+      PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+        ("Updater.exe was launched and run successfully"));
nit: updater.exe

>+    }
>+  } else {
>+    // TODO: Need to tell the app that we couldn't run the update
>+    // because the cert is not valid.  The app will do the update 
>+    // itself the old way.  rs do you think via update status file?
>+    // I don't want it to keep using the service in case it keeps getting 
>+    // the same error on each launch.
>+    PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+      ("Could not start process due to certificate check.  Last error: %d", GetLastError()));
For the TODO's above a new error code for the update.status file should suffice here
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/readstrings/errors.h

I'd like to separate the readstrings error codes and the updater error codes but not in this bug.

>+  } 
>+#endif
>+
>+  // We processed a work item, whether or not it was successful.
>+  return true;
>+}
>+
>+
>+BOOL StartDirectoryChangeMonitor() 
>+{
>+  PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+    ("Starting directory change monitor..."));
>+
>+  // Init the update directory path and ensure it exists.
>+  // Example: C:\programData\Mozilla\Firefox\updates\[channel]
>+  // The channel is not included here as we want to monitor the base directory
>+  WCHAR programData[MAX_PATH+1];
perhaps name this updateData or something similar to avoid confusion with c:\programdata\

>+  if (!GetUpdateDirectoryPath(programData)) {
>+
>+    PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+      ("Could not obtain update directory path"));
>+
>+    return FALSE;
>+  }

>diff --git a/toolkit/components/maintenanceservice/workmonitor.h b/toolkit/components/maintenanceservice/workmonitor.h
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/maintenanceservice/workmonitor.h
>@@ -0,0 +1,41 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Application Update service fs monitoring.
s/Application Update service fs/Maintenance service file system monitoring/

Also, looks good though I need to dive into this one a bit more.

Comment 158

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

Comment on attachment 568678 [details] [diff] [review]
Patch 3 - Code for installing the service

I need to dive into this one a bit deeper but I wanted to give you what I have so far so you have a chance to address the version check comment before I am done.

>diff --git a/toolkit/components/maintenanceservice/serviceinstall.cpp b/toolkit/components/maintenanceservice/serviceinstall.cpp
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/maintenanceservice/serviceinstall.cpp
>@@ -0,0 +1,382 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Application Update service.
s/Application Update service/Maintenance service/

Not going to call this out in further cases nut please change here and else where.


>+#include <windows.h>
>+#include <aclapi.h>
>+#include <stdlib.h>
>+
>+#include <nsAutoPtr.h>
>+#include <nsAutoServiceHandle.h>
>+#include <nsMemory.h>
>+
>+#include "serviceinstall.h"
>+#include "servicebase.h"
>+#include "shellapi.h"
>+
>+#pragma comment(lib, "version.lib")
>+#define DEFERRED_DELETE_TIMEOUT_SECONDS 10
>+
>+static bool GetVersionNumberFromPath(LPWSTR path, DWORD &version) 
>+{
>+  DWORD fileVersionInfoSize = GetFileVersionInfoSizeW(path, 0);
>+  nsAutoArrayPtr<char> fileVersionInfo = new char[fileVersionInfoSize];
>+  if (!GetFileVersionInfoW(path, 0, 
>+    fileVersionInfoSize, fileVersionInfo.get())) {
>+      PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+        ("Could not obtain file info of old service.  (%d)", GetLastError()));
>+      return false;
>+  }
>+
>+  VS_FIXEDFILEINFO *fixedFileInfo = 
>+    reinterpret_cast<VS_FIXEDFILEINFO *>(fileVersionInfo.get());
>+  UINT size;
>+  if (!VerQueryValueW(fileVersionInfo.get(), L"\\", 
>+    reinterpret_cast<LPVOID*>(&fixedFileInfo), &size)) {
>+      PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+        ("Could not query file version info of old service.  (%d)", GetLastError()));
>+      return false;
>+  }  
>+
>+  version = fixedFileInfo->dwFileVersionLS;
I don't think build number will be enough here. Need something like CVersionInfo since it supports full version comparison.
http://www.codeguru.com/cpp/w-p/win32/versioning/article.php/c4539

There is also the following which compares version strings
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsVersionComparator.cpp

>+  return true;
>+}
>+
>+// Return value does not indicate anything except that the 
>+// operation was attempted
>+DWORD DeferredDeletePath(LPCWSTR pathToDelete) 
>+{
>+  WCHAR deferredSlefDeleteCmdLine[MAX_PATH + 32];
>+  wsprintfW(deferredSlefDeleteCmdLine, 
>+            L"/C SLEEP %i && DEL \"%s\"", 
>+            DEFERRED_DELETE_TIMEOUT_SECONDS, 
>+            pathToDelete);
>+
>+  SetLastError(ERROR_SUCCESS);
>+  ShellExecuteW(NULL, L"open", L"cmd", deferredSlefDeleteCmdLine, NULL, SW_HIDE);
>+  return GetLastError();
>+}
>+
>+bool SvcInstall(bool upgradeOnly)
>+{
>+  // Get a handle to the local computer SCM database with full access rights.  
>+  nsAutoServiceHandle schSCManager(OpenSCManager(NULL, NULL, SC_MANAGER_ALL_ACCESS));
>+  if (!schSCManager) {
>+    PR_LOG(gServiceLog, PR_LOG_ALWAYS,
>+      ("Could not open service manager.  (%d)", GetLastError()));
>+    return false;
>+  }
>+
>+  WCHAR modulePath[MAX_PATH + 1];
>+  if(!GetModuleFileNameW(NULL, modulePath, NS_ARRAY_LENGTH(modulePath))) {
nit: I'd prefer if you used sizeof(modulePath)/sizeof(modulePath[0]) since this is standalone code.

Comment 159

[Brian R. Bondy [:bbondy]]

Attachment: Patch 5 - XUL Runtime Environment code. v2.

Minor change (removed static for one function) didn't want to put in another patch to avoid future rebasing.

Comment 160

[Brian R. Bondy [:bbondy]]

Attachment: Patch 16 - Support for launching callback app as unelevated.

This new patch has handling for launching the callback application with the unelevated token.  
The way the new code works is that it trims off the callback application from the command lines and executes that separately.  It calls updater.exe without the callback information.

I also noticed something we didn't think about which is that the callback app is also specified via command line and therefore is suseptible to session ID spoofing.  It's not a huge deal because we execute it with the unelevated token though.  The is not a new problem in this patch, but also a problem even without this latest patch.  To fix it, I added a new check for certificate checking on the callback app as well.

I think it is fine to not have the callback app (firefox.exe) signed for Nightly (until we have it automated) because the only side effect is that the callback application will not be run after updates.

Comment 161

[Brian R. Bondy [:bbondy]]

Attachment: Patch 15 - UAC helper functions.

No changes, just moved some code out of this patch and into patch 2 to avoid future rebasing.

Comment 162

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2 - Main service logic. v3.

Implemented review comments.

Comment 163

[Brian R. Bondy [:bbondy]]

For anyone doing QA on the build, any installer you use after today has the NSPR module changed from nsFirefoxService to nsMaintenanceService.

Comment 164

[Brian R. Bondy [:bbondy]]

Re questions on patch 1 review of manifest file:

> Out of curiousity, could this safely be asInvoker instead? 
> If so, is there any reason not to just use asInvoker?

There is a lot of code in the service that wouldn't work asInvoker so I think it needs to stay asAdministrator.

>+      <supportedOS Id="{35138b9a-5d96-4fbd-8e2d-a2440225f93a}" />
> Why isn't Vista included here?

This was carried over from me originally copying the manifest file from updater.exe.  It is a problem there too, I will include a fix in that manfiest as well in the next patch.

Comment 165

[Brian R. Bondy [:bbondy]]

Attachment: Patch 1 - Base service code. v2.

Implemented review comments.

Comment 166

[Ian Melven :imelven]

(In reply to Brian R. Bondy [:bbondy] from comment #160)
> Created attachment 569679 [details] [diff] [review] [diff] [details] [review]
> Patch 16 - Support for launching callback app as unelevated.
> 
> This new patch has handling for launching the callback application with the
> unelevated token.  
> The way the new code works is that it trims off the callback application
> from the command lines and executes that separately.  It calls updater.exe
> without the callback information.
> 
> I also noticed something we didn't think about which is that the callback
> app is also specified via command line and therefore is suseptible to
> session ID spoofing.  It's not a huge deal because we execute it with the
> unelevated token though.  The is not a new problem in this patch, but also a
> problem even without this latest patch.  To fix it, I added a new check for
> certificate checking on the callback app as well.
> 
> I think it is fine to not have the callback app (firefox.exe) signed for
> Nightly (until we have it automated) because the only side effect is that
> the callback application will not be run after updates.

i took a look through this and it looks good, checking the signature on the callback app helps mitigate some of my worries about the session id spoofing problem as well.

Comment 167

[Brian R. Bondy [:bbondy]]

Attachment: Patch 3 - Code for installing the service. v2.

Implemented review comments (+some review comments from other patches into this one) and better version checking.

nsVersionComparator was not used because I'd have to link to more stuff and it works with strings, but I have the version components in DWORDs.
The codeguru code uses the same APIs as I was using but did save me some time to see how to get the other version components.
Only a small change was needed from my existing code to check the other 3 components of the version.

Comment 168

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

Many of the PR_LOG messages seem appropriate as event log messages. If you think it would be a good thing to add please file a followup bug.

Comment 169

[Brian R. Bondy [:bbondy]]

I had some code initially that did event log messages (in the first base patch which is now obsolete) but you need to have a special compiling step and also I wasn't sure about how to manage the localization. I know it would be good for enterprise customers though.  I'll post a follow up bug for it, thanks for the suggestion.

Comment 170

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

Comment on attachment 568685 [details] [diff] [review]
Patch 8 - NSPR wide string logging support on Windows

This needs to be reviewed by one of the NSPR people such as [e-mail].

Before asking for review it would be a good thing to make the indentation consistent with the rest of the file.

Comment 171

[Brian R. Bondy [:bbondy]]

> Before asking for review it would be a good thing to make the 
> indentation consistent with the rest of the file.

Sorry looked aligned in my local patch file (tortoiseHg) and Visual Studio. 
They use a lot of mixed tab/space characters.

I'll do th same as them and re-submit.

Comment 172

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

Comment on attachment 568679 [details] [diff] [review]
Patch 4 - Certificate check code inside service

># HG changeset patch
># Parent f04152f2f05006759cc53b386dcdc35e81035d90
># User Brian R. Bondy <[e-mail]>
>Bug481815 - Windows silent update service; Certificate check code
>
>diff --git a/toolkit/components/maintenanceservice/certificatecheck.cpp b/toolkit/components/maintenanceservice/certificatecheck.cpp
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/maintenanceservice/certificatecheck.cpp
>@@ -0,0 +1,545 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is certificate check code
Maintenance service certificate check code.

here and else where

>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Brian R. Bondy <[e-mail]>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * 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 <stdio.h>
>+#include <stdlib.h>
>+#include <windows.h>
>+#include <softpub.h>
>+#include <wintrust.h>
>+
>+#include "certificatecheck.h"
>+#include "servicebase.h"
>+
>+// Link with the Wintrust.lib file.
This comment isn't of any value... is it?

>+#pragma comment(lib, "wintrust.lib")
>+#pragma comment(lib, "crypt32.lib")
>+
>+static const int ENCODING = X509_ASN_ENCODING | PKCS_7_ASN_ENCODING;
>+
>+
>+// Checks to see if a PE file stored at filename matches the specified issuer
>+// and name.
javadoc comment please

Ideally, we would allow the application to specify what cert attributes to check though I think this is fine at least for now.

>+DWORD CheckCertificateForPEFile(LPCWSTR filename, 
>+                                CertificateCheckInfo &infoToMatch)
filename is really a filepath unless I am mistaken

nit: forgot the following from style guide here and elsewhere... I'm a tad of a stickler with following the style guide with new files.
int
MyFunction(...)
{
  ...
}

int
MyClass::Method(...)
{
  ...
}

>+{
>+  HCERTSTORE hStore = NULL;
>+  HCRYPTMSG hMsg = NULL; 
>+  PCCERT_CONTEXT pCertContext = NULL;
>+  PCMSG_SIGNER_INFO pSignerInfo = NULL;
>+  DWORD lastError = ERROR_SUCCESS;
>+  PublisherInfo progPubInfo;
>+  ZeroMemory(&progPubInfo, sizeof(progPubInfo));
>+
>+  // SEH like exception handling is only used for cleanup of ugly C API calls.
>+  __try
>+  {
nit: __try {

>+    // Get the HCERTSTORE and HCRYPTMSG from the signed file.
>+    DWORD dwEncoding, dwContentType, dwFormatType;
>+    BOOL result = CryptQueryObject(CERT_QUERY_OBJECT_FILE,
>+                                   filename, 
>+                                   CERT_QUERY_CONTENT_FLAG_PKCS7_SIGNED_EMBED,
>+                                   CERT_QUERY_CONTENT_FLAG_ALL, 
>+                                   0, &dwEncoding, &dwContentType,
>+                                   &dwFormatType, &hStore, &hMsg, NULL);

>...
>+    }
>+  }
>+  __finally
>+  {
nit: __finally {

>+    if (progPubInfo.programName) {
>+      LocalFree(progPubInfo.programName);
>+    }
>+    if (progPubInfo.publisherLink) {

>...
>+// Returns FALSE if the issuer or name do not match or if any error 
>+// occurs in the check
javadoc comment please

>+BOOL DoesCertificateMatch(PCCERT_CONTEXT pCertContext, 
>+                          CertificateCheckInfo &infoToMatch)
>+{
>+  DWORD dwData;

>...
>+LPWSTR AllocateAndCopyWideString(LPCWSTR inputString)
>+{
>+  LPWSTR outputString = NULL;
>+
>+  outputString = (LPWSTR)LocalAlloc(LPTR,
>+    (wcslen(inputString) + 1) * sizeof(WCHAR));
>+  if (outputString != NULL)
>+  {
nit:
if (outputString != NULL) {

>+    lstrcpyW(outputString, inputString);
>+  }
>+  return outputString;
>+}
>+
>+BOOL GetProgAndPublisherInfo(PCMSG_SIGNER_INFO pSignerInfo,
>+                             PublisherInfo &info)
>+{
>+  BOOL result = FALSE;
>+  PSPC_SP_OPUS_INFO OpusInfo = NULL;  
>+  DWORD dwData;
>+
>+  __try
>+  {
nit: __try {

>+    // Loop through authenticated attributes and find SPC_SP_OPUS_INFO_OBJID OID.
>+    for (DWORD n = 0; n < pSignerInfo->AuthAttrs.cAttr; n++) {           
>+      if (lstrcmpA(SPC_SP_OPUS_INFO_OBJID, 
>+                   pSignerInfo->AuthAttrs.rgAttr[n].pszObjId) == 0) {
>+        // Get Size of SPC_SP_OPUS_INFO structure.
>+        _CRYPTOAPI_BLOB &attr1 = pSignerInfo->AuthAttrs.rgAttr[n].rgValue[0];
>+        result = CryptDecodeObject(ENCODING,

>...
>+        // Fill in Program Name if present.
>+        if (OpusInfo->pwszProgramName) {
>+          info.programName =
>+            AllocateAndCopyWideString(OpusInfo->pwszProgramName);
>+        } else {
>+          info.programName = NULL;
>+        }
>+
>+        // Fill in Publisher Information if present.
>+        if (OpusInfo->pPublisherInfo) {
>+          switch (OpusInfo->pPublisherInfo->dwLinkChoice)
>+          {
nit: format as follows from the style guide
https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

switch (...) {  
  case 1: {  
    // When you need to declare a variable in a switch, put the block in braces  
    int var;  
    break;  
  }  
  case 2:  
    ...  
    break;  
  default:  
    break;  
}

>+          case SPC_URL_LINK_CHOICE:
>+            info.publisherLink =
>+              AllocateAndCopyWideString(OpusInfo->pPublisherInfo->pwszUrl);
>+            break;
>+
>+          case SPC_FILE_LINK_CHOICE:
>+            info.publisherLink =
>+              AllocateAndCopyWideString(OpusInfo->pPublisherInfo->pwszFile);
>+            break;
>+
>+          default:
>+            info.publisherLink = NULL;
>+            break;
>+          }
>+        } else {
>+          info.publisherLink = NULL;
>+        }
>+
>+        // Fill in More Info if present.
>+        if (OpusInfo->pMoreInfo) {
>+          switch (OpusInfo->pMoreInfo->dwLinkChoice)
>+          {
nit: same as previous formating of switch

>+          case SPC_URL_LINK_CHOICE:
>+            info.moreInfoLink =
>+              AllocateAndCopyWideString(OpusInfo->pMoreInfo->pwszUrl);
>+            break;
>+
>+          case SPC_FILE_LINK_CHOICE:
>+            info.moreInfoLink =
>+              AllocateAndCopyWideString(OpusInfo->pMoreInfo->pwszFile);
>+            break;
>+
>+          default:
>+            info.moreInfoLink = NULL;
>+            break;
>+          }
>+        } else {
>+          info.moreInfoLink = NULL;
>+        }
>+        result = TRUE;
>+        break; // Break from for loop.
>+      } // lstrcmp SPC_SP_OPUS_INFO_OBJID                 
>+    } // for 
cleanup

>+  }
>+  __finally
>+  {
nit: __finally {

>...
>+  // WinVerifyTrust verifies signatures as specified by the GUID 
>+  // and Wintrust_Data.
>+  LONG lStatus = WinVerifyTrust(NULL, &WVTPolicyGUID, &WinTrustData);
>+  DWORD dwLastError = GetLastError();
>+  BOOL validated = FALSE;
>+  switch (lStatus) {
>+    case ERROR_SUCCESS:
>+      // The hash that represents the subject is trusted and there were no
>+      // verification errors.  No publisher nor time stamp chain errors.
>+      PR_LOG(gServiceLog, PR_LOG_ALWAYS, 
>+             ("The file \"%S\" is signed and the signature was verified.",
>+              pwszSourceFile));
>+      validated = TRUE;
>+      break;
>+    case TRUST_E_NOSIGNATURE:
>+      // The file was not signed or had a signature that was not valid.
>+      // Get the reason for no signature.
>+      if (TRUST_E_TIME_STAMP == dwLastError) {
>+        // The file was not signed.
>+        PR_LOG(gServiceLog, PR_LOG_ALWAYS, 
>+               ("The file \"%S\" is had a timestamp error.", pwszSourceFile));
nit: The file \"%S\" has a timestamp error.

>+      } else if (TRUST_E_NOSIGNATURE == dwLastError ||
>+                TRUST_E_SUBJECT_FORM_UNKNOWN == dwLastError ||
>+                TRUST_E_PROVIDER_UNKNOWN == dwLastError) {
>+        // The file was not signed.
>+        PR_LOG(gServiceLog, PR_LOG_ALWAYS, 
>+               ("The file \"%S\" is not signed.", pwszSourceFile));
>+      } else {
>+        // The signature was not valid or there was an error 
>+        // opening the file.
>+        PR_LOG(gServiceLog, PR_LOG_ALWAYS, 
>+               ("An unknown error occurred trying to verify the signature of the \"%S\" file.", pwszSourceFile));
nit: I'm not too much of a stickler for line length but this and a few other lines in the other patches are really long.

>+      }
>+      break;
>+    case TRUST_E_EXPLICIT_DISTRUST:
>+      // The hash that represents the subject or the publisher 
>+      // is not allowed by the admin or user.
>+      PR_LOG(gServiceLog, PR_LOG_ALWAYS, 
>+             ("The signature is present, but specifically disallowed."));
>+      break;
>+    case TRUST_E_SUBJECT_NOT_TRUSTED:
>+      // The user clicked "No" when asked to install and run.
This implies that we present the user with ui to click "No". Please elaborate.

>diff --git a/toolkit/components/maintenanceservice/certificatecheck.h b/toolkit/components/maintenanceservice/certificatecheck.h
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/maintenanceservice/certificatecheck.h
>@@ -0,0 +1,65 @@
>...
>+#ifndef _CERTIFICATECHECK_H_
>+#define _CERTIFICATECHECK_H_
>+
>+#include <wincrypt.h>
>+
>+struct PublisherInfo
>+{
>+  LPWSTR programName;
>+  LPWSTR publisherLink;
>+  LPWSTR moreInfoLink;
>+};
>+
>+struct CertificateCheckInfo
>+{
>+  LPCWSTR name;
>+  LPCWSTR issuer;
>+  PublisherInfo signerInfo;
>+};
>+
>+BOOL GetProgAndPublisherInfo(PCMSG_SIGNER_INFO pSignerInfo, 
>+                             PublisherInfo &info);
nit: perhaps GetProgramAndPublisherInfo

>+BOOL DoesCertificateMatch(PCCERT_CONTEXT pCertContext, 
>+                          CertificateCheckInfo &infoToMatch);
nit: perhaps DoCertificateAttributesMatch

>+DWORD VerifyCertificateTrustForFile(LPCWSTR pwszSourceFile);
nit: consistency - drop the hungarian notation
nit: I'd just go with filepath

>+DWORD CheckCertificateForPEFile(LPCWSTR filename, 
>+                                CertificateCheckInfo &infoToMatch);
nit: I'd just go with filepath

>+
>+#endif
>diff --git a/toolkit/components/maintenanceservice/registrycertificates.cpp b/toolkit/components/maintenanceservice/registrycertificates.cpp
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/maintenanceservice/registrycertificates.cpp
>@@ -0,0 +1,179 @@
>...
>+BOOL CheckIfBinaryMatchesAllowedCertificates(LPCWSTR application) 
nit: A tad verbose of a name... need to think about it a bit. For example, you have DoesCertificateMatch and this might be better as DoesCertificateMatchFile though that could likely be improved as well.

nit: application is just a filepath.

>+{ 
>+  LPCWSTR maintenanceServiceKey = L"SOFTWARE\\Mozilla\\Certs";
>+
>+  // We use KEY_WOW64_64KEY to always force 64-bit view.
This comment leaves me wondering why... please elaborate.

It isn't clear to me whether it is better to use a common key for all cert attributes for all installs or to have each install store the cert attributes in an install specific key. I am leaning install specific keys since another app should not be able to provide cert attributes that are then used by another app. What do you think?

Holding off on the rest of the registry code review until the above is answered

Comment 173

[Brian R. Bondy [:bbondy]]

Attachment: (PUSHED) Patch 8 - NSPR wide string logging support on Windows. v2.

I don't fully understand the whitespacing in that file:
sometimes a line has tab + 4 spaces, sometimes 2 tabs, and sometimes all spaces.

But I made it all consistent with the similar parts of the file.

Comment 174

[Brian R. Bondy [:bbondy]]

Because of the CSS styling by the way somtimes the green stuff looks 1 space off but it is the same whitespace, I have show whitespace on.

Comment 175

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

Comment on attachment 569786 [details] [diff] [review]
(PUSHED) Patch 8 - NSPR wide string logging support on Windows. v2.

Thanks! Looks much more consistent with the existing code.

I'm fairly certain that wtc will need to review these changes so changing reviewer.

Comment 176

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

Comment on attachment 569605 [details] [diff] [review]
Patch 5 - XUL Runtime Environment code. v2.

>diff --git a/toolkit/xre/nsUpdateDriver.cpp b/toolkit/xre/nsUpdateDriver.cpp
>--- a/toolkit/xre/nsUpdateDriver.cpp
>+++ b/toolkit/xre/nsUpdateDriver.cpp
>@@ -475,18 +480,29 @@ ApplyUpdate(nsIFile *greDir, nsIFile *up
>     PR_SetEnv("MOZ_SAFE_MODE_RESTART=1");
>   }
> 
>   LOG(("spawning updater process [%s]\n", updaterPath.get()));
> 
> #if defined(USE_EXECV)
>   execv(updaterPath.get(), argv);
> #elif defined(XP_WIN)
>-  if (!WinLaunchChild(updaterPathW.get(), argc, argv))
>-    return;
>+
>+  bool attemptToUseService = 
>+    mozilla::Preferences::GetBool(kPrefAppUpdateService, false);
Have you tested this? Unless I'm much mistaken this won't work because we don't actually have a profile at this time. Not sure how this should be handled if this is the case at this time.

>+  // First try to launch the update operation using the service
nit: this should call out "if the user hasn't disabled updating with the service"

>+  if (!attemptToUseService || 
>+      !WinLaunchServiceCommand(updaterPathW.get(), argc, argv)) {
>+    // If that fails then launch the update using updater.exe
nit: remove "If that fails" and just go with "Launch the update using updater.exe"

>+    if (!WinLaunchChild(updaterPathW.get(), argc, argv)) {
>+      return;
>+    }
>+  }
>+
>+  // We are going to process an update so we should exit now
>   _exit(0);

>diff --git a/toolkit/xre/nsWindowsRestart.cpp b/toolkit/xre/nsWindowsRestart.cpp
>--- a/toolkit/xre/nsWindowsRestart.cpp
>+++ b/toolkit/xre/nsWindowsRestart.cpp
>...
>@@ -219,16 +230,219 @@ FreeAllocStrings(int argc, PRUnichar **a
>   while (argc) {
>     --argc;
>     delete [] argv[argc];
>   }
> 
>   delete [] argv;
> }
> 
>+BOOL 
>+EnsureWindowsServiceStarted() {
>+  // Get a handle to the SCM database.
>+  nsAutoServiceHandle serviceManager(OpenSCManager(NULL, NULL, 
>+                                                   SC_MANAGER_CONNECT | 
>+                                                   SC_MANAGER_ENUMERATE_SERVICE));
>+  if (!serviceManager)  {
>+    return FALSE;
>+  }
>+
>+  // Get a handle to the service.
>+  nsAutoServiceHandle service(OpenServiceW(serviceManager, 
>+                                           L"MozillaMaintenance", 
>+                                           SERVICE_QUERY_STATUS | SERVICE_START));
>+  if (!service) { 
>+    return FALSE;
>+  }
>+
>+  // Make sure the service is not stopped.
>+  SERVICE_STATUS_PROCESS ssp;
>+  DWORD bytesNeeded;
>+  if (!QueryServiceStatusEx(service, SC_STATUS_PROCESS_INFO, (LPBYTE)&ssp,
>+    sizeof(SERVICE_STATUS_PROCESS), &bytesNeeded)) {
>+      return FALSE;
>+  }
>+
>+  if (ssp.dwCurrentState == SERVICE_STOPPED) {
>+    if (!::StartService(service, 0, NULL)) {
>+      return FALSE;
>+    }
>+
>+    // Make sure we can get into a started state without waiting too long.
>+    DWORD totalWaitTime = 0;
>+    static const int maxWaitTime = 1000 * 5; // Never wait more than 5 seconds
What is the normal flow for this case? Specifically, does the service typically have to be started in this code path? How long does it take for the service to start on your system?

>+    while (QueryServiceStatusEx(service, SC_STATUS_PROCESS_INFO, (LPBYTE)&ssp,
>+        sizeof(SERVICE_STATUS_PROCESS), &bytesNeeded)) {
>+      if (ssp.dwCurrentState == SERVICE_RUNNING) {
>+        break;
>+      } else if (ssp.dwCurrentState == SERVICE_START_PENDING &&
>+        totalWaitTime > maxWaitTime) {
>+          // We will probably eventually start, but we can't wait any longer.
>+          break;
>+      } else if (ssp.dwCurrentState != SERVICE_START_PENDING) {
>+        return FALSE;
>+      }
>+
>+      Sleep(ssp.dwWaitHint);
>+      totalWaitTime += (ssp.dwWaitHint + 10);
Where does 10 come from?

>+    }
>+  }
>+
>+  return ssp.dwCurrentState == SERVICE_RUNNING;
>+}
>+
>+
>+BOOL PathAppendSafe(LPWSTR base, LPCWSTR extra)
>+{
>+  if (wcslen(base) + wcslen(extra) > MAX_PATH) {
>+    return FALSE;
>+  }
>+
>+  return ::PathAppendW(base, extra);
>+}
>+
>+BOOL GetUpdateDirectoryPath(WCHAR *path) 
>+{
>+  HRESULT hr = SHGetFolderPathW(NULL, CSIDL_COMMON_APPDATA, NULL, 
>+    SHGFP_TYPE_CURRENT, path);
>+  if (FAILED(hr)) {
>+    return FALSE;
>+  }
>+  if (!PathAppendSafe(path, L"Mozilla")) {
>+    return FALSE;
>+  }
>+  ::CreateDirectoryW(path, NULL);
>+
>+  if (!PathAppendSafe(path, L"updates")) {
>+    return FALSE;
>+  }
>+  ::CreateDirectoryW(path, NULL);
I am fairly certain that these directories will be created with only the current user and accounts running as admin having write access.

A better method might be to do something similar to XRE_UPDATE_ROOT_DIR and set the permissions on creation.
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#261

Another option is to have the installer set the permissions on the Mozilla directory under CSIDL_COMMON_APPDATA. We already have the NSIS plugin for setting the permissions.

>+  return TRUE;
>+}
>+
>+/**
>+ * Launch a service initiated action with the specified arguments.
>+ * @note argv[0] is ignored
>+ * @note The form of this function that takes char **argv expects UTF-8
>+ */
Include the parameters as well
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html

>+BOOL
>+WinLaunchServiceCommand(const PRUnichar *exePath, int argc, PRUnichar **argv)
>+{
>+  // Ensure the service is started, if not we should try to start it, if it is
>+  // not in a started state we cannot execute a service command.
s/started state/running state/

>+  if (!EnsureWindowsServiceStarted()) {
>+    return FALSE;
>+  }
>+
>+  WCHAR programData[MAX_PATH + 1];
Perhaps updateData?

>+  if (!GetUpdateDirectoryPath(programData)) {
>+    return FALSE;
>+  }
>+
>+  // Get a new GUID for the filename
I'm curious why you are using a GUID for the filename instead of just a temp file, etc.?

>+  UUID uuid;
>+  ::ZeroMemory(&uuid, sizeof(UUID));
>+  ::UuidCreate(&uuid);
>+  WCHAR* wUUID = NULL;
>+  ::UuidToStringW(&uuid, (RPC_WSTR*)&wUUID);
>+  if(!wUUID) {
>+    return FALSE;
>+  }
>+  
>+  // Generate the filename for the service command
>+  WCHAR fileName[128];
>+  wsprintfW(fileName, L"%s.ma", wUUID);
>+  ::RpcStringFreeW((RPC_WSTR*)&wUUID);
>+  wUUID = NULL;
>+  if (!PathAppendSafe(programData, fileName)) {
>+    return FALSE;
>+  }
>+  const int FILE_SHARE_NONE = 0;
>+  nsAutoHandle updateMetaFile(CreateFileW(programData, GENERIC_WRITE, 
>+                                          FILE_SHARE_NONE, NULL, CREATE_NEW, 
>+                                          0, NULL));
>+  if (updateMetaFile == INVALID_HANDLE_VALUE) {
>+    return FALSE;
>+  }
>+
>+  // Write out the command line arguments that needs to be passed to updater.exe
>+  PRUnichar *commandLineBuffer = MakeCommandLine(argc, argv);
>+  DWORD wrote1, wrote2, wrote3, wrote4;
Please use descriptive names for wrote#

>+  
>+  DWORD sessionID = 0;
>+  ProcessIdToSessionId(GetCurrentProcessId(), &sessionID);
>+  BOOL result1 = ::WriteFile(updateMetaFile, &sessionID, 
>+                             sizeof(DWORD), 
>+                             &wrote1, NULL);
>+
>+  WCHAR appBuffer[MAX_PATH + 1];
>+  ZeroMemory(appBuffer, sizeof(appBuffer));
>+  wcscpy(appBuffer, exePath);
>+  BOOL result2 = ::WriteFile(updateMetaFile, appBuffer, 
>+                             MAX_PATH * sizeof(WCHAR), 
>+                             &wrote2, NULL);
>+
>+  WCHAR workingDirectory[MAX_PATH + 1];
>+  ZeroMemory(workingDirectory, sizeof(appBuffer));
>+  GetCurrentDirectoryW(sizeof(workingDirectory)/sizeof(workingDirectory[0]), 
nit: sizeof(workingDirectory) / sizeof(workingDirectory[0])

>+                       workingDirectory);
>+  BOOL result3 = ::WriteFile(updateMetaFile, workingDirectory, 
>+                             MAX_PATH * sizeof(WCHAR), 
>+                             &wrote3, NULL);
>+
>+  DWORD commandLineLength = wcslen(commandLineBuffer) * sizeof(WCHAR);
>+  BOOL result4 = ::WriteFile(updateMetaFile, commandLineBuffer, 
>+                             commandLineLength, 
>+                             &wrote4, NULL);
How about?
BOOL result;
result |= ::WriteFile
result |= ::WriteFile
etc.

>+  free(commandLineBuffer);
>+  if (!result1 || !result2 || !result3 || !result4 ||
>+      wrote1 != sizeof(DWORD) ||
>+      wrote2 != MAX_PATH * sizeof(WCHAR) ||
>+      wrote3 != MAX_PATH * sizeof(WCHAR) ||
>+      wrote4 != commandLineLength) {
>+    updateMetaFile.forget();
>+    DeleteFileW(programData);
>+    return FALSE;
>+  }
>+
>+  // Note we construct the 'service work' meta object with a ma extension
>+  // When we want the service to start processing it we simply rename it to
>+  // have a .mz extension.  This ensures that the service will never try to
>+  // process a partial update work meta file. 
Would it make sense to create the file in a different directory (temp?) and when it is ready move it to the directory?

>+  updateMetaFile.forget();
>+  WCHAR completedMetaFilePath[MAX_PATH + 1];
>+  wcscpy(completedMetaFilePath, programData);
>+  completedMetaFilePath[wcslen(completedMetaFilePath) -1] = 'z';
>+  return MoveFileExW(programData, completedMetaFilePath, 
>+                     MOVEFILE_REPLACE_EXISTING);
>+}
>+
>+BOOL
>+WinLaunchServiceCommand(const PRUnichar *exePath, int argc, char **argv)
>+{
>+  PRUnichar** argvConverted = new PRUnichar*[argc];
>+  if (!argvConverted)
>+    return FALSE;
>+
>+  for (int i = 0; i < argc; ++i) {
>+    argvConverted[i] = AllocConvertUTF8toUTF16(argv[i]);
>+    if (!argvConverted[i]) {
>+      FreeAllocStrings(i, argvConverted);
>+      return FALSE;
>+    }
>+  }
>+
>+  BOOL ok = WinLaunchServiceCommand(exePath, argc, argvConverted);
>+  FreeAllocStrings(argc, argvConverted);
>+  return ok;
>+}
>+
>+
>+
> /**
>  * Launch a child process with the specified arguments.
>  * @note argv[0] is ignored
>  * @note The form of this function that takes char **argv expects UTF-8
>  */
Include the parameters as well
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html

> 
> BOOL
> WinLaunchChild(const PRUnichar *exePath, int argc, PRUnichar **argv);

Comment 177

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

Comment on attachment 568950 [details] [diff] [review]
Patch 6 - Firefox preferences update page changes. v2.

># HG changeset patch
># Parent 7de33ffbb8ecba3bd67dcbb39e25f77a2fd01b94
># User Brian R. Bondy <[e-mail]>
>Bug481815 - Windows silent update service; Preferences for whether or not to use the service.
>
>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
>--- a/browser/app/profile/firefox.js
>+++ b/browser/app/profile/firefox.js
>@@ -184,16 +184,19 @@ pref("app.update.showInstalledUI", false
> 
> // 0 = suppress prompting for incompatibilities if there are updates available
> //     to newer versions of installed addons that resolve them.
> // 1 = suppress prompting for incompatibilities only if there are VersionInfo
> //     updates available to installed addons that resolve them, not newer
> //     versions.
> pref("app.update.incompatible.mode", 0);
> 
>+// Whether or not to attempt using the service for updates.
>+pref("app.update.service", true);
As stated in the last patch, I'm not certain this pref will be available when the update is applied during startup.

minusing until the pref issue is figured out.

Comment 178

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

Comment on attachment 568684 [details] [diff] [review]
Patch 7 - RAII base helpers

Since this is new code under XPCOM let's get Jim to review it and bsmedberg to sr it since he owns XPCOM after it has been reviewed by Jim.

Comment 179

[Brian R. Bondy [:bbondy]]

>+  bool attemptToUseService = 
>+    mozilla::Preferences::GetBool(kPrefAppUpdateService, false);
> Have you tested this? Unless I'm much mistaken this won't work because we don't
> actually have a profile at this time. Not sure how this should be handled if this
> is the case at this time.

Yup I've tested it.
I did have that problem and but please see Patch 13 - Moving the update check after init.

Comment 180

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

I forgot that Ted is a peer for NSPR... feel free to change the request

Comment 181

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

Comment on attachment 568688 [details] [diff] [review]
Patch 10 - Shared installer code and service upgrade

>diff --git a/browser/installer/windows/nsis/installer.nsi b/browser/installer/windows/nsis/installer.nsi
>--- a/browser/installer/windows/nsis/installer.nsi
>+++ b/browser/installer/windows/nsis/installer.nsi
>...
>@@ -34,16 +35,17 @@
> #
> # ***** END LICENSE BLOCK *****
> 
> # Required Plugins:
> # AppAssocReg   http://nsis.sourceforge.net/Application_Association_Registration_plug-in
> # ApplicationID http://nsis.sourceforge.net/ApplicationID_plug-in
> # ShellLink     http://nsis.sourceforge.net/ShellLink_plug-in
> # UAC           http://nsis.sourceforge.net/UAC_plug-in
>+# SimpleSC      http://nsis.sourceforge.net/NSIS_Simple_Service_Plugin
Remove if you go with the suggested method below

> 
> ; Set verbosity to 3 (e.g. no script) to lessen the noise in the build logs
> !verbose 3
> 
> ; 7-Zip provides better compression than the lzma from NSIS so we add the files
> ; uncompressed and use 7-Zip to create a SFX archive of it
> SetDatablockOptimize on
> SetCompress off
>@@ -363,16 +382,22 @@ Section "-Application" APP_IDX
>     ${If} $AddDesktopSC == 1
>     ${OrIf} $AddStartMenuSC == 1
>       WriteRegDWORD HKLM "$0" "IconsVisible" 1
>     ${Else}
>       WriteRegDWORD HKLM "$0" "IconsVisible" 0
>     ${EndIf}
>   ${EndIf}
> 
>+  ${IF} $InstallMaintenanceService == 1
>+    ; The user wants to install the maintenance service, so execute
>+    ; the pre-packaged maintenance service installer. /S for silent.
>+    nsExec::Exec '"$INSTDIR\maintenanceservice_installer\maintenanceservice_installer.exe" /S' 
note: I think this will end up being different than this but I'll call it out in the review of the new install code.

>@@ -787,16 +812,53 @@ Function leaveShortcuts
>     ${MUI_INSTALLOPTIONS_READ} $AddQuickLaunchSC "shortcuts.ini" "Field 4" "State"
>   ${EndUnless}
> 
>   ${If} $InstallType == ${INSTALLTYPE_CUSTOM}
>     Call CheckExistingInstall
>   ${EndIf}
> FunctionEnd
> 
>+Function preComponents
>+  ; Don't show the custom components page if the
>+  ; user is not an admin
>+  Call IsUserAdmin
>+  Pop $R9
>+  ${If} $R9 != "true"
>+    Abort
>+  ${EndIf}
>+
>+  ; If the service already exists, don't show this page
>+  ; We will always install again (which will upgrade)
>+  ; as long as the user is admin
>+  ${Unicode2Ansi} "MozillaMaintenance" $R8
>+  SimpleSC::ExistsService "$R8"
Instead of using the plugin couldn't you just check for the existence of the binary since we require it to be installed into a specific location under %ProgramFiles% or %ProgramFiles(x86)%? It would require two checks but it would remove the need for the plugin and the Unicode2Ansi or recompiling the plugin as x64.

>diff --git a/browser/installer/windows/nsis/uninstaller.nsi b/browser/installer/windows/nsis/uninstaller.nsi
>--- a/browser/installer/windows/nsis/uninstaller.nsi
>+++ b/browser/installer/windows/nsis/uninstaller.nsi
>@@ -14,16 +14,17 @@
> # The Original Code is the Mozilla Installer code.
> #
> # The Initial Developer of the Original Code is Mozilla Foundation
> # Portions created by the Initial Developer are Copyright (C) 2006
> # the Initial Developer. All Rights Reserved.
> #
> # Contributor(s):
> #  Robert Strong <[e-mail]>
>+#  Brian R. Bondy <[e-mail]>
> #
> # Alternatively, the contents of this file may be used under the terms of
> # either the GNU General Public License Version 2 or later (the "GPL"), or
> # the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
> # in which case the provisions of the GPL or the LGPL are applicable instead
> # of those above. If you wish to allow use of your version of this file only
> # under the terms of either the GPL or the LGPL, and not to allow others to
> # use your version of this file under the terms of the MPL, indicate your
>@@ -155,26 +156,28 @@ ShowUnInstDetails nevershow
> !define MUI_WELCOMEPAGE_TITLE_3LINES
> !define MUI_HEADERIMAGE
> !define MUI_HEADERIMAGE_RIGHT
> !define MUI_UNWELCOMEFINISHPAGE_BITMAP wizWatermark.bmp
> 
> ; Use a right to left header image when the language is right to left
> !ifdef ${AB_CD}_rtl
> !define MUI_HEADERIMAGE_BITMAP_RTL wizHeaderRTL.bmp
>+!define MUI_HEADERIMAGE_UNBITMAP_RTL wizHeaderRTL.bmp
> !else
> !define MUI_HEADERIMAGE_BITMAP wizHeader.bmp
>+!define MUI_HEADERIMAGE_UNBITMAP wizHeader.bmp
> !endif
It uses the correct header anyway due to the custom branding code. Since we don't display the installer ui for this code could you check if it does the right thing with MUI_HEADERIMAGE_BITMAP and MUI_HEADERIMAGE_BITMAP_RTL undefined and that the default headers aren't included then?

> /**
>  * Uninstall Pages
>  */
> ; Welcome Page
>-!define MUI_PAGE_CUSTOMFUNCTION_PRE un.preWelcome
>-!define MUI_PAGE_CUSTOMFUNCTION_LEAVE un.leaveWelcome
>+;!define MUI_PAGE_CUSTOMFUNCTION_PRE un.preWelcome
>+;!define MUI_PAGE_CUSTOMFUNCTION_LEAVE un.leaveWelcome
These are needed so the UI can use custom branding.

> !insertmacro MUI_UNPAGE_WELCOME
> 
> ; Custom Uninstall Confirm Page
> UninstPage custom un.preConfirm un.leaveConfirm
> 
> ; Remove Files Page
> !insertmacro MUI_UNPAGE_INSTFILES
> 
>diff --git a/browser/locales/en-US/installer/custom.properties b/browser/locales/en-US/installer/custom.properties
>--- a/browser/locales/en-US/installer/custom.properties
>+++ b/browser/locales/en-US/installer/custom.properties
>@@ -52,28 +52,32 @@
> 
> REG_APP_DESC=$BrandShortName delivers safe, easy web browsing. A familiar user interface, enhanced security features including protection from online identity theft, and integrated search let you get the most out of the web.
> CONTEXT_OPTIONS=$BrandShortName &Options
> CONTEXT_SAFE_MODE=$BrandShortName &Safe Mode
> OPTIONS_PAGE_TITLE=Setup Type
> OPTIONS_PAGE_SUBTITLE=Choose setup options
> SHORTCUTS_PAGE_TITLE=Set Up Shortcuts
> SHORTCUTS_PAGE_SUBTITLE=Create Program Icons
>+COMPONENTS_PAGE_TITLE=Set Up Optional Components
>+COMPONENTS_PAGE_SUBTITLE=Optional Recommended Components
> SUMMARY_PAGE_TITLE=Summary
> SUMMARY_PAGE_SUBTITLE=Ready to start installing $BrandShortName
> SUMMARY_INSTALLED_TO=$BrandShortName will be installed to the following location:
> SUMMARY_REBOOT_REQUIRED_INSTALL=A restart of your computer may be required to complete the installation.
> SUMMARY_REBOOT_REQUIRED_UNINSTALL=A restart of your computer may be required to complete the uninstall.
> SUMMARY_TAKE_DEFAULTS=U&se $BrandShortName as my default web browser
> SUMMARY_INSTALL_CLICK=Click Install to continue.
> SUMMARY_UPGRADE_CLICK=Click Upgrade to continue.
> SURVEY_TEXT=&Tell us what you thought of $BrandShortName
> LAUNCH_TEXT=&Launch $BrandShortName now
> CREATE_ICONS_DESC=Create icons for $BrandShortName:
>+OPTIONAL_COMPONENTS_DESC=The Maintenance Service will allow you to update $BrandShortName silently in the background.
> ICONS_DESKTOP=On my &Desktop
>+MAINTENANCE_SERVICE_CHECKBOX_DESC=Install &Maintenance Service
Just put OPTIONAL_COMPONENTS_DESC and MAINTENANCE_SERVICE_CHECKBOX_DESC directly following COMPONENTS_PAGE_SUBTITLE.

Please get faaborg or limi to approve the text.

Overall looks good and this should be a quick review after it is resubmitted

Comment 182

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

Comment on attachment 568688 [details] [diff] [review]
Patch 10 - Shared installer code and service upgrade

That was meant for patch 9 :/

Comment 183

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

Comment on attachment 568688 [details] [diff] [review]
Patch 10 - Shared installer code and service upgrade

>diff --git a/browser/installer/windows/nsis/shared.nsh b/browser/installer/windows/nsis/shared.nsh
>--- a/browser/installer/windows/nsis/shared.nsh
>+++ b/browser/installer/windows/nsis/shared.nsh
>@@ -14,32 +14,39 @@
>...
> !macro PostUpdate
>+
>+  ; 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.
>+  nsExec::Exec '"$INSTDIR\maintenanceservice_installer\maintenanceservice_installer.exe" /S /Upgrade' 
note: I think this will end up being different than this but I'll call it out in the review of the new install code.

>@@ -935,19 +942,57 @@
>   Push "nspr4.dll"
>   Push "nssdbm3.dll"
>   Push "mozsqlite3.dll"
>   Push "xpcom.dll"
>   Push "crashreporter.exe"
>   Push "updater.exe"
>   Push "${FileMainEXE}"
> !macroend
>+
>+!define Unicode2Ansi "!insertmacro Unicode2Ansi"
>+!macro Unicode2Ansi String outVar
>+  System::Call 'kernel32::WideCharToMultiByte(i 0, i 0, w "${String}", i -1, t .s, i ${NSIS_MAX_STRLEN}, i 0, i 0) i'
>+  Pop "${outVar}"
>+!macroend
Won't be needed if the file check approach is used.

>+
>+; Copied from: http://nsis.sourceforge.net/IsUserAdmin
>+Function IsUserAdmin
>+  Push $R0
>+  Push $R1
>+  Push $R2
>+ 
>+  ClearErrors
>+  UserInfo::GetName
>+  IfErrors Win9x
>+  Pop $R1
>+  UserInfo::GetAccountType
>+  Pop $R2
>+ 
>+  StrCmp $R2 "Admin" 0 Continue
>+  StrCpy $R0 "true"
>+  Goto Done
>+ 
>+  Continue:
>+
>+  StrCmp $R2 "" Win9x
>+  StrCpy $R0 "false"
>+  Goto Done
>+ 
>+  Win9x:
>+  StrCpy $R0 "true"
>+ 
>+  Done:
>+  Pop $R2
>+  Pop $R1
>+  Exch $R0
>+FunctionEnd
>+
I'd prefer if IsUserAdmin was turned into a macro and added to common.nsh so all apps can use it

Should be a quick review with those changes

Comment 184

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

Comment on attachment 568689 [details] [diff] [review]
Patch 11 - New maintenance service installer

>diff --git a/browser/installer/windows/nsis/maintenanceservice_installer.nsi b/browser/installer/windows/nsis/maintenanceservice_installer.nsi
It seems to me that this could always run silently and thereby simplify the code quite a lot.

Also, it would lessen the size of the overall installer if this installer didn't include the files inside of it. Instead, you can just install the files in the root of the installation directory and copy them to the destination directory.

Are you ok with doing it that way?

Comment 185

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

Regarding "always run silently", I am referring to the install process. I haven't looked at the uninstall process yet.

Comment 186

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

Comment on attachment 568689 [details] [diff] [review]
Patch 11 - New maintenance service installer

This is dependent on whether the file check is sufficient or the plugin is actually needed. Removing review until that is answered.

Comment 187

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

Comment on attachment 568690 [details] [diff] [review]
Patch 12 - Simple SC plugin inclusion

Too many reviews! :)

This is the patch that is dependent on whether the file check is sufficient or the plugin is actually needed. Removing review until that is answered.

Comment 188

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

Comment on attachment 568691 [details] [diff] [review]
Patch 13 - Moving the update check after init

I'm overall ok with this though it does scare me a bit since there may be unintended consequences. Does the profile manager display twice when it is set to be displayed for example?

I'm going to test this out before r+ing and we might want to list the behavior changes so they are understood.

Comment 189

[Brian R. Bondy [:bbondy]]

> I'm overall ok with this though it does scare me a bit

Me too, I hate moving things around at init.  I haven't sen any issues yet though.

> Does the profile manager display twice when it is set to be displayed for example?

Do you mean just: 
firefox.exe -ProfileManager ?

If that's what you mean no it only displays once.  There isn't a second init call it's just a move of the update call a little lower in the init.

Or did you mean when there is an update applied?

Comment 190

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

When an update is applied and all cases where the profile manager is displayed. I suspect it does in that case and we added code elsewhere to cover that case.

Comment 191

[Brian R. Bondy [:bbondy]]

Patch 13 - Moving the update check after init.

OK I tested it and you're absolutely right.

When you start Firefox -ProfileManager when there is a pending update, it will show the profile selection screen.
You select the profile you want, then it does the update.

After the update, the service then relaunches with -ProfileManager but the profile selection screen doesn't display. 
So there must be some other code taking care of that already even know the command line was passed.

If there are some other special cases like this that are problems, we can filter out those command line parameters similar to how Patch 16 works.

Comment 192

[Brian R. Bondy [:bbondy]]

> It isn't clear to me whether it is better to use a common key for 
> all cert attributes for all installs or to have each install store 
> the cert attributes in an install specific key. I am leaning install 
> specific keys since another app should not be able to provide cert 
> attributes that are then used by another app. What do you think?

The way it currently works is that you can have any amount of subfolders under the main Certs key.  Each subkey has the attributes which are allowed.  How do you envision appliction specific locations working? How does the service know where to look?

Comment 193

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

We could use the install path or a hash of the install path in the registry.

Comment 194

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

See bug 577867 for an example

Comment 195

[Brian R. Bondy [:bbondy]]

Attachment: Patch 4 - Certificate check code inside service. v2.

Implemented review comments.

> Ideally, we would allow the application to specify what cert attributes to check 
> though I think this is fine at least for now.

This is already supported just leave the fields blank to skip that check.

Comment 196

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2 - Main service logic. v3'

Same as last patch but updated function name.

Comment 197

[Brian R. Bondy [:bbondy]]

Attachment: Patch 16 - Support for launching callback app as unelevated. v1'

Same as original patch, just has updated function name.

Comment 198

[Brian R. Bondy [:bbondy]]

>+  // Note we construct the 'service work' meta object with a ma extension
>+  // When we want the service to start processing it we simply rename it to
>+  // have a .mz extension.  This ensures that the service will never try to
>+  // process a partial update work meta file. 
> Would it make sense to create the file in a different directory (temp?) and when 
> it is ready move it to the directory?

I'd prefer to keep it in the same directory to avoid complications.  For example if the temp directory was in a different volume and you move the file across volumes you will lose some informatin like security descriptors and it might be of a different fs type hence losing even more info. Also you'd need to specify the flag MOVEFILE_COPY_ALLOWED in that case which uses more space and could fail due to really low disk space situations.  On a different volume w/ MoveFileEx copying it might even cause the service to process a partial work item file.

Comment 199

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Brian R. Bondy [:bbondy] from comment #198)
> > Would it make sense to create the file in a different directory (temp?) and when it is ready move it to the directory?
> 
> I'd prefer to keep it in the same directory to avoid complications.  For
> example if the temp directory was in a different volume and you move the
> file across volumes you will lose some informatin like security descriptors
> and it might be of a different fs type hence losing even more info. Also
> you'd need to specify the flag MOVEFILE_COPY_ALLOWED in that case which uses
> more space and could fail due to really low disk space situations.  On a
> different volume w/ MoveFileEx copying it might even cause the service to
> process a partial work item file.
Brian, are you aware of bug 307181?

Comment 200

[Brian R. Bondy [:bbondy]]

> Brian, are you aware of bug 307181?

Yup this MoveFileEx call is admittedly much smaller of a potential problem than that but I was just explaining why I chose to keep it in the same directory while programming it.  I do plan on doing a feedback review on bug 307181 though so thanks for checking to make sure.

Comment 201

[Brian R. Bondy [:bbondy]]

Attachment: Patch 5 - XUL Runtime Environment code. v3.

Review comments implemented.
Regarding the CreateDirectory / owner issue you brought up I'll add the directory creations as well to the installer to avoid this problem.

Comment 202

[Brian R. Bondy [:bbondy]]

>+  SimpleSC::ExistsService "$R8"
> Instead of using the plugin couldn't you just check for the existence of the binary since 
> we require it to be installed into a specific location under %ProgramFiles% or %ProgramFiles(x86)%? 
> It would require two checks but it would remove the need for the plugin and the Unicode2Ansi or recompiling the plugin as x64.

I think even for x64 compilations NSIS always produces x86 installers.
There is no real need security wise or coding logic wise that requires the service to be in one of those 2 locations.
Also in case someone manually uninstalls the service but the file exists I think the check for existence of the service itself is best.

Also I had planned to change this eventually but for any future version after this first release, we'll need to use Simple SC's stop service as well before installing the service.
It's not a problem right now because nspr4.dll will never be new (since it's a first install) but there is potential right now for it being in use if a user manually started the service.

For now I'll go ahead with keeping Simple SC and also adding the Stop Service call.
But please let me know if you are strongly for not including this plug-in, then I will rethink the situation and make a different patch.

Comment 203

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

(In reply to Brian R. Bondy [:bbondy] from comment #202)
> >+  SimpleSC::ExistsService "$R8"
> > Instead of using the plugin couldn't you just check for the existence of the binary since 
> > we require it to be installed into a specific location under %ProgramFiles% or %ProgramFiles(x86)%? 
> > It would require two checks but it would remove the need for the plugin and the Unicode2Ansi or recompiling the plugin as x64.
> 
> I think even for x64 compilations NSIS always produces x86 installers.
> There is no real need security wise or coding logic wise that requires the
> service to be in one of those 2 locations.
> Also in case someone manually uninstalls the service but the file exists I
> think the check for existence of the service itself is best.
> 
> Also I had planned to change this eventually but for any future version
> after this first release, we'll need to use Simple SC's stop service as well
> before installing the service.
> It's not a problem right now because nspr4.dll will never be new (since it's
> a first install) but there is potential right now for it being in use if a
> user manually started the service.
> 
> For now I'll go ahead with keeping Simple SC and also adding the Stop
> Service call.
> But please let me know if you are strongly for not including this plug-in,
> then I will rethink the situation and make a different patch.
My main concern is that this will need to work with nsis 2.33u (I don't recall if it supports ANSI plugins) since that is what is deployed to the build machines since bug 594474 hasn't been fixed yet.

Comment 204

[Brian R. Bondy [:bbondy]]

> My main concern is that this will need to work with nsis 2.33u (I don't recall if > it supports ANSI plugins) since that is what is deployed to the build machines > since bug 594474 hasn't been fixed yet.

Would you be OK if I make SimpleSC part of the build process and make it unicode?

Comment 205

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

That won't work (at least not easily) because localizers repackage builds for locales at which time we rebuild the installer. You could convert it to unicode and checkin the binary as is done here:
http://mxr.mozilla.org/mozilla-central/source/other-licenses/nsis/

Comment 206

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

Comment on attachment 569681 [details] [diff] [review]
Patch 15 - UAC helper functions.

>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
>@@ -44,16 +44,17 @@ include $(DEPTH)/config/autoconf.mk
> 
> CPPSRCS = \
>   maintenanceservice.cpp \
> 	serviceinstall.cpp \
> 	workmonitor.cpp \
> 	certificatecheck.cpp \
> 	servicebase.cpp \
> 	registrycertificates.cpp \
>+	uachelper.cpp \
spaces are prefered

>diff --git a/toolkit/components/maintenanceservice/uachelper.cpp b/toolkit/components/maintenanceservice/uachelper.cpp
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/maintenanceservice/uachelper.cpp
>@@ -0,0 +1,157 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is UAC helper functions.
Maintenance service UAC helper functions

>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Brian R. Bondy <[e-mail]>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * 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 "uachelper.h"
>+
>+typedef BOOL (WINAPI *LPWTSQueryUserToken)(ULONG, PHANDLE);
>+
>+// static
>+bool UACHelper::IsVistaOrLater() 
as mentioned earlier the function name should be on its own line here and elsewhere

>+{
>+  // Check if we are running Vista or later.
>+  OSVERSIONINFO osInfo;
>+  osInfo.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
>+  return GetVersionEx(&osInfo) && osInfo.dwMajorVersion >= 6;
>+}
>+
>+// static
>+HANDLE UACHelper::OpenUserToken(DWORD sessionID)
>+{
>+  HMODULE module = LoadLibraryW(L"wtsapi32.dll");
>+  HANDLE token = NULL;
>+  LPWTSQueryUserToken wtsQueryUserToken = (LPWTSQueryUserToken)GetProcAddress(module, "WTSQueryUserToken");
>+  if (wtsQueryUserToken) {
>+    wtsQueryUserToken(sessionID, &token);
>+  }
>+  FreeModule(module);
>+  return token;
>+}
>+
>+// static
>+HANDLE UACHelper::OpenLinkedToken(HANDLE token) 
>+{
>+  // Magic below...
>+  // UAC creates 2 tokens.  One is the restricted token which we have.
>+  // the other is the UAC elevated one. Since we are running as a service
>+  // as the system account we have access to both.
>+  TOKEN_LINKED_TOKEN tlt;
>+  HANDLE hNewLinkedToken = NULL;
>+  DWORD len;
>+  if(::GetTokenInformation(token, (TOKEN_INFORMATION_CLASS)TokenLinkedToken, 
>+    &tlt, sizeof(TOKEN_LINKED_TOKEN), &len)) {
nit: alignment

>+      token = tlt.LinkedToken;
>+      hNewLinkedToken = token;
>+  }
>+  return hNewLinkedToken;
>+}
>+
>+// static
>+bool UACHelper::IsUserAdmin(HANDLE token, BOOL &isAdmin)
>+{
>+  bool success = false;
>+  SID_IDENTIFIER_AUTHORITY NtAuthority = SECURITY_NT_AUTHORITY;
>+  PSID administratorsGroup; 
>+  if (AllocateAndInitializeSid(&NtAuthority, 2, SECURITY_BUILTIN_DOMAIN_RID,
>+                               DOMAIN_ALIAS_RID_ADMINS, 0, 0, 0, 0, 0, 0,
>+                               &administratorsGroup)) {
>+    if (CheckTokenMembership(token, administratorsGroup, &isAdmin)) {
>+      success = true;
nit I'm fine with either TRUE / FALSE or true / false for the standalone app... just be consistent across the files.

>+    } 
>+    FreeSid(administratorsGroup); 
>+  }
>+  return success;
>+}
>+
>+// static
>+bool UACHelper::GetElevationType(HANDLE token, UserType &userType)
>+{
>+  if (!IsVistaOrLater()) {
>+    BOOL isAdmin;
>+    if (!IsUserAdmin(token, isAdmin)) {
>+      return false;
>+    }
>+
>+    if (isAdmin) {
>+      userType = AdministratorUACIsOff;
>+    } else {
>+      userType = LimitedUser;
>+    }
>+    return true;
>+  }
>+
>+  // TokenElevationTypeDefault: The token does not have a linked token 
>+  // This happens when the user is a limited account or UAC is off.
>+  // TokenElevationTypeFull: The token is an elevated token.
>+  // TokenElevationTypeLimited: The token is a limited token.
>+  DWORD returnLength = 0;
>+  TOKEN_ELEVATION_TYPE elevationType;
>+  if (!::GetTokenInformation(token, TokenElevationType,
>+    &elevationType, sizeof(elevationType), &returnLength)) {
nit: alignment

>+      return false;
>+  }
>+
>+  switch (elevationType) {
nit: switch alignment as mentioned earlier

>+  case TokenElevationTypeFull:
>+    userType = AdministratorElevated;
>+    break;
>+  case TokenElevationTypeLimited:
>+    userType = AdministratorUnelevated;
>+    break;
>+  case TokenElevationTypeDefault: 
>+    {
>+      // TokenElevationTypeDefault is returned when either the user is a 
>+      // limited account or the user has UAC offa limited user or UAC is off
>+      BOOL isAdmin;
>+      if (!IsUserAdmin(token, isAdmin)) {
>+        return false;
>+      }
>+
>+      if (isAdmin) {
>+        userType = AdministratorUACIsOff;
Kind of prefer just Administrator but I am fine with this.

>+      } else {
>+        userType = LimitedUser;
>+      }
>+    }
>+    break;
>+  default:
>+    return false;
>+  }
>+
>+  // If we had an error we would have returned already
>+  return true;
>+}

Comment 207

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

Comment on attachment 568691 [details] [diff] [review]
Patch 13 - Moving the update check after init

I don't see any other way we can accomplish this reasonably... we'll just need to keep a close eye on this.

Comment 208

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

Comment on attachment 568693 [details] [diff] [review]
Patch 14 - Build process

>diff --git a/browser/Makefile.in b/browser/Makefile.in
>--- a/browser/Makefile.in
>+++ b/browser/Makefile.in
>@@ -65,11 +65,11 @@ include $(topsrcdir)/config/rules.mk
> 
> ifeq ($(OS_ARCH),WINNT)
> ifdef MOZ_INSTALLER
> 
> # For Windows build the uninstaller during the application build since the
> # uninstaller is included with the application for mar file generation.
> libs::
> 	$(MAKE) -C installer/windows uninstaller
>-
>+	$(MAKE) -C installer/windows maintenanceservice_installer
> endif
> endif
>diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in
>--- a/browser/installer/package-manifest.in
>+++ b/browser/installer/package-manifest.in
>@@ -28,16 +28,17 @@
> @BINPATH@/defaults/profile/mimeTypes.rdf
> @BINPATH@/defaults/profile/chrome/*
> @BINPATH@/update.locale
> @BINPATH@/updater.ini
> @BINPATH@/dictionaries/*
> @BINPATH@/hyphenation/*
> #ifdef XP_WIN32
> @BINPATH@/uninstall/helper.exe
>+@BINPATH@/maintenanceservice_installer/maintenanceservice_installer.exe
Per phone conversation, no maintenanceservice_installer directory and it appears to be proper below.

> #endif
> 
> [xpcom]
> @BINPATH@/dependentlibs.list
> #ifndef MOZ_STATIC_JS
> @BINPATH@/@DLL_PREFIX@mozjs@DLL_SUFFIX@
> #endif
> @BINPATH@/@DLL_PREFIX@plc4@DLL_SUFFIX@
>@@ -522,16 +523,22 @@ bin/libfreebl_32int64_3.so
> ; [Updater]
> ;
> #ifdef XP_MACOSX
> @BINPATH@/updater.app/
> #else
> @BINPATH@/updater@BIN_SUFFIX@
> #endif
> 
>+; [MaintenanceService]
>+;
>+#ifdef XP_WIN32
>+@BINPATH@/maintenanceservice.exe
>+#endif
>+

>diff --git a/browser/installer/windows/Makefile.in b/browser/installer/windows/Makefile.in
>--- a/browser/installer/windows/Makefile.in
>+++ b/browser/installer/windows/Makefile.in
>...
>@@ -70,16 +71,21 @@ BRANDING_FILES = \
> 
> DEFINES += \
> 	-DAB_CD=$(AB_CD) \
> 	-DMOZ_APP_NAME=$(MOZ_APP_NAME) \
> 	-DMOZ_APP_DISPLAYNAME=${MOZ_APP_DISPLAYNAME} \
> 	-DMOZILLA_VERSION=${MOZILLA_VERSION} \
> 	$(NULL)
> 
>+MAINTENANCESERVICE_FILES = \
>+	maintenanceservice.exe \
>+	nspr4.dll \
>+	$(NULL)
Needs to be updated per discussion

>@@ -99,16 +105,30 @@ uninstaller::
> 	$(INSTALL) $(addprefix $(srcdir)/,$(INSTALLER_FILES)) $(CONFIG_DIR)
> 	$(INSTALL) $(addprefix $(DIST)/branding/,$(BRANDING_FILES)) $(CONFIG_DIR)
> 	$(PYTHON) $(topsrcdir)/config/Preprocessor.py -Fsubstitution $(DEFINES) $(ACDEFINES) \
> 	  $(srcdir)/nsis/defines.nsi.in > $(CONFIG_DIR)/defines.nsi
> 	$(PYTHON) $(topsrcdir)/toolkit/mozapps/installer/windows/nsis/preprocess-locale.py \
> 	  --preprocess-locale $(topsrcdir) \
> 	  $(PPL_LOCALE_ARGS) $(AB_CD) $(CONFIG_DIR)
> 
>+# For building the maintenanceservice installer
>+maintenanceservice_installer::
>+	$(RM) -r $(CONFIG_DIR)
>+	$(MKDIR) $(CONFIG_DIR)
>+	$(INSTALL) $(addprefix $(srcdir)/,$(INSTALLER_FILES)) $(CONFIG_DIR)
>+	$(INSTALL) $(addprefix $(DIST)/branding/,$(BRANDING_FILES)) $(CONFIG_DIR)
>+	$(INSTALL) $(addprefix $(DIST)/bin/,$(MAINTENANCESERVICE_FILES)) $(CONFIG_DIR)
>+	$(PYTHON) $(topsrcdir)/config/Preprocessor.py -Fsubstitution $(DEFINES) $(ACDEFINES) \
>+	  $(srcdir)/nsis/defines.nsi.in > $(CONFIG_DIR)/defines.nsi
>+	$(PYTHON) $(topsrcdir)/toolkit/mozapps/installer/windows/nsis/preprocess-locale.py \
>+	  --preprocess-locale $(topsrcdir) \
>+	  $(PPL_LOCALE_ARGS) $(AB_CD) $(CONFIG_DIR)
Needs to be updated per discussion

>diff --git a/mobile/installer/package-manifest.in b/mobile/installer/package-manifest.in
>--- a/mobile/installer/package-manifest.in
>+++ b/mobile/installer/package-manifest.in
>@@ -30,16 +30,17 @@
> #ifdef MOZ_UPDATER
> @BINPATH@/update.locale
> @BINPATH@/updater.ini
> #endif
> @BINPATH@/dictionaries/*
> @BINPATH@/hyphenation/*
> #ifdef XP_WIN32
> @BINPATH@/uninstall/helper.exe
>+@BINPATH@/maintenanceservice_installer/maintenanceservice_installer.exe
Mobile?

>diff --git a/toolkit/components/maintenanceservice/Makefile.in b/toolkit/components/maintenanceservice/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/maintenanceservice/Makefile.in
>@@ -0,0 +1,105 @@
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+#
>+# The contents of this file are subject to the Mozilla Public License Version
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# The Original Code is Application Update Service.
Maintenance service.

>+include $(DEPTH)/config/autoconf.mk
>+
>+CPPSRCS = \
>+  maintenanceservice.cpp \
>+	serviceinstall.cpp \
>+	workmonitor.cpp \
>+	certificatecheck.cpp \
>+	servicebase.cpp \
>+	registrycertificates.cpp \
>+  $(NULL)
Just use spaces... iirc this is per Ted and / or Kyle

>+
>+ifneq ($(OS_TARGET),Android)
>+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 =
>+endif
>+
>+LIBS += \
>+  $(DEPTH)/modules/libmar/src/$(LIB_PREFIX)mar.$(LIB_SUFFIX) \
>+  $(BZ2_LIBS) \
>+  $(NSPR_LIBS) \
>+  $(NULL)
>+
>+ifeq ($(OS_ARCH),WINNT)
>+USE_STATIC_LIBS = 1
>+HAVE_PROGRESSUI = 1
>+RCINCLUDE = maintenanceservice.rc
>+
>+OS_LIBS += $(call EXPAND_LIBNAME,comctl32 ws2_32 shell32)
>+DEFINES += -DUNICODE -D_UNICODE
>+ifndef GNU_CC
>+RCFLAGS += -I$(srcdir)
>+else
>+RCFLAGS += --include-dir $(srcdir)
>+endif
>+
>+endif
>+
>+ifndef MOZ_WINCONSOLE
>+ifdef MOZ_DEBUG
>+MOZ_WINCONSOLE = 1
>+else
>+MOZ_WINCONSOLE = 0
>+endif
>+endif
>+
>+include $(topsrcdir)/config/rules.mk
>+
>+DEFINES += -DNS_NO_XPCOM
>+
>+ifdef _MSC_VER
>+WIN32_EXE_LDFLAGS += -ENTRY:wmainCRTStartup
>+endif
>+
>+ifeq (,$(filter-out WINNT,$(OS_ARCH)))
>+# Pick up nsWindowsRestart.cpp
>+LOCAL_INCLUDES += -I$(topsrcdir)/toolkit/xre
>+endif
>+
>+CXXFLAGS += $(BZ2_CFLAGS)
This looks like it was copied and has a bunch of cruft

Comment 209

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

Comment on attachment 568693 [details] [diff] [review]
Patch 14 - Build process

>diff --git a/toolkit/locales/l10n.mk b/toolkit/locales/l10n.mk
>--- a/toolkit/locales/l10n.mk
>+++ b/toolkit/locales/l10n.mk
>@@ -139,16 +139,18 @@ endif
> repackage-zip: UNPACKAGE="$(ZIP_IN)"
> repackage-zip:  libs-$(AB_CD)
> # Adjust jar logs with the new locale (can't use sed -i because of bug 373784)
> 	mkdir -p $(JARLOG_DIR_AB_CD)
> 	-cp -r $(JARLOG_DIR)/en-US/*.jar.log $(JARLOG_DIR_AB_CD)
> 	-$(PERL) -pi.old -e "s/en-US/$(AB_CD)/g" $(JARLOG_DIR_AB_CD)/*.jar.log
> # call a hook for apps to put their uninstall helper.exe into the package
> 	$(UNINSTALLER_PACKAGE_HOOK)
>+# call a hook for apps to put their maintenance service into the package
>+	$(MAINTENANCESERVICE_PACKAGE_HOOK)
Not all apps will build the maintenance service.

>diff --git a/toolkit/mozapps/installer/windows/nsis/makensis.mk b/toolkit/mozapps/installer/windows/nsis/makensis.mk
>--- a/toolkit/mozapps/installer/windows/nsis/makensis.mk
>+++ b/toolkit/mozapps/installer/windows/nsis/makensis.mk
>@@ -55,25 +55,34 @@ TOOLKIT_NSIS_FILES = \
> 
> CUSTOM_NSIS_PLUGINS = \
> 	AccessControl.dll \
> 	AppAssocReg.dll \
> 	ApplicationID.dll \
> 	InvokeShellVerb.dll \
> 	ShellLink.dll \
> 	UAC.dll \
>+	SimpleSC.dll \
> 	$(NULL)
> 
>+MAINTENANCESERVICE_FILES = \
>+	maintenanceservice.exe \
>+	nspr4.dll \
>+	$(NULL)
Per phone discussion, just use the files in the root of the app dir

>+
>+
> $(CONFIG_DIR)/setup.exe::
> 	$(INSTALL) $(addprefix $(MOZILLA_DIR)/toolkit/mozapps/installer/windows/nsis/,$(TOOLKIT_NSIS_FILES)) $(CONFIG_DIR)
> 	$(INSTALL) $(addprefix $(MOZILLA_DIR)/other-licenses/nsis/Plugins/,$(CUSTOM_NSIS_PLUGINS)) $(CONFIG_DIR)
> 	cd $(CONFIG_DIR) && $(MAKENSISU) installer.nsi
>-# Support for building the uninstaller when repackaging locales
>+# Support for building the uninstaller and maintenance service when repackaging locales
> ifeq ($(CONFIG_DIR),l10ngen)
>+	$(INSTALL) $(addprefix $(DIST)/bin/,$(MAINTENANCESERVICE_FILES)) $(CONFIG_DIR)
Should not be necessary. Also, you can't install it before building it which happens after this

> 	cd $(CONFIG_DIR) && $(MAKENSISU) uninstaller.nsi
>+	cd $(CONFIG_DIR) && $(MAKENSISU) maintenanceservice_installer.nsi
This should only be called when the app has maintenanceservice_installer.nsi

> endif
> 
> $(CONFIG_DIR)/7zSD.sfx:
> 	$(CYGWIN_WRAPPER) upx --best -o $(CONFIG_DIR)/7zSD.sfx $(SFX_MODULE)
> 
> installer::
> 	$(INSTALL) $(CONFIG_DIR)/setup.exe $(DEPTH)/installer-stage
> 	cd $(DEPTH)/installer-stage && $(CYGWIN_WRAPPER) 7z a -r -t7z $(ABS_CONFIG_DIR)/app.7z -mx -m0=BCJ2 -m1=LZMA:d24 -m2=LZMA:d19 -m3=LZMA:d19  -mb0:1 -mb0s1:2 -mb0s2:3
>@@ -85,8 +94,17 @@ installer::
> # For building the uninstaller during the application build so it can be
> # included for mar file generation.
> uninstaller::
> 	$(INSTALL) $(addprefix $(MOZILLA_DIR)/toolkit/mozapps/installer/windows/nsis/,$(TOOLKIT_NSIS_FILES)) $(CONFIG_DIR)
> 	$(INSTALL) $(addprefix $(MOZILLA_DIR)/other-licenses/nsis/Plugins/,$(CUSTOM_NSIS_PLUGINS)) $(CONFIG_DIR)
> 	cd $(CONFIG_DIR) && $(MAKENSISU) uninstaller.nsi
> 	$(NSINSTALL) -D $(DIST)/bin/uninstall
> 	cp $(CONFIG_DIR)/helper.exe $(DIST)/bin/uninstall
>+
>+# For building the maintenanceservice installer during the application build
>+maintenanceservice_installer::
>+	$(INSTALL) $(addprefix $(MOZILLA_DIR)/toolkit/mozapps/installer/windows/nsis/,$(TOOLKIT_NSIS_FILES)) $(CONFIG_DIR)
>+	$(INSTALL) $(addprefix $(MOZILLA_DIR)/other-licenses/nsis/Plugins/,$(CUSTOM_NSIS_PLUGINS)) $(CONFIG_DIR)
>+	$(INSTALL) $(addprefix $(DIST)/bin/,$(MAINTENANCESERVICE_FILES)) $(CONFIG_DIR)
Should not be necessary

>+	cd $(CONFIG_DIR) && $(MAKENSISU) maintenanceservice_installer.nsi
>+	$(NSINSTALL) -D $(DIST)/bin/maintenanceservice_installer
>+	cp $(CONFIG_DIR)/maintenanceservice_installer.exe $(DIST)/bin/maintenanceservice_installer
Should be in the root of the app dir

I *think* having a separate target is the right choice so other apps that don't use the service don't try to build it but the $(CONFIG_DIR)/setup.exe target builds maintenanceservice_installer.exe so that will break other apps.

Comment 210

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

Comment on attachment 568689 [details] [diff] [review]
Patch 11 - New maintenance service installer

Needs updating per phone discussion

Comment 211

[Brian R. Bondy [:bbondy]]

Attachment: Patch 15 - UAC helper functions. v2.

Implemented review comments, added javadoc comments and other formatting fixes.  r+'ed myself since it was already r+.  rs, let me know if you have any extra comments though you want me to change.

Comment 212

[Brian R. Bondy [:bbondy]]

Attachment: Patch 1 - Base service code. v3.

Fixed bool/BOOL consistency and return type of functions on its own line

Comment 213

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2 - Main service logic. v4

Fixed bool/BOOL consistency and return type of functions on its own line.

Comment 214

[Brian R. Bondy [:bbondy]]

Attachment: Patch 3 - Code for installing the service. v3.

Fixed bool/BOOL consistency and return type of functions on its own line.

Comment 215

[Brian R. Bondy [:bbondy]]

Comment on attachment 569895 [details] [diff] [review]
Patch 4 - Certificate check code inside service. v2.

Cancelling review on this until I implement changes as per discussion on phone with registry layout (hashing per install path).

Comment 216

[Brian R. Bondy [:bbondy]]

Attachment: Patch 5 - XUL Runtime Environment code. v4.

Fixed return type of functions on its own line.

Comment 217

[Brian R. Bondy [:bbondy]]

Comment on attachment 568950 [details] [diff] [review]
Patch 6 - Firefox preferences update page changes. v2.

As per the review on patch 6 previously:

> As stated in the last patch, I'm not certain this pref will be 
> available when the update is applied during startup.
> minusing until the pref issue is figured out.

re-marking as r? since I think we're going ahead with this method now.

Comment 218

[Brian R. Bondy [:bbondy]]

Attachment: Patch 16 - Support for launching callback app as unelevated. v2

Rebased.

Comment 219

[Brian R. Bondy [:bbondy]]

Attachment: Patch 15 - UAC helper functions. v2'.

Moved out Makefile.in to patch 14.  Was already r+, remarking.

Comment 220

[Brian R. Bondy [:bbondy]]

Turns out SimpleSC is pascal and hence why no one has tried to port it to Unicode.  I'm going to go ahead and make my own plugin, I've made some before so should only take a couple hours.

Comment 221

[Brian R. Bondy [:bbondy]]

Attachment: Patch 12 - Simple SC plugin inclusion. v2.

- Removed SimpleSC.
- Created new plugin ServicesHelper.  

This is created from scratch so I'm not sure of the best location in the tree.
It doesn't seem ideal to put it under other-licenses/nsis/Contrib/ but to keep it alongside the other plugins I think that is best.  Let me know if you want me to move it somewhere else.

Comment 222

[Brian R. Bondy [:bbondy]]

Attachment: Patch 9 - Firefox installer/uninstaller changes. v3.

- Removed passing /S to the installer since it is silent now for installation.
- Now use the new plugin ServicesHelper instead of SimpleSC.
- Review comments implemented.

Comment 223

[Brian R. Bondy [:bbondy]]

Attachment: Patch 11 - New maintenance service installer. v2.

- Now use the new plugin ServicesHelper.
- Made maintenanceservice installer always silent.
- Fixed up uninstaller to be at par with Firefox uninstaller.
- Review comments implemented.

Comment 224

[Brian R. Bondy [:bbondy]]

Attachment: Patch 14 - Build process. v2.

- Review comments implemented
- removed extra folder for maintenanceservice_installer throughout

Comment 225

[Brian R. Bondy [:bbondy]]

Comment on attachment 567667 [details]
Screenshot: New installer step inside Firefox installer

You previously r+'ed this but rs mentioned he wanted you to specifically r+ the text I came up with.  If you have any other suggestions please let me know.

+COMPONENTS_PAGE_TITLE=Set Up Optional Components
+COMPONENTS_PAGE_SUBTITLE=Optional Recommended Components
+OPTIONAL_COMPONENTS_DESC=The Maintenance Service will allow you to update $BrandShortName silently in the background.
+MAINTENANCE_SERVICE_CHECKBOX_DESC=Install &Maintenance Service

Comment 226

[Brian R. Bondy [:bbondy]]

Attachment: Patch 14 - Build process. v3.

Minor change to fix build when only building maintenanceservice_installer target