Closed Bug 307181 (bgupdates) Opened 19 years ago Closed 12 years ago

Eliminate wait while restarting Firefox after update (apply update in background)

Categories

(Toolkit :: Application Update, enhancement)

x86
Windows XP
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox15 --- fixed

People

(Reporter: jruderman, Assigned: ehsan.akhgari)

References

(Depends on 2 open bugs, Blocks 2 open bugs, Regressed 1 open bug, )

Details

(Keywords: user-doc-needed, Whiteboard: [parity-chrome])

Attachments

(18 files, 30 obsolete files)

217.60 KB, image/png
Details
215.83 KB, image/png
Details
283.59 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
155.07 KB, patch
robert.strong.bugs
: review-
Details | Diff | Splinter Review
34.82 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
22.42 KB, patch
robert.strong.bugs
: review-
Details | Diff | Splinter Review
5.74 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
7.09 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
31.40 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
8.88 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
3.17 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
2.87 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
3.39 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
1.35 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
6.88 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
3.02 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
5.79 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
9.21 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
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.
> 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 ;-)
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.
I don't think this will be changed in the Firefox 1.5 timeframe.
Target Milestone: --- → Future
Version: 1.5 Branch → Trunk
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).
Target Milestone: Future → Firefox 2 beta1
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?
Target Milestone should probably be cleared or set to Future, Firefox 2 beta1 was a long time ago :)
Product: Firefox → Toolkit
Target Milestone: mozilla1.8.1beta1 → ---
Blocks: 489138
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.
Summary: Eliminate wait while restarting Firefox after update → Eliminate wait while restarting Firefox after update (apply update in background)
Whiteboard: [parity-chrome]
Assignee: nobody → ehsan
Keywords: dev-doc-needed
Keywords: dev-doc-needed
Depends on: 692887
Alias: bgupdates
sec-review sched for 11:00 PST Oct-21
I'm working on a prototype here: <https://github.com/ehsan/mozilla-central/tree/bgupdate-proto>
Depends on: 696891
Attached patch WIP 3 (obsolete) — Splinter Review
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.
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
Attached patch WIP 4 (obsolete) — Splinter Review
This version of the patch fixes a few build problems, makes the unit tests green, and fixes a race condition.
Attachment #569235 - Attachment is obsolete: true
Attached patch WIP 5 (obsolete) — Splinter Review
This version should build successfully on Windows, and the tests should be green on Linux as well.
Attachment #569556 - Attachment is obsolete: true
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.
Attached patch WIP 6 (obsolete) — Splinter Review
This version of the patch mostly works fine on Windows.  There are still a number of test issues which I need to sort out...
Attachment #570024 - Attachment is obsolete: true
Attached patch WIP7 (obsolete) — Splinter Review
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.
Attachment #570728 - Attachment is obsolete: true
Attached patch WIP 8 (obsolete) — Splinter Review
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.
Attachment #570871 - Attachment is obsolete: true
Depends on: 481815
Attached patch WIP 9 (obsolete) — Splinter Review
New in this version is the About dialog UI, and more fixes on the tests, with yet more fixes to come.
Attachment #571172 - Attachment is obsolete: true
Attached patch WIP 10 (obsolete) — Splinter Review
This version of the patch should pass all tests on all platforms.  More tests to come.
Attachment #572050 - Attachment is obsolete: true
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.
Attachment #572817 - Flags: ui-review?(limi)
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.
Attachment #572818 - Flags: ui-review?(limi)
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?"
(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.
Target Milestone: --- → mozilla10
Target Milestone: mozilla10 → ---
Attached patch WIP 11 (obsolete) — Splinter Review
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.
Attachment #572687 - Attachment is obsolete: true
It sounds like there is some overlap between this and Bug 481815.
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.
Attachment #572818 - Flags: ui-review?(limi) → ui-review+
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).
Attachment #572817 - Flags: ui-review?(limi) → ui-review+
(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?
(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!
(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.
Blocks: 701375
Attached patch WIP 12 (obsolete) — Splinter Review
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.
Attachment #573297 - Attachment is obsolete: true
Attached patch WIP 13 (obsolete) — Splinter Review
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.
Attachment #573726 - Attachment is obsolete: true
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.
(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.
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.
Attached patch WIP 14 (obsolete) — Splinter Review
This patch moves the updater process initialization to a background thread, in preparation of my work being rebased on top of Brian's.
Attachment #573943 - Attachment is obsolete: true
Attached patch WIP 15 (obsolete) — Splinter Review
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.
Attachment #574479 - Attachment is obsolete: true
Attached patch Patch (v1) (obsolete) — Splinter Review
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.
Attachment #576084 - Flags: review?(netzen)
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.
Depends on: 594474
Depends on: 704591
No longer depends on: 594474
Attached patch Patch (v2) (obsolete) — Splinter Review
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.
Attachment #576084 - Attachment is obsolete: true
Attachment #576084 - Flags: review?(netzen)
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.
(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"
Attached patch Patch (v1) (obsolete) — Splinter Review
This patch refactors the tests.  This refactoring is needed for creating tests which use the maintenance service.
Attachment #576315 - Attachment is obsolete: true
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).
Thanks Jesse & Robert for that re-assuring info.  Sorry for wrong location. (newbie, <sigh> )
Attached patch Patch (v1.1) (obsolete) — Splinter Review
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.
Attachment #576665 - Attachment is obsolete: true
Attachment #575045 - Attachment is obsolete: true
Attached patch Code changes (v1) (obsolete) — Splinter Review
I've split the tests into a separate patch.
Attachment #584133 - Attachment is obsolete: true
Attachment #586261 - Flags: review?(netzen)
Attachment #586261 - Flags: review?(robert.bugzilla)
Attached patch Test changes (obsolete) — Splinter Review
Attachment #586263 - Flags: review?(robert.bugzilla)
Attachment #586263 - Flags: review?(netzen)
Attached patch Code changes (v2) (obsolete) — Splinter Review
This is a new version of the code changes with a few fixes to a few bugs I encountered working on some new tests.
Attachment #586261 - Attachment is obsolete: true
Attachment #587073 - Flags: review?(robert.bugzilla)
Attachment #587073 - Flags: review?(netzen)
Attachment #586261 - Flags: review?(robert.bugzilla)
Attachment #586261 - Flags: review?(netzen)
Attached patch New tests (obsolete) — Splinter Review
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.
Attachment #587076 - Flags: review?(robert.bugzilla)
Attachment #587076 - Flags: review?(netzen)
Attachment #587073 - Flags: feedback?(imelven)
This should fix one Mac test issue which happens on our Tinderboxes, which use a different mac bundle prefix.
Attachment #587125 - Flags: review?(robert.bugzilla)
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
Attachment #587073 - Flags: feedback?(imelven)
(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.
(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.
(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 !
(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.
Blocks: 685614
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.
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.
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.
It would be interesting to know what Robert thinks about this.
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 {
Attachment #587073 - Flags: review?(netzen)
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.
Attachment #586263 - Flags: review?(netzen)
Attachment #587076 - Flags: review?(netzen)
(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.
Attachment #589054 - Attachment is patch: true
> > +# 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 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 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.
(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.
(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.
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).
> 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.
(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.
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\).
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.
> 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.
(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.
(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!
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.
(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.
(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?
(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.
This patch removes the alternate directory support as per the recent discussion in the bug.
Attachment #589621 - Flags: review?(robert.bugzilla)
Whiteboard: [parity-chrome] → [parity-chrome][secr:imelven]
Depends on: 732078
No longer depends on: 732078
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.
This fixes the first part of comment 88.
Attachment #603516 - Flags: review?(robert.bugzilla)
Attached patch Code changes (v3) (obsolete) — Splinter Review
Attachment #586263 - Attachment is obsolete: true
Attachment #587073 - Attachment is obsolete: true
Attachment #587076 - Attachment is obsolete: true
Attachment #587125 - Attachment is obsolete: true
Attachment #587427 - Attachment is obsolete: true
Attachment #589051 - Attachment is obsolete: true
Attachment #589054 - Attachment is obsolete: true
Attachment #589621 - Attachment is obsolete: true
Attachment #603516 - Attachment is obsolete: true
Attachment #604208 - Flags: review?(robert.bugzilla)
Attachment #586263 - Flags: review?(robert.bugzilla)
Attachment #587073 - Flags: review?(robert.bugzilla)
Attachment #587076 - Flags: review?(robert.bugzilla)
Attachment #587125 - Flags: review?(robert.bugzilla)
Attachment #589621 - Flags: review?(robert.bugzilla)
Attachment #603516 - Flags: review?(robert.bugzilla)
Attachment #604211 - Flags: review?(robert.bugzilla)
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.
The update notification toaster/growl should go away.
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.
Attachment #604208 - Attachment is obsolete: true
Attachment #605640 - Flags: review?(robert.bugzilla)
Attachment #604208 - Flags: review?(robert.bugzilla)
I pushed https://hg.mozilla.org/projects/oak/rev/ef570d008beb to the Oak branch to get the Android builds working again.
(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 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!
Attachment #605640 - Flags: review?(robert.bugzilla) → review-
(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.)
(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 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.
FWIW, I will publish separate patches on top of these ones for addressing the comments, to make the reviews easier.
We'll also want to keep the last-update.log and backup-update.log located in the updates directory if they are present.
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.
Attachment #615880 - Flags: review?
Attachment #615880 - Flags: review? → review?(robert.bugzilla)
(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?
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.
Attachment #615902 - Flags: review?(robert.bugzilla)
(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.)
This fixes comment 102, and adds tests for it.
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.
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 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.
(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"
Addressed the comments.
Attachment #615902 - Attachment is obsolete: true
Attachment #615902 - Flags: review?(robert.bugzilla)
(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?
(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?
(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.
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?
(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.)
(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.
For now, an undetermined progress bar on the download page should suffice.
(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!)
Oh, also, how should I trigger this UI for testing?
(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.
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.
Attachment #616251 - Flags: review?(robert.bugzilla)
Attachment #617035 - Flags: review?(robert.bugzilla)
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?
Attachment #617655 - Flags: review?(robert.bugzilla)
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?
Attachment #618478 - Flags: review?(robert.bugzilla)
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
Whiteboard: [parity-chrome][secr:imelven] → [parity-chrome]
(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 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 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 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 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 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 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 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 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.
Attachment #620046 - Flags: review?(robert.bugzilla)
(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;
(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.
Attachment #620800 - Flags: review?(robert.bugzilla)
(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.
(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.
Attachment #620813 - Flags: review?(robert.bugzilla)
(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.
(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.  :-)
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?
Is it possible to get the log from the C:\ProgramData\Mozilla\logs for a failed run?
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.
Attachment #621663 - Flags: review?(robert.bugzilla)
(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?  :/
> 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...
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.
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.
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.
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.
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)
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
(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.
Depends on: 749956
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.
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.
Attachment #622916 - Flags: review?(robert.bugzilla)
Attachment #615880 - Flags: review?(robert.bugzilla) → review+
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?
Attachment #616251 - Flags: review?(robert.bugzilla) → review-
Attachment #617035 - Flags: review?(robert.bugzilla) → review+
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
Attachment #617655 - Flags: review?(robert.bugzilla) → review+
(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 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.
Attachment #618478 - Flags: review?(robert.bugzilla) → review-
Attachment #620046 - Flags: review?(robert.bugzilla) → review+
Attachment #620800 - Flags: review?(robert.bugzilla) → review+
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.
Attachment #621663 - Flags: review?(robert.bugzilla) → review+
Attachment #622916 - Flags: review?(robert.bugzilla) → review+
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.
Attachment #604211 - Flags: review?(robert.bugzilla) → review+
Attachment #620813 - Flags: review?(robert.bugzilla) → review+
Depends on: 756463
Attached patch Rename the prefSplinter Review
Attachment #625131 - Flags: review?(robert.bugzilla)
Attachment #625134 - Flags: review?(robert.bugzilla)
(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.
Attached patch Fix the review nits (obsolete) — Splinter Review
Nothing really interesting here, this just adds comments and renames a method.  Asking for review to be on the safe side.
Attachment #625136 - Flags: review?(robert.bugzilla)
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.
Attachment #618478 - Flags: review- → review?(robert.bugzilla)
I replaced one too many removeDownloadListener's in the previous patch.
Attachment #625136 - Attachment is obsolete: true
Attachment #625183 - Flags: review?(robert.bugzilla)
Attachment #625136 - Flags: review?(robert.bugzilla)
Attachment #625131 - Flags: review?(robert.bugzilla) → review+
Attachment #625134 - Flags: review?(robert.bugzilla) → review+
Attachment #625142 - Flags: review?(robert.bugzilla) → review+
Attachment #625183 - Flags: review?(robert.bugzilla) → review+
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.
Attachment #618478 - Flags: review?(robert.bugzilla) → review+
No longer depends on: 696891, 756463, 481815, 692887, 704591, 749956
Depends on: 757437
Depends on: 757439
http://hg.mozilla.org/mozilla-central/rev/c20d415ef1b5
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Blocks: 757449
Blocks: 757448
Will the be also available in firefox on linux?
And what about portable versions on windows?
(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.
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
(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)
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?
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
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
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
For future reference, I took a copy of the Oak branch here: <http://hg.mozilla.org/users/eakhgari_mozilla.com/bgupdates-oak/>.
(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.
Depends on: 757568
(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.
Blocks: 757611
No longer blocks: 757611
Depends on: 757611
Depends on: 757632
(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
(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
(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
Depends on: 757755
Depends on: 757794
Blocks: 757835
Depends on: 757717
Depends on: 757849
Blocks: 757885
Blocks: 757907
No longer blocks: 757835, 757885
Depends on: 757835, 757885
(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
Depends on: 757965
Depends on: 757963
Depends on: 757971
Depends on: 757981
Depends on: 757711
FWIW, this is preffed off in bug 757971 for now.
Depends on: 758068
Depends on: 758092
I filed bug 758101 to track the work remaining to re-enable background updates on mozilla-central.
No longer blocks: daily_beta_tracking
Depends on: 758101
Depends on: 758324
No longer blocks: 757907
Depends on: 757907
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.
Depends on: 758742
(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.
Depends on: 758998
Depends on: 759065
Depends on: 759063
Blocks: 759615
No longer blocks: 759615
Depends on: 759615
Depends on: 759390
Depends on: 760206
Depends on: 760290
Depends on: 760577
Depends on: 760665
Depends on: 760818
(This is going to be disabled for Windows on Firefox 15 for now...)
Depends on: 761248
No longer depends on: 761248
Depends on: 762032
Depends on: 764269
Depends on: 764377
No longer depends on: 764377
Depends on: 764751
Depends on: 765227
Depends on: 766264
Depends on: 766269
Depends on: 767125
Depends on: 767968
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
(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!
No longer depends on: 768417
Depends on: 771766
Depends on: 774789
Depends on: 775096
Depends on: 774014
Depends on: 765598
Depends on: 780575
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.
Depends on: 759639
Please file a new bug report if things aren't working for you. Thanks!
Flags: sec-review+
Blocks: 758326
Depends on: 789422
Depends on: 789958
Depends on: 789743
No longer depends on: 794160
Depends on: 823996
Depends on: CVE-2013-0799
Depends on: 921148
Regressions: 1632284
You need to log in before you can comment on or make changes to this bug.