Last Comment Bug 307181 - (bgupdates) Eliminate wait while restarting Firefox after update (apply update in background)
(bgupdates)
: Eliminate wait while restarting Firefox after update (apply update in backgro...
Status: RESOLVED FIXED
[parity-chrome]
: dev-doc-needed, user-doc-needed
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: Trunk
: x86 Windows XP
: -- enhancement with 17 votes (vote)
: mozilla15
Assigned To: :Ehsan Akhgari (away Aug 1-5)
:
Mentors:
https://wiki.mozilla.org/Background_U...
: 388230 499788 692638 692887 (view as bug list)
Depends on: 696891 760697 771766 481815 692887 704591 749956 753730 756463 757437 757439 757568 757611 757632 757711 757717 757755 757794 757835 757849 757885 757907 757963 757965 757971 757981 758068 758092 758101 758324 758742 758998 759063 759065 759333 759390 759615 759639 760027 760206 760290 760577 760665 760818 762032 764269 764587 764751 765227 765598 766264 766269 767125 767968 773077 774014 774789 775096 780575 789422 789743 789958 797973 823996 CVE-2013-0799 882007 CVE-2013-1707 921148 1098112
Blocks: 489138 757449 685614 701375 757448 758326
  Show dependency treegraph
 
Reported: 2005-09-06 01:33 PDT by Jesse Ruderman
Modified: 2016-06-19 11:22 PDT (History)
81 users (show)
dveditz: sec‑review+
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
WIP 3 (43.00 KB, patch)
2011-10-24 16:58 PDT, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
WIP 4 (52.62 KB, patch)
2011-10-25 17:13 PDT, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
WIP 5 (62.75 KB, patch)
2011-10-27 10:43 PDT, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
WIP 6 (81.73 KB, patch)
2011-10-31 09:05 PDT, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
WIP7 (89.18 KB, patch)
2011-10-31 15:47 PDT, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
WIP 8 (182.65 KB, patch)
2011-11-01 16:04 PDT, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
WIP 9 (197.04 KB, patch)
2011-11-04 12:47 PDT, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
WIP 10 (199.81 KB, patch)
2011-11-07 17:31 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
About dialog UI - "Applying update..." (217.60 KB, image/png)
2011-11-08 08:04 PST, :Ehsan Akhgari (away Aug 1-5)
limi: ui‑review+
Details
About dialog UI - "Restart" (215.83 KB, image/png)
2011-11-08 08:06 PST, :Ehsan Akhgari (away Aug 1-5)
limi: ui‑review+
Details
WIP 11 (218.07 KB, patch)
2011-11-09 13:12 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
WIP 12 (303.89 KB, patch)
2011-11-10 19:46 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
WIP 13 (364.07 KB, patch)
2011-11-11 15:44 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
WIP 14 (367.36 KB, patch)
2011-11-14 16:59 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
WIP 15 (367.39 KB, patch)
2011-11-16 17:15 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Patch (v1) (378.35 KB, patch)
2011-11-21 21:15 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Patch (v2) (580.86 KB, patch)
2011-11-22 15:00 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Patch (v1) (403.46 KB, patch)
2011-11-23 17:41 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Patch (v1.1) (354.29 KB, patch)
2011-12-23 15:18 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Code changes (v1) (120.47 KB, patch)
2012-01-05 15:54 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Test changes (188.79 KB, patch)
2012-01-05 16:08 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Code changes (v2) (152.48 KB, patch)
2012-01-09 12:18 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
New tests (66.70 KB, patch)
2012-01-09 12:19 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Use MOZ_MACBUNDLE_NAME instead of building our own bundle name (2.67 KB, patch)
2012-01-09 14:04 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Part 2. Addressing imelven's review comments (4.25 KB, patch)
2012-01-10 12:34 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Part 3. Address Brian's code review comments (25.33 KB, patch)
2012-01-16 15:36 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Part 4. Address Brian's test review comments (3.48 KB, patch)
2012-01-16 15:43 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Do not use the alternate directory (72.79 KB, patch)
2012-01-18 13:33 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Part 5. Remove the channel change file related code from tests (2.52 KB, patch)
2012-03-06 16:33 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Code changes (v3) (152.21 KB, patch)
2012-03-08 14:53 PST, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Test changes (v3) (283.59 KB, patch)
2012-03-08 14:54 PST, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review+
Details | Diff | Splinter Review
Code changes (v4) (155.07 KB, patch)
2012-03-13 20:18 PDT, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review-
Details | Diff | Splinter Review
Address review comments (34.82 KB, patch)
2012-04-17 14:34 PDT, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review+
Details | Diff | Splinter Review
Prompt when updates have been applied (18.02 KB, patch)
2012-04-17 15:17 PDT, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Preserve the last-update.log file after background updates (22.42 KB, patch)
2012-04-18 12:45 PDT, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review-
Details | Diff | Splinter Review
Prompt when updates have been applied (5.74 KB, patch)
2012-04-20 10:47 PDT, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review+
Details | Diff | Splinter Review
Make the update wizard aware of background updates (7.09 KB, patch)
2012-04-23 14:52 PDT, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review+
Details | Diff | Splinter Review
Append to the update log when performing a replace request (31.40 KB, patch)
2012-04-25 16:27 PDT, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review+
Details | Diff | Splinter Review
Addressed comment 129 and 130 (8.88 KB, patch)
2012-05-01 13:19 PDT, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review+
Details | Diff | Splinter Review
Add an error code for the case when GetInstallationDir fails (3.17 KB, patch)
2012-05-03 11:54 PDT, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review+
Details | Diff | Splinter Review
Add a new test for the applied-service case (2.87 KB, patch)
2012-05-03 12:36 PDT, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review+
Details | Diff | Splinter Review
Fix the version downgrade and product channel check tests on Windows (3.39 KB, patch)
2012-05-07 10:36 PDT, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review+
Details | Diff | Splinter Review
Make sure that the directory passed in argv[2] does not include a trailing backslash (1.35 KB, patch)
2012-05-10 14:21 PDT, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review+
Details | Diff | Splinter Review
Rename the pref (6.88 KB, patch)
2012-05-18 08:55 PDT, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review+
Details | Diff | Splinter Review
Rename the notification (3.02 KB, patch)
2012-05-18 08:59 PDT, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review+
Details | Diff | Splinter Review
Fix the review nits (9.45 KB, patch)
2012-05-18 09:17 PDT, :Ehsan Akhgari (away Aug 1-5)
no flags Details | Diff | Splinter Review
Remove the Mac specific hacks used for getting xpcshell tests to work (5.79 KB, patch)
2012-05-18 09:41 PDT, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review+
Details | Diff | Splinter Review
Fix the review nits (9.21 KB, patch)
2012-05-18 11:45 PDT, :Ehsan Akhgari (away Aug 1-5)
robert.strong.bugs: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2005-09-06 01:33:17 PDT
Currently, I have to wait 10 seconds for Firefox to apply a patch to itself
while it restarts itself.  This seems silly to me.

The old version of Firefox should apply the patch into a temporary directory
while it is running, after it downloads the patch (either as part of the same
process or in a separate process).  That way, when I restart, the only work left
is to move the new files into the correct location, since the work of unpacking
and applying binary diffs will already be done.
Comment 1 Darin Fisher 2005-09-06 14:00:59 PDT
> Currently, I have to wait 10 seconds for Firefox to apply a patch to itself
> while it restarts itself.  This seems silly to me.

I don't understand.  You see a progress meter, right?  If you choose the option
to "restart and install the update now," then it doesn't matter if the
application of the patch happens before or after the restart.  If you choose the
later option, then it suggests that you don't want your disk to thrash until
later ;-)
Comment 2 Jesse Ruderman 2005-09-06 14:26:56 PDT
Firefox shouldn't even give me the choice between "Restart and install update
now" and "Later" until after it has done the work of applying the patch.  I
don't care about disk thrashing, I just want to be able to continue using the
old version of Firefox until the new version is actually ready.
Comment 3 Darin Fisher 2005-09-06 14:56:05 PDT
I don't think this will be changed in the Firefox 1.5 timeframe.
Comment 4 Mike Beltzner [:beltzner, not reading bugmail] 2006-04-19 20:47:54 PDT
This makes sense to me, or alternatively that the patch be applied when Firefox shuts down after downloading an update (bug 334767 will hopefully make the most common use case for automatic updates be that a user's Firefox will be updated silently and without forcing a restart).
Comment 5 Jesse Ruderman 2007-07-16 15:05:06 PDT
*** Bug 388230 has been marked as a duplicate of this bug. ***
Comment 6 Hans 2007-07-16 23:37:23 PDT
This enhancement still considered? 

Can this also include updates of add-ons - if new add-ons are available FF goes and downloads it (if auto updates are on), but in the meantime FF is not loaded - can take some time to get FF up if say my laptop is not connected to the Internet and I have to wait for the updates to timeout.
Preferred behavior should be that FF updates in background and inform me (with icon or such) that updates are ready to install).

It seems the code is already there. When I goto Add-ons and Find Updates it is done separately (threaded) form the main FF window - so doing this should be easy?
Comment 7 AndrewM 2007-11-23 21:09:43 PST
Target Milestone should probably be cleared or set to Future, Firefox 2 beta1 was a long time ago :)
Comment 8 Nickolay_Ponomarev 2009-08-28 08:36:29 PDT
Adjusting summary to make this easier to find.

From the duplicate bug 499788:
-------  Comment #1 From  Mike Beltzner [:beltzner]   2009-06-24 13:33:29 PDT 

This simply must be a dupe.

Of course this is desired, and if it were trivial we'd have done it before.
(Right now this is kind of a basic RFE, as well I'd like to see more of a set
of suggestions around how we'd actually do this.)

The experience should be (I'm really, really sure I've written this before):

 - download comes in the background
 - we apply it
 - if the user hasn't restarted in 24 hours, we show a small indication asking
them to do so

--- Comment #2 From Robert Strong [:rs] (do not email) 2009-06-24 13:37:04 PDT

I suspect it is a dupe but I haven't checked yet.

Also, I've been considering how we could apply the update on exit but if we
start using MSI's the update process will need to change significantly for
Windows so I haven't been spending many cycles on update yet.
Comment 9 Nickolay_Ponomarev 2009-08-28 08:36:48 PDT
*** Bug 499788 has been marked as a duplicate of this bug. ***
Comment 10 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2011-10-18 12:28:57 PDT
sec-review sched for 11:00 PST Oct-21
Comment 11 :Ehsan Akhgari (away Aug 1-5) 2011-10-19 11:07:16 PDT
I'm working on a prototype here: <https://github.com/ehsan/mozilla-central/tree/bgupdate-proto>
Comment 12 :Ehsan Akhgari (away Aug 1-5) 2011-10-24 16:58:14 PDT
Created attachment 569235 [details] [diff] [review]
WIP 3

This is a work in progress patch.  This works fine on OS X at least with my local testing.  It requires way more testing, addressing of the "XXX ehsan" comments inside the patch, a dirent.h implementation on Windows, making sure that the existing unit tests work, and writing new ones.
Comment 13 Mozilla RelEng Bot 2011-10-24 23:40:22 PDT
Try run for 4f34da68d13c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4f34da68d13c
Results (out of 84 total builds):
    success: 61
    warnings: 8
    failure: 15
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-4f34da68d13c
Comment 14 :Ehsan Akhgari (away Aug 1-5) 2011-10-25 17:13:56 PDT
Created attachment 569556 [details] [diff] [review]
WIP 4

This version of the patch fixes a few build problems, makes the unit tests green, and fixes a race condition.
Comment 15 :Ehsan Akhgari (away Aug 1-5) 2011-10-27 10:43:44 PDT
Created attachment 570024 [details] [diff] [review]
WIP 5

This version should build successfully on Windows, and the tests should be green on Linux as well.
Comment 16 Brian R. Bondy [:bbondy] 2011-10-31 09:02:32 PDT
Comment on attachment 570024 [details] [diff] [review]
WIP 5

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

::: toolkit/mozapps/update/nsUpdateService.js
@@ +1256,5 @@
> +      // we really want to start from a clean state.
> +      // XXX ehsan we should make sure that the About dialog UI waits for the
> +      // update to be applied before showing the "Apply update" (or "Restart")
> +      // button.
> +      if (update && update.state == STATE_PENDING) {

Just an FYI there is a new state from my task called STATE_PENDING_NO_SERVICE  (const STATE_PENDING_NO_SVC =  "pending-no-service";).  This state is used when the service failed to apply an update so it wants to do an update the normal way on the next startup.  So when we merge together you'll need to have:
if (update && update.state == STATE_PENDING ||
    update && update.state == STATE_PENDING_NO_SVC) {

::: toolkit/mozapps/update/updater/updater.cpp
@@ +691,5 @@
> +    }
> +  }
> +  return rv;
> +}
> +

This remove recursive remove scares me a bit, we need to ensure that it is for sure the right directory that we are removing and not someone spoofing a bad directory.

@@ +2012,5 @@
> +  if (rv) {
> +    LOG(("Removing tmpDir failed, err: %d\n", rv));
> +#ifdef XP_WIN
> +    if (MoveFileExW(tmpDir, NULL, MOVEFILE_DELAY_UNTIL_REBOOT)) {
> +      LOG(("tmpDir will be removed on OS reboot: " LOG_S "\n", tmpDir));

Maybe needs handling if something happens after this and tmpDir points to a valid dir again, next reboot it'll be removed.

::: toolkit/xre/nsUpdateDriver.cpp
@@ +220,5 @@
> +    char buf[32];
> +    if (GetStatusFileContents(statusFile, buf)) {
> +      const char kPending[] = "pending";
> +      const char kApplied[] = "applied";
> +      if (!strncmp(buf, kPending, sizeof(kPending) - 1)) {

Ditto pending-no-service

@@ +335,5 @@
>  #endif
> +  if (showUI) {
> +    // Only copy updater.ini if we need to display the UI
> +    CopyFileIntoUpdateDir(appDir, kUpdaterINI, updateDir);
> +  }

My code assumes that showUI is false for updates of limited user accounts on Vista and higher (since the update itself happens from the SYSTEM account, session 0 cannot display a UI on vista and above).  Let me know if it will ever be true in that case, if so I'll add code to delete the updater.ini file from the update dir in that case.
Comment 17 :Ehsan Akhgari (away Aug 1-5) 2011-10-31 09:05:47 PDT
Created attachment 570728 [details] [diff] [review]
WIP 6

This version of the patch mostly works fine on Windows.  There are still a number of test issues which I need to sort out...
Comment 18 :Ehsan Akhgari (away Aug 1-5) 2011-10-31 15:47:08 PDT
Created attachment 570871 [details] [diff] [review]
WIP7

This version of the patch is tested on Windows, Linux, and OS X, and should pass all of the existing tests.  The next steps are writing unit tests for the background update, and hooking up the About dialog UI for this.
Comment 19 :Ehsan Akhgari (away Aug 1-5) 2011-11-01 16:04:30 PDT
Created attachment 571172 [details] [diff] [review]
WIP 8

This version of the patch finally adds some automated tests.  All of them should run successfully on all platforms, except for test_0201_app_launch_apply_update.js, which I've only tested on Mac so far.  I will fix that test for Linux and Windows tomorrow, and will then look to see if I should add more tests.  test_0201_app_launch_apply_update.js is extremely valuable, as it tests the main code path that an application would go through when installing an update in the background, and switching to it the next time it starts up.
Comment 20 Brian R. Bondy [:bbondy] 2011-11-04 06:01:23 PDT
*** Bug 692638 has been marked as a duplicate of this bug. ***
Comment 21 Brian R. Bondy [:bbondy] 2011-11-04 06:02:35 PDT
*** Bug 692887 has been marked as a duplicate of this bug. ***
Comment 22 :Ehsan Akhgari (away Aug 1-5) 2011-11-04 12:47:12 PDT
Created attachment 572050 [details] [diff] [review]
WIP 9

New in this version is the About dialog UI, and more fixes on the tests, with yet more fixes to come.
Comment 23 :Ehsan Akhgari (away Aug 1-5) 2011-11-07 17:31:19 PST
Created attachment 572687 [details] [diff] [review]
WIP 10

This version of the patch should pass all tests on all platforms.  More tests to come.
Comment 24 :Ehsan Akhgari (away Aug 1-5) 2011-11-08 08:04:58 PST
Created attachment 572817 [details]
About dialog UI - "Applying update..."

I'm submitting the About dialog UI for ui-review.  Here's what happens in the About dialog.

As the update is being downloaded, we show the downloading status as before.  After the update has been downloaded (when it's being applied in the background), we should this in the About dialog.  When it has finished downloading, we show a Restart button as opposed to the "Apply Updates" button we do today.

I'm submitting that part in the next attachment.

The only part that I'm not sure about is what should happen if the About dialog is opened after an update has been applied in the background.  Should we just show "Restart"?  In fact, we could still show the Apply Updates button like we do today (and keep that string too).  Although that wouldn't be very accurate, technically speaking.
Comment 25 :Ehsan Akhgari (away Aug 1-5) 2011-11-08 08:06:06 PST
Created attachment 572818 [details]
About dialog UI - "Restart"

To test this in action, you can download this build <http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2011/11/2011-11-04-07-58-29-ash/> and open the About dialog.
Comment 26 Steffen Wilberg 2011-11-08 08:28:26 PST
The "Apply Update" button label is correct in any case because even if the updater applied the update in the background, you still need to replace the old app directory by the "updated" app directory. In other words, you're still "applying" the update.

Anyway, it would be confusing if the About dialog is opened after an update has been applied in the background already, and it would just show a "Restart" button. That would lead to the question "why should I restart?"
Comment 27 :Ehsan Akhgari (away Aug 1-5) 2011-11-09 12:43:44 PST
(In reply to Steffen Wilberg from comment #26)
> The "Apply Update" button label is correct in any case because even if the
> updater applied the update in the background, you still need to replace the
> old app directory by the "updated" app directory. In other words, you're
> still "applying" the update.
> 
> Anyway, it would be confusing if the About dialog is opened after an update
> has been applied in the background already, and it would just show a
> "Restart" button. That would lead to the question "why should I restart?"

Yeah, I can sort of see this argument, as I mentioned before.  I'll wait for Alex's feedback before changing the patch though.
Comment 28 :Ehsan Akhgari (away Aug 1-5) 2011-11-09 13:12:53 PST
Created attachment 573297 [details] [diff] [review]
WIP 11

This patch removes the first UAC dialog on Windows.  The way that I do this is that if writing to the installation directory or its parent is not possible, I will attempt to write the updated application to the local data directory (if it's on the same volume as the installation directory).  If it's not, then I will give up and fallback to non-background updates.
Comment 29 alanjstr 2011-11-09 13:16:14 PST
It sounds like there is some overlap between this and Bug 481815.
Comment 30 Alex Limi (:limi) — Firefox UX Team 2011-11-09 13:35:15 PST
Comment on attachment 572818 [details]
About dialog UI - "Restart"

I'd change the button to "Update & Restart" if we are able to have new strings.
Comment 31 Alex Limi (:limi) — Firefox UX Team 2011-11-09 13:37:31 PST
Comment on attachment 572817 [details]
About dialog UI - "Applying update..."

Showing "Applying update" is fine here. Please remove the italicized text though, it looks bad (and isn't really used in the OS).
Comment 32 Robert Strong [:rstrong] (use needinfo to contact me) 2011-11-09 13:40:02 PST
(In reply to Alex Limi (:limi) — Firefox UX Team from comment #31)
> Comment on attachment 572817 [details]
> About dialog UI - "Applying update..."
> 
> Showing "Applying update" is fine here. Please remove the italicized text
> though, it looks bad (and isn't really used in the OS).
Limi, italics is used consistently for that text so do you want it removed for all of the other cases as well?
Comment 33 Alex Limi (:limi) — Firefox UX Team 2011-11-09 14:24:34 PST
(In reply to Robert Strong [:rstrong] (do not email) from comment #32)
> Limi, italics is used consistently for that text so do you want it removed
> for all of the other cases as well?

Yes. It's more common on Windows (although not very much), and is never used on OS X, so I'd say get rid of it while you're in there changing it anyway.

Thanks!
Comment 34 :Ehsan Akhgari (away Aug 1-5) 2011-11-09 14:55:11 PST
(In reply to Alex Limi (:limi) — Firefox UX Team from comment #33)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #32)
> > Limi, italics is used consistently for that text so do you want it removed
> > for all of the other cases as well?
> 
> Yes. It's more common on Windows (although not very much), and is never used
> on OS X, so I'd say get rid of it while you're in there changing it anyway.
> 
> Thanks!

Filed bug 701205 for this.
Comment 35 :Ehsan Akhgari (away Aug 1-5) 2011-11-10 19:46:21 PST
Created attachment 573726 [details] [diff] [review]
WIP 12

With yet more tests.  also addressed the ui-r comments.  I still have a couple of more ideas for some new tests, but this is nearly ready for review.
Comment 36 :Ehsan Akhgari (away Aug 1-5) 2011-11-11 15:44:28 PST
Created attachment 573943 [details] [diff] [review]
WIP 13

This patch adds tests for the feature I implemented in comment 28.

This work is mostly done as far as I can tell.  I will look at applying my work on top of Brian's next.
Comment 37 Brian R. Bondy [:bbondy] 2011-11-11 17:00:11 PST
I suggest to pop your patch from you patch queue, back it up, and then qimport my consolidated patch (bug 481815) before it.  

T hen rebase your patch after it. 
That way you can easily later replace the first patch in your patch queue with the updated post-review comment one.
Comment 38 :Ehsan Akhgari (away Aug 1-5) 2011-11-14 16:11:54 PST
(In reply to Brian R. Bondy [:bbondy] from comment #37)
> I suggest to pop your patch from you patch queue, back it up, and then
> qimport my consolidated patch (bug 481815) before it.  
> 
> T hen rebase your patch after it. 
> That way you can easily later replace the first patch in your patch queue
> with the updated post-review comment one.

(Un)fortunately, I'm not using patch queues for my work here at all.  I'll throw this at git (which should be better at merging this kind of stuff) and will see what happens.  Will attach a patch for my work on top of yours when I've done that.
Comment 39 :Ehsan Akhgari (away Aug 1-5) 2011-11-14 16:29:14 PST
This is an attempt to document the parameters that the updater application accepts with my changes:

usage:
updater update-root-dir apply-to-dir pid_string [cb-working-dir cb [cb-args...]]

* `updater` is the path name to the updater application.  This is almost always outside of the installation directory.  updater looks inside this directory in order to load updater.ini.
* `update-root-dir` is the path name to the update root directory, containing the mar file, and various update related information files.
* `apply-to-dir` is the path name pointing to where the updated application needs to go.  For foreground updates, its value is the installation directory.  For background updates, its value is dependent on the platform:
** On Windows, it's one of these two values:
*** $INSTALLDIR\updated if the process has write access to $INSTALLDIR and its parent.  updater finds the installation directory location by looking at the parent of `apply-to-dir`.
*** $LocalAppData\Mozilla\$MOZ_APP_NAME\updates\0\updated;$INSTALLDIR otherwise.  updater uses the part before ";" to find the destination directory, and the part after it to find the installation directory.
** On Linux, it's $INSTALLDIR/updated.  updater finds the installation directory location by looking at the parent of `apply-to-dir`.
** On Mac, it's $APP_BUNDLE_DIR/Updater.app.  updater finds the installation directory location by looking at the parent of `apply-to-dir`.
* `pid_string` is one of the following values:
** an integer >= 0.  In this case, the updater attempts to perform a foreground update, and waits for the pid if it's a positive number before starting the update process.  The value 0 is used on Linux where we don't need to wait for another process.
** "-1".  In this case, the updater attempts to perform a background update, and doesn't wait for any process before starting the update process.
** $PID/replace where $PID is an integer >= 0.  This means that the update has already been applied, and the updater is only expected to replace the existing installation with the updated one.  The value 0 is used on Linux where we don't need to wait for another process.

(The meaning of the rest of the arguments does not change with my work.)

* `cb-working-dir` is the working directory of the callback application if one needs to be launched when the updater is finished doing its job.
* `cb` is the path name of the callback application if one needs to be launched when the updater is finished doing its job.
* Any following arguments are passed on to the callback application.
Comment 40 :Ehsan Akhgari (away Aug 1-5) 2011-11-14 16:59:46 PST
Created attachment 574479 [details] [diff] [review]
WIP 14

This patch moves the updater process initialization to a background thread, in preparation of my work being rebased on top of Brian's.
Comment 41 :Ehsan Akhgari (away Aug 1-5) 2011-11-16 17:15:27 PST
Created attachment 575045 [details] [diff] [review]
WIP 15

This version should address a test issue on XP.  Also, it keeps copying the updater.ini file to the update root directory, but prevents the UI from showing up for background updates.
Comment 42 :Ehsan Akhgari (away Aug 1-5) 2011-11-21 21:15:59 PST
Created attachment 576084 [details] [diff] [review]
Patch (v1)

This is my patch on top of Brian's patch to eliminate the UAC dialogs.  I think this is ready for review now.  Requesting review from Brian on the maintenanceservice parts (and any other part of the patch that he wants to look at).

My commit tree on top of Brian's work is located here: <https://github.com/ehsan/mozilla-central/tree/bgupdate>.

The current tests should cover everything about the background updates, but none of them involve testing with the service turned on.  I will work with Brian to see what we can do for automated testing of the update service with the maintenance service involved.

I will continue to work on this patch and improve it.  The only big piece that I'm going to look at tomorrow is the handling of the installation directories that we don't have write access to which I added in comment 28.  Other than that part, I don't expect things to change much.

I will also push this patch to the oak twig.
Comment 43 Brian R. Bondy [:bbondy] 2011-11-22 08:19:02 PST
Requestd on IRC that the patch be split up into 2, one for the maintenance service and related xre code.  And the other for the rest of the task's code.
Comment 44 :Ehsan Akhgari (away Aug 1-5) 2011-11-22 15:00:32 PST
Created attachment 576315 [details] [diff] [review]
Patch (v2)

Fixed some test problems.  Also, changed the logic of doing only one UAC prompt to the case where updating through the service fails or is skipped.
Comment 45 rohnski 2011-11-23 16:07:20 PST
Please make silent updates a configurable option. 

Allowing big brother to change my software will end my association with FF/mozilla.  The rapid release cycle is bad enough at breaking addons, I don't need to have unexplained failures due to silent updates.  I don't need to be wasting time troubleshooting when I'm missing a key piece of information, that the software has been updated silently!

Make it default fine, but let me turn it off in about:config. It is my machine, I need to control when updates are applied.
Comment 46 Robert Strong [:rstrong] (use needinfo to contact me) 2011-11-23 16:10:18 PST
(In reply to rohnski from comment #45)
> Please make silent updates a configurable option. 
> <snip>
The initial specification included the ability to disable it via the preferences ui as well as administratively so yes, whether to use the service for silent updates is configurable.

btw: you want bug 481815... this bug is about "Eliminate wait while restarting Firefox after update"
Comment 47 :Ehsan Akhgari (away Aug 1-5) 2011-11-23 17:41:21 PST
Created attachment 576665 [details] [diff] [review]
Patch (v1)

This patch refactors the tests.  This refactoring is needed for creating tests which use the maintenance service.
Comment 48 Jesse Ruderman 2011-11-23 17:44:20 PST
rohnski, try checking Options > Advanced > Update > "Check for updates, but let me choose whether to install them".  No need to muck around in about:config :)

This bug removes a post-install notification, which isn't quite what you want anyway (and one that only appears on slow computers).
Comment 49 rohnski 2011-11-23 23:13:13 PST
Thanks Jesse & Robert for that re-assuring info.  Sorry for wrong location. (newbie, <sigh> )
Comment 50 :Ehsan Akhgari (away Aug 1-5) 2011-12-23 15:18:49 PST
Created attachment 584133 [details] [diff] [review]
Patch (v1.1)

I rebased the patch based on Brian's latest work, and made sure that it works as expected and passes all of the tests.  I need to write some more tests for background updates using the Windows service.
Comment 51 :Ehsan Akhgari (away Aug 1-5) 2012-01-05 15:54:50 PST
Created attachment 586261 [details] [diff] [review]
Code changes (v1)

I've split the tests into a separate patch.
Comment 52 :Ehsan Akhgari (away Aug 1-5) 2012-01-05 16:08:11 PST
Created attachment 586263 [details] [diff] [review]
Test changes
Comment 53 :Ehsan Akhgari (away Aug 1-5) 2012-01-09 12:18:01 PST
Created attachment 587073 [details] [diff] [review]
Code changes (v2)

This is a new version of the code changes with a few fixes to a few bugs I encountered working on some new tests.
Comment 54 :Ehsan Akhgari (away Aug 1-5) 2012-01-09 12:19:52 PST
Created attachment 587076 [details] [diff] [review]
New tests

This patch adds a bunch of new tests to test background updates using the maintenance service.  The tests in the test_svc/unit directory are based on their corresponding tests in the test/unit directory, with necessary adjustments to make them use the maintenance service.
Comment 55 :Ehsan Akhgari (away Aug 1-5) 2012-01-09 14:04:55 PST
Created attachment 587125 [details] [diff] [review]
Use MOZ_MACBUNDLE_NAME instead of building our own bundle name

This should fix one Mac test issue which happens on our Tinderboxes, which use a different mac bundle prefix.
Comment 56 Ian Melven :imelven 2012-01-09 18:17:01 PST
Comment on attachment 587073 [details] [diff] [review]
Code changes (v2)

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

Overall looks good - a couple of comments/questions.

What platforms is this going to land for initially ? All of Mac/Windows/Linux ? 

also can you explain what the precomplete file on OSX is used for ? 

I believe you said in the review earlier today that the service is only used to do the actual replace/move if needed (which it usually will
be since most installs on Windows will be in Program Files and will require elevation) - did I understand that correctly ? (the code
definitely looks that way).

::: toolkit/components/maintenanceservice/workmonitor.cpp
@@ +113,5 @@
>  }
>  
>  
>  /**
> + * Gets the installation directory from the arguments passed to updater.exe.

is this really the arguments passed to the service when starting it 
by updater.exe ? (which were originally passed to updater.exe by firefox.exe)

@@ +127,5 @@
> +  if (argcTmp < 2) {
> +    return false;
> +  }
> +  wcscpy(installDir, argvTmp[2]);
> +  bool backgroundUpdate = (argcTmp == 4 && !wcscmp(argvTmp[3], L"-1"));

maybe add a comment saying the -1 is passed as the PID when a background update
should be done ?

@@ +141,5 @@
> +      *backSlash = L'\0';
> +      backSlash = wcsrchr(installDir, L'\\');
> +    }
> +    if (!pathSeparator) {
> +      *backSlash = L'\0';

backSlash can be NULL here if there were no backslashes in installDir ?

@@ +332,5 @@
>    // Verify that the updater.exe that we are executing is the same
>    // as the one in the installation directory which we are updating.
>    // The installation dir that we are installing to is argv[2].
>    WCHAR installDirUpdater[MAX_PATH + 1];
> +  wcsncpy(installDirUpdater, installDir, MAX_PATH);

wcsncpy won't NULL terminate this if there's MAX_PATH or greater chars
in installDir - explicitly NULL terminating it is the safest bet

::: toolkit/mozapps/update/updater/updater.cpp
@@ +1809,5 @@
> +
> +#ifdef XP_MACOSX
> +  NS_tchar sourceDir[MAXPATHLEN];
> +  NS_tsnprintf(sourceDir, sizeof(sourceDir)/sizeof(sourceDir[0]),
> +               NS_T("%s/Contents"), installDir);

possible overflow if someone passes a huge install dir as an arg to the service
which may then run this elevated ? (leading to local elevation of privilege).
there are other instances of %S below, please make sure that this data can't be controlled
by a local user since AIUI this code path can happen when elevated to SYSTEM.

also some NULL termination possible issues with NS_tsnprintf in this function etc if we're not using our own version which always explicitly NULL terminates..

@@ +2042,5 @@
>    gSourcePath = argv[1];
> +  // The directory we're going to update to.
> +  // We copy this string because we need to remove trailing slashes.  The C++
> +  // standard says that it's always safe to write to strings pointed to by argv
> +  // elements, but I don't necessarily believe it.

i respect and admire your skepticism and caution :D

@@ +2126,5 @@
> +    if (pid == -1) {
> +      // This is a signal from the parent process that the updater should work
> +      // in the background.
> +      // For now, we just print some debugging information to see if this works
> +      // at all.

is this comment still valid ?

@@ +2222,5 @@
>      // case it exists attempt to remove it and exit if that fails to prevent
>      // simultaneous updates occurring.
>      if (!_waccess(updateLockFilePath, F_OK) &&
>          NS_tremove(updateLockFilePath) != 0) {
> +      LOG(("Update already in progress! Exiting\n"));

so if i made a lock file that could for example only be deleted by SYSTEM,
this would permanently block updates ? (of course if i had SYSTEM i could do much much worse).
what about if it's only removable by Admins ? is that handled by running the elevated updater ?

@@ +2319,5 @@
>  
> +      // If we could not use the service in the background update case,
> +      // we need to make sure that we will never show a UAC prompt!
> +      // In this case, we would just set the status to pending and will
> +      // apply the update at the next startup.

which would then show a UAC prompt right ? (assuming the service still couldn't be used)

::: toolkit/mozapps/update/updater/win_dirent.cpp
@@ +48,5 @@
> +DIR::DIR(const WCHAR* path)
> +  : findHandle(NULL)
> +{
> +  wcsncpy(name, path, sizeof(name)/sizeof(name[0]));
> +  wcsncat(name, L"\\*", sizeof(name)/sizeof(name[0]) - wcslen(name) - 1);

should probably explicitly null terminate here too

@@ +101,5 @@
> +    }
> +    dir->findHandle = find;
> +  }
> +  wcsncpy(gDirEnt.d_name, data.cFileName,
> +           sizeof(gDirEnt.d_name)/sizeof(gDirEnt.d_name[0]));

explicitly null terminate please
Comment 57 :Ehsan Akhgari (away Aug 1-5) 2012-01-10 12:34:02 PST
Created attachment 587427 [details] [diff] [review]
Part 2. Addressing imelven's review comments

(In reply to Ian Melven :imelven from comment #56)
> Comment on attachment 587073 [details] [diff] [review]
> Code changes (v2)
> 
> Review of attachment 587073 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good - a couple of comments/questions.
> 
> What platforms is this going to land for initially ? All of
> Mac/Windows/Linux ? 

Correct.

> also can you explain what the precomplete file on OSX is used for ? 

It's a script run before complete updates I believe (to remove the existing dirs etc if needed.)  Robert, please correct me if I'm wrong here.

> I believe you said in the review earlier today that the service is only used
> to do the actual replace/move if needed (which it usually will
> be since most installs on Windows will be in Program Files and will require
> elevation) - did I understand that correctly ? (the code
> definitely looks that way).

That is the case, yes.  If the Program Files directory is on a different volume than the user's local appdata directory though we'll use the service for the first step of the background update as well, but that should be very rare.

> ::: toolkit/components/maintenanceservice/workmonitor.cpp
> @@ +113,5 @@
> >  }
> >  
> >  
> >  /**
> > + * Gets the installation directory from the arguments passed to updater.exe.
> 
> is this really the arguments passed to the service when starting it 
> by updater.exe ? (which were originally passed to updater.exe by firefox.exe)

I'm not sure what you mean here...

> @@ +127,5 @@
> > +  if (argcTmp < 2) {
> > +    return false;
> > +  }
> > +  wcscpy(installDir, argvTmp[2]);
> > +  bool backgroundUpdate = (argcTmp == 4 && !wcscmp(argvTmp[3], L"-1"));
> 
> maybe add a comment saying the -1 is passed as the PID when a background
> update
> should be done ?

Sure, done.

> @@ +141,5 @@
> > +      *backSlash = L'\0';
> > +      backSlash = wcsrchr(installDir, L'\\');
> > +    }
> > +    if (!pathSeparator) {
> > +      *backSlash = L'\0';
> 
> backSlash can be NULL here if there were no backslashes in installDir ?

Right, added a null check.  Note that if there is no backslashes in the installDir, then something is very wrong.  :-)

> @@ +332,5 @@
> >    // Verify that the updater.exe that we are executing is the same
> >    // as the one in the installation directory which we are updating.
> >    // The installation dir that we are installing to is argv[2].
> >    WCHAR installDirUpdater[MAX_PATH + 1];
> > +  wcsncpy(installDirUpdater, installDir, MAX_PATH);
> 
> wcsncpy won't NULL terminate this if there's MAX_PATH or greater chars
> in installDir - explicitly NULL terminating it is the safest bet

Done.

> ::: toolkit/mozapps/update/updater/updater.cpp
> @@ +1809,5 @@
> > +
> > +#ifdef XP_MACOSX
> > +  NS_tchar sourceDir[MAXPATHLEN];
> > +  NS_tsnprintf(sourceDir, sizeof(sourceDir)/sizeof(sourceDir[0]),
> > +               NS_T("%s/Contents"), installDir);
> 
> possible overflow if someone passes a huge install dir as an arg to the
> service
> which may then run this elevated ? (leading to local elevation of privilege).
> there are other instances of %S below, please make sure that this data can't
> be controlled
> by a local user since AIUI this code path can happen when elevated to SYSTEM.
> 
> also some NULL termination possible issues with NS_tsnprintf in this
> function etc if we're not using our own version which always explicitly NULL
> terminates..

On Windows, we use our own implementation of snprintf which null-terminates.  On Linux and Mac, the libc version of this function does that.

> @@ +2042,5 @@
> >    gSourcePath = argv[1];
> > +  // The directory we're going to update to.
> > +  // We copy this string because we need to remove trailing slashes.  The C++
> > +  // standard says that it's always safe to write to strings pointed to by argv
> > +  // elements, but I don't necessarily believe it.
> 
> i respect and admire your skepticism and caution :D

lol :)

> @@ +2126,5 @@
> > +    if (pid == -1) {
> > +      // This is a signal from the parent process that the updater should work
> > +      // in the background.
> > +      // For now, we just print some debugging information to see if this works
> > +      // at all.
> 
> is this comment still valid ?

No!

> @@ +2222,5 @@
> >      // case it exists attempt to remove it and exit if that fails to prevent
> >      // simultaneous updates occurring.
> >      if (!_waccess(updateLockFilePath, F_OK) &&
> >          NS_tremove(updateLockFilePath) != 0) {
> > +      LOG(("Update already in progress! Exiting\n"));
> 
> so if i made a lock file that could for example only be deleted by SYSTEM,
> this would permanently block updates ? (of course if i had SYSTEM i could do
> much much worse).

I think so, yes.  Note that this is already the case.

> what about if it's only removable by Admins ? is that handled by running the
> elevated updater ?

I don't know if this is something that we should worry about.  If you are able to write something to program files, you can just disable the updates by changing prefs.js, replace firefox.exe, etc.

> @@ +2319,5 @@
> >  
> > +      // If we could not use the service in the background update case,
> > +      // we need to make sure that we will never show a UAC prompt!
> > +      // In this case, we would just set the status to pending and will
> > +      // apply the update at the next startup.
> 
> which would then show a UAC prompt right ? (assuming the service still
> couldn't be used)

Yes.  The point is that we don't want to show the UAC prompt as the user is using the browser.  So we set the status to pending, so that the next time that Firefox starts up, the user gets a UAC prompt (or not if the service starts to work again for some reason!).  That is, if we can't launch a background update, we just want to fall back to a normal update as we have today.

> ::: toolkit/mozapps/update/updater/win_dirent.cpp
> @@ +48,5 @@
> > +DIR::DIR(const WCHAR* path)
> > +  : findHandle(NULL)
> > +{
> > +  wcsncpy(name, path, sizeof(name)/sizeof(name[0]));
> > +  wcsncat(name, L"\\*", sizeof(name)/sizeof(name[0]) - wcslen(name) - 1);
> 
> should probably explicitly null terminate here too

Done.

> @@ +101,5 @@
> > +    }
> > +    dir->findHandle = find;
> > +  }
> > +  wcsncpy(gDirEnt.d_name, data.cFileName,
> > +           sizeof(gDirEnt.d_name)/sizeof(gDirEnt.d_name[0]));
> 
> explicitly null terminate please

Done.
Comment 58 Robert Strong [:rstrong] (use needinfo to contact me) 2012-01-10 12:40:02 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #57)
> Created attachment 587427 [details] [diff] [review]
> Part 2. Addressing imelven's review comments
> 
> (In reply to Ian Melven :imelven from comment #56)
>...
> > also can you explain what the precomplete file on OSX is used for ? 
> 
> It's a script run before complete updates I believe (to remove the existing
> dirs etc if needed.)  Robert, please correct me if I'm wrong here.
The precomplete file exists for all platforms that use the updater. It contains the relative paths from the install dir to the files for the installed build. These files are staged for removal when updating to a complete update so we don't have to manually track and maintain files that are removed when updating. When updating with a partial update we have the previous build and the new build so we are able to add remove instructions for the files and directories that no longer exist in the newer build that is being updated to.
Comment 59 Ian Melven :imelven 2012-01-10 12:58:56 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #57)
> Created attachment 587427 [details] [diff] [review]
> Part 2. Addressing imelven's review comments
> 
> > ::: toolkit/components/maintenanceservice/workmonitor.cpp
> > @@ +113,5 @@
> > >  }
> > >  
> > >  
> > >  /**
> > > + * Gets the installation directory from the arguments passed to updater.exe.
> > 
> > is this really the arguments passed to the service when starting it 
> > by updater.exe ? (which were originally passed to updater.exe by firefox.exe)
> 
> I'm not sure what you mean here...

this is service code i think - so the args have been passed to the service
when started by updater.exe which got its args from being started by firefox.exe right ?

> On Windows, we use our own implementation of snprintf which null-terminates.
> On Linux and Mac, the libc version of this function does that.

thanks for confirming we aren't using Windows' troublesome snprintf.

> Yes.  The point is that we don't want to show the UAC prompt as the user is
> using the browser.  So we set the status to pending, so that the next time
> that Firefox starts up, the user gets a UAC prompt (or not if the service
> starts to work again for some reason!).  That is, if we can't launch a
> background update, we just want to fall back to a normal update as we have
> today.

thanks for clarifying !
Comment 60 :Ehsan Akhgari (away Aug 1-5) 2012-01-10 14:43:17 PST
(In reply to Ian Melven :imelven from comment #59)
> (In reply to Ehsan Akhgari [:ehsan] from comment #57)
> > Created attachment 587427 [details] [diff] [review]
> > Part 2. Addressing imelven's review comments
> > 
> > > ::: toolkit/components/maintenanceservice/workmonitor.cpp
> > > @@ +113,5 @@
> > > >  }
> > > >  
> > > >  
> > > >  /**
> > > > + * Gets the installation directory from the arguments passed to updater.exe.
> > > 
> > > is this really the arguments passed to the service when starting it 
> > > by updater.exe ? (which were originally passed to updater.exe by firefox.exe)
> > 
> > I'm not sure what you mean here...
> 
> this is service code i think - so the args have been passed to the service
> when started by updater.exe which got its args from being started by
> firefox.exe right ?

Correct.

> > On Windows, we use our own implementation of snprintf which null-terminates.
> > On Linux and Mac, the libc version of this function does that.
> 
> thanks for confirming we aren't using Windows' troublesome snprintf.

Sure thing.  :-)  The code is in toolkit/mozapps/update/common/updatedefines.h if you're curious.
Comment 61 Brian R. Bondy [:bbondy] 2012-01-16 09:53:39 PST
I don't know if this is a problem or not but just wanted to bring it up so security could comment.

After an update is applied in the background, but before it is replaced into the program files directory it sits in a directory like:
C:\Users\USERNAmE\AppData\Local\Mozilla\Firefox\Nightly\updates\0\updated

On the next startup, the service is used to move that folder into Program Files (a protected directory).

I'm not sure if it's a problem or not that a file could be moved into that unprivileged location, and the service will then move it to a privileged location at some later time.  For example maybe it is a problem that a dll could be replace with some other dll with the same exports but with different code that would have access to the process' memory.

It's my understanding that the updated directory can sit there for a long time while the user is browsing until the next browser restart.

If it's a problem then it could be easily solved (when the service is installed at least) by having the update directory inside Program Files perhaps as a subdirectory of the directory where the update is being applied to eventually.

I'm not sure if this is something we need to protect against or not but just wanted to bring it up as a topic for discussion.
Comment 62 :Ehsan Akhgari (away Aug 1-5) 2012-01-16 11:36:04 PST
I originally implemented this as a way to make sure that background updates can be used without the maintenance service being available.  The logic is that on Windows, if we can't write to the installation dir and it parent, and if the appdata dir and the installation dir are on the same volume, we apply the service into an updated directory under appdata.  This is done in order to make sure that applying the update in the background will never trigger a UAC dialog.

I can easily remove this check and always attempt to put the updated directory under the installation directory, and fall back to the non-background update code path if the service is not available.
Comment 63 Brian R. Bondy [:bbondy] 2012-01-16 11:38:50 PST
I don't think this is a huge deal by the way since we allow users to install into the application data directory, but it does in essence turn the program files directory into a non protected directory for those users who did install it into the protected program files directory.
Comment 64 :Ehsan Akhgari (away Aug 1-5) 2012-01-16 11:46:35 PST
It would be interesting to know what Robert thinks about this.
Comment 65 Brian R. Bondy [:bbondy] 2012-01-16 13:06:29 PST
Comment on attachment 587073 [details] [diff] [review]
Code changes (v2)

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

::: browser/app/profile/firefox.js
@@ +173,5 @@
>  pref("app.update.silent", false);
>  
> +// If set to true, the Update Service will apply updates in the background
> +// when it finishes downloading them.
> +pref("app.update.background", true);

app.update.background.enabled to be consistent.

::: browser/base/content/aboutDialog.js
@@ +245,5 @@
>  
> +  // true when updating in background is enabled.
> +  get backgroundUpdateEnabled() {
> +    return this.updateEnabled &&
> +           Services.prefs.getBoolPref("app.update.background");

app.update.background.enabled

@@ +587,5 @@
> +            self.setupUpdateButton("update.restart." +
> +                                   (self.isMajor ? "upgradeButton" : "restartButton"));
> +            timer.cancel();
> +            timer = null;
> +          } else if (status == "failed") {

Do we need to status.split(":")?

::: build/automation.py.in
@@ +363,5 @@
>  user_pref("network.http.prompt-temp-redirect", false);
>  user_pref("media.cache_size", 100);
>  user_pref("security.warn_viewing_mixed", false);
>  user_pref("app.update.enabled", false);
> +user_pref("app.update.background", false);

app.update.background.enabled to be consistent.

::: toolkit/components/maintenanceservice/workmonitor.cpp
@@ +118,5 @@
> + *
> + * @param argcTmp    The argc value normally sent to updater.exe
> + * @param argvTmp    The argv value normally sent to updater.exe
> + * @param installDir Buffer to hold the installation directory.
> + *                   The size of the buffer should be 2 * MAX_PATH.

Instead of putting a comment for the size of the buffer should be this size, why not enforce that you pass a pointer to a buffer of that size exactly?  For example char (*installationDir)[2 * MAX_PATH].  Also calling the parameter installDir is a bit confusing since the function is called GetInstallationDir.

@@ +120,5 @@
> + * @param argvTmp    The argv value normally sent to updater.exe
> + * @param installDir Buffer to hold the installation directory.
> + *                   The size of the buffer should be 2 * MAX_PATH.
> + */
> +bool

BOOL for now to be consistent, we'll replace all BOOL with bool in this module in the future.

@@ +134,5 @@
> +    LPWSTR pathSeparator = wcschr(installDir, L';');
> +    if (pathSeparator) {
> +      wcscpy(installDir, pathSeparator + 1);
> +    }
> +    LPWSTR backSlash = wcsrchr(installDir, L'\\');

Are you sure the path will never have a frontslash instead?  I prefer using functions like PathRemoveFileSpec which lets the Win32 API do the work in a safe way.

@@ +323,5 @@
>      return FALSE;
>    }
>  
> +  WCHAR installDir[2 * MAX_PATH];
> +  ZeroMemory(installDir, 2 * MAX_PATH * sizeof(WCHAR));

I prefer 2 * MAX_PATH + 1, but as you wish. You could also do = { 0 } instead of ZeroMemory.  An empty initializer also works but I think not with the C spec so I like to always put a 0.

@@ +325,5 @@
>  
> +  WCHAR installDir[2 * MAX_PATH];
> +  ZeroMemory(installDir, 2 * MAX_PATH * sizeof(WCHAR));
> +  if (!GetInstallationDir(argc, argv, installDir)) {
> +    return false;

FALSE for now to match BOOL return type, we'll change it all at once :)
Also there should be a WriteStatusFailure in this case like the other failure return types.  A new error code should be added to the readstrings error.h

::: toolkit/mozapps/update/nsUpdateService.js
@@ +78,5 @@
>  const PREF_APP_UPDATE_POSTUPDATE          = "app.update.postupdate";
>  const PREF_APP_UPDATE_PROMPTWAITTIME      = "app.update.promptWaitTime";
>  const PREF_APP_UPDATE_SHOW_INSTALLED_UI   = "app.update.showInstalledUI";
>  const PREF_APP_UPDATE_SILENT              = "app.update.silent";
> +const PREF_APP_UPDATE_BACKGROUND          = "app.update.background";

Call this app.update.background.enabled to be consistent with app.update.enabled and app.update.service.enabled.

@@ +389,5 @@
>        }
>      }
>  
>      /**
> +#      On Windows, we no longer store the update under the app dir.

in which cases?

::: toolkit/mozapps/update/updater/updater.cpp
@@ +674,5 @@
> +  int rv = NS_tstat(path, &sInfo);
> +  if (rv) {
> +    LOG(("ensure_copy_recursive: path doesn't exist: " LOG_S ", rv: %d, err: %d\n",
> +          path, rv, errno));
> +    return rv;

We will return rv directly from NS_tstat, but we want to return a specific updater error which can live in the update.status file.

@@ +694,5 @@
> +  dir = NS_topendir(path);
> +  if (!dir) {
> +    LOG(("ensure_copy_recursive: path is not a directory: " LOG_S ", rv: %d, err: %d\n",
> +          path, rv, errno));
> +    return rv;

We will return rv directly from NS_topendir, but we want to return a specific updater error which can live in the update.status file.

@@ +1239,5 @@
>    size_t r = header.slen;
>    unsigned char *rb = buf;
>    while (r) {
>      size_t c = fread(rb, 1, r, ofile);
> +    if (ferror(ofile)) {

You need to rebase, this error handling was already fixed in m-c via checking != r.

@@ +1636,5 @@
>    char buf[32];
>    if (status == OK) {
> +    if (sBackgroundUpdate) {
> +#if defined(XP_WIN)
> +      text = "applied-service\n";

Maybe additionally only set applied-service if MOZ_MAINTENANCE_SERVICE is defined, not too important though.  More of a protection against future code.

@@ +1766,5 @@
> +  // First extract the installation directory from gSourcePath by going two
> +  // levels above it.  This is effectively skipping over "updates/0".
> +  NS_tchar installDir[MAXPATHLEN];
> +  if (!GetInstallationDir(installDir)) {
> +    return 1;

Is this the appropriate error code? I think this is a mem error which is not right.

@@ +1803,5 @@
> +  // 3. Delete tmpDir (or defer it to the next reboot).
> +
> +  NS_tchar installDir[MAXPATHLEN];
> +  if (!GetInstallationDir(installDir)) {
> +    return 1;

Ditto wrong error code.  Should be defined and added to telemetry data, or use an existing appropriate one.

@@ +1983,5 @@
> +    }
> +    if (rv == OK) {
> +      rv = DoUpdate();
> +      gArchiveReader.Close();
> +    }

Should we fall back if the background update's CopyInstallDirToDestDir fails to not use a background update?

@@ +2074,5 @@
>  
>    // We never want the service to be used unless we build with
>    // the maintenance service.
>  #ifdef MOZ_MAINTENANCE_SERVICE
> +  useService = IsUpdateStatusPendingService();

Use your enum function instead after you move it to update common.

@@ +2552,5 @@
> +
> +      // Since the process may be signaled as exited by WaitForSingleObject before
> +      // the release of the executable image try to lock the main executable file
> +      // multiple times before giving up.
> +      int retries = 5;

This code changed on m-c slightly

@@ +2996,5 @@
>    size_t r = ms.st_size;
>    char *rb = mbuf;
>    while (r) {
>      size_t c = fread(rb, 1, mmin(SSIZE_MAX, r), mfile);
> +    if (ferror(mfile)) {

This code changed on m-c recently.

::: toolkit/xre/nsUpdateDriver.cpp
@@ +203,1 @@
>  {

Maybe add something like: 
PR_STATIC_ASSERT(Size >= 16);
Or whatever the current max size is of a status.

@@ +218,5 @@
> +  ePendingService,
> +  eAppliedUpdate,
> +  eAppliedService
> +} UpdateStatus;
> +

Move this and the associated function into update common code, seems like we can use it everywhere and expand on it to reduce some other functions in the future.

@@ +220,5 @@
> +  eAppliedService
> +} UpdateStatus;
> +
> +static UpdateStatus
> +GetUpdateStatus(nsIFile* dir, nsCOMPtr<nsILocalFile> &statusFile)

even know it's a static function, javadoc would make it easier to understand what the function is for and what the parameter is for.  Also I won't repeat the same comment but the same applies for all new functions everywhere.

@@ +357,5 @@
>  }
>  
> +#if defined(XP_WIN)
> +static bool
> +CanWriteToOneDirectory(nsIFile* aDir)

CanWriteToDirectory sounds less confusing than CanWriteToOneDirectory.

@@ +368,5 @@
> +                            GENERIC_READ | GENERIC_WRITE,
> +                            0,
> +                            NULL,
> +                            OPEN_ALWAYS,
> +                            FILE_FLAG_DELETE_ON_CLOSE,

Why delete on close?

@@ +371,5 @@
> +                            OPEN_ALWAYS,
> +                            FILE_FLAG_DELETE_ON_CLOSE,
> +                            NULL);
> +  bool success = (file != INVALID_HANDLE_VALUE);
> +  CloseHandle(file);

Use nsAutoHandle or else only CloseHandle when file != INVALID_HANDLE_VALUE

@@ +405,5 @@
> +
> +  volume1Len = wcslen(volume1);
> +  volume2Len = wcslen(volume2);
> +  return volume1Len == volume2Len &&
> +         volume1Len == PathCommonPrefixW(volume1, volume2, NULL);

This is fine but why not use wcsicmp instead of PathCommonPrefixW?

@@ +417,5 @@
> +  nsAutoString applyToDirW;
> +  nsresult rv = aUpdatedDir->GetPath(applyToDirW);
> +  if (NS_FAILED(rv))
> +    return false;
> +  aApplyToDir.Assign(NS_ConvertUTF16toUTF8(applyToDirW));

This conversion is Unicode safe but I'm not sure if this will pass the arguments to updater code in a unicode safe way. Should be tested at least if not already done.

@@ +782,5 @@
> +  if (!restart) {
> +    // Signal the updater application that it should apply the update in the
> +    // background.
> +    pid.AssignASCII("-1");
> +  }

You could probably avoid 2 assignments to pid by putting if (!restart) { ... } else { ifdefs here }

@@ +822,5 @@
>  
> +  // If we're about to try to apply the update in the background, first make
> +  // sure that we have write access to the directories in question.  If we
> +  // don't, just launch updater.exe in the background and hope that we can
> +  // use the service to apply the update.

What about if the service is not installed? Please add to the comment.

@@ +975,5 @@
>    return NS_OK;
>  }
> +
> +NS_IMPL_THREADSAFE_ISUPPORTS1(nsUpdateProcessor, nsIUpdateProcessor)
> +

Why not move all of this nsUpdateProcessor stuff into a new file?

@@ +982,5 @@
> +{
> +}
> +
> +NS_IMETHODIMP
> +nsUpdateProcessor::ProcessUpdate(nsIUpdate* aUpdate)

I wonder if there are any exit(0) calls at places that we should be making sure we're handling properly.

::: toolkit/xre/nsWindowsRestart.cpp
@@ +54,5 @@
> +#include <shlwapi.h>
> +#include <shlobj.h>
> +#include <stdio.h>
> +#include <wchar.h>
> +#include <rpc.h>

I don't think all of these extra includes are necessary here.

::: toolkit/xre/nsXREDirProvider.cpp
@@ +114,1 @@
>    gDirServiceProvider = this;

This is a strange way to create a singleton.
It allows you to create multiple instances of the nsXREDirProvider, and the global variable will point to a new one each time you do.  Why not instead change it so you cannot create multipl instances at all by creating a single static variable within nsXREDriProvider::GetSingleton()? 

See toolkit\mozapps\update\common\updatelogging.h for an example. Also the constructor should be made private if it is a singleton.  Maybe there is some xpcom related thing I'm not thinking of though and that's maybe why it's being done this way?

@@ +122,5 @@
> +nsXREDirProvider*
> +nsXREDirProvider::GetSingleton()
> +{
> +  return gDirServiceProvider;
> +}

Since you are providing this better accessor for the singleton, the other things that use this should no longer use the global variable directly.

@@ +949,5 @@
> +    // directory, because otherwise the updater writing the log file can cause
> +    // the directory to be locked, which prevents it from being replaced after
> +    // background updates.
> +    programName.AssignASCII(MOZ_APP_NAME);
> +  }

Would this cause a problem if someone installed the same product name under 2 different locations?  For example 2 different limited user accounts installing the same Firefox product.

::: xpcom/io/nsILocalFileWin.idl
@@ +84,5 @@
> +    /*
> +     * WFA_READWRITE: Used to clear the readonly attribute.
> +     */
> +    const unsigned long WFA_READWRITE = 4;
> +

I think it might be cleaner if we only have a WFA_READONLY.
The lack of such an attribute would imply read/write.

::: xpcom/io/nsLocalFileWin.cpp
@@ +2716,5 @@
>  
> +    if (aAttribs & WFA_READONLY) {
> +      dwAttrs |= FILE_ATTRIBUTE_READONLY;
> +    } else if ((aAttribs & WFA_READWRITE) &&
> +               (dwAttrs & FILE_ATTRIBUTE_READONLY)) {

- else if ((aAttribs & WFA_READWRITE) &&
-		(dwAttrs & FILE_ATTRIBUTE_READONLY)) {
+ else {
Comment 66 Brian R. Bondy [:bbondy] 2012-01-16 13:40:11 PST
Comment on attachment 586263 [details] [diff] [review]
Test changes

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

::: toolkit/mozapps/update/test/shared.js
@@ +44,5 @@
>  const AUS_Cr = Components.results;
>  const AUS_Cu = Components.utils;
>  
>  const PREF_APP_UPDATE_AUTO                = "app.update.auto";
> +const PREF_APP_UPDATE_BACKGROUND          = "app.update.background";

app.update.background.enabled

::: toolkit/mozapps/update/test/unit/head_update.js.in
@@ +452,5 @@
> +}
> +
> +if (IS_WIN) {
> +  const kLockFileName = "updated.update_in_progress.lock";
> +  const FILE_ATTRIBUTE_READONLY = 0x00000001;

- const FILE_ATTRIBUTE_READONLY = 0x00000001;

@@ +472,5 @@
> +  function unlockDirectory(aDir) {
> +    var file = aDir.clone();
> +    file.append(kLockFileName);
> +    file.QueryInterface(AUS_Ci.nsILocalFileWin);
> +    file.fileAttributesWin |= file.WFA_READWRITE;

Adjust if you do my suggestion from the other patch.

@@ +588,5 @@
>    }
>  
> +#ifdef DISABLE_UPDATER_AUTHENTICODE_CHECK
> +  // We won't be performing signature checks.
> +  return true;

There are some reasons why we don't want to run service tests even know it is not signed within the #else.  This clause shoudl only surround the "// Make sure the binaries are signed" block of code.
Comment 67 :Ehsan Akhgari (away Aug 1-5) 2012-01-16 15:04:37 PST
(In reply to Brian R. Bondy [:bbondy] from comment #65)
> @@ +587,5 @@
> > +            self.setupUpdateButton("update.restart." +
> > +                                   (self.isMajor ? "upgradeButton" : "restartButton"));
> > +            timer.cancel();
> > +            timer = null;
> > +          } else if (status == "failed") {
> 
> Do we need to status.split(":")?

No, update.state should be "failed" if he update has failed, the error code is already removed by the code in nsUpdateService.js which sets update.state.

> @@ +120,5 @@
> > + * @param argvTmp    The argv value normally sent to updater.exe
> > + * @param installDir Buffer to hold the installation directory.
> > + *                   The size of the buffer should be 2 * MAX_PATH.
> > + */
> > +bool
> 
> BOOL for now to be consistent, we'll replace all BOOL with bool in this
> module in the future.

/me hopes for that day to come sooner!

> @@ +389,5 @@
> >        }
> >      }
> >  
> >      /**
> > +#      On Windows, we no longer store the update under the app dir.
> 
> in which cases?

Not sure what you're asking.  I just updated that comment to reflect the reality, and didn't change anything there.

> @@ +1983,5 @@
> > +    }
> > +    if (rv == OK) {
> > +      rv = DoUpdate();
> > +      gArchiveReader.Close();
> > +    }
> 
> Should we fall back if the background update's CopyInstallDirToDestDir fails
> to not use a background update?

In that case, the update operation fails, so we will automatically fall back to the regular updates case.  Or am I missing something?

> @@ +2074,5 @@
> >  
> >    // We never want the service to be used unless we build with
> >    // the maintenance service.
> >  #ifdef MOZ_MAINTENANCE_SERVICE
> > +  useService = IsUpdateStatusPendingService();
> 
> Use your enum function instead after you move it to update common.
> @@ +218,5 @@
> > +  ePendingService,
> > +  eAppliedUpdate,
> > +  eAppliedService
> > +} UpdateStatus;
> > +
> 
> Move this and the associated function into update common code, seems like we
> can use it everywhere and expand on it to reduce some other functions in the
> future.

These functions use XPCOM file abstractions, so moving them to update/common won't buy us much as we won't be able to use them from inside the updater application.

> @@ +368,5 @@
> > +                            GENERIC_READ | GENERIC_WRITE,
> > +                            0,
> > +                            NULL,
> > +                            OPEN_ALWAYS,
> > +                            FILE_FLAG_DELETE_ON_CLOSE,
> 
> Why delete on close?

Because we only create this file as a test for writing to a directory, and we don't want the file to remain on the disk when we're done with it.

> @@ +405,5 @@
> > +
> > +  volume1Len = wcslen(volume1);
> > +  volume2Len = wcslen(volume2);
> > +  return volume1Len == volume2Len &&
> > +         volume1Len == PathCommonPrefixW(volume1, volume2, NULL);
> 
> This is fine but why not use wcsicmp instead of PathCommonPrefixW?

No particular reason, switched to _wcsnicmp.

> @@ +417,5 @@
> > +  nsAutoString applyToDirW;
> > +  nsresult rv = aUpdatedDir->GetPath(applyToDirW);
> > +  if (NS_FAILED(rv))
> > +    return false;
> > +  aApplyToDir.Assign(NS_ConvertUTF16toUTF8(applyToDirW));
> 
> This conversion is Unicode safe but I'm not sure if this will pass the
> arguments to updater code in a unicode safe way. Should be tested at least
> if not already done.

Do you mean testing when Firefox is installed in a path name with Unicode chars in it?  Note that this code may be removed if we decide to address comment 61.

> @@ +975,5 @@
> >    return NS_OK;
> >  }
> > +
> > +NS_IMPL_THREADSAFE_ISUPPORTS1(nsUpdateProcessor, nsIUpdateProcessor)
> > +
> 
> Why not move all of this nsUpdateProcessor stuff into a new file?

See the comment in nsUpdateDriver.h.  :-)

> @@ +982,5 @@
> > +{
> > +}
> > +
> > +NS_IMETHODIMP
> > +nsUpdateProcessor::ProcessUpdate(nsIUpdate* aUpdate)
> 
> I wonder if there are any exit(0) calls at places that we should be making
> sure we're handling properly.

Can you explain your concern a bit more clearly please?

> ::: toolkit/xre/nsXREDirProvider.cpp
> @@ +114,1 @@
> >    gDirServiceProvider = this;
> 
> This is a strange way to create a singleton.
> It allows you to create multiple instances of the nsXREDirProvider, and the
> global variable will point to a new one each time you do.  Why not instead
> change it so you cannot create multipl instances at all by creating a single
> static variable within nsXREDriProvider::GetSingleton()? 
> 
> See toolkit\mozapps\update\common\updatelogging.h for an example. Also the
> constructor should be made private if it is a singleton.  Maybe there is
> some xpcom related thing I'm not thinking of though and that's maybe why
> it's being done this way?

While I hear your complaints about this code, I would rather defer doing the above to another bug, since none of this code has much to do with what this bug is about.

> @@ +122,5 @@
> > +nsXREDirProvider*
> > +nsXREDirProvider::GetSingleton()
> > +{
> > +  return gDirServiceProvider;
> > +}
> 
> Since you are providing this better accessor for the singleton, the other
> things that use this should no longer use the global variable directly.

Same here.

> @@ +949,5 @@
> > +    // directory, because otherwise the updater writing the log file can cause
> > +    // the directory to be locked, which prevents it from being replaced after
> > +    // background updates.
> > +    programName.AssignASCII(MOZ_APP_NAME);
> > +  }
> 
> Would this cause a problem if someone installed the same product name under
> 2 different locations?  For example 2 different limited user accounts
> installing the same Firefox product.

Maybe, but not for the example you give (since the two user accounts will have two different appdata base path names.)

Note that this will only be an issue for Firefox installations in two different directories none of which being under Program Files.  I don't have a better solution for this, so please feel free to suggest better solutions if you can think of any.

> ::: xpcom/io/nsILocalFileWin.idl
> @@ +84,5 @@
> > +    /*
> > +     * WFA_READWRITE: Used to clear the readonly attribute.
> > +     */
> > +    const unsigned long WFA_READWRITE = 4;
> > +
> 
> I think it might be cleaner if we only have a WFA_READONLY.
> The lack of such an attribute would imply read/write.

Having one of them would not be enough since we would have no way to clear the other flag.  For example, if you have a readonly file that you want to mark as read-write, you pass WFA_READWRITE.  We can't mark the file as read-write only if WFA_READONLY is absent.

And vice versa.

I'll attach a patch addressing the rest of the comments here.
Comment 68 :Ehsan Akhgari (away Aug 1-5) 2012-01-16 15:36:03 PST
Created attachment 589051 [details] [diff] [review]
Part 3. Address Brian's code review comments
Comment 69 :Ehsan Akhgari (away Aug 1-5) 2012-01-16 15:43:37 PST
Created attachment 589054 [details] [diff] [review]
Part 4. Address Brian's test review comments
Comment 70 Brian R. Bondy [:bbondy] 2012-01-16 16:00:44 PST
> > +# On Windows, we no longer store the update under the app dir.
> in which cases?

Sorry I read it as app data dir and not app dir.  It's ok as is.

> > Should we fall back if the background update's CopyInstallDirToDestDir fails
> > to not use a background update?

> In that case, the update operation fails, so we will automatically fall back to 
> the regular updates case.  Or am I missing something?

The difference is you don't need the callback to be restarted in between and the user won't get an error like they need to download the full instead.


> Do you mean testing when Firefox is installed in a path name with 
> Unicode chars in it?  

Yes.

> Note that this code may be removed if we 
> decide to address comment 61.

OK it can wait, just something to make sure works/test.


> > I wonder if there are any exit(0) calls at places that we should be making
> > sure we're handling properly.
> Can you explain your concern a bit more clearly please?

I just wanted to make sure that there is no intermittent state that can be entered that is dangerous if there is a forceful shutdown of the application.

>> re singleton global variable stuff...
> While I hear your complaints about this code, I would rather defer doing the
> above to another bug, since none of this code has much to do with what this
> bug is about.

That sounds fine.

> > I think it might be cleaner if we only have a WFA_READONLY.
> > The lack of such an attribute would imply read/write.

> Having one of them would not be enough since we would have no way to 
> clear the other flag.

Ya sorry I didn't notice the existing function modified the current flags based on what was passed in.  I think it would be much cleaner for this function to just set the attributes to what is specified but that isn't related to this task.
Comment 71 Brian R. Bondy [:bbondy] 2012-01-16 16:12:55 PST
Comment on attachment 589051 [details] [diff] [review]
Part 3. Address Brian's code review comments

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

Thanks these changes look good, and I agree with your other comments on things not changed in this patch.
Comment 72 Brian R. Bondy [:bbondy] 2012-01-16 16:14:20 PST
Comment on attachment 589054 [details] [diff] [review]
Part 4. Address Brian's test review comments

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

Changes look good on the test patch as well, thanks.
Comment 73 :Ehsan Akhgari (away Aug 1-5) 2012-01-16 16:46:45 PST
(In reply to Brian R. Bondy [:bbondy] from comment #70)
> > > Should we fall back if the background update's CopyInstallDirToDestDir fails
> > > to not use a background update?
> 
> > In that case, the update operation fails, so we will automatically fall back to 
> > the regular updates case.  Or am I missing something?
> 
> The difference is you don't need the callback to be restarted in between and
> the user won't get an error like they need to download the full instead.

We don't use a callback for background updates.  I think the only likely case in which the copying phase would fail is when we run out of disk space, or when some file in the installation directory is locked and  cannot be read.  Regular updates could also fail under both of those situations, so protecting against this here might not make a lot of sense.  But if Robert wants me to, I'll change this.

> > Do you mean testing when Firefox is installed in a path name with 
> > Unicode chars in it?  
> 
> Yes.

I should probably ask QA to verify this.

> > > I wonder if there are any exit(0) calls at places that we should be making
> > > sure we're handling properly.
> > Can you explain your concern a bit more clearly please?
> 
> I just wanted to make sure that there is no intermittent state that can be
> entered that is dangerous if there is a forceful shutdown of the application.

We rely on a set of (hopefully) atomic operations, such as reading/writing the status file, testing for the existence of a directory, etc.  I think those atomic operations should take care of your concerns here.
Comment 74 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-01-17 09:29:02 PST
(In reply to Brian R. Bondy [:bbondy] from comment #61)
> I don't know if this is a problem or not but just wanted to bring it up so
> security could comment.
I will give my 2¢ and let imelven take account.
> 
> After an update is applied in the background, but before it is replaced into
> the program files directory it sits in a directory like:
> C:\Users\USERNAmE\AppData\Local\Mozilla\Firefox\Nightly\updates\0\updated
> 
So this dir has rights just for the user or this is ACL for all users?

> On the next startup, the service is used to move that folder into Program
> Files (a protected directory).
> 
So this moves from the users profile with the set of ACLs asked about above, to Program Files with the default ACLs for that dir?

> I'm not sure if it's a problem or not that a file could be moved into that
> unprivileged location, and the service will then move it to a privileged
> location at some later time.  For example maybe it is a problem that a dll
> could be replace with some other dll with the same exports but with
> different code that would have access to the process' memory.
> 
> It's my understanding that the updated directory can sit there for a long
> time while the user is browsing until the next browser restart.
> 
If the ACLs are for the user or all users than the bar is lower for malware to access the directory, but if malware is already installed then the game is lost and this is moot. However, if the ACLs are so low that a process from "the web" could write here and add a file that could be worse. I think the odds of this are low though.

> If it's a problem then it could be easily solved (when the service is
> installed at least) by having the update directory inside Program Files
> perhaps as a subdirectory of the directory where the update is being applied
> to eventually.
> 
> I'm not sure if this is something we need to protect against or not but just
> wanted to bring it up as a topic for discussion.

Only thing I could think of doing would be to set the ACLs on the dir for the system only (we run as system IIRC) that would at least keep casual stuff out.
Comment 75 Brian R. Bondy [:bbondy] 2012-01-17 09:55:03 PST
I believe that:
> C:\Users\USERNAME\AppData\Local\Mozilla\Firefox\Nightly\updates\0\updated

Has write access by an unelevated user named USERNAME, not by all users.

I believe that the program files dir:

Has write access by elevated users only. 

> So this moves from the users profile with the set of ACLs asked about 
> above, to Program Files with the default ACLs for that dir?

Correct, so what I'm asking about is if it's a problem that an unelevated user can in effect write into our Program files directory.  They cannot write into other directories.

> if the ACLs are so low that a process from "the web" could write here and 
> add a file that could be worse. I think the odds of this are low though.

A web page would not have access, malware that is installed but does not have elevated permissions would be able to write into our program files directory though (indirectly by writing into the appdata dir).
Comment 76 Brian R. Bondy [:bbondy] 2012-01-17 10:09:53 PST
> A web page would not have access, malware that is installed but does not 
> have elevated permissions would be able to write into our program files 
> directory though (indirectly by writing into the appdata dir).

Whereas that malware that is unelevated would not have access to different products installed into program files.
Comment 77 :Ehsan Akhgari (away Aug 1-5) 2012-01-17 11:08:28 PST
(In reply to Brian R. Bondy [:bbondy] from comment #75)
> > So this moves from the users profile with the set of ACLs asked about 
> > above, to Program Files with the default ACLs for that dir?
> 
> Correct, so what I'm asking about is if it's a problem that an unelevated
> user can in effect write into our Program files directory.  They cannot
> write into other directories.

To be precise, the concern is for an unelevated user to write into Program Files\Mozilla Firefox, not any other directory under Program Files.

> > if the ACLs are so low that a process from "the web" could write here and 
> > add a file that could be worse. I think the odds of this are low though.
> 
> A web page would not have access, malware that is installed but does not
> have elevated permissions would be able to write into our program files
> directory though (indirectly by writing into the appdata dir).

I'm not sure what Curtis means by a process from the web.  Web pages should not be allowed to run code as a process at all.  If you mean a program downloaded from the web, that gets run as the logged in user, and will have write access to the appdata directory.

(In reply to Brian R. Bondy [:bbondy] from comment #76)
> > A web page would not have access, malware that is installed but does not 
> > have elevated permissions would be able to write into our program files 
> > directory though (indirectly by writing into the appdata dir).
> 
> Whereas that malware that is unelevated would not have access to different
> products installed into program files.

Again to be precise, only the Firefox installation directory is subject to unwanted modifications here.  For the updater for other Mozilla based products, just replace "Firefox" with that product name for everything I said here.
Comment 78 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-01-17 13:58:02 PST
I did not read all the comments above. But, I read comment 61+. bbondy's concerns in comment 61 are very justified.

The reason that we check the MAR file signature in the elevated process is to ensure that we are not relying on the contents of any files that have been modified between the time that we downloaded the MAR file and the time that we write the contents of the MAR file into Program Files. This order of signature checking is what will enable us to safely apply updates that are downloaded even by non-administrator untrusted users. (e.g. Firefox can be updated even when your kid is logged into his non-administrator account on the computer.) This order of operations also prevents our service from facilitating malware elevation-of-privilege attacks.

Unpacking the MAR file into the unprivileged user's profile directory and then having (an application elevated by) our service copy those files into Program Files without re-verifying them is very much like Firefox checking the MAR file signature, stripping the signature from the MAR file, and then having the service apply the update using the unsigned MAR file without checking the signature. This would eliminate the two huge advantages of the current approach, which would be extremely unfortunate.

Possible solutions would be (a) have (the program elevated by) the service verify that each file hasn't been tampered with, or (b) have (the program elevated by) the service unpack the file into a place where only administrators can write to (and avoid race conditions), or (c) apply the update in the background when the user isn't using Firefox and block the startup of Firefox until the update is complete if the user tries to start Firefox when the update is still in progress. I think (c) probably isn't going to be the best way forward and (a) is extremely error-prone w.r.t. security issues to the point where I almost didn't even suggest it as a possibility.

(In reply to Brian R. Bondy [:bbondy] from comment #75)
> > if the ACLs are so low that a process from "the web" could write here and 
> > add a file that could be worse. I think the odds of this are low though.
> 
> A web page would not have access, malware that is installed but does not
> have elevated permissions would be able to write into our program files
> directory though (indirectly by writing into the appdata dir).

Right. There is a big difference between unelevated malware (which I expect to be very common, even considering only the local code execution bugs in Java and Flash) and elevated malware (which is more-or-less effectively prevented by UAC and UAC-like mechanisms). If we don't guard against unelevated malware then we are making it more dangerous and more like elevated malware, in a way that UAC and othre countermeasures cannot prevent.

But, it isn't just malware. There are also many cases where the user is simply untrusted to place files into Program Files but is trusted to use the computer. (e.g. a computer in a hotel lobby, school computers, corporate computers, home computer being used by a child, etc.) Updates should (eventually) be able to happen in these cases (because updating ASAP is probably the best thing for security), but those updates should happen in a way that doesn't give untrusted users dangerous abilities they did not have before (e.g. choosing which files to put under C:\Program Files\).
Comment 79 :Ehsan Akhgari (away Aug 1-5) 2012-01-17 14:17:45 PST
After giving all of this a bit more thought, I would like to remove the code path for putting the updated directory in the appdir on Windows altogether, and just attempt to use the service if it's available, and fall back to updates-on-startup if it's not.

If we find a better solution in the future, we can adopt it, but I think this would be an acceptable middle ground for now.
Comment 80 Brian R. Bondy [:bbondy] 2012-01-17 15:27:44 PST
> I would like to remove the code path for putting the updated directory 
> in the appdir on Windows altogether, and just attempt to use the service 
> if it's available, and fall back to updates-on-startup if it's not

I agree.
Comment 81 Ian Melven :imelven 2012-01-17 15:34:37 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #79)
> After giving all of this a bit more thought, I would like to remove the code
> path for putting the updated directory in the appdir on Windows altogether,
> and just attempt to use the service if it's available, and fall back to
> updates-on-startup if it's not.
> 
> If we find a better solution in the future, we can adopt it, but I think
> this would be an acceptable middle ground for now.

just to clarify, this means the downloaded update will be stored under Program Files/Mozilla Firefox ? and then the service will be used if it's installed/available to do the directory switch ?

then if the service isn't available, Firefox itself does the update on next start as it currently does ? 

I also had serious concerns about installing from a user writeable folder - if the proposal is as I've restated above, that seems fine to me.
Comment 82 :Ehsan Akhgari (away Aug 1-5) 2012-01-17 15:43:17 PST
(In reply to Ian Melven :imelven from comment #81)
> (In reply to Ehsan Akhgari [:ehsan] from comment #79)
> > After giving all of this a bit more thought, I would like to remove the code
> > path for putting the updated directory in the appdir on Windows altogether,
> > and just attempt to use the service if it's available, and fall back to
> > updates-on-startup if it's not.
> > 
> > If we find a better solution in the future, we can adopt it, but I think
> > this would be an acceptable middle ground for now.
> 
> just to clarify, this means the downloaded update will be stored under
> Program Files/Mozilla Firefox ? and then the service will be used if it's
> installed/available to do the directory switch ?

The update gets installed in Program Files\Mozilla Firefox\updated.

The service will be used to handle both phases in the update.

> then if the service isn't available, Firefox itself does the update on next
> start as it currently does ? 

Correct.

> I also had serious concerns about installing from a user writeable folder -
> if the proposal is as I've restated above, that seems fine to me.

Great!
Comment 83 Robert Strong [:rstrong] (use needinfo to contact me) 2012-01-17 15:47:40 PST
I don't think the following has been commented on

from irc on 1/17:

ehsan> so what do you suggest should happen if the service is disabled and we don't have write permission to the install dir on windows?
rs> or has permissions to write to the install dir then stage it. On Windows if unable to write to the install dir and service is available use it
rs> that should fallback to the current update mechanism.

So, in the case where the user already has write access to the install directory it should just stage it.
Comment 84 :Ehsan Akhgari (away Aug 1-5) 2012-01-17 15:50:29 PST
(In reply to Robert Strong [:rstrong] (do not email) from comment #83)
> I don't think the following has been commented on
> 
> from irc on 1/17:
> 
> ehsan> so what do you suggest should happen if the service is disabled and
> we don't have write permission to the install dir on windows?
> rs> or has permissions to write to the install dir then stage it. On Windows
> if unable to write to the install dir and service is available use it
> rs> that should fallback to the current update mechanism.
> 
> So, in the case where the user already has write access to the install
> directory it should just stage it.

Yes, that is what I'm planning to do.
Comment 85 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-01-17 18:35:18 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #84)
> > So, in the case where the user already has write access to the install
> > directory it should just stage it.
> 
> Yes, that is what I'm planning to do.

I am not up to speed with all your guys' jargon. But, what I would expect is that the update's files will get put a subfolder that is next to the currently-installed files So, if Firefox is installed in "C:\Program Files\Firefox" then this would be "C:\Program Files\Firefox\updated" and if it was installed in "C:\Users\bsmith\AppData\Firefox" then this would be "C:\Users\bsmith\AppData\Firefox\updated". Is that correct?
Comment 86 :Ehsan Akhgari (away Aug 1-5) 2012-01-18 13:22:01 PST
(In reply to Brian Smith (:bsmith) from comment #85)
> (In reply to Ehsan Akhgari [:ehsan] from comment #84)
> > > So, in the case where the user already has write access to the install
> > > directory it should just stage it.
> > 
> > Yes, that is what I'm planning to do.
> 
> I am not up to speed with all your guys' jargon. But, what I would expect is
> that the update's files will get put a subfolder that is next to the
> currently-installed files So, if Firefox is installed in "C:\Program
> Files\Firefox" then this would be "C:\Program Files\Firefox\updated" and if
> it was installed in "C:\Users\bsmith\AppData\Firefox" then this would be
> "C:\Users\bsmith\AppData\Firefox\updated". Is that correct?

Yes, that is correct.
Comment 87 :Ehsan Akhgari (away Aug 1-5) 2012-01-18 13:33:00 PST
Created attachment 589621 [details] [diff] [review]
Do not use the alternate directory

This patch removes the alternate directory support as per the recent discussion in the bug.
Comment 88 :Ehsan Akhgari (away Aug 1-5) 2012-03-06 12:20:14 PST
There are two remaining issues that I need to look at.

1. Test failures with the recent merges from m-c on Oak (for example, see https://tbpl.mozilla.org/?tree=Oak&rev=eca94af42dda).  These failures are Windows only, and are probably something simple that I need to fix, most likely caused by the removal of channel switching support.

2. QA notified me that the Oak builds fail to update on Windows Vista.  I have a Vista VM now and I will attempt to reproduce the problem and debug it.
Comment 89 :Ehsan Akhgari (away Aug 1-5) 2012-03-06 16:33:42 PST
Created attachment 603516 [details] [diff] [review]
Part 5. Remove the channel change file related code from tests

This fixes the first part of comment 88.
Comment 90 :Ehsan Akhgari (away Aug 1-5) 2012-03-08 14:53:41 PST
Created attachment 604208 [details] [diff] [review]
Code changes (v3)
Comment 91 :Ehsan Akhgari (away Aug 1-5) 2012-03-08 14:54:25 PST
Created attachment 604211 [details] [diff] [review]
Test changes (v3)
Comment 92 :Ehsan Akhgari (away Aug 1-5) 2012-03-09 15:01:41 PST
I debugged the vista problem and I found a bug which I fixed here: https://hg.mozilla.org/projects/oak/rev/1af564e622ce

I've triggered nightlies on that to test to see whether that fixes the update issue on Vista.
Comment 93 Guillaume C. [:ge3k0s] 2012-03-12 05:18:15 PDT
The update notification toaster/growl should go away.
Comment 94 :Ehsan Akhgari (away Aug 1-5) 2012-03-13 20:18:42 PDT
Created attachment 605640 [details] [diff] [review]
Code changes (v4)

This version of the patch fixes the update problem on Vista, and also makes sure that the progress UI never appears when doing a background upgrade on Vista.
Comment 95 :Ehsan Akhgari (away Aug 1-5) 2012-04-12 13:21:47 PDT
I pushed https://hg.mozilla.org/projects/oak/rev/ef570d008beb to the Oak branch to get the Android builds working again.
Comment 96 :Ehsan Akhgari (away Aug 1-5) 2012-04-16 12:43:27 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #95)
> I pushed https://hg.mozilla.org/projects/oak/rev/ef570d008beb to the Oak
> branch to get the Android builds working again.

Plus https://hg.mozilla.org/projects/oak/rev/384de26cf7ba
Comment 97 Robert Strong [:rstrong] (use needinfo to contact me) 2012-04-17 01:56:56 PDT
Comment on attachment 605640 [details] [diff] [review]
Code changes (v4)

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>...
>@@ -2860,28 +2927,30 @@ Downloader.prototype = {
>    * @param   status
>    *          Status code containing the reason for the cessation.
>    */
>   onStopRequest: function  Downloader_onStopRequest(request, context, status) {
>     if (request instanceof Ci.nsIIncrementalDownload)
>       LOG("Downloader:onStopRequest - original URI spec: " + request.URI.spec +
>           ", final URI spec: " + request.finalURI.spec + ", status: " + status);
> 
>+    // XXX ehsan shouldShowPrompt should always be false here.
>+    // But what happens when there is already a UI showing?
The UI is suppose to keep track and handle the failure state (e.g. inform the user, download a complete after a partial failure, etc.). In this instance I think it should just fallback to trying to apply on startup.

>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
>--- a/toolkit/mozapps/update/updater/updater.cpp
>+++ b/toolkit/mozapps/update/updater/updater.cpp
>...
>@@ -1431,76 +1635,80 @@ WriteStatusFile(int status)
>   AutoFile file = NS_tfopen(filename, NS_T("wb+"));
>   if (file == NULL)
>     return;
> 
>   const char *text;
> 
>   char buf[32];
>   if (status == OK) {
>-    text = "succeeded\n";
>+    if (sBackgroundUpdate) {
>+#if defined(MOZ_MAINTENANCE_SERVICE)
>+      text = "applied-service\n";
What if the maintenance servics is disabled via preferences?

>@@ -1523,17 +1731,254 @@ IsUpdateStatusSucceeded(bool &isSucceede
>   char buf[32] = { 0 };
>   fread(buf, sizeof(buf), 1, file);
> 
>   const char kSucceeded[] = "succeeded";
>   isSucceeded = strncmp(buf, kSucceeded, 
>                         sizeof(kSucceeded) - 1) == 0;
>   return true;
> }
>-
>+#endif
>+
>+/*
>+ * Get the application installation directory.
>+ *
>+ * @param installDir Out parameter for specifying the installation directory.
>+ * @return true if successful, false otherwise.
>+ */
>+template <size_t N>
>+static bool
>+GetInstallationDir(NS_tchar (&installDir)[N])
>+{
>+  NS_tsnprintf(installDir, N, NS_T("%s"), gDestinationPath);
>+  NS_tchar *slash = (NS_tchar *) NS_tstrrchr(installDir, NS_SLASH);
>+  // Make sure we're not looking at a trailing slash
>+  if (slash && slash[1] == NS_T('\0')) {
>+    *slash = NS_T('\0');
>+    slash = (NS_tchar *) NS_tstrrchr(installDir, NS_SLASH);
>+  }
>+  if (slash) {
>+    *slash = NS_T('\0');
>+  } else {
>+    return false;
>+  }
>+  return true;
>+}
>+
>+/*
>+ * Copy the entire contents of the application installation directory to the
>+ * destination directory for the update process.
>+ *
>+ * @return 0 if successful, an error code otherwise.
>+ */
>+static int
>+CopyInstallDirToDestDir()
>+{
>+  // First extract the installation directory from gSourcePath by going two
>+  // levels above it.  This is effectively skipping over "updates/0".
>+  NS_tchar installDir[MAXPATHLEN];
>+  if (!GetInstallationDir(installDir)) {
>+    return UNEXPECTED_ERROR;
>+  }
>+
>+  // These files should not be copied over to the updated app
>+#ifdef XP_WIN
>+#define SKIPLIST_COUNT 4
>+#else
>+#define SKIPLIST_COUNT 3
>+#endif
>+  copy_recursive_skiplist<SKIPLIST_COUNT> skiplist;
>+#ifdef XP_MACOSX
>+  skiplist.append(0, installDir, NS_T("Updated.app"));
>+  skiplist.append(1, installDir, NS_T("Contents/MacOS/updates"));
>+  skiplist.append(2, installDir, NS_T("Contents/MacOS/active-update.xml"));
>+#else
>+  skiplist.append(0, installDir, NS_T("updated"));
>+  skiplist.append(1, installDir, NS_T("updates"));
>+  skiplist.append(2, installDir, NS_T("active-update.xml"));
>+#ifdef XP_WIN
>+  skiplist.append(3, installDir, NS_T("updated.update_in_progress.lock"));
>+#endif
>+#endif
Why are you skipping active-update.xml? It is needed after the update.

>+  // XXX ehsan Should precomplete on Mac be in the skiplist?
Why do you think it should be?

>+#ifdef XP_MACOSX
>+  NS_tchar sourceDir[MAXPATHLEN];
>+  NS_tsnprintf(sourceDir, sizeof(sourceDir)/sizeof(sourceDir[0]),
>+               NS_T("%s/Contents"), installDir);
I have been trying to come up with a decent way to run against a non .app without any luck. I would much prefer skipping the xpcshell tests when they aren't being run against a .app instead of copying over the /Contents directory, etc.

>@@ -1788,45 +2302,50 @@ int NS_main(int argc, NS_tchar **argv)
>       // CreateProcess will use the same token as this process.
>       UACHelper::DisablePrivileges(NULL);
>     }
> 
>     if (updateLockFileHandle == INVALID_HANDLE_VALUE || 
>         (useService && testOnlyFallbackKeyExists && noServiceFallback)) {
>       if (!_waccess(elevatedLockFilePath, F_OK) &&
>           NS_tremove(elevatedLockFilePath) != 0) {
>-        fprintf(stderr, "Update already elevated! Exiting\n");
>+        LOG(("Update already elevated! Exiting\n"));
>         return 1;
>       }
> 
>       HANDLE elevatedFileHandle;
>       elevatedFileHandle = CreateFileW(elevatedLockFilePath,
>                                        GENERIC_READ | GENERIC_WRITE,
>                                        0,
>                                        NULL,
>                                        OPEN_ALWAYS,
>                                        FILE_FLAG_DELETE_ON_CLOSE,
>                                        NULL);
> 
>       if (elevatedFileHandle == INVALID_HANDLE_VALUE) {
>-        fprintf(stderr, "Unable to create elevated lock file! Exiting\n");
>+        LOG(("Unable to create elevated lock file! Exiting\n"));
iirc this wasn't written to the log file since we can have multiple instances trying to write to the file.

Overall this patch is in very good shape after I was able to wrap my head around the changes. Thanks!
Comment 98 :Ehsan Akhgari (away Aug 1-5) 2012-04-17 09:08:06 PDT
(In reply to Robert Strong [:rstrong] (do not email) from comment #97)
> Comment on attachment 605640 [details] [diff] [review]
> Code changes (v4)
> 
> >diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
> >--- a/toolkit/mozapps/update/nsUpdateService.js
> >+++ b/toolkit/mozapps/update/nsUpdateService.js
> >...
> >@@ -2860,28 +2927,30 @@ Downloader.prototype = {
> >    * @param   status
> >    *          Status code containing the reason for the cessation.
> >    */
> >   onStopRequest: function  Downloader_onStopRequest(request, context, status) {
> >     if (request instanceof Ci.nsIIncrementalDownload)
> >       LOG("Downloader:onStopRequest - original URI spec: " + request.URI.spec +
> >           ", final URI spec: " + request.finalURI.spec + ", status: " + status);
> > 
> >+    // XXX ehsan shouldShowPrompt should always be false here.
> >+    // But what happens when there is already a UI showing?
> The UI is suppose to keep track and handle the failure state (e.g. inform
> the user, download a complete after a partial failure, etc.). In this
> instance I think it should just fallback to trying to apply on startup.

This is not a failure scenario.  The case of interest here is when an update finishes downloading successfull in the background.  Previously, this would set shouldShowPrompt to true, which would cause us to prompt the user to apply the update and restart.  My patch changes this logic so that shouldShowPrompt is always false for background updates, and instead calls applyUpdateInBackground to initiate the process of applying the update in the background.

This comment was a question about what we should do in the case where the update is downloaded successfully, applyUpdateInBackground is called, and it finishes applying the update in the background.  If we still want to show a prompt once the update has finished being applied successfully, we should have some code which gets triggered once the update has finished being applied, and show the prompt then (in which case we should also change the update prompter to make it aware of this additional case -- and this will probably require more string changes.)

> >diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
> >--- a/toolkit/mozapps/update/updater/updater.cpp
> >+++ b/toolkit/mozapps/update/updater/updater.cpp
> >...
> >@@ -1431,76 +1635,80 @@ WriteStatusFile(int status)
> >   AutoFile file = NS_tfopen(filename, NS_T("wb+"));
> >   if (file == NULL)
> >     return;
> > 
> >   const char *text;
> > 
> >   char buf[32];
> >   if (status == OK) {
> >-    text = "succeeded\n";
> >+    if (sBackgroundUpdate) {
> >+#if defined(MOZ_MAINTENANCE_SERVICE)
> >+      text = "applied-service\n";
> What if the maintenance servics is disabled via preferences?

Right.  I'll address this in the next version of the patch, so that the choice of whether to use the -service variant of the update status would all be inside nsUpdateService.js.

> >+  // These files should not be copied over to the updated app
> >+#ifdef XP_WIN
> >+#define SKIPLIST_COUNT 4
> >+#else
> >+#define SKIPLIST_COUNT 3
> >+#endif
> >+  copy_recursive_skiplist<SKIPLIST_COUNT> skiplist;
> >+#ifdef XP_MACOSX
> >+  skiplist.append(0, installDir, NS_T("Updated.app"));
> >+  skiplist.append(1, installDir, NS_T("Contents/MacOS/updates"));
> >+  skiplist.append(2, installDir, NS_T("Contents/MacOS/active-update.xml"));
> >+#else
> >+  skiplist.append(0, installDir, NS_T("updated"));
> >+  skiplist.append(1, installDir, NS_T("updates"));
> >+  skiplist.append(2, installDir, NS_T("active-update.xml"));
> >+#ifdef XP_WIN
> >+  skiplist.append(3, installDir, NS_T("updated.update_in_progress.lock"));
> >+#endif
> >+#endif
> Why are you skipping active-update.xml? It is needed after the update.

Will fix.

> >+  // XXX ehsan Should precomplete on Mac be in the skiplist?
> Why do you think it should be?

Not sure, mainly wanted to check with you.  :-)

> >+#ifdef XP_MACOSX
> >+  NS_tchar sourceDir[MAXPATHLEN];
> >+  NS_tsnprintf(sourceDir, sizeof(sourceDir)/sizeof(sourceDir[0]),
> >+               NS_T("%s/Contents"), installDir);
> I have been trying to come up with a decent way to run against a non .app
> without any luck. I would much prefer skipping the xpcshell tests when they
> aren't being run against a .app instead of copying over the /Contents
> directory, etc.

Hmm, OK.  This will make it impossible to run the updater tests on Mac without packaging the tests and binaries the same way that the Tinderbox machines do.  I'll remove this code, but it will take some time to test as I need Tinderbox builds for the testing.  (Or alternatively, figure out exactly how the packaging steps work on Tinderbox machines.)

> >@@ -1788,45 +2302,50 @@ int NS_main(int argc, NS_tchar **argv)
> >       // CreateProcess will use the same token as this process.
> >       UACHelper::DisablePrivileges(NULL);
> >     }
> > 
> >     if (updateLockFileHandle == INVALID_HANDLE_VALUE || 
> >         (useService && testOnlyFallbackKeyExists && noServiceFallback)) {
> >       if (!_waccess(elevatedLockFilePath, F_OK) &&
> >           NS_tremove(elevatedLockFilePath) != 0) {
> >-        fprintf(stderr, "Update already elevated! Exiting\n");
> >+        LOG(("Update already elevated! Exiting\n"));
> >         return 1;
> >       }
> > 
> >       HANDLE elevatedFileHandle;
> >       elevatedFileHandle = CreateFileW(elevatedLockFilePath,
> >                                        GENERIC_READ | GENERIC_WRITE,
> >                                        0,
> >                                        NULL,
> >                                        OPEN_ALWAYS,
> >                                        FILE_FLAG_DELETE_ON_CLOSE,
> >                                        NULL);
> > 
> >       if (elevatedFileHandle == INVALID_HANDLE_VALUE) {
> >-        fprintf(stderr, "Unable to create elevated lock file! Exiting\n");
> >+        LOG(("Unable to create elevated lock file! Exiting\n"));
> iirc this wasn't written to the log file since we can have multiple
> instances trying to write to the file.

OK, will revert to writing the error to stderr.

> Overall this patch is in very good shape after I was able to wrap my head
> around the changes. Thanks!

Good to hear that!  :-)

I will incorporate the changes you requested and will submit a new patch after some testing (hopefully later today.)
Comment 99 Lawrence Mandel [:lmandel] (use needinfo) 2012-04-17 09:55:23 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #98)
> This is not a failure scenario.  The case of interest here is when an update
> finishes downloading successfull in the background.  Previously, this would
> set shouldShowPrompt to true, which would cause us to prompt the user to
> apply the update and restart.  My patch changes this logic so that
> shouldShowPrompt is always false for background updates, and instead calls
> applyUpdateInBackground to initiate the process of applying the update in
> the background.
> 
> This comment was a question about what we should do in the case where the
> update is downloaded successfully, applyUpdateInBackground is called, and it
> finishes applying the update in the background.  If we still want to show a
> prompt once the update has finished being applied successfully, we should
> have some code which gets triggered once the update has finished being
> applied, and show the prompt then (in which case we should also change the
> update prompter to make it aware of this additional case -- and this will
> probably require more string changes.)

For security sake we need to prompt the user to restart (currently after 24 hours has passed) after the update has been installed/staged in order to ensure that the update is applied to the working copy of Firefox.
Comment 100 :Ehsan Akhgari (away Aug 1-5) 2012-04-17 10:24:24 PDT
(In reply to Lawrence Mandel [:lmandel] from comment #99)
> (In reply to Ehsan Akhgari [:ehsan] from comment #98)
> > This is not a failure scenario.  The case of interest here is when an update
> > finishes downloading successfull in the background.  Previously, this would
> > set shouldShowPrompt to true, which would cause us to prompt the user to
> > apply the update and restart.  My patch changes this logic so that
> > shouldShowPrompt is always false for background updates, and instead calls
> > applyUpdateInBackground to initiate the process of applying the update in
> > the background.
> > 
> > This comment was a question about what we should do in the case where the
> > update is downloaded successfully, applyUpdateInBackground is called, and it
> > finishes applying the update in the background.  If we still want to show a
> > prompt once the update has finished being applied successfully, we should
> > have some code which gets triggered once the update has finished being
> > applied, and show the prompt then (in which case we should also change the
> > update prompter to make it aware of this additional case -- and this will
> > probably require more string changes.)
> 
> For security sake we need to prompt the user to restart (currently after 24
> hours has passed) after the update has been installed/staged in order to
> ensure that the update is applied to the working copy of Firefox.

In that case I need to rework this patch a bit to make sure that happens.
Comment 101 :Ehsan Akhgari (away Aug 1-5) 2012-04-17 11:47:43 PDT
FWIW, I will publish separate patches on top of these ones for addressing the comments, to make the reviews easier.
Comment 102 Robert Strong [:rstrong] (use needinfo to contact me) 2012-04-17 12:57:02 PDT
We'll also want to keep the last-update.log and backup-update.log located in the updates directory if they are present.
Comment 103 :Ehsan Akhgari (away Aug 1-5) 2012-04-17 14:34:24 PDT
Created attachment 615880 [details] [diff] [review]
Address review comments

This patch addresses the review comments, except for the simplifying the Mac specific xpcshell case, and prompting after the update has been applied in the background.
Comment 104 :Ehsan Akhgari (away Aug 1-5) 2012-04-17 14:45:47 PDT
(In reply to Robert Strong [:rstrong] (do not email) from comment #102)
> We'll also want to keep the last-update.log and backup-update.log located in
> the updates directory if they are present.

I don't remove them explicitly in my patches.  Are they removed as a side effect of something?
Comment 105 :Ehsan Akhgari (away Aug 1-5) 2012-04-17 15:17:23 PDT
Created attachment 615902 [details] [diff] [review]
Prompt when updates have been applied

This patch shows a prompt when the update gets applied in the background.  I also added a test to make sure that the newly added page works correctly.

I chose not to change the strings for this patch, as the distinction will probably be invisible to the user.  The important detail shown in the dialog is that the user needs to restart, and that will still be true no matter if the update has been applied in the background.
Comment 106 :Ehsan Akhgari (away Aug 1-5) 2012-04-17 16:59:27 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #98)
> > >+#ifdef XP_MACOSX
> > >+  NS_tchar sourceDir[MAXPATHLEN];
> > >+  NS_tsnprintf(sourceDir, sizeof(sourceDir)/sizeof(sourceDir[0]),
> > >+               NS_T("%s/Contents"), installDir);
> > I have been trying to come up with a decent way to run against a non .app
> > without any luck. I would much prefer skipping the xpcshell tests when they
> > aren't being run against a .app instead of copying over the /Contents
> > directory, etc.
> 
> Hmm, OK.  This will make it impossible to run the updater tests on Mac
> without packaging the tests and binaries the same way that the Tinderbox
> machines do.  I'll remove this code, but it will take some time to test as I
> need Tinderbox builds for the testing.  (Or alternatively, figure out
> exactly how the packaging steps work on Tinderbox machines.)

Actually I think I spoke too soon.  Our Tinderbox machines also run some of the updater tests using an updater binary outside of the bundle.  Basically I think if we want to take out this code, we're going to need to spend a considerable amount of time and energy to keep our existing test coverage on Mac, for very little gain.  Therefore I'd like to suggest not doing that in this bug.  What do you think, Robert?

(Next, I'm going to look try and figure out comment 102.  The rest of the comments are addressed by my additional patches here.)
Comment 107 :Ehsan Akhgari (away Aug 1-5) 2012-04-18 12:45:25 PDT
Created attachment 616251 [details] [diff] [review]
Preserve the last-update.log file after background updates

This fixes comment 102, and adds tests for it.
Comment 108 :Ehsan Akhgari (away Aug 1-5) 2012-04-18 12:46:11 PDT
Also, note that I'm pushing my work regularly to the Oak branch, in case you need to take a look at the current state of the code.
Comment 109 :Ehsan Akhgari (away Aug 1-5) 2012-04-18 14:27:26 PDT
Robert, is the update wizard living in toolkit/mozapps/update/content/update.xul used in the wizard mode?  If yes, we need to make that aware of background updates as well.  I tried looking into it, but I don't know how to invoke and test that wizard.  Can you please let me know if this is needed, and what I need to do in order to test the wizard?
Comment 110 Robert Strong [:rstrong] (use needinfo to contact me) 2012-04-20 04:51:03 PDT
Comment on attachment 615902 [details] [diff] [review]
Prompt when updates have been applied

Why not just use finishedBackground for both finishedBackground and appliedBackground?

Why not just use showUpdateDownloaded with true for the background param instead of adding showUpdateApplied?

Both of the above have the same end result as the new code and if we want to add a different end result in the future it can be split out then.

Instead of comments with "Background updates" please state "Applying updates in the background" since we can support downloading updates in the background.
Comment 111 Robert Strong [:rstrong] (use needinfo to contact me) 2012-04-20 05:04:42 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #109)
> Robert, is the update wizard living in
> toolkit/mozapps/update/content/update.xul used in the wizard mode?  If yes,
> we need to make that aware of background updates as well.  I tried looking
> into it, but I don't know how to invoke and test that wizard.  Can you
> please let me know if this is needed, and what I need to do in order to test
> the wizard?
It is used and since the ui is the same for finishedBackground it should just use that page at least until (if ever) that it is decided that this ui should be different. The mochitest-chrome tests test that ui and I think you can just test the UI displaying properly in a similar manner as test_0092_finishedBackground.xul with the settings for the update being staged in the background.

(In reply to Robert Strong [:rstrong] (do not email) from comment #110)
>...
> Instead of comments with "Background updates" please state "Applying updates
> in the background" since we can support downloading updates in the
> background.
Instead of "Applying updates in the background" please use "Staging updates in the background"
Comment 112 :Ehsan Akhgari (away Aug 1-5) 2012-04-20 10:47:10 PDT
Created attachment 617035 [details] [diff] [review]
Prompt when updates have been applied

Addressed the comments.
Comment 113 :Ehsan Akhgari (away Aug 1-5) 2012-04-20 10:49:21 PDT
(In reply to Robert Strong [:rstrong] (do not email) from comment #111)
> (In reply to Ehsan Akhgari [:ehsan] from comment #109)
> > Robert, is the update wizard living in
> > toolkit/mozapps/update/content/update.xul used in the wizard mode?  If yes,
> > we need to make that aware of background updates as well.  I tried looking
> > into it, but I don't know how to invoke and test that wizard.  Can you
> > please let me know if this is needed, and what I need to do in order to test
> > the wizard?
> It is used and since the ui is the same for finishedBackground it should
> just use that page at least until (if ever) that it is decided that this ui
> should be different. The mochitest-chrome tests test that ui and I think you
> can just test the UI displaying properly in a similar manner as
> test_0092_finishedBackground.xul with the settings for the update being
> staged in the background.

OK, but the question I had was specifically about this workflow:

You'll get prompted that an update is available, you click Next and start downloading the update, *then* the wizard will change to say that the update download was successful and you should restart Firefox to apply, while the update has in fact being staged in the background.  Should I rework this wizard to show a new progress bar or something when this happens?
Comment 114 Lawrence Mandel [:lmandel] (use needinfo) 2012-04-20 10:53:57 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #113)
> You'll get prompted that an update is available, you click Next and start
> downloading the update, *then* the wizard will change to say that the update
> download was successful and you should restart Firefox to apply, while the
> update has in fact being staged in the background.  Should I rework this
> wizard to show a new progress bar or something when this happens?

I'm confused. Do you mean that we need a progress bar because we have to delay the restart while the update is being staged?
Comment 115 :Ehsan Akhgari (away Aug 1-5) 2012-04-20 11:00:16 PDT
(In reply to Lawrence Mandel [:lmandel] from comment #114)
> (In reply to Ehsan Akhgari [:ehsan] from comment #113)
> > You'll get prompted that an update is available, you click Next and start
> > downloading the update, *then* the wizard will change to say that the update
> > download was successful and you should restart Firefox to apply, while the
> > update has in fact being staged in the background.  Should I rework this
> > wizard to show a new progress bar or something when this happens?
> 
> I'm confused. Do you mean that we need a progress bar because we have to
> delay the restart while the update is being staged?

Precisely.
Comment 116 Lawrence Mandel [:lmandel] (use needinfo) 2012-04-20 11:04:41 PDT
My first response is that this is suitable for a follow-up bug. Is the browser unresponsive while the update is being staged? How long does this typically take?
Comment 117 :Ehsan Akhgari (away Aug 1-5) 2012-04-20 11:22:16 PDT
(In reply to Lawrence Mandel [:lmandel] from comment #116)
> My first response is that this is suitable for a follow-up bug.

That would be fine with me, but I'd like Robert to weigh in.

> Is the
> browser unresponsive while the update is being staged?

Yes.

> How long does this
> typically take?

That depends on the exact hardware, system load, etc.  But if you have a notion of the average amount of time that it takes for an update to be applied at start-up, the update process itself is going to be maybe 2x slower when the update is being staged (because staging the update has the initial cost of making a full copy of the installation directory first.)
Comment 118 Robert Strong [:rstrong] (use needinfo to contact me) 2012-04-20 12:43:35 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #113)
>...
> You'll get prompted that an update is available, you click Next and start
> downloading the update, *then* the wizard will change to say that the update
> download was successful and you should restart Firefox to apply, while the
> update has in fact being staged in the background.  Should I rework this
> wizard to show a new progress bar or something when this happens?
Yes... otherwise it would be borked.
Comment 119 Robert Strong [:rstrong] (use needinfo to contact me) 2012-04-20 12:48:06 PDT
For now, an undetermined progress bar on the download page should suffice.
Comment 120 :Ehsan Akhgari (away Aug 1-5) 2012-04-20 13:10:42 PDT
(In reply to Robert Strong [:rstrong] (do not email) from comment #119)
> For now, an undetermined progress bar on the download page should suffice.

So do you think this needs a follow-up bug?  Or do you want me to work on this now?  (I can do that, just wanna make sure I prioritize things correctly.  Thanks!)
Comment 121 :Ehsan Akhgari (away Aug 1-5) 2012-04-20 13:11:19 PDT
Oh, also, how should I trigger this UI for testing?
Comment 122 Robert Strong [:rstrong] (use needinfo to contact me) 2012-04-20 13:16:34 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #120)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #119)
> > For now, an undetermined progress bar on the download page should suffice.
> 
> So do you think this needs a follow-up bug?  Or do you want me to work on
> this now?  (I can do that, just wanna make sure I prioritize things
> correctly.  Thanks!)
It would be broken without the wait so please make the download page show an undetermined progress bar while it is staging the update similar to how the about dialog just displays applying update. I don't think a followup is necessary with this implemented.

(In reply to Ehsan Akhgari [:ehsan] from comment #121)
> Oh, also, how should I trigger this UI for testing?
I would think with the settings in place to stage the update that it would automatically get triggered by any of the mochitest-chrome tests that perform a download.
Comment 123 :Ehsan Akhgari (away Aug 1-5) 2012-04-20 15:16:51 PDT
OK, I didn't get to work on this today, I'll try to spend some time on it during the weekend.  I'll have a patch on Monday at latest, hopefully.
Comment 124 :Ehsan Akhgari (away Aug 1-5) 2012-04-23 14:52:31 PDT
Created attachment 617655 [details] [diff] [review]
Make the update wizard aware of background updates

OK, this patch works and doesn't break any of the existing tests.  The thing is that I'm not sure how to write a test for it.  Applying the background update can take a long time, which will make the mochitest to timeout (at least intermittently on test machines) and staging an update as a result of a mochitest seems a bit scary.  Robert, can you please let me know what you think about testing this patch?
Comment 125 :Ehsan Akhgari (away Aug 1-5) 2012-04-23 20:09:18 PDT
I just thought of a potential issue for log files.  On Mac and Linux where we store the logs under the app dir, the logs for the replace step will get lost.  Here's what happens:

Once we stage the update, we move the log to installdir/updated/updates/0/last-update.log.  Then, when Firefox gets restarted, the updater app opens installdir/updates/0/update.log and writes the updates to it, them it moves installdir/updated over to installdir, and then closes the log file it had opened.

Now this causes the update.log file written by the updater app during a replace request get unlinked when it's closed.  I can think of two solutions:

1. In replace request mode, make the updater app open the log in an alternate location (installdir/updated/updates/0/update.log).
2. In replace request mode, make the updater app open the log in installdir/updated/updates/0/last-update.log in append mode and write to it.

Both of these solutions should be easy to implement, but #2 has the advantage that last-update.log would mean the same thing as before (really, even though we split the update process into two chunks, they're still part of the same logical operation, and if we wanna take a look at an update log to see what went wrong, we need both pieces of log.)  If we decide to take this path, we should probably make updater to append to last-update.log on Windows as well (in replace request mode of course.)

Robert, what do you think?
Comment 126 :Ehsan Akhgari (away Aug 1-5) 2012-04-25 16:27:44 PDT
Created attachment 618478 [details] [diff] [review]
Append to the update log when performing a replace request
Comment 127 Ian Melven :imelven 2012-04-26 10:12:55 PDT
After discussing with curtisk, marking sec-review-complete

See https://wiki.mozilla.org/Security/Reviews/BackGroundUpdates

Ehsan, please let curtisk know if this has significantly changed since the security review and needs to be looked at again
Comment 128 :Ehsan Akhgari (away Aug 1-5) 2012-04-26 10:47:38 PDT
(In reply to Ian Melven :imelven from comment #127)
> Ehsan, please let curtisk know if this has significantly changed since the
> security review and needs to be looked at again

No, there have been no significant changes.
Comment 129 Brian R. Bondy [:bbondy] 2012-05-01 11:29:12 PDT
Comment on attachment 604211 [details] [diff] [review]
Test changes (v3)

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

::: toolkit/mozapps/update/test/unit/head_update.js.in
@@ +448,5 @@
> +    var file = aDir.clone();
> +    file.append(kLockFileName);
> +    file.create(file.NORMAL_FILE_TYPE, 0444);
> +    file.QueryInterface(AUS_Ci.nsILocalFileWin);
> +    file.fileAttributesWin |= file.WFA_READONLY;

Should we be clearing file.WFA_READWRITE here in case it is set?

@@ +459,5 @@
> +  function unlockDirectory(aDir) {
> +    var file = aDir.clone();
> +    file.append(kLockFileName);
> +    file.QueryInterface(AUS_Ci.nsILocalFileWin);
> +    file.fileAttributesWin |= file.WFA_READWRITE;

Should we be clearing file.WFA_READONLY here in case it is set?

@@ +1275,5 @@
>  /**
>   * Helper function for updater binary tests for verifying the state of files and
>   * directories after a failed update.
> + *
> + * @aGetDirectory: the function used to get the files in the target directory.

nit:
* @param aGetDirectory the function used to get the files in the target directory.
Comment 130 Brian R. Bondy [:bbondy] 2012-05-01 11:29:27 PDT
Comment on attachment 605640 [details] [diff] [review]
Code changes (v4)

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

::: browser/base/content/aboutDialog.js
@@ +576,5 @@
> +            self.setupUpdateButton("update.restart." +
> +                                   (self.isMajor ? "upgradeButton" : "restartButton"));
> +            timer.cancel();
> +            timer = null;
> +          } else if (status == "failed") {

Not sure if there is anything specific to do here for service specific errors where we normally fallback in those cases.

::: toolkit/components/maintenanceservice/workmonitor.cpp
@@ +121,5 @@
> + * @param argvTmp    The argv value normally sent to updater.exe
> + * @param aResultDir Buffer to hold the installation directory.
> + */
> +BOOL
> +GetInstallationDir(int argcTmp, LPWSTR *argvTmp, WCHAR aResultDir[2 * MAX_PATH])

Why does aResultDir need to be 2*MAX_PATH?

@@ +315,5 @@
> +  WCHAR installDir[2 * MAX_PATH] = {L'\0'};
> +  if (!GetInstallationDir(argc, argv, installDir)) {
> +    LOG(("Could not get the installation directory"));
> +    if (!WriteStatusFailure(argv[1],
> +                            SERVICE_INSTALLDIR_ERROR)) {

You need to add SERVICE_INSTALLDIR_ERROR to toolkit\mozapps\update\nsUpdateService.js along with the other error codes. Then use the fallback if that error is hit.  See the other service specific error codes in that file.

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2467,5 @@
>        }
>  
> +      if (argc > callbackIndex && !usingService) {
> +        LaunchCallbackApp(argv[4], argc - callbackIndex,
> +                          argv + callbackIndex, false);

Don't we want to call this even when usingService is true to set the env variable etc?  I know you changed this specifically so I'm sure there's a good reason just wondering for my own knowledge.

::: toolkit/mozapps/update/updater/win_dirent.cpp
@@ +45,5 @@
> +
> +static dirent gDirEnt;
> +
> +DIR::DIR(const WCHAR* path)
> +  : findHandle(NULL)

Init this to INVALID_HANDLE_VALUE since FindFirstFile returns INVALID_HANDLE_VALUE on error

@@ +54,5 @@
> +}
> +
> +DIR::~DIR()
> +{
> +  if (findHandle) {

if (findHandle != INVALID_HANDLE_VALUE_ {

@@ +80,5 @@
> +
> +dirent* readdir(DIR* dir)
> +{
> +  WIN32_FIND_DATAW data;
> +  if (dir->findHandle) {

if (dir->findHandle != INVALID_HANDLE_VALUE) {

@@ +96,5 @@
> +      if (GetLastError() == ERROR_FILE_NOT_FOUND) {
> +        errno = ENOENT;
> +      } else {
> +        errno = EBADF;
> +      }

I think you should set dir->findHandle here too in case someone calls readdir twice.

::: xpcom/io/nsLocalFileWin.cpp
@@ +2987,5 @@
> +    if (aAttribs & WFA_READONLY) {
> +      dwAttrs |= FILE_ATTRIBUTE_READONLY;
> +    } else if ((aAttribs & WFA_READWRITE) &&
> +               (dwAttrs & FILE_ATTRIBUTE_READONLY)) {
> +      dwAttrs &= ~FILE_ATTRIBUTE_READONLY;

Maybe these flags should also be set for the call to GetFileAttributes.  As a side note I hate how we use Get/set for file attributes. It should always get/set all values.
Comment 131 Brian R. Bondy [:bbondy] 2012-05-01 11:29:55 PDT
Comment on attachment 615880 [details] [diff] [review]
Address review comments

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

::: toolkit/mozapps/update/test/unit/head_update.js.in
@@ +102,5 @@
>  const UPDATER_BIN_FILE = "updater" + BIN_SUFFIX;
>  const MAINTENANCE_SERVICE_BIN_FILE = "maintenanceservice.exe";
>  const MAINTENANCE_SERVICE_INSTALLER_BIN_FILE = "maintenanceservice_installer.exe";
>  const UPDATE_SETTINGS_INI_FILE = "update-settings.ini";
> +const UPDATE_SETTINGS_CONTENTS = "[Settings]\nACCEPTED_MAR_CHANNEL_IDS=xpcshell-test\n"

This is already fixed in Bug 735970, but it is waiting review.

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2022,5 @@
>  
>      if (rv == OK) {
> +      NS_tchar installDir[MAXPATHLEN];
> +      if (!GetInstallationDir(installDir)) {
> +        rv = UNEXPECTED_ERROR;

I would prefer to use a new error code here.  The telemetry status codes already have room for new ones so that won't need to be adjusted. Just add a new one to toolkit\mozapps\update\common\errors.h.  There's a bug somewhere to get rid of UNEXPECTED_ERROR completely.
Comment 132 Brian R. Bondy [:bbondy] 2012-05-01 11:30:21 PDT
Comment on attachment 616251 [details] [diff] [review]
Preserve the last-update.log file after background updates

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

::: toolkit/mozapps/update/nsUpdateService.js
@@ +590,5 @@
> + * background updates.
> + * @return The active updates directory inside the updated directory, as a
> + *         nsIFile object.
> + */
> +function getUpdatesDirInApplyToDir() {

Not sure if you need an if !PREF_APP_UPDATE_BACKGROUND then return f.parent.parent in here
Comment 133 Brian R. Bondy [:bbondy] 2012-05-01 11:30:42 PDT
Comment on attachment 617035 [details] [diff] [review]
Prompt when updates have been applied

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

::: toolkit/mozapps/update/nsUpdateService.js
@@ +2374,5 @@
>      cleanUpUpdatesDir(true);
> +
> +    // Do this after *everything* else, since it will likely cause the app
> +    // to shut down.
> +    if (update.state == STATE_APPLIED) {

why not || update.status == STATE_APPLIED_SVC here?

::: toolkit/mozapps/update/test/chrome/test_0092_finishedBackground.xul
@@ +38,5 @@
>  
>    reloadUpdateManagerData();
>  
> +  is(gUpdateManager.activeUpdate.state, "applied",
> +     "The active update should have a state of applied");

Maybe there should be another test for service applied state?
Comment 134 Brian R. Bondy [:bbondy] 2012-05-01 11:30:58 PDT
Comment on attachment 617655 [details] [diff] [review]
Make the update wizard aware of background updates

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

::: toolkit/mozapps/update/content/updates.js
@@ +1641,5 @@
> +      if (aData == STATE_APPLIED ||
> +          aData == STATE_APPLIED_SVC) {
> +        gUpdates.wiz.goTo("finished");
> +      } else {
> +        gUpdates.wiz.goTo("errors");

I don't think we need any special handling for service related error codes (that we usually fall back silently for), but just bringing it up in case you think of anything. Maybe restarting the wizard in those cases? (error codes 24-30)
Comment 135 Brian R. Bondy [:bbondy] 2012-05-01 11:31:10 PDT
Comment on attachment 618478 [details] [diff] [review]
Append to the update log when performing a replace request

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

::: toolkit/mozapps/update/common/updatelogging.cpp
@@ +66,5 @@
>  
> +  if (alternateFileName && NS_taccess(logFile, F_OK)) {
> +    NS_tsnprintf(logFile, sizeof(logFile)/sizeof(logFile[0]),
> +      NS_T("%s/%s"), sourcePath, alternateFileName);
> +  }

I would prefer to not have an alternateFileName argument here at all and move this logic to when you call LogInitAppend.  For example this can lead to appending to either file.  But if the logic is outside it is clear that you are appending to a specific file that you want to.

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2196,5 @@
> +#endif
> +                 installDir);
> +#endif
> +
> +    LogInitAppend(logDir, NS_T("last-update.log"), NS_T("update.log"));

Since on Windows this is the directory containing the update information, is this really necessary at all on Windows?  Maybe to keep things consistent but there are already other differences per platform.
Comment 136 :Ehsan Akhgari (away Aug 1-5) 2012-05-01 12:55:03 PDT
Comment on attachment 605640 [details] [diff] [review]
Code changes (v4)

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

::: browser/base/content/aboutDialog.js
@@ +576,5 @@
> +            self.setupUpdateButton("update.restart." +
> +                                   (self.isMajor ? "upgradeButton" : "restartButton"));
> +            timer.cancel();
> +            timer = null;
> +          } else if (status == "failed") {

Yeah, I'm not sure if we need to do anything specific either.

::: toolkit/components/maintenanceservice/workmonitor.cpp
@@ +121,5 @@
> + * @param argvTmp    The argv value normally sent to updater.exe
> + * @param aResultDir Buffer to hold the installation directory.
> + */
> +BOOL
> +GetInstallationDir(int argcTmp, LPWSTR *argvTmp, WCHAR aResultDir[2 * MAX_PATH])

Oh this is left-over from a previous iteration of this patch.  I'll fix this.

::: xpcom/io/nsLocalFileWin.cpp
@@ +2987,5 @@
> +    if (aAttribs & WFA_READONLY) {
> +      dwAttrs |= FILE_ATTRIBUTE_READONLY;
> +    } else if ((aAttribs & WFA_READWRITE) &&
> +               (dwAttrs & FILE_ATTRIBUTE_READONLY)) {
> +      dwAttrs &= ~FILE_ATTRIBUTE_READONLY;

I didn't want to add more than necessary to nsLocalFile here, but I'll do this if Robert wants me to.

And agreed on the second part.
Comment 137 :Ehsan Akhgari (away Aug 1-5) 2012-05-01 13:19:45 PDT
Created attachment 620046 [details] [diff] [review]
Addressed comment 129 and 130
Comment 138 Biju 2012-05-02 20:23:06 PDT
(please advice, if I am making too much noise here)

Comment on attachment 605640 [details] [diff] [review]


    if (aAttribs & WFA_READONLY) {
      dwAttrs |= FILE_ATTRIBUTE_READONLY;
    } else if (aAttribs & WFA_READWRITE) {
      dwAttrs &= ~FILE_ATTRIBUTE_READONLY;
    }

I think above will have the same effect as below

+    if (aAttribs & WFA_READONLY) {
+      dwAttrs |= FILE_ATTRIBUTE_READONLY;
+    } else if ((aAttribs & WFA_READWRITE) &&
+               (dwAttrs & FILE_ATTRIBUTE_READONLY)) {
+      dwAttrs &= ~FILE_ATTRIBUTE_READONLY;
+    }

==============================
Also... 

+        ++s;
+
+      // Copy the string and replace backslashes with forward slashes along the
+      // way.
+      do {
+        if (*s == NS_T('\\'))
+          *d = NS_T('/');
+        else
+          *d = *s;
+        ++s;
+        ++d;
+      } while (*s);
+      *d = NS_T('\0');

----------- can be rewriten as 

     // Copy the string and replace backslashes with forward slashes along the
     // way.
      ++s;
      do {
        *d = *s == NS_T('\\') ?  NS_T('/') : *s;
        ++s;
        ++d;
      } while (*s);
      *d = NS_T('\0');

----------- or just as

     // Copy the string and replace backslashes with forward slashes along the
     // way.
      while(*d = *(++s)){
        if (*d == NS_T('\\')) *d = NS_T('/');          
        ++d;
      };


==============================
and....


+      if (GetLastError() == ERROR_FILE_NOT_FOUND) {
+        errno = ENOENT;
+      } else {
+        errno = EBADF;
+      }

----------- as

        errno = GetLastError() == ERROR_FILE_NOT_FOUND ? ENOENT : EBADF;
Comment 139 :Ehsan Akhgari (away Aug 1-5) 2012-05-03 11:54:37 PDT
Created attachment 620800 [details] [diff] [review]
Add an error code for the case when GetInstallationDir fails

(In reply to Brian R. Bondy [:bbondy] from comment #131)
> Comment on attachment 615880 [details] [diff] [review]
> Address review comments
> 
> Review of attachment 615880 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/test/unit/head_update.js.in
> @@ +102,5 @@
> >  const UPDATER_BIN_FILE = "updater" + BIN_SUFFIX;
> >  const MAINTENANCE_SERVICE_BIN_FILE = "maintenanceservice.exe";
> >  const MAINTENANCE_SERVICE_INSTALLER_BIN_FILE = "maintenanceservice_installer.exe";
> >  const UPDATE_SETTINGS_INI_FILE = "update-settings.ini";
> > +const UPDATE_SETTINGS_CONTENTS = "[Settings]\nACCEPTED_MAR_CHANNEL_IDS=xpcshell-test\n"
> 
> This is already fixed in Bug 735970, but it is waiting review.

This has vanished in the latest merge.

> ::: toolkit/mozapps/update/updater/updater.cpp
> @@ +2022,5 @@
> >  
> >      if (rv == OK) {
> > +      NS_tchar installDir[MAXPATHLEN];
> > +      if (!GetInstallationDir(installDir)) {
> > +        rv = UNEXPECTED_ERROR;
> 
> I would prefer to use a new error code here.  The telemetry status codes
> already have room for new ones so that won't need to be adjusted. Just add a
> new one to toolkit\mozapps\update\common\errors.h.  There's a bug somewhere
> to get rid of UNEXPECTED_ERROR completely.

Done.
Comment 140 :Ehsan Akhgari (away Aug 1-5) 2012-05-03 12:04:03 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #132)
> Comment on attachment 616251 [details] [diff] [review]
> Preserve the last-update.log file after background updates
> 
> Review of attachment 616251 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/nsUpdateService.js
> @@ +590,5 @@
> > + * background updates.
> > + * @return The active updates directory inside the updated directory, as a
> > + *         nsIFile object.
> > + */
> > +function getUpdatesDirInApplyToDir() {
> 
> Not sure if you need an if !PREF_APP_UPDATE_BACKGROUND then return
> f.parent.parent in here

The caller does this.
Comment 141 :Ehsan Akhgari (away Aug 1-5) 2012-05-03 12:36:30 PDT
Created attachment 620813 [details] [diff] [review]
Add a new test for the applied-service case

(In reply to Brian R. Bondy [:bbondy] from comment #133)
> Comment on attachment 617035 [details] [diff] [review]
> Prompt when updates have been applied
> 
> Review of attachment 617035 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/nsUpdateService.js
> @@ +2374,5 @@
> >      cleanUpUpdatesDir(true);
> > +
> > +    // Do this after *everything* else, since it will likely cause the app
> > +    // to shut down.
> > +    if (update.state == STATE_APPLIED) {
> 
> why not || update.status == STATE_APPLIED_SVC here?

That case can never happen, see earlier code in the function.  :-)

> ::: toolkit/mozapps/update/test/chrome/test_0092_finishedBackground.xul
> @@ +38,5 @@
> >  
> >    reloadUpdateManagerData();
> >  
> > +  is(gUpdateManager.activeUpdate.state, "applied",
> > +     "The active update should have a state of applied");
> 
> Maybe there should be another test for service applied state?

Done.
Comment 142 :Ehsan Akhgari (away Aug 1-5) 2012-05-03 12:45:18 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #134)
> Comment on attachment 617655 [details] [diff] [review]
> Make the update wizard aware of background updates
> 
> Review of attachment 617655 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/content/updates.js
> @@ +1641,5 @@
> > +      if (aData == STATE_APPLIED ||
> > +          aData == STATE_APPLIED_SVC) {
> > +        gUpdates.wiz.goTo("finished");
> > +      } else {
> > +        gUpdates.wiz.goTo("errors");
> 
> I don't think we need any special handling for service related error codes
> (that we usually fall back silently for), but just bringing it up in case
> you think of anything. Maybe restarting the wizard in those cases? (error
> codes 24-30)

I'm not sure, let's wait to hear what Robert thinks.
Comment 143 :Ehsan Akhgari (away Aug 1-5) 2012-05-03 12:56:50 PDT
(In reply to Biju from comment #138)
> (please advice, if I am making too much noise here)
> 
> Comment on attachment 605640 [details] [diff] [review]
> 
> 
>     if (aAttribs & WFA_READONLY) {
>       dwAttrs |= FILE_ATTRIBUTE_READONLY;
>     } else if (aAttribs & WFA_READWRITE) {
>       dwAttrs &= ~FILE_ATTRIBUTE_READONLY;
>     }
> 
> I think above will have the same effect as below
> 
> +    if (aAttribs & WFA_READONLY) {
> +      dwAttrs |= FILE_ATTRIBUTE_READONLY;
> +    } else if ((aAttribs & WFA_READWRITE) &&
> +               (dwAttrs & FILE_ATTRIBUTE_READONLY)) {
> +      dwAttrs &= ~FILE_ATTRIBUTE_READONLY;
> +    }

Sure.  But I don't see what you mean here.

> ==============================
> Also... 
> 
> +        ++s;
> +
> +      // Copy the string and replace backslashes with forward slashes along
> the
> +      // way.
> +      do {
> +        if (*s == NS_T('\\'))
> +          *d = NS_T('/');
> +        else
> +          *d = *s;
> +        ++s;
> +        ++d;
> +      } while (*s);
> +      *d = NS_T('\0');
> 
> ----------- can be rewriten as 
> 
>      // Copy the string and replace backslashes with forward slashes along
> the
>      // way.
>       ++s;
>       do {
>         *d = *s == NS_T('\\') ?  NS_T('/') : *s;
>         ++s;
>         ++d;
>       } while (*s);
>       *d = NS_T('\0');
> 
> ----------- or just as
> 
>      // Copy the string and replace backslashes with forward slashes along
> the
>      // way.
>       while(*d = *(++s)){
>         if (*d == NS_T('\\')) *d = NS_T('/');          
>         ++d;
>       };

Right, but IMO they get less readable that way.  I'm happy to change them if Robert doesn't like my style though.

> +      if (GetLastError() == ERROR_FILE_NOT_FOUND) {
> +        errno = ENOENT;
> +      } else {
> +        errno = EBADF;
> +      }
> 
> ----------- as
> 
>         errno = GetLastError() == ERROR_FILE_NOT_FOUND ? ENOENT : EBADF;

Sure, but still I don't see why this matters.  In C/C++, there are multiple ways of doing the same thing, and unless you see something wrong about the way I do things, I'm not willing to change my coding style just for the sake of it.  :-)
Comment 144 :Ehsan Akhgari (away Aug 1-5) 2012-05-04 15:15:35 PDT
So, I'm getting test failures on the oak branch on Windows while running the service tests (see https://tbpl.mozilla.org/php/getParsedLog.php?id=11449190&tree=Oak&full=1 for example).  These failures started when I did a merge from mozilla-central.  They do not reproduce on my machine locally, and I got access to a test slave, and they do not reproduce there either.  At this point I'm not sure how to debug this...  :/

Brian, do you have any ideas?
Comment 145 Brian R. Bondy [:bbondy] 2012-05-04 16:55:04 PDT
Is it possible to get the log from the C:\ProgramData\Mozilla\logs for a failed run?
Comment 146 :Ehsan Akhgari (away Aug 1-5) 2012-05-07 10:36:35 PDT
Created attachment 621663 [details] [diff] [review]
Fix the version downgrade and product channel check tests on Windows

Now updater.exe returns the correct status code.  This patch also fixes a bug where GetInstallationDir was being used for non-background updates as well.
Comment 147 :Ehsan Akhgari (away Aug 1-5) 2012-05-07 10:37:11 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #145)
> Is it possible to get the log from the C:\ProgramData\Mozilla\logs for a
> failed run?

Hmm, I need to be able to reproduce the bug in order to get the relevant log files, right?  :/
Comment 148 Brian R. Bondy [:bbondy] 2012-05-07 10:45:28 PDT
> Hmm, I need to be able to reproduce the bug in order to get the relevant log 
> files, right?  :/

You said that you had access to a talos machine, presumably that was a machine that originally reproduced it.  You said you couldn't reproduce it anymore on that test machine but luckily we keep more logs than just one log in that directory.  

I thought it would be obvious that I meant from a machine that has reproduced it in the past...
Comment 149 Brian R. Bondy [:bbondy] 2012-05-07 10:47:44 PDT
On a side note, I answered you asking for logs in just over 1.5 hours.  The response back was 3 days later.  I'm not sure what the :/ is about but it seems negative.
Comment 150 :Ehsan Akhgari (away Aug 1-5) 2012-05-07 10:57:01 PDT
Well we only keep 10 logs around.  I have been running tests on this machine at least 30 times by now, so all of those logs are long gone.  Maybe I can try to grab them from another machine...  But that may be tricky since it has to be a machine which has run Oak xpcshell tests and *nothing* else after it.
Comment 151 Brian R. Bondy [:bbondy] 2012-05-07 11:02:06 PDT
I think it would be easy enough to take an extremely recent fail, ask releng for access or to give you the log, and then attach the log.  

When you asked me if I had ideas, I looked at the code, and seen that there are a few different places that it can fail and give that error.  Each of those places are logged, which is why I asked for the log.  If you are against this approach  you can instead temporarily add new error codes to each of those places that we log the error.
Comment 152 :Ehsan Akhgari (away Aug 1-5) 2012-05-07 15:58:44 PDT
I landed a debugging patch <https://hg.mozilla.org/projects/oak/rev/d6e7da21acad> which should cause the service logs to be dumped to the test output unconditionally.  This should be a more realibale way for us to get access to the service logs.
Comment 153 :Ehsan Akhgari (away Aug 1-5) 2012-05-07 16:17:48 PDT
Note that these test failures seem to be intermittent, as I sometimes do get green runs (for example, https://tbpl.mozilla.org/php/getParsedLog.php?id=11542527&tree=Oak&full=1)
Comment 154 Brian R. Bondy [:bbondy] 2012-05-08 06:01:54 PDT
I took a look at the service logs:
toolkit\components\maintenanceservice\servicebase.cpp

Are these 2 paths as expected?

Path 1: c:\talos-slave\test\build\xpcshell\tests\toolkit\mozapps\update\test_svc\unit\0114_mar\updater.exe
Path 2: c:\talos-slave\test\build\xpcshell\tests\toolkit\mozapps\update\test_svc\unit\0114_applyToDir\updated\updater.exe

If so I think the next step is to put a log message on each of the return FALSE statements from within VerifySameFiles
Comment 155 :Ehsan Akhgari (away Aug 1-5) 2012-05-08 07:51:15 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #154)
> I took a look at the service logs:
> toolkit\components\maintenanceservice\servicebase.cpp
> 
> Are these 2 paths as expected?
> 
> Path 1:
> c:\talos-
> slave\test\build\xpcshell\tests\toolkit\mozapps\update\test_svc\unit\0114_mar
> \updater.exe
> Path 2:
> c:\talos-
> slave\test\build\xpcshell\tests\toolkit\mozapps\update\test_svc\unit\0114_app
> lyToDir\updated\updater.exe
> 
> If so I think the next step is to put a log message on each of the return
> FALSE statements from within VerifySameFiles

The second path name there is not valid.  Note the "updated\" in the path name.  I think what's happening is that a path name with a trailing backslash is passed to GetInstallationDir, which confuses it.  I have pushed a patch which would fix that problem <https://hg.mozilla.org/projects/oak/rev/46cb55310ec9>, to see if it fixes the test failures.  I'll ask for review if it proves to be effective.
Comment 156 :Ehsan Akhgari (away Aug 1-5) 2012-05-09 14:20:43 PDT
Today we found out that the maintenance service is not being replaced with the one provided in the build.  Brian helped fix the bug in bug 749956, and we're waiting for RelEng to deploy the new package, so that I can retest this on Oak.
Comment 157 :Ehsan Akhgari (away Aug 1-5) 2012-05-10 14:21:49 PDT
Created attachment 622916 [details] [diff] [review]
Make sure that the directory passed in argv[2] does not include a trailing backslash

OK, turns out that this patch was indeed effective.  This is the last piece of this bug I think, except for where I'm going to need to address review comments, etc.
Comment 158 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-18 02:20:51 PDT
Comment on attachment 616251 [details] [diff] [review]
Preserve the last-update.log file after background updates

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>@@ -38,16 +38,19 @@
> # 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 ***** */
> */
>+
>+#filter substitution
>+
> Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> Components.utils.import("resource://gre/modules/FileUtils.jsm");
> Components.utils.import("resource://gre/modules/AddonManager.jsm");
> Components.utils.import("resource://gre/modules/Services.jsm");
> Components.utils.import("resource://gre/modules/ctypes.jsm")
> 
> const Cc = Components.classes;
> const Ci = Components.interfaces;
>@@ -109,16 +112,21 @@ const KEY_GRED            = "GreD";
> #define USE_UPDROOT
> #endif
> 
> #ifdef USE_UPDROOT
> const KEY_UPDROOT         = "UpdRootD";
> #endif
> 
> const DIR_UPDATES         = "updates";
>+#ifdef XP_MACOSX
>+const UPDATED_DIR         = "Updated.app";
>+#else
>+const UPDATED_DIR         = "updated";
>+#endif
> const FILE_UPDATE_STATUS  = "update.status";
> const FILE_UPDATE_VERSION = "update.version";
> #ifdef MOZ_WIDGET_ANDROID
> const FILE_UPDATE_ARCHIVE = "update.apk";
> #else
> const FILE_UPDATE_ARCHIVE = "update.mar";
> #endif
> const FILE_UPDATE_LOG     = "update.log"
>@@ -571,16 +579,51 @@ function getStatusTextFromCode(code, def
>  * @return The active updates directory, as a nsIFile object
>  */
> function getUpdatesDir() {
>   // Right now, we only support downloading one patch at a time, so we always
>   // use the same target directory.
>   return getUpdateDirCreate([DIR_UPDATES, "0"]);
> }
> 
>+#ifndef USE_UPDROOT
>+/**
>+ * Get the Active Updates directory inside the directory where we apply the
>+ * background updates.
>+ * @return The active updates directory inside the updated directory, as a
>+ *         nsIFile object.
>+ */
>+function getUpdatesDirInApplyToDir() {
>+  var dir = FileUtils.getDir(KEY_APPDIR, []);
>+#ifdef XP_MACOSX
>+  var bundle = dir.parent.parent; // the bundle directory
>+  if (bundle.leafName != @MOZ_MACBUNDLE_NAME@) {
As stated previously, I would much prefer to just skip the tests instead of adding client code for the non-bundled case. :/

I definitely do not want extra complexity on the client like this. For example, if the person installs Firefox and then changes the bundle name this will break... right?

>+    // xpcshell test being run from dist/bin.
>+    // Attempt to find the bundle directory inside dist/.
>+    bundle = dir.parent;
>+    bundle.append(@MOZ_MACBUNDLE_NAME@);
>+    if (!bundle.exists()) {
>+      do_throw("Cannot find the bundle directory");
do_throw is legal outside of tests?

>+    }
>+  }
>+  dir = bundle;
>+#endif
>+  dir.append(UPDATED_DIR);
>+#ifdef XP_MACOSX
>+  dir.append("Contents");
>+  dir.append("MacOS");
>+#endif
>+  dir.append(DIR_UPDATES);
>+  if (!dir.exists()) {
>+    dir.create(Ci.nsILocalFile.DIRECTORY_TYPE, 0755);
>+  }
>+  return dir;
>+}
>+#endif

>...
>+      var dir;
>       try {
>-        var dir = f.parent.parent;
>+#ifdef USE_UPDROOT
>+        // If we're on a platform which uses the update root directory, the log
>+        // files are written outside of the application directory, so they will
>+        // not get overwritten when we replace the directories after a
>+        // background update.  Therefore, we don't need any special logic for
>+        // that case here.
>+        // Note that this currently only applies to Windows, as we don't use
>+        // background updates on Android.
nit: should be
Note that this currently only applies to Windows.

>+        dir = f.parent.parent;
>+#else
>+        // If we don't use the update root directory, the log files are written
>+        // inside the application directory.  In that case, we want to write
>+        // the log files to the updated directory in the case of background
>+        // updates, so that they would be available when we replace that
>+        // directory with the application directory later on.
>+        if (aBackgroundUpdate) {
>+          dir = getUpdatesDirInApplyToDir();
>+        } else {
>+          dir = f.parent.parent;
>+        }
>+#endif
>         var logFile = dir.clone();
>         logFile.append(FILE_LAST_LOG);
>         if (logFile.exists()) {
>           try {
>             logFile.moveTo(dir, FILE_BACKUP_LOG);
>           }
>           catch (e) {
>             LOG("cleanUpUpdatesDir - failed to rename file " + logFile.path +
>@@ -676,16 +745,20 @@ function cleanUpUpdatesDir() {
>         }
>         f.moveTo(dir, FILE_LAST_LOG);
>         continue;
>       }
>       catch (e) {
>         LOG("cleanUpUpdatesDir - failed to move file " + f.path + " to " +
>             dir.path + " and rename it to " + FILE_LAST_LOG);
>       }
>+    } else if (f.leafName == FILE_UPDATE_STATUS && aBackgroundUpdate) {
>+      // Leave the update.status file alone when a background update
>+      // has been performed.
Please add to the comment how it is handled

>+      continue;
>     }
>     // Now, recursively remove this file.  The recursive removal is really
>     // only needed on Mac OSX because this directory will contain a copy of
>     // updater.app, which is itself a directory.
>     try {
>       f.remove(true);
>     }
>     catch (e) {
>@@ -2292,25 +2365,28 @@ UpdateManager.prototype = {
>     }
>     if (update.state == STATE_APPLIED && shouldUseService()) {
>       writeStatusFile(getUpdatesDir(), update.state = STATE_APPLIED_SVC);
>     }
>     var um = Cc["@mozilla.org/updates/update-manager;1"].
>              getService(Ci.nsIUpdateManager);
>     um.saveUpdates();
> 
>+    // Destroy the updates directory, since we're done with it.
>+    cleanUpUpdatesDir(true);
>+
>     // Do this after *everything* else, since it will likely cause the app
>     // to shut down.
>     if (update.state == STATE_APPLIED) {
>       // Notify the user that an update has been applied and is ready for
>       // installation (i.e. that they should restart the application). We do
>       // not notify on failed update attempts.
>       var prompter = Cc["@mozilla.org/updates/update-prompt;1"].
>                      createInstance(Ci.nsIUpdatePrompt);
>-      prompter.showUpdateApplied(this._update, true);
>+      prompter.showUpdateApplied(update, true);
There have been problems before update no longer being available hence why it uses this._update. Did you fix this elsewhere?
Comment 159 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-18 02:32:52 PDT
Comment on attachment 617655 [details] [diff] [review]
Make the update wizard aware of background updates

>diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js
>--- a/toolkit/mozapps/update/content/updates.js
>+++ b/toolkit/mozapps/update/content/updates.js
>@@ -45,16 +45,17 @@ Components.utils.import("resource://gre/
> // Firefox's macBrowserOverlay.xul includes scripts that define Cc, Ci, and Cr
> // so we have to use different names.
> const CoC = Components.classes;
> const CoI = Components.interfaces;
> const CoR = Components.results;
> 
> const XMLNS_XUL               = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> 
>+const PREF_APP_UPDATE_BACKGROUND         = "app.update.background.enabled";
How about app.update.stage.enabled instead. That way it won't get confused with downloading the update in the background which is already in use.

>@@ -1248,16 +1249,21 @@ var gDownloadingPage = {
>    */
>   _lastSec: Infinity,
>   _startTime: null,
>   _pausedStatus: "",
> 
>   _hiding: false,
> 
>   /**
>+   * Have we registered an observer for a background update being staged
>+   */
>+  _updateApplyingObserver: false,
>+
>+  /**
>    * Initialize
>    */
>   onPageShow: function() {
>     this._downloadStatus = document.getElementById("downloadStatus");
>     this._downloadProgress = document.getElementById("downloadProgress");
>     this._pauseButton = document.getElementById("pauseButton");
>     this._label_downloadStatus = this._downloadStatus.textContent;
> 
>@@ -1382,22 +1388,40 @@ var gDownloadingPage = {
>       this._pauseButton.setAttribute("paused", "false");
>       this._pauseButton.setAttribute("tooltiptext",
>                                      gUpdates.getAUSString("pauseButtonPause"));
>       this._setStatus(this._label_downloadStatus);
>     }
>   },
> 
>   /**
>+   * Wait for an update being staged in the background.
>+   */
>+  _setUpdateApplying: function() {
>+    this._downloadProgress.mode = "undetermined";
>+    this._pauseButton.hidden = true;
>+    let applyingStatus = gUpdates.getAUSString("applyingUpdate");
>+    this._setStatus(applyingStatus);
>+
>+    Services.obs.addObserver(this, "background-update-staged", false);
How about update-staged instead? I think this makes much more sense due to the term background already being used for the case where the update is downloaded in the background whether it is staged or not and staging can happen for both a foreground update with UI or a background update.

>+    this._updateApplyingObserver = true;
>+  },
>+
>+  /**
>    * Removes the download listener.
>    */
>   removeDownloadListener: function() {
nit: the name is no longer correct but I am ok with it to get this landed as long as the comment above is updated.

>     var aus = CoC["@mozilla.org/updates/update-service;1"].
>               getService(CoI.nsIApplicationUpdateService);
>     aus.removeDownloadListener(this);
>+
>+    if (this._updateApplyingObserver) {
>+      Services.obs.removeObserver(this, "background-update-staged");
>+      this._updateApplyingObserver = false;
>+    }
>   },
> 
>   /**
>    * When the user clicks the Pause/Resume button
>    */
>   onPause: function() {
>     var aus = CoC["@mozilla.org/updates/update-service;1"].
>               getService(CoI.nsIApplicationUpdateService);

r=me with those changes
Comment 160 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-18 02:42:35 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #142)
> (In reply to Brian R. Bondy [:bbondy] from comment #134)
> > Comment on attachment 617655 [details] [diff] [review]
> > Make the update wizard aware of background updates
> > 
> > Review of attachment 617655 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/mozapps/update/content/updates.js
> > @@ +1641,5 @@
> > > +      if (aData == STATE_APPLIED ||
> > > +          aData == STATE_APPLIED_SVC) {
> > > +        gUpdates.wiz.goTo("finished");
> > > +      } else {
> > > +        gUpdates.wiz.goTo("errors");
> > 
> > I don't think we need any special handling for service related error codes
> > (that we usually fall back silently for), but just bringing it up in case
> > you think of anything. Maybe restarting the wizard in those cases? (error
> > codes 24-30)
> 
> I'm not sure, let's wait to hear what Robert thinks.
I believe this is only for the case where the staging of the update fails and if that is the case it should just do what it does today when an update has successfully downloaded which is go to the finish page.
Comment 161 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-18 02:53:11 PDT
Comment on attachment 618478 [details] [diff] [review]
Append to the update log when performing a replace request

I'm a tad confused as to why last-update.log is being used at all. What we had talked about is that after an update is staged the update.log will contain the operations, etc. used to apply the update. When the update is copied from the staged location to the final destination the operations logged to accomplish this should be appended to the update.log.

Minusing for clarification regarding the above.
Comment 162 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-18 03:02:42 PDT
Comment on attachment 621663 [details] [diff] [review]
Fix the version downgrade and product channel check tests on Windows

Please fix up the comment as to why the exitValue when using USE_EXECV is 0 and 1 otherwise.
Comment 163 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-18 03:09:06 PDT
Comment on attachment 604211 [details] [diff] [review]
Test changes (v3)

The tests are fine except for the changes needed for not running except when in a bundle. I'll review a followup patch to handle the changes needed.
Comment 164 :Ehsan Akhgari (away Aug 1-5) 2012-05-18 08:55:16 PDT
Created attachment 625131 [details] [diff] [review]
Rename the pref
Comment 165 :Ehsan Akhgari (away Aug 1-5) 2012-05-18 08:59:31 PDT
Created attachment 625134 [details] [diff] [review]
Rename the notification
Comment 166 :Ehsan Akhgari (away Aug 1-5) 2012-05-18 09:10:52 PDT
(In reply to Robert Strong [:rstrong] (do not email) from comment #158)
> >     // Do this after *everything* else, since it will likely cause the app
> >     // to shut down.
> >     if (update.state == STATE_APPLIED) {
> >       // Notify the user that an update has been applied and is ready for
> >       // installation (i.e. that they should restart the application). We do
> >       // not notify on failed update attempts.
> >       var prompter = Cc["@mozilla.org/updates/update-prompt;1"].
> >                      createInstance(Ci.nsIUpdatePrompt);
> >-      prompter.showUpdateApplied(this._update, true);
> >+      prompter.showUpdateApplied(update, true);
> There have been problems before update no longer being available hence why
> it uses this._update. Did you fix this elsewhere?

I originally copied this code from the Downloader object's code.  this._update is a property of the Downloader object.  This is a method of the UpdateManager object, however.  So effectively this is just me fixing a typo (which the tests caught).  :-)

I'll address the rest of comment 158 in other patches.
Comment 167 :Ehsan Akhgari (away Aug 1-5) 2012-05-18 09:17:34 PDT
Created attachment 625136 [details] [diff] [review]
Fix the review nits

Nothing really interesting here, this just adds comments and renames a method.  Asking for review to be on the safe side.
Comment 168 :Ehsan Akhgari (away Aug 1-5) 2012-05-18 09:41:07 PDT
Created attachment 625142 [details] [diff] [review]
Remove the Mac specific hacks used for getting xpcshell tests to work
Comment 169 :Ehsan Akhgari (away Aug 1-5) 2012-05-18 10:00:28 PDT
Comment on attachment 618478 [details] [diff] [review]
Append to the update log when performing a replace request

(In reply to Robert Strong [:rstrong] (do not email) from comment #161)
> Comment on attachment 618478 [details] [diff] [review]
> Append to the update log when performing a replace request
> 
> I'm a tad confused as to why last-update.log is being used at all. What we
> had talked about is that after an update is staged the update.log will
> contain the operations, etc. used to apply the update. When the update is
> copied from the staged location to the final destination the operations
> logged to accomplish this should be appended to the update.log.
> 
> Minusing for clarification regarding the above.

The update.log file that is written down as part of staging the update will be moved to last-update.log once the update is staged.  This is the reason we open the last-update.log file when performing a replace.  Everything is done according to what we talked about here.  :-)

(Looking at the tests and the expected log files will probably make this a bit clearer.)

Resetting the review flag.
Comment 170 :Ehsan Akhgari (away Aug 1-5) 2012-05-18 11:45:43 PDT
Created attachment 625183 [details] [diff] [review]
Fix the review nits

I replaced one too many removeDownloadListener's in the previous patch.
Comment 171 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-18 13:35:09 PDT
Comment on attachment 618478 [details] [diff] [review]
Append to the update log when performing a replace request

I am ok with this and if we want to change this it can be done in other bugs.
Comment 172 :Ehsan Akhgari (away Aug 1-5) 2012-05-22 07:51:37 PDT
http://hg.mozilla.org/mozilla-central/rev/c20d415ef1b5
Comment 173 Willy_ Foo_Foo 2012-05-22 10:21:22 PDT
Will the be also available in firefox on linux?
And what about portable versions on windows?
Comment 174 :Ehsan Akhgari (away Aug 1-5) 2012-05-22 10:24:07 PDT
(In reply to magnumarchonbasileus from comment #173)
> Will the be also available in firefox on linux?

Yes.

> And what about portable versions on windows?

If the people building those version don't disable it, then yes.  But I don't know how they handle updates, and if they use the built-in updater.
Comment 175 Willy_ Foo_Foo 2012-05-22 10:25:00 PDT
in our daily use we(office workers) use portable versions(ie6 on the machine)
So only option is using Firefox Portable from portable apps
so will this effect that too(non admin user)
because partial update does not install in portable version before as well
Comment 176 Willy_ Foo_Foo 2012-05-22 10:28:16 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #174)

> If the people building those version don't disable it, then yes.  But I
> don't know how they handle updates, and if they use the built-in updater.

Portable Apps allows portable version to update(update is downloaded in ff folder)
but the partial update fails(try nightly on portable apps)
only full update applies(very annoying)
Comment 177 Lawrence Mandel [:lmandel] (use needinfo) 2012-05-22 10:32:40 PDT
magnumarchonbasileus - Your issues with portable apps sound like a good candidate for one or more new bugs. Care to file them so that we can understand and evaluate the issues?
Comment 178 Willy_ Foo_Foo 2012-05-22 10:40:34 PDT
Just a summary

Download portable app
http://portableapps.com/apps/internet/firefox_portable/test
install
replace nightly data(zip/ installed folder)
inside portable app folder
\App\Firefox

Try partial update


error Cannot apply update downloading full update

P.S- install in other partitions(say from xp in E:)
restart & Open win 7 and run FF from the directory & when update available check for it & apply

error

error Cannot apply update downloading full update
Comment 179 Willy_ Foo_Foo 2012-05-22 10:49:04 PDT
going out to catch my flight will mail you once i file the bug when i return
till then you can try the way i explained to see for your self
thankyou
Comment 180 Willy_ Foo_Foo 2012-05-22 10:49:45 PDT
going out to catch my flight will mail you once i file the bug when i return
till then you can try the way i explained to see for your self
thankyou
Comment 181 :Ehsan Akhgari (away Aug 1-5) 2012-05-22 11:40:48 PDT
For future reference, I took a copy of the Oak branch here: <http://hg.mozilla.org/users/eakhgari_mozilla.com/bgupdates-oak/>.
Comment 182 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-22 12:12:44 PDT
(In reply to Lawrence Mandel [:lmandel] from comment #177)
> magnumarchonbasileus - Your issues with portable apps sound like a good
> candidate for one or more new bugs. Care to file them so that we can
> understand and evaluate the issues?
Portable Firefox changes some files we update and we require that the files we update with a partial update to be the same as the files we distribute. Nothing we can do here though Portable Firefox could likely not change the files.
Comment 183 Lawrence Mandel [:lmandel] (use needinfo) 2012-05-22 12:55:49 PDT
(In reply to Robert Strong [:rstrong] (do not email) from comment #182)
> (In reply to Lawrence Mandel [:lmandel] from comment #177)
> > magnumarchonbasileus - Your issues with portable apps sound like a good
> > candidate for one or more new bugs. Care to file them so that we can
> > understand and evaluate the issues?
> Portable Firefox changes some files we update and we require that the files
> we update with a partial update to be the same as the files we distribute.
> Nothing we can do here though Portable Firefox could likely not change the
> files.

Fair enough. In any case, this discussion seems out of scope for this bug.
Comment 184 Willy_ Foo_Foo 2012-05-22 22:46:36 PDT
(In reply to Robert Strong [:rstrong] (do not email) from comment #182)
> Portable Firefox changes some files we update and we require that the files
> we update with a partial update to be the same as the files we distribute.
> Nothing we can do here though Portable Firefox could likely not change the
> files.

Fine but Full update does not fail
& after the full update the files are restored(original)
then partial update should be available
Contacted Portableapss developers, they said its a Firefox problem
Comment 185 Willy_ Foo_Foo 2012-05-22 22:47:57 PDT
(In reply to Lawrence Mandel [:lmandel] from comment #183)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #182)
> Fair enough. In any case, this discussion seems out of scope for this bug.

Yes it is the new update mechanism make it more difficult to update
Comment 186 Willy_ Foo_Foo 2012-05-22 22:50:50 PDT
(In reply to Robert Strong [:rstrong] (do not email) from comment #182)
> (In reply to Lawrence Mandel [:lmandel] from comment #177)

Its not just limited to portable apps
try this
install in other partitions(say from XP/8 in E:)
restart & Open win 7 and run FF from the directory & when update available check for it & apply

error

error Cannot apply update downloading full update

Here the original Files are installed but still the problem occurs
Comment 187 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-23 12:12:24 PDT
(In reply to magnumarchonbasileus from comment #186)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #182)
> > (In reply to Lawrence Mandel [:lmandel] from comment #177)
> 
> Its not just limited to portable apps
> try this
> install in other partitions(say from XP/8 in E:)
> restart & Open win 7 and run FF from the directory & when update available
> check for it & apply
> 
> error
> 
> error Cannot apply update downloading full update
> 
> Here the original Files are installed but still the problem occurs
Fine though I am unable to reproduce the problem you are experiencing.

Please file a new bug (all future discussion should be in the new bug you have filed) and do the following immediately after you experience the problem when using a build other than a nightly at this time. If you want to use a nightly to report this then do so *after* the fallout (other new bugs) from this bug have been resolved.

Please attach (using the 'Add an attachment' link above) the following files. They should be located in the installation directory.

navigate to the updates subdirectory
last-update.log and backup-update.log

navigate to the 0 subdirectory under updates
update.status and if present update.log
Comment 188 :Ehsan Akhgari (away Aug 1-5) 2012-05-23 15:35:37 PDT
FWIW, this is preffed off in bug 757971 for now.
Comment 189 :Ehsan Akhgari (away Aug 1-5) 2012-05-23 20:40:00 PDT
I filed bug 758101 to track the work remaining to re-enable background updates on mozilla-central.
Comment 190 neil@parkwaycc.co.uk 2012-05-25 12:15:23 PDT
Comment on attachment 605640 [details] [diff] [review]
Code changes (v4)

>+    // We need argv[0] to point to the current executable's name.  The rest of
>+    // the entries in this array will be ignored if argc<2.  Therefore, for
>+    // xpcshell, we only fill out that item, and leave the rest empty.
>+    argc = 1;
>+    nsCAutoString binPath;
>+    nsCOMPtr<nsIFile> binary;
>+    rv = ds->Get(XRE_EXECUTABLE_FILE, NS_GET_IID(nsIFile),
>+                 getter_AddRefs(binary));
>+    NS_ASSERTION(NS_SUCCEEDED(rv), "Can't get the binary path");
>+    binary->GetNativePath(binPath);
>+    char* binPathCString = const_cast<char*> (nsPromiseFlatCString(binPath).get());
>+    argv = &binPathCString;
>+  }
So, my compiler drew my attention to this code...
* nsPromiseFlatCString is an implementation detail
* binPath is flat, and doesn't need to be promised
* Use BeginWriting to get a char* rather than const_cast<>ing
* binPath goes out of scope, so that *binPathCString is a use-after-free
As if 4 errors in one line wasn't enough:
* binPathCString goes out of scope, so that *argv is a use-after-free
Also, you uselessly use PromiseFlatString(mInfo.mAppVersion) later on.
Comment 191 :Ehsan Akhgari (away Aug 1-5) 2012-05-25 13:00:24 PDT
(In reply to neil@parkwaycc.co.uk from comment #190)
> Comment on attachment 605640 [details] [diff] [review]
> Code changes (v4)
> 
> >+    // We need argv[0] to point to the current executable's name.  The rest of
> >+    // the entries in this array will be ignored if argc<2.  Therefore, for
> >+    // xpcshell, we only fill out that item, and leave the rest empty.
> >+    argc = 1;
> >+    nsCAutoString binPath;
> >+    nsCOMPtr<nsIFile> binary;
> >+    rv = ds->Get(XRE_EXECUTABLE_FILE, NS_GET_IID(nsIFile),
> >+                 getter_AddRefs(binary));
> >+    NS_ASSERTION(NS_SUCCEEDED(rv), "Can't get the binary path");
> >+    binary->GetNativePath(binPath);
> >+    char* binPathCString = const_cast<char*> (nsPromiseFlatCString(binPath).get());
> >+    argv = &binPathCString;
> >+  }
> So, my compiler drew my attention to this code...
> * nsPromiseFlatCString is an implementation detail
> * binPath is flat, and doesn't need to be promised
> * Use BeginWriting to get a char* rather than const_cast<>ing
> * binPath goes out of scope, so that *binPathCString is a use-after-free
> As if 4 errors in one line wasn't enough:
> * binPathCString goes out of scope, so that *argv is a use-after-free
> Also, you uselessly use PromiseFlatString(mInfo.mAppVersion) later on.

Filed bug 758742 for this.
Comment 192 :Ehsan Akhgari (away Aug 1-5) 2012-06-04 08:18:40 PDT
(This is going to be disabled for Windows on Firefox 15 for now...)
Comment 193 Tim Nguyen :ntim (use needinfo?) 2012-06-26 19:42:06 PDT
Still doesn't work for me.
Using :
UX 16.0a1 2012-06-23
Windows Vista home basic
Here's the update process for me :
- Downloading update
- Applying update
- I click on the Restart to update button
- UAC Control (UAC Control doesn't turn off)
- Still got that annoying window with the progress bar
- Takes the same amount of time as before
Comment 194 :Ehsan Akhgari (away Aug 1-5) 2012-06-27 10:45:12 PDT
(In reply to ntim007 from comment #193)
> Still doesn't work for me.
> Using :
> UX 16.0a1 2012-06-23
> Windows Vista home basic
> Here's the update process for me :
> - Downloading update
> - Applying update
> - I click on the Restart to update button
> - UAC Control (UAC Control doesn't turn off)
> - Still got that annoying window with the progress bar
> - Takes the same amount of time as before

I'm not sure what set of my patches are now on the UX branch...  Can you please test using a regular nightly that you can grab from http://nightly.mozilla.org?  Thanks!
Comment 195 Worcester12345 2012-08-06 10:02:52 PDT
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0 ID:20120724191344

Still stuck on the above version. It is saying: "You are currently on the auroroa update channel. Change it!"

It spins its wheels and TRIES to update, but in the end it fails and sends me to download it. The worst part is, the download link is to the Android version. Clicking "Desktop" (the only other available "tab"), it says it is "up to date". 

Thanks.
Comment 196 Jesse Ruderman 2012-08-07 10:44:44 PDT
Please file a new bug report if things aren't working for you. Thanks!

Note You need to log in before you can comment on or make changes to this bug.