Closed Bug 572162 Opened 10 years ago Closed 7 years ago

Remove requirement for app dir to be under program files for UAC elevation (breaks side by side installs of x86 and x64 - mixing of updates including update history)

Categories

(Toolkit :: Application Update, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: robert.strong.bugs, Assigned: bbondy)

References

(Depends on 1 open bug)

Details

Attachments

(5 files, 22 obsolete files)

9.63 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
3.60 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
11.49 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
9.49 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
7.07 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
I've been considering the removal of this requirement for a while now and think it is the right thing to do. As an added benefit this fixes the 64 bit case where it is installed under C:\Program Files(x86)]\
bah... this is not simple to say the least.
Possible solutions:
1. Full path to install dir under the user's profile. For example,
%LOCALAPPDATA%\Mozilla\Firefox\C\Program Files(x86)\Mozilla Firefox\

2. unique dir name under the user's profile with a file to map install dirs to the usnique dir.

3. Create a new dir under the user's profile to differentiate between C:\Program Files\ and C:\Program Files(x86)\


The first two also makes it so we will attempt to UAC elevate for admin users.

I'm leaning towards 1 at the moment but that is highly subject to change.
(In reply to comment #2)
> The first two also makes it so we will attempt to UAC elevate for admin users.
when not installed under program files that is.
Assignee: nobody → robert.bugzilla
(In reply to comment #2)
> Possible solutions:
> 1. Full path to install dir under the user's profile. For example,
> %LOCALAPPDATA%\Mozilla\Firefox\C\Program Files(x86)\Mozilla Firefox\
> 
> 2. unique dir name under the user's profile with a file to map install dirs to
> the usnique dir.
> 
> 3. Create a new dir under the user's profile to differentiate between
> C:\Program Files\ and C:\Program Files(x86)\

I feel stupid to ask this, but why is it necessary to make a profile directory in different parent directories for different installations?
It isn't the profile directory. It is where we download the update to when we don't have the permissions to write to the installation directory. We currently use the installation directory name but in the x64 case that means both C:\Program Files\Mozilla Firefox\ and C:\Program Files(x86)\Mozilla Firefox\ end up using the same directory.
Oh, I see, Thanks.
Summary: Remove requirement for app dir to be under program files for UAC elevation → Remove requirement for app dir to be under program files for UAC elevation (breaks side by side installs of x86 and x64)
Summary: Remove requirement for app dir to be under program files for UAC elevation (breaks side by side installs of x86 and x64) → Remove requirement for app dir to be under program files for UAC elevation (breaks side by side installs of x86 and x64 - mixing of updates including update history)
No longer blocks: 585934
Assignee: robert.bugzilla → netzen
Why not download to %temp% directory? You always can create a path for at install time, or just use a hash of install directory name.
There are programs that auto delete the contents of the temp directory and group policy has an option to delete the contents on reboot... these two behaviors alone are enough not to do this.
Unassigned myself for now since I won't be looking at this in the near future.
Assignee: netzen → nobody
> 1. Full path to install dir under the user's profile. For example:
> %LOCALAPPDATA%\Mozilla\Firefox\C\Program Files(x86)\Mozilla Firefox\

Maybe just:
%LOCALAPPDATA%\Mozilla\Firefox\Base64EncodedPath

This dir should always be used for updates and never the installation dir in case the user installs to a directory that requires elevated access which is not Program Files (as per Bug 755423).
Duplicate of this bug: 755423
This is part of the limited user account upgrade problem that Adri and I will be working on.
Assignee: nobody → no52fear
Whiteboard: [mentor=bbondy][lang=js]
Assignee: no52fear → netzen
Whiteboard: [mentor=bbondy][lang=js] → [lang=js]
Attached patch Patch v1. (obsolete) — Splinter Review
- Haven't run this through talos yet, but I am expecting it to not be a perf hit.
- Haven't run this through oak updates yet but I suspect it'll be OK.

For Metro updates I needed to add add code here to share the same update directory. So I decided it made sense to just do this task instead which auto solves that problem.
Attachment #735582 - Flags: review?(robert.bugzilla)
Depends on: 794937
Doesn't seem to have an effect I can see on tp5 talos first paint.
Noticed Metro and Desktop were sometimes hashing to different paths because of different case. Now I make the path lowercase before hashing.
Attachment #735582 - Attachment is obsolete: true
Attachment #735582 - Flags: review?(robert.bugzilla)
Attachment #735829 - Flags: review?(robert.bugzilla)
Whiteboard: [lang=js]
Blocks: 833182
No longer depends on: 794937
Attached patch Patch v1 - tests (obsolete) — Splinter Review
So the idea here is to add an env variable which skips the directory path hashing.

What happens is we have code for handling XRE_UPDATE_ROOT_DIR inside xpcshell.cpp, and then code for handling it in toolkit/xre for firefox.exe.

The problem is even if we duplicate the hashing code in both places (or refactor it into a common header file which I did at first), the hashes will compute to a different value.

I know we have an updater override env variable that we may be able to use to get things working, but I fought with that for about half a day and still had a lot of tests failing.  The problem is we don't have existing handling for that everywhere, we instead just rely on non program-files dir means it will go to vendor/appname folder under appdata.

Currently I'm only using the new env variable for 3 tests in /test and all of the tests in /test_svc. I think I can tighten up /test_svc using the env var if needed.
Attachment #738161 - Flags: review?(robert.bugzilla)
Attachment #738648 - Flags: review?(robert.bugzilla)
Attachment #735829 - Attachment description: Patch v2. → Patch v2 - Use a hash of the full path instead of the program files subdir name
Attached patch Patch v2 - Tests (obsolete) — Splinter Review
Fix for tests, all tests are passing on oak now for this and the other recent updater work.
Attachment #738161 - Attachment is obsolete: true
Attachment #738161 - Flags: review?(robert.bugzilla)
Attachment #739584 - Flags: review?(robert.bugzilla)
Comment on attachment 735829 [details] [diff] [review]
Patch v2 - Use a hash of the full path instead of the program files subdir name

>diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp
>--- a/toolkit/xre/nsXREDirProvider.cpp
>+++ b/toolkit/xre/nsXREDirProvider.cpp
>...
>@@ -949,16 +950,154 @@ GetRegWindowsAppDataFolder(bool aLocal, 
>     _retval.SetLength(0);
>     return NS_ERROR_NOT_AVAILABLE;
>   }
> 
>   return NS_OK;
> }
> #endif
> 
>+
>+#ifdef XP_WIN
The previous block is XP_WIN as well... just continue it and get rid of the 'extra' lines.

>+
>+
>+/**
>+ * Converts the input data into a binary hash using CryptoAPI.
>+ *
>+ * @param inputData The input data to hash
>+ * @param inputLength The number of bytes in the input data to process
>+ * @param hash The address of a pointer to fill to store the hash at.
nit: s/ at././

>+ *             Caller must free the buffer.
>+ * @param hashLen The address of a DWORD to fill with the hash length.
>+ * @return true on success
>+ */
>+static bool
>+CreateHashCryptoAPI(const LPBYTE inputData, DWORD inputLength,
>+                    LPBYTE *hash, LPDWORD hashLen)
>+{
>+  if (!inputData || !inputLength || !hash || !hashLen) {
>+    return false;
>+  }
>+
>+  bool result = false;
>+  HCRYPTPROV provider = NULL;
>+  HCRYPTHASH cryptHash = NULL;
>+  *hash = nullptr;
>+
>+  if(!CryptAcquireContext(&provider, NULL, NULL, PROV_RSA_FULL, 0)) {
>+    goto cleanup;
>+  }
>+
>+  if(!CryptCreateHash(provider, CALG_MD5, 0, 0, &cryptHash)) {
>+    goto cleanup;
>+  }
>+
>+  if(!CryptHashData(cryptHash, inputData, inputLength, 0)) {
>+    goto cleanup;
>+  }
>+
>+  DWORD hashLenSize = sizeof(DWORD);
>+  if(!CryptGetHashParam(cryptHash, HP_HASHSIZE, (BYTE *)hashLen,
>+                        &hashLenSize, 0)) {
>+    goto cleanup;
>+  }
>+
>+  *hash = (LPBYTE) CryptMemAlloc(*hashLen);
>+  if(!CryptGetHashParam(cryptHash, HP_HASHVAL, *hash, hashLen, 0)) {
>+    goto cleanup;
>+  }
Though I don't hate using goto this looks very straight forward to write without using goto. :(

>+
>+  // If we reached here we succeeded in creating the hash
>+  result = true;
>+
>+cleanup:
>+ if(cryptHash) {
>+   CryptDestroyHash(cryptHash);
>+ }
>+
>+ if(provider) {
>+   CryptReleaseContext(provider, 0);
>+ }
>+
>+ if (!result && *hash) {
>+   CryptMemFree(*hash);
>+ }
>+
>+ return result;
>+}
>+
>+/**
>+ * Converts the input data into a base64 representation using CryptoAPI
>+ *
>+ * @param inputData The input data to convert to base64
>+ * @param inputLength The number of bytes in the input data to process
>+ * @param base64Hash The address of a pointer to fill to store the base64
>+ *                   encoded data to. Caller must free the buffer.
nit: s/ to././

>+ * @param base64HashLen The address of a DWORD to fill with the length.
>+ * @return true on success
>+ */
>+static bool
>+ToBase64CryptoAPI(const LPBYTE inputData, DWORD inputLength,
>+                  LPBYTE *base64Hash, LPDWORD base64HashLen)
>+{
>+  if (!inputData || !inputLength || !base64Hash || !base64HashLen) {
>+    return false;
>+  }
>+
>+  // Get the required length
>+  if (!CryptBinaryToString(inputData, inputLength,
>+                           CRYPT_STRING_BASE64 | CRYPT_STRING_NOCRLF,
>+                           NULL, base64HashLen)) {
>+    return false;
>+  }
>+
>+  // Allocate the hash and get the actual hash
>+  *base64Hash = (LPBYTE) CryptMemAlloc(*base64HashLen);
>+  if (!CryptBinaryToStringA(inputData, inputLength,
>+                            CRYPT_STRING_BASE64 | CRYPT_STRING_NOCRLF,
>+                            (LPSTR) *base64Hash, base64HashLen)) {
>+    CryptMemFree(*base64Hash);
>+    return false;
>+  }
>+
>+  return true;
>+}
>+
>+/**
>+ * Obtains a hash of the passed in file path
>+ *
>+ * @param path The file path to hash
>+ * @param base64EncodedPath Out string storing a hash of that path
nit: s/that path/the provided path

>+ * @return true on success
>+ */
>+static bool
>+HashPath(const nsAString &path, nsAString &base64EncodedPath)
>+{
>+  nsAutoString lowercasePath(path);
>+  _wcslwr(lowercasePath.BeginWriting());
>+  LPBYTE hash = nullptr;
>+  DWORD hashLen;
>+  if (!CreateHashCryptoAPI((LPBYTE) lowercasePath.BeginReading(), lowercasePath.Length(),
>+                            &hash, &hashLen)) {
nit: lowercasePath.Length() on next line

>+    return false;
>+  }
>+
>+  LPBYTE base64Hash = nullptr;
>+  DWORD base64HashLen;
>+  if (!ToBase64CryptoAPI(hash, hashLen, &base64Hash, &base64HashLen)) {
>+    CryptMemFree(hash);
>+    return false;
>+  }
>+
>+  base64EncodedPath.AssignASCII((const char*)(base64Hash), base64HashLen);
>+  CryptMemFree(base64Hash);
>+  return true;
>+}
>+#endif
>+
> nsresult
> nsXREDirProvider::GetUpdateRootDir(nsIFile* *aResult)
> {
>   nsCOMPtr<nsIFile> updRoot;
> #if defined(MOZ_WIDGET_GONK)
> 
>   nsresult rv = NS_NewNativeLocalFile(nsDependentCString("/data/local"),
>                                       true,
>@@ -969,16 +1108,44 @@ nsXREDirProvider::GetUpdateRootDir(nsIFi
>   nsCOMPtr<nsIFile> appFile;
>   bool per = false;
>   nsresult rv = GetFile(XRE_EXECUTABLE_FILE, &per, getter_AddRefs(appFile));
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = appFile->GetParent(getter_AddRefs(updRoot));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
> #ifdef XP_WIN
>+
>+  // We first try to obtain a path which is a hash of the current process path
>+  nsAutoString pathHash;
>+  nsAutoString appFilePath;
>+  rv = appFile->GetPath(appFilePath);
I think it would be best to use updRoot here (e.g. the parent directory of appFile).

r=me with that
Attachment #735829 - Flags: review?(robert.bugzilla) → review+
The following will need to be updated. The added registry setting should be removed along with the directory on install and uninstall.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/common.nsh#3238

Note: we want to keep the last-update.log and backup-update.log so if you could copy or move those files to a new directory (prefixed with mozupdate- perhaps?) in %TMP% while fixing that up it would be appreciated.
So, Thunrderbird does not set vendor. :(

I would suggest using gAppData->vendor and gAppData->name if both are present and just gAppData->name if it is present (it damn well should be) with gAppData->vendor not being present.
If vendor is not present I can use gAppData->name.

But the reason I only use gAppData->vendor and not gAppData->name is because I want the same base folder for both MetroFirefox and Firefox. 

Otherwise Firefox would hash to:
C:\Users\Brian\AppData\Local\Mozilla\Firefox\HashHere
and Metro Firefox would hash to:
C:\Users\Brian\AppData\Local\Mozilla\MetroFirefox\HashHere

But we want the same update folder.
(In reply to Brian R. Bondy [:bbondy] from comment #25)
> If vendor is not present I can use gAppData->name.
> 
> But the reason I only use gAppData->vendor and not gAppData->name is because
> I want the same base folder for both MetroFirefox and Firefox. 
> 
> Otherwise Firefox would hash to:
> C:\Users\Brian\AppData\Local\Mozilla\Firefox\HashHere
> and Metro Firefox would hash to:
> C:\Users\Brian\AppData\Local\Mozilla\MetroFirefox\HashHere
> 
> But we want the same update folder.
Right... one thing that has always been a pet peeve to me about the current paths used is vendor\name\etc... I think a better approach would be if the path was vendor\updates\ when vendor is present and name\updates\ when vendor is not present. What do you think?
that sounds good.
Comment on attachment 738648 [details] [diff] [review]
Patch v1 - Cache the hashed value for better startup perf

I am for the most part OK with this approach but I would very much like to use a similar approach for Application User Model IDs (e.g. using the same hashing API's) at some point in the future.

Updated note from previous review: I am ok with using the path for appFile to create the hash. It should be possible to use this same value at some point in the future for the app's Application User Model ID. This would lessen the number of hashes lying around. Can you think of any reason not to go this route?

I think it would be a good thing to calculate this out in the NSIS code as well and store the value in HKLM. I would very much prefer it if we continued to only write to HKCU when necessary (e.g. apps not using the application association registration APIs, user not having write access to HKLM, etc.).

With the above in mind, we will need to clean this up on update so versioning of the hash as we discussed earlier today would be unnecessary.
(In reply to Robert Strong [:rstrong] (do not email) from comment #28)
>...
> With the above in mind, we will need to clean this up on update so
> versioning of the hash as we discussed earlier today would be unnecessary.
When I say, "we will need to clean this up on update" I am referring to the current situation where we already do this.
Comment on attachment 738648 [details] [diff] [review]
Patch v1 - Cache the hashed value for better startup perf

Clearing due to current set of review comments.
Attachment #738648 - Flags: review?(robert.bugzilla)
Comment on attachment 739584 [details] [diff] [review]
Patch v2 - Tests

>diff --git a/toolkit/mozapps/update/test/unit/head_update.js.in b/toolkit/mozapps/update/test/unit/head_update.js.in
>--- a/toolkit/mozapps/update/test/unit/head_update.js.in
>+++ b/toolkit/mozapps/update/test/unit/head_update.js.in
>...
>@@ -2213,16 +2215,20 @@ function setEnvironment() {
>   }
>   else {
>     logTestInfo("setting the XPCOM_DEBUG_BREAK environment variable to " +
>                 "warn... previously it didn't exist");
>   }
> 
>   env.set("XPCOM_DEBUG_BREAK", "warn");
> 
>+  if (IS_WIN && gEnvSKipUpdateDirHashing) {
>+    env.set("MOZ_UPDATE_NO_HASH_DIR", "1");
>+  }
>+
Just in case the environment is ever shared between tests please reset this in resetEnvironment as applicable.

Any other cases you can think of that should be tested?
Attachment #739584 - Flags: review?(robert.bugzilla) → review+
Is there any reason why this can't be the primary method used for all installation paths going forward? Please note that I think I am ok with leaving the old method as a fallback for now and that any work to remove it can be done in a followup bug.

Please let QA (Juan, Anthoney, etc.) know about the new paths since they rely on the existing paths when troubleshooting.
(In reply to Robert Strong [:rstrong] (do not email) from comment #31)
> Comment on attachment 739584 [details] [diff] [review]
> Patch v2 - Tests
> 
> >diff --git a/toolkit/mozapps/update/test/unit/head_update.js.in b/toolkit/mozapps/update/test/unit/head_update.js.in
> >--- a/toolkit/mozapps/update/test/unit/head_update.js.in
> >+++ b/toolkit/mozapps/update/test/unit/head_update.js.in
> >...
> >@@ -2213,16 +2215,20 @@ function setEnvironment() {
> >   }
> >   else {
> >     logTestInfo("setting the XPCOM_DEBUG_BREAK environment variable to " +
> >                 "warn... previously it didn't exist");
> >   }
> > 
> >   env.set("XPCOM_DEBUG_BREAK", "warn");
> > 
> >+  if (IS_WIN && gEnvSKipUpdateDirHashing) {
> >+    env.set("MOZ_UPDATE_NO_HASH_DIR", "1");
> >+  }
> >+
> Just in case the environment is ever shared between tests please reset this
> in resetEnvironment as applicable.
> 
> Any other cases you can think of that should be tested?

I actually did that originally but this caused a few tests to fail on the service side of things. I think because we call resetEnvironment for different reasons relating to the callback.  I had a messageBox inside xpcshell.exe though and it looks to me like each test gets its own process.
(In reply to Robert Strong [:rstrong] (do not email) from comment #32)
> Is there any reason why this can't be the primary method used for all
> installation paths going forward? Please note that I think I am ok with
> leaving the old method as a fallback for now and that any work to remove it
> can be done in a followup bug.
> 

I don't know of any but you would know better than me because I don't currently know of the differences in the directory handling for the other platforms.  I'm just not sure if there is any benefit to it on other platforms and it looks like each platform does something different right now.  

> Please let QA (Juan, Anthoney, etc.) know about the new paths since they
> rely on the existing paths when troubleshooting.

Hi guys,
This path:
C:\Users\Brian\AppData\Local\Mozilla\Firefox

Is going to be changing to
C:\Users\Brian\AppData\Local\Mozilla\hash_of_install_dir

I'm not sure if this will affect anything in your tests or in mozmill.

--- 

Also CC'ing some Thunderbird and SeaMonkey people.  Please see Comment 26.  Basically the update dir may change and I'm not sure if you have any addition tests or other concerns.
(In reply to Brian R. Bondy [:bbondy] from comment #34)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #32)
> > Is there any reason why this can't be the primary method used for all
> > installation paths going forward? Please note that I think I am ok with
> > leaving the old method as a fallback for now and that any work to remove it
> > can be done in a followup bug.
> > 
> 
> I don't know of any but you would know better than me because I don't
> currently know of the differences in the directory handling for the other
> platforms.  I'm just not sure if there is any benefit to it on other
> platforms and it looks like each platform does something different right
> now.  
> 
> > Please let QA (Juan, Anthoney, etc.) know about the new paths since they
> > rely on the existing paths when troubleshooting.
> 
> Hi guys,
> This path:
> C:\Users\Brian\AppData\Local\Mozilla\Firefox
> 
> Is going to be changing to
> C:\Users\Brian\AppData\Local\Mozilla\hash_of_install_dir
> 
> I'm not sure if this will affect anything in your tests or in mozmill.
Also, the value for the hash will be available in the registry
(In reply to Brian R. Bondy [:bbondy] from comment #33)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #31)
> > Comment on attachment 739584 [details] [diff] [review]
> > Patch v2 - Tests
> > 
> > >diff --git a/toolkit/mozapps/update/test/unit/head_update.js.in b/toolkit/mozapps/update/test/unit/head_update.js.in
> > >--- a/toolkit/mozapps/update/test/unit/head_update.js.in
> > >+++ b/toolkit/mozapps/update/test/unit/head_update.js.in
> > >...
> > >@@ -2213,16 +2215,20 @@ function setEnvironment() {
> > >   }
> > >   else {
> > >     logTestInfo("setting the XPCOM_DEBUG_BREAK environment variable to " +
> > >                 "warn... previously it didn't exist");
> > >   }
> > > 
> > >   env.set("XPCOM_DEBUG_BREAK", "warn");
> > > 
> > >+  if (IS_WIN && gEnvSKipUpdateDirHashing) {
> > >+    env.set("MOZ_UPDATE_NO_HASH_DIR", "1");
> > >+  }
> > >+
> > Just in case the environment is ever shared between tests please reset this
> > in resetEnvironment as applicable.
> > 
> > Any other cases you can think of that should be tested?
> 
> I actually did that originally but this caused a few tests to fail on the
> service side of things. I think because we call resetEnvironment for
> different reasons relating to the callback.  I had a messageBox inside
> xpcshell.exe though and it looks to me like each test gets its own process.
Would be good to know why and fix it.... file a followup bug?
> Would be good to know why and fix it.... file a followup bug?
OK
(In reply to Brian R. Bondy [:bbondy] from comment #34)
> Hi guys,
> This path:
> C:\Users\Brian\AppData\Local\Mozilla\Firefox
> 
> Is going to be changing to
> C:\Users\Brian\AppData\Local\Mozilla\hash_of_install_dir
> 
> I'm not sure if this will affect anything in your tests or in mozmill.

Thanks Brian. Does this also affect the Roaming folder? For instance, do we need to update support documentation like https://support.mozilla.org/en-US/kb/back-and-restore-information-firefox-profiles#os=win7&browser=fx20

CCing Henrik Skupin to comment about this change's impact on our Mozmill tests.
It only affects the update folder and not the profile folder in both of the local & roaming.
rstrong I'm going to scrap all of the crypto API code and just read from the registry from: HKEY_CURRENT_USER\Software\Mozilla\Firefox\TaskBarIDs w/ the reg value of dirpath. 

Are you OK with that approach?
The env variable for the tests would still exist, and we'd fallback to the old directory if that path isn't present.  Using vendor/hash would still be applicable as well.
They are also be stored in HKLM... this really should be cleaned up even though the fact that we are always adding gives us little time to clean things up. Having said that, I'm ok with this.

Think about whether it would be best to give precedence on HKLM or HKCU.
(In reply to Robert Strong [:rstrong] (do not email) from comment #41)
> They are also be stored in HKLM... this really should be cleaned up even
> though the fact that we are always adding gives us little time to clean
> things up. Having said that, I'm ok with this.
> 
> Think about whether it would be best to give precedence on HKLM or HKCU.

Well if it exists in either it should be the same value, but I guess I'll give precedence to HKLM.
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #38)
> > This path:
> > C:\Users\Brian\AppData\Local\Mozilla\Firefox
> > 
> > Is going to be changing to
> > C:\Users\Brian\AppData\Local\Mozilla\hash_of_install_dir
> > 
> > I'm not sure if this will affect anything in your tests or in mozmill.
> 
> CCing Henrik Skupin to comment about this change's impact on our Mozmill
> tests.

Thanks Brian that you remember our tests! Given that with the landing of bug 860703 we make use of getUpdatesDirectory() from the AUS service now, we will be fine as long as this method returns the correct path.
Status: NEW → ASSIGNED
Depends on: 867023
Note: we still need that env variable because the lookup dir name would be the temp dir because of the way we do callbacks in xpcshell tests.

I'll do an independent patch in this bug for the installer cleanup so this one is ready for review.
Attachment #735829 - Attachment is obsolete: true
Attachment #738648 - Attachment is obsolete: true
Attachment #743421 - Flags: review?(robert.bugzilla)
Sorry I thought you were referring to the dir service before when you mentioned updRoot
Attachment #743421 - Attachment is obsolete: true
Attachment #743421 - Flags: review?(robert.bugzilla)
Attachment #743834 - Flags: review?(robert.bugzilla)
Attachment #743834 - Flags: review?(robert.bugzilla) → review+
Attachment #744193 - Flags: review?(robert.bugzilla)
Comment on attachment 744193 [details] [diff] [review]
Patch v1 - Install, uninstall and postupdate changes

There are consumers other than Firefox that would need to be updated. :(

I'll check what other options there are.
agreed but I purposely wanted them to get a build error here so they know about the change and can make the appropriate changes.
To be a good neighbor we either create a new macro, use that, and file bugs for other apps or fix all apps.
I think it would be better to file bugs because they may be cleaning the wrong dir as of the other patches in this bug. But then again I don't want to hold up landing this since this patch is the last thing holding this up.  Let me know how you want me to proceed.  

I think the right path is to make a duplicate macro/function with the changes and leave the old one untouched. Then file bugs about a possible regression in cleanup because of this bug and a suggestion for them to migrate to the new macro/function.  If you agree do I file two? One for Thunderbird one for Seamonkey?
(In reply to Brian R. Bondy [:bbondy] from comment #50)
> I think it would be better to file bugs because they may be cleaning the
> wrong dir as of the other patches in this bug. But then again I don't want
> to hold up landing this since this patch is the last thing holding this up. 
> Let me know how you want me to proceed.  
> 
> I think the right path is to make a duplicate macro/function with the
> changes and leave the old one untouched. Then file bugs about a possible
> regression in cleanup because of this bug and a suggestion for them to
> migrate to the new macro/function.  If you agree do I file two? One for
> Thunderbird one for Seamonkey?
That's fine but file three (Calendar / Sunbird as well) along with a bug to remove the old macro after they have been fixed. Calendar / Sunbird isn't maintained but I don't want the old macro sitting in code in comm-central and they have taken patches recently.
cleanUpdateDirectories as the new name?
Might be easier to spot the differences from the obsolete patch.
I'll file the followups after review but before landing.
Attachment #744193 - Attachment is obsolete: true
Attachment #744193 - Flags: review?(robert.bugzilla)
Attachment #744270 - Flags: review?(robert.bugzilla)
Comment on attachment 744270 [details] [diff] [review]
Patch v2 - Install, uninstall and postupdate changes

>+ * If present removes the updates directory located in the profile's local
>+ * directory for this installation.
>+ *
>+ * @param   _OLD_REL_PATH
>+ *          The relative path to the profile directory from $LOCALAPPDATA.
>+ *          Calculated for the old update directory not based on a hash.
>+ * @param   _NEW_REL_PATH
>+ *          The relative path to the profile directory from $LOCALAPPDATA.
>+ *          Calculated for the new update directory based on a hash.
>+ * @param   _IS_UPDATING
>+ *          Should be "1" if an update is being performed, or "0" if an install
>+ *          or uninstall is being performed. When it is "0" update directories
>+ *          will be removed.
We are already using "true". Do you think we should change things to "1"?

>+ *
>+ * $0  = _IS_UPDATING
>+ * $R1 = _NEW_REL_PATH
>+ * $R2 = path to the new update directory built from _NEW_REL_PATH and
>+ *       the taskbar ID.
>+ * $R3 = taskBar ID hash located in registry at SOFTWARE\_OLD_REL_PATH\TaskBarIDs
>+ * $R4 = various path values.
>+ * $R5 = length of the long path to $PROGRAMFILES
>+ * $R6 = length of the long path to $INSTDIR
>+ * $R7 = long path to $PROGRAMFILES
>+ * $R8 = long path to $INSTDIR
>+ * $R9 = _OLD_REL_PATH
>+ */
>+!macro CleanUpdateDirectories
>+
>+  !ifndef ${_MOZFUNC_UN}CleanUpdateDirectories
>+    !define _MOZFUNC_UN_TMP ${_MOZFUNC_UN}
>+    !insertmacro ${_MOZFUNC_UN_TMP}GetLongPath
>+    !undef _MOZFUNC_UN
>+    !define _MOZFUNC_UN ${_MOZFUNC_UN_TMP}
>+    !undef _MOZFUNC_UN_TMP
>+
>+    !verbose push
>+    !verbose ${_MOZFUNC_VERBOSE}
>+    !define ${_MOZFUNC_UN}CleanUpdateDirectories "!insertmacro ${_MOZFUNC_UN}CleanUpdateDirectoriesCall"
>+
>+    Function ${_MOZFUNC_UN}CleanUpdateDirectories
>+      Exch $0
>+      Exch 1
>+      Exch $R1
>+      Exch 2
>+      Exch $R9
>+      Push $R8
>+      Push $R7
>+      Push $R6
>+      Push $R5
>+      Push $R4
>+      Push $R3
>+      Push $R2
Please start with $R9 and descend

>+
>+      StrCmp $R9 "" end +1 ; The relative path to the app's profiles is required
>+      ${${_MOZFUNC_UN}GetLongPath} "$INSTDIR" $R8
>+      StrCmp $R8 "" end +1
>+      ${${_MOZFUNC_UN}GetLongPath} "$PROGRAMFILES" $R7
>+      StrCmp $R7 "" end +1
Before the first Exch use
${If} $R9 == ""
${OrIf} $R8 == ""
${OrIf} $R7 == ""
  Return
${EndIf}

If it isn't too ugly it is best to use the LogicLib ${If}, ${Unless}, etc. functions
Attachment #744270 - Attachment is obsolete: true
Attachment #744270 - Flags: review?(robert.bugzilla)
Attachment #744397 - Flags: review?(robert.bugzilla)
Comment on attachment 744397 [details] [diff] [review]
Patch v3 - Install, uninstall and postupdate changes

Thanks!
Attachment #744397 - Flags: review?(robert.bugzilla) → review+
Removed postupdate call for cleanUpdateDirectories, it'll be replaced by a new patch I'll post later.  

cleanUpdateDirectories is only called on install/uninstall now.  It cleans up both the old and new update directories now and backs up the log files from each of those directories.
Attachment #744397 - Attachment is obsolete: true
Attachment #745327 - Flags: review?(robert.bugzilla)
Attachment #745327 - Flags: review?(robert.bugzilla) → review+
Mostly working WIP, will finish tomorrow and request review.
Attachment #746209 - Attachment is patch: true
Attachment #746209 - Attachment mime type: text/x-patch → text/plain
This is the last patch for this bug (not including resubmits for review comments)
Attachment #746209 - Attachment is obsolete: true
Attachment #746435 - Flags: review?(robert.bugzilla)
Comment on attachment 746435 [details] [diff] [review]
Patch v1 - Post Update processing to migrate update files

>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
>...
>@@ -2006,16 +2022,78 @@ UpdateService.prototype = {
>     try {
>       let h = Services.telemetry.getHistogramById("UPDATER_SERVICE_MANUALLY_UNINSTALLED");
>       h.add(!installed && attempted ? 1 : 0);
>     } catch(e) {
>       // Don't allow any exception to be propagated.
>       Components.utils.reportError(e);
>     }
>   },
>+
>+  /*
>+   * Migrates old update directory files to the new update directory
>+   * which is based on a hash of the installation.
>+   */
>+  _migrateOldUpdateDir: function() {
>+    // Don't do this migrate-check more than once
>+    var alreadyMigrated = getPref("getIntPref",
>+                                  PREF_APP_UPDATE_MIGRATE_APP_DIR, 0);
>+    if (alreadyMigrated) {
>+      return;
>+    }
>+    Services.prefs.setIntPref(PREF_APP_UPDATE_MIGRATE_APP_DIR, 1);
Instead of using a preference, could you just check for existence of the new update dir? If not or there is a noticeable perf impact (since we have to check for its existence anyways I don't think there will be) I'm ok with a new pref though I'd like to know why it isn't possible.

>+
>+
>+    // Obtain the old update root
>+    var exeFile = FileUtils.getFile("XREExeF", []);
>+    var updateLeafName = exeFile.parent.leafName;
>+    var oldUpdateRoot = FileUtils.getDir("LocalAppData",
>+                                         [Services.appinfo.vendor,
>+                                          Services.appinfo.name,
>+                                          updateLeafName], false);
Don't forget about Thunderbird... would be good to check Seamonkey as well.

Also, do all apps set the hash in the registry? If not, how will they get migrated if they hit this code path?

>+
>+    // Obtain the new update root
>+    var newUpdateRoot = FileUtils.getDir("UpdRootD", [], true);
>+
>+    // Return early when there's no work to do
>+    if (!oldUpdateRoot.exists() || oldUpdateRoot.path == newUpdateRoot.path ||
>+        updateLeafName.length == 0) {
These checks don't make sense to me... under what conditions would the old and new paths be the same? Is it for other apps that haven't set the value in the registry? Why the check for the zero length leaf name?

If the check is changed to the new update dir's existence then a bunch of this will need to be reworked anyways.

>+      return;
>+    }
Attachment #746435 - Flags: review?(robert.bugzilla) → review-
(In reply to Robert Strong [:rstrong] (do not email) from comment #61)
> Comment on attachment 746435 [details] [diff] [review]
> Patch v1 - Post Update processing to migrate update files
> 
> >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
> >...
> Instead of using a preference, could you just check for existence of the new
> update dir? If not or there is a noticeable perf impact (since we have to
> check for its existence anyways I don't think there will be) I'm ok with a
> new pref though I'd like to know why it isn't possible.

Checking the existance of the new update dir is actually easier to do, and I was going to do that but I guessed (possibly wrongly) that a pref check is less of a perf hit than a disk access check. 
I don't know exactly how pref handling works, but a guess is that all prefs can be read in one or 2 big reads on startup and then stored in memory for fast access.



> >+    // Obtain the old update root
> >+    var exeFile = FileUtils.getFile("XREExeF", []);
> >+    var updateLeafName = exeFile.parent.leafName;
> >+    var oldUpdateRoot = FileUtils.getDir("LocalAppData",
> >+                                         [Services.appinfo.vendor,
> >+                                          Services.appinfo.name,
> >+                                          updateLeafName], false);
> Don't forget about Thunderbird... would be good to check Seamonkey as well.


What needs to change for those? Maybe if oldUpdateRoot doesn't exist, try for the same but without Services.appinfo.vendor?

> Also, do all apps set the hash in the registry? If not, how will they get
> migrated if they hit this code path?

If they have a taskbar ID, but the check below handles this oldUpdateRoot.path == newUpdateRoot.path.


> 
> >+
> >+    // Obtain the new update root
> >+    var newUpdateRoot = FileUtils.getDir("UpdRootD", [], true);
> >+
> >+    // Return early when there's no work to do
> >+    if (!oldUpdateRoot.exists() || oldUpdateRoot.path == newUpdateRoot.path ||
> >+        updateLeafName.length == 0) {
> These checks don't make sense to me... under what conditions would the old
> and new paths be the same? 

When there is no taskbar ID stored in the registry.

> Is it for other apps that haven't set the value
> in the registry? Why the check for the zero length leaf name?

That was a sanity check in case for some reason it's an empty string or backslash, and we'd end up clearing the whole appdata\mozilla\firefox
If they *don't* have a taskbar ID, but the check below handles this oldUpdateRoot.path == newUpdateRoot.path.
(In reply to Brian R. Bondy [:bbondy] from comment #62)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #61)
> > Comment on attachment 746435 [details] [diff] [review]
> > Patch v1 - Post Update processing to migrate update files
> > 
> > >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
> > >...
> > Instead of using a preference, could you just check for existence of the new
> > update dir? If not or there is a noticeable perf impact (since we have to
> > check for its existence anyways I don't think there will be) I'm ok with a
> > new pref though I'd like to know why it isn't possible.
> 
> Checking the existance of the new update dir is actually easier to do, and I
> was going to do that but I guessed (possibly wrongly) that a pref check is
> less of a perf hit than a disk access check. 
It isn't but we do have to perform other filesystem checks along the way so I think it should be possible. Also keep in mind that there is nsUpdateServiceStub.js to lessen the startup time perf hit.

> I don't know exactly how pref handling works, but a guess is that all prefs
> can be read in one or 2 big reads on startup and then stored in memory for
> fast access.
> 
> 
> 
> > >+    // Obtain the old update root
> > >+    var exeFile = FileUtils.getFile("XREExeF", []);
> > >+    var updateLeafName = exeFile.parent.leafName;
> > >+    var oldUpdateRoot = FileUtils.getDir("LocalAppData",
> > >+                                         [Services.appinfo.vendor,
> > >+                                          Services.appinfo.name,
> > >+                                          updateLeafName], false);
> > Don't forget about Thunderbird... would be good to check Seamonkey as well.
> 
> 
> What needs to change for those? Maybe if oldUpdateRoot doesn't exist, try
> for the same but without Services.appinfo.vendor?
Thunderbird doesn't set vendor. Not sure about Seamonkey.

> > Also, do all apps set the hash in the registry? If not, how will they get
> > migrated if they hit this code path?
> 
> If they have a taskbar ID, but the check below handles this
> oldUpdateRoot.path == newUpdateRoot.path.
So, they would have the pref set that they have been migrated but they might not add the taskbar id until sometime after.

> 
> > 
> > >+
> > >+    // Obtain the new update root
> > >+    var newUpdateRoot = FileUtils.getDir("UpdRootD", [], true);
> > >+
> > >+    // Return early when there's no work to do
> > >+    if (!oldUpdateRoot.exists() || oldUpdateRoot.path == newUpdateRoot.path ||
> > >+        updateLeafName.length == 0) {
> > These checks don't make sense to me... under what conditions would the old
> > and new paths be the same? 
> 
> When there is no taskbar ID stored in the registry.
> 
> > Is it for other apps that haven't set the value
> > in the registry? Why the check for the zero length leaf name?
> 
> That was a sanity check in case for some reason it's an empty string or
> backslash, and we'd end up clearing the whole appdata\mozilla\firefox
Thanks!
Got rid of the pref and rely on if the old update directory exists or not.  If it does not exist then no migration work needs to be done (since the migration removes it when it's done)
Attachment #746435 - Attachment is obsolete: true
Attachment #746606 - Flags: review?(robert.bugzilla)
Comment on attachment 746606 [details] [diff] [review]
Patch v2 - Post Update processing to migrate update files

I thought you would check for the existence of the new update directory or some child within the update directory and take that condition as a need to migrate. Also, it seems to me that nsUpdateServiceStub.js isn't taken into account.
So, nsUpdateServiceStub.js will instantiate the client AUS in nsUpdateService.js which will then complete the update and update the info in updates.xml using the data provided by update.status and active-update.xml. As it currently stands with the patches nsUpdateServiceStub.js will never see the update.status file, etc.

I *think* there could be a check for an intermediate directory though I haven't thought that through entirely. If there can't I'd prefer using a user pref.
Comment on attachment 746606 [details] [diff] [review]
Patch v2 - Post Update processing to migrate update files

There should also be a test for the migration.
Attachment #746606 - Flags: review?(robert.bugzilla) → review-
(In reply to Robert Strong [:rstrong] (do not email) from comment #66)
> Comment on attachment 746606 [details] [diff] [review]
> Patch v2 - Post Update processing to migrate update files
> 
> I thought you would check for the existence of the new update directory or
> some child within the update directory and take that condition as a need to
> migrate. 

I'm not sure if the new directory will be created or not by the time it gets to that code.  Maybe some other code will create it.  Is there a reason why checking for the existence of the old directory is bad?  After the migration we remove the old directory so as far as I can tell that seems fine, but maybe I'm missing something.

> Also, it seems to me that nsUpdateServiceStub.js isn't taken into
> account.

Ah I see, I had tested the function locally but not yet on oak.  Does it sound reasonable to do this from within nsUpdateServiceStub.js?

> if (statusFile.exists() ***|| oldStatusFile.exists()***) {
>    let aus = Components.classes["@mozilla.org/updates/update-service;1"].
>              getService(Ci.nsIApplicationUpdateService).
>              QueryInterface(Ci.nsIObserver);
>    aus.observe(null, "post-update-processing", "");
>  }


---

> There should also be a test for the migration.

I'll do a test for this in a new patch before landing.
(In reply to Brian R. Bondy [:bbondy] from comment #69)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #66)
> > Comment on attachment 746606 [details] [diff] [review]
> > Patch v2 - Post Update processing to migrate update files
> > 
> > I thought you would check for the existence of the new update directory or
> > some child within the update directory and take that condition as a need to
> > migrate. 
> 
> I'm not sure if the new directory will be created or not by the time it gets
> to that code.  Maybe some other code will create it.  Is there a reason why
> checking for the existence of the old directory is bad?  After the migration
> we remove the old directory so as far as I can tell that seems fine, but
> maybe I'm missing something.
As you stated in comment #64 "a pref check is less of a perf hit than a disk access check."

> > Also, it seems to me that nsUpdateServiceStub.js isn't taken into
> > account.
> 
> Ah I see, I had tested the function locally but not yet on oak.  Does it
> sound reasonable to do this from within nsUpdateServiceStub.js?
It does.
> As you stated in comment #64 "a pref check is less of a perf hit than 
> a disk access check."

Well I stated that I thought a pref check was less expensive (but it was just a guess) and then I think you told me I was wrong about that guess. 

> > I was going to do that but I guessed (possibly wrongly) that a pref check is
> > less of a perf hit than a disk access check. 

> It isn't but we do have to perform other filesystem checks along the way so I 
> think it should be possible. Also keep in mind that there is 
> nsUpdateServiceStub.js to lessen the startup time perf hit.

So anyway sounds like a misunderstanding about what is faster. Should I use a pref check only in the stub check?

So we want this right?

> if (statusFile.exists() ***|| migratePrefIsNotYetSet ***) {
>    let aus = Components.classes["@mozilla.org/updates/update-service;1"].
>              getService(Ci.nsIApplicationUpdateService).
>              QueryInterface(Ci.nsIObserver);
>    aus.observe(null, "post-update-processing", "");
>  }

And then set the migrate pref after the old updatedir is cleared and after an empty old updatedir is found inside nsUpdateService.js?
(In reply to Brian R. Bondy [:bbondy] from comment #71)
> > As you stated in comment #64 "a pref check is less of a perf hit than 
> > a disk access check."
> 
> Well I stated that I thought a pref check was less expensive (but it was
> just a guess) and then I think you told me I was wrong about that guess. 
Please read comment #61.

> > > I was going to do that but I guessed (possibly wrongly) that a pref check is
> > > less of a perf hit than a disk access check. 
> 
> > It isn't but we do have to perform other filesystem checks along the way so I 
> > think it should be possible. Also keep in mind that there is 
> > nsUpdateServiceStub.js to lessen the startup time perf hit.
> 
> So anyway sounds like a misunderstanding about what is faster. Should I use
> a pref check only in the stub check?
Please read comment #61 and after looking at the code and thinking about it doing it in the stub will be required.

> So we want this right?
See above.

> 
> > if (statusFile.exists() ***|| migratePrefIsNotYetSet ***) {
> >    let aus = Components.classes["@mozilla.org/updates/update-service;1"].
> >              getService(Ci.nsIApplicationUpdateService).
> >              QueryInterface(Ci.nsIObserver);
> >    aus.observe(null, "post-update-processing", "");
> >  }
> 
> And then set the migrate pref after the old updatedir is cleared and after
> an empty old updatedir is found inside nsUpdateService.js?
That seems reasonable.

Just so you know, the component (in this case nsUpdateServiceStub.js) needs to instantiate after profile prefs are available. This can be found in nsUpdateService.manifest and since it load during profile-after-change they are available.
I think this is the route you wanted to take, but let me know if I misunderstood anything.  Thanks for all the reviews.
Attachment #746606 - Attachment is obsolete: true
Attachment #747250 - Flags: review?(robert.bugzilla)
Attached patch Patch v4 - Post update migration (obsolete) — Splinter Review
Fix for getPref from the last patch (defined in nsUpdateService but not stub)
Attachment #747250 - Attachment is obsolete: true
Attachment #747250 - Flags: review?(robert.bugzilla)
Attachment #747521 - Flags: review?(robert.bugzilla)
Comment on attachment 747521 [details] [diff] [review]
Patch v4 - Post update migration

>diff --git a/toolkit/mozapps/update/nsUpdateServiceStub.js b/toolkit/mozapps/update/nsUpdateServiceStub.js
>--- a/toolkit/mozapps/update/nsUpdateServiceStub.js
>+++ b/toolkit/mozapps/update/nsUpdateServiceStub.js
>@@ -8,29 +8,122 @@ Components.utils.import("resource://gre/
> 
> const Ci = Components.interfaces;
> 
> const DIR_UPDATES         = "updates";
> const FILE_UPDATE_STATUS  = "update.status";
> 
> const KEY_UPDROOT         = "UpdRootD";
> 
>+#ifdef XP_WIN
>+
>+const PREF_APP_UPDATE_MIGRATE_APP_DIR = "app.update.migrated.updateDir";
>+
>+/*
>+ * Migrates old update directory files to the new update directory
>+ * which is based on a hash of the installation.
>+ */
>+function migrateOldUpdateDir() {
>+  // Obtain the old update root
>+  var exeFile = FileUtils.getFile("XREExeF", []);
>+  var updateLeafName = exeFile.parent.leafName;
>+  var oldUpdateRoot;
>+  if (Services.appinfo.vendor) {
>+    oldUpdateRoot = FileUtils.getDir("LocalAppData",
>+                                      [Services.appinfo.vendor,
>+                                      Services.appinfo.name,
>+                                      updateLeafName], false);
nit: alignment in the array

>+  } else {
>+    oldUpdateRoot = FileUtils.getDir("LocalAppData",
>+                                      [Services.appinfo.name,
>+                                      updateLeafName], false);
nit: same here

>+  }
>+
>+  // Obtain the new update root
>+  var newUpdateRoot = FileUtils.getDir("UpdRootD", [], true);
FileUtils.getDir will create the directory if it doesn't exist when the 3rd param is true!

  /**
   * Gets a directory at the specified hierarchy under a nsIDirectoryService
   * key.
   * @param   key
   *          The Directory Service Key to start from
   * @param   pathArray
   *          An array of path components to locate beneath the directory
   *          specified by |key|
   * @param   shouldCreate
   *          true if the directory hierarchy specified in |pathArray|
   *          should be created if it does not exist, false otherwise.
   * @param   followLinks (optional)
   *          true if links should be followed, false otherwise.
   * @return  nsIFile object for the location specified.
   */
  getDir: function FileUtils_getDir(key, pathArray, shouldCreate, followLinks) {


>+
>+  // If the old update root doesn't exist then we have already migrated
>+  // or else there is no work to do.
>+  if (!oldUpdateRoot.exists()) {
>+    Services.prefs.setIntPref(PREF_APP_UPDATE_MIGRATE_APP_DIR, 1);
Not needed per later comment to move where the pref is set.

>+    return;
>+  }
I was asking if it would be possible to use the existence of the new update directory to determine if migration should occur instead of a pref and not as an additional check during migration. I still *think* this is a good thing to prevent multiple migrations such as could occur with a second profile. That might be handled better in a different manner though I haven't thought of one as of yet.

>+
>+  // oldUpdateRoot and newUpdateRoot will be the same if the application has
>+  // no taskbar ID in the registry.
>+  // updateLeafName.length == 0 is just a sanity check so we don't end up
>+  // deleting too much at the end of this function.
>+  if (oldUpdateRoot.path.toLowerCase() == newUpdateRoot.path.toLowerCase() ||
>+      updateLeafName.length == 0) {
>+    return;
>+  }
>+
>+  // Get an array of all of the files we want to migrate.
>+  // We do this so we don't copy anything extra.
>+  var filesToMigrate = [FILE_UPDATES_DB, FILE_UPDATE_ACTIVE,
>+                        ["updates", FILE_LAST_LOG],
>+                        ["updates", FILE_BACKUP_LOG],
>+                        ["updates", "0", FILE_UPDATE_STATUS]];
>+
>+  // Move each of those files to the new directory
>+  filesToMigrate.forEach(relPath => {
>+    let oldFile = oldUpdateRoot.clone();
>+    let newFile = newUpdateRoot.clone();
>+    if (relPath instanceof Array) {
>+      relPath.forEach(relPathPart => {
>+        oldFile.append(relPathPart);
>+        newFile.append(relPathPart);
>+      });
>+    } else {
>+      oldFile.append(relPath);
>+      newFile.append(relPath);
>+    }
>+
>+    try {
>+      oldFile.moveTo(newFile.parent, newFile.leafName);
>+    } catch (e) {
>+      Components.utils.reportError(e);
>+    }
>+  });
>+
>+  oldUpdateRoot.remove(true);
>+  Services.prefs.setIntPref(PREF_APP_UPDATE_MIGRATE_APP_DIR, 1);
Please set this pref elsewhere so there isn't a chance that someone could run this code path more than once.

Why int vs. bool?

>+}
>+#endif
>+
> /**
>  * Gets the specified directory at the specified hierarchy under the update root
>  * directory without creating it if it doesn't exist.
>  * @param   pathArray
>  *          An array of path components to locate beneath the directory
>  *          specified by |key|
>  * @return  nsIFile object for the location specified.
>  */
> function getUpdateDirNoCreate(pathArray) {
>   return FileUtils.getDir(KEY_UPDROOT, pathArray, false);
> }
> 
> function UpdateServiceStub() {
>+#ifdef XP_WIN
>+  // Don't attempt this migration more than once for perf reasons
>+  var migrated = 0;
>+  try {
>+    migrated = Services.prefs.getIntPref(PREF_APP_UPDATE_MIGRATE_APP_DIR);
>+  } catch (e) {
>+  }
>+
>+  if (!migrated) {
>+    try {
>+      migrateOldUpdateDir();
>+    } catch (e) {
>+      Components.utils.reportError(e);
>+    }
I *think* this is a good place to set the pref.

>+  }
>+#endif
>+
>   let statusFile = getUpdateDirNoCreate([DIR_UPDATES, "0"]);
>   statusFile.append(FILE_UPDATE_STATUS);
>   // If the update.status file exists then initiate post update processing.
>   if (statusFile.exists()) {
>     let aus = Components.classes["@mozilla.org/updates/update-service;1"].
>               getService(Ci.nsIApplicationUpdateService).
>               QueryInterface(Ci.nsIObserver);
>     aus.observe(null, "post-update-processing", "");

I'd like to take one final look at the updated patch and would like to hear your take on some of the comments such as the best way to prevent multiple migrations overwriting files, downgrades, etc.
Attachment #747521 - Flags: review?(robert.bugzilla) → review-
> FileUtils.getDir will create the directory if it doesn't exist 
> when the 3rd param is true!

I'm aware what the 3rd param does and of the function signature :)
I wanted to create it so I can copy files to it, I don't think that was a problem in my patch.
You'll notice that the oldUpdateRootDir had that param set to false because that one I didn't want to create.

> I was asking if it would be possible to use the existence of the new update directory to determine if migration should occur instead of a pref 
> and not as an additional check during migration. I still *think* this is a good thing to prevent multiple migrations such as could occur with a 
> second profile. That might be handled better in a different manner though I haven't thought of one as of yet.

I'm not sure if the new directory will be created at that point.  Also I'm not 100% confident it doesn't ever get deleted.
Checking if the old update dir exists seems more logical to me.

> Why int vs. bool?

No reason, I'll change to bool.

I think it would be easiest to stick with a pref and to just set it right after we do oldUpdateRoot.path.ToLowerCase() != newUpdateRoot.path.ToLowerCase whether or not there are errors after that.  In the worst case the files don't get migrated and it's not a big deal.
(In reply to Brian R. Bondy [:bbondy] from comment #76)
> > FileUtils.getDir will create the directory if it doesn't exist 
> > when the 3rd param is true!
> 
> I'm aware what the 3rd param does and of the function signature :)
> I wanted to create it so I can copy files to it, I don't think that was a
> problem in my patch.
> You'll notice that the oldUpdateRootDir had that param set to false because
> that one I didn't want to create.
Sorry about that... immediately following it there is an existence check and I misread it thinking you were checking for the existence of the new update dir.

Please move the check for existence of the old update dir to before the creation of the new update dir.

> > I was asking if it would be possible to use the existence of the new update directory to determine if migration should occur instead of a pref 
> > and not as an additional check during migration. I still *think* this is a good thing to prevent multiple migrations such as could occur with a 
> > second profile. That might be handled better in a different manner though I haven't thought of one as of yet.
> 
> I'm not sure if the new directory will be created at that point.  Also I'm
> not 100% confident it doesn't ever get deleted.
> Checking if the old update dir exists seems more logical to me.
Make sense now that you pointed out my misread.

So, if someone downgrades they won't re-migrate unless they use a new profile? Would be a good thing to document how it behaves in case bugs are filed.

> 
> > Why int vs. bool?
> 
> No reason, I'll change to bool.
Thanks

> 
> I think it would be easiest to stick with a pref and to just set it right
> after we do oldUpdateRoot.path.ToLowerCase() !=
> newUpdateRoot.path.ToLowerCase whether or not there are errors after that. 
> In the worst case the files don't get migrated and it's not a big deal.
Can you verify that Seamonkey sets the TaskBarID registry keys and the code does the right thing with SeaMonkey and Thunderbird in regards to reading the correct location for TaskBarID?
> Can you verify that Seamonkey sets the TaskBarID registry keys and the code 
> does the right thing with SeaMonkey and Thunderbird in regards to reading 
> the correct location for TaskBarID?

I've never built them but I can do this, it will just take me a bit to do.  I'll do that once I'm done this test I'm writing.

So you're OK with the pref after I address the pref int->bool and the code line move?

> re downgrades:
I think the only way to downgrade will be via the installer and in that case I think we clear both directories.
(In reply to Brian R. Bondy [:bbondy] from comment #78)
> > Can you verify that Seamonkey sets the TaskBarID registry keys and the code 
> > does the right thing with SeaMonkey and Thunderbird in regards to reading 
> > the correct location for TaskBarID?
> 
> I've never built them but I can do this, it will just take me a bit to do. 
> I'll do that once I'm done this test I'm writing.
Please don't... just verify that Seamonkey sets its TaskBarID and a visual inspection that the code will do the right thing regarding reading from the correct registry locations.

> 
> So you're OK with the pref after I address the pref int->bool and the code
> line move?
I think so... just as long as the pref always gets set even if the migration code fails.

> > re downgrades:
> I think the only way to downgrade will be via the installer and in that case
> I think we clear both directories.
Just want to make sure the scenarios are understood preferably before this lands. For example, a secondary user won't have the filesystem cleaned.
For 2 user accounts on windows I think we want to do the migration twice (once for each user).

For the same user account on Windows, for the first profile it would:
1) first profile migrates successfully - in this case the 2nd profile user would see that the old update dir does not exist and would no longer attempt to migrate after that (it would abort the migrate and set the pref for itself).
2) first profile fails to migrate - To fix this, I'll do an additional check to set the pref if any of the files in the new update dir already exist.
I haven't spent enough time thinking about this but what about users that don't perform an update but have the old files? Perhaps it would make sense to just move the update.status in nsUpdateServiceStub.js when the pref isn't set and do the rest of the work in nsUpdateService.js so these users get migrated as well?
(In reply to Robert Strong [:rstrong] (do not email) from comment #81)
> I haven't spent enough time thinking about this but what about users that
> don't perform an update but have the old files?

Why would such a user have update files if they didn't perform an update?
I don't think it matters anyway, if they have files in their old update dir, they should be in their new update dir.

> Perhaps it would make sense
> to just move the update.status in nsUpdateServiceStub.js when the pref isn't
> set and do the rest of the work in nsUpdateService.js so these users get
> migrated as well?

Are you talking about 2 people on the same computer with different profiles or 2 windows user accounts?  If we're talking about 2 windows users accounts then they have different profiles and different update directories already.  I don't see a problem with that.  I laid out each possibility in Comment 80.
(In reply to Brian R. Bondy [:bbondy] from comment #82)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #81)
> > I haven't spent enough time thinking about this but what about users that
> > don't perform an update but have the old files?
> 
> Why would such a user have update files if they didn't perform an update?
> I don't think it matters anyway, if they have files in their old update dir,
> they should be in their new update dir.
Two users both being able to update and both having updated previously. User 1 updates Firefox to the new update dir and user 2 runs Firefox after user 1 has updated.
User 1 and 2 have different profiles right?
And they have different update directories right?
c:\users\user1\...
c:\users\user2\...

Those users already have different update files in their old directories. We want both of the migrations to happen so that it works as if they didn't change update directories at all.  Having 2 sets of update files is an existing problem, not something that was introduced in this bug.
For example:
Before the patches
User 1 and user 2 can see the updates they have previously performed in updates.xml

After the patches
User 1 updates, is migrated, and see the new update and previous updates in updates.xml
User 2 sees no previous updates.
In your example, it's my understanding that:

User 1 updates, is migrated, and sees the new updates that he previously performed in updates.xml

User 2 starts firefox, and also gets his updates.xml and other files migrated. Since he has a different applocal directory than user1 and therefore different profile than user 1.  Because he has a different profile his pref is not set, so he gets migrated. His old update root directory still exists since it's located at a different location than user1.
In the User 2 instance, nsUpdateServiceStub.js won't run until after they apply an update. During that time they could check for an update and an empty updates.xml file will be created.
already discussed on irc, but as per that conversation:
UpdateServiceStub.js runs on every startup so migration will happen before a new update is downloaded. So both user's update files would be migrated per Comment 86.
Seamonkey seems to set TaskbarIDs: comm-central\suite\installer 
Thunderbird seems to set TaskbarIDs: comm-central\mail\installer 
Calendar does not seem to set TasskbarIDs: comm-central\calendar\installer

I added a check for a missing taskbar ID and to abort the migration in that case without setting the pref.
That way if it gets a taskbar ID in the future it would still get migrated.
Attached patch Patch v5 - Post update migration (obsolete) — Splinter Review
Attachment #747521 - Attachment is obsolete: true
Attachment #748044 - Flags: review?(robert.bugzilla)
Attachment #748045 - Flags: review?(robert.bugzilla)
Attachment #748044 - Flags: review?(robert.bugzilla) → review+
Attachment #748045 - Flags: review?(robert.bugzilla) → review+
Attached patch Patch v6 - Post update migration (obsolete) — Splinter Review
Added HKLM check fist for TaskbarIDs from PostUpdate.

Robert, btw I just remembered that the TaskbarIDs registry key is in HKCU for win8+ but HKLM for win7-.  This is so default changes never require a UAC elevation (which is only possible on win8+).
Attachment #748147 - Flags: review+
*Added HKLM check fist for TaskbarIDs from <strike>PostUpdate</strike> nsUpdateServiceStub.js.
Same as before just added commit message
Attachment #739584 - Attachment is obsolete: true
Attachment #748158 - Flags: review+
Same as before but with commit message
Attachment #743834 - Attachment is obsolete: true
Attachment #748161 - Flags: review+
Attachment #748044 - Attachment is obsolete: true
Changed regkey.open to regkey.create when setting the taskbar ID in HKCU, because on Winy- that key doesn't already exist and it causes tests to fail on non-win8
Attachment #748045 - Attachment is obsolete: true
Attachment #748335 - Flags: review+
Attachment #745327 - Attachment is obsolete: true
Attachment #749500 - Flags: review?(robert.bugzilla)
Comment on attachment 749500 [details] [diff] [review]
v5 - Install, uninstall changes

makes sense and interdiff changes look fine to me
Attachment #749500 - Flags: review?(robert.bugzilla) → review+
Attached patch Patch v7 - Post update migration (obsolete) — Splinter Review
:%s/programFiles.length/programFiles.path.length
Attachment #748147 - Attachment is obsolete: true
Attachment #749530 - Flags: review+
:%s/programFiles.length/programFiles.path.length 
mirror fix for test as well
Attachment #748335 - Attachment is obsolete: true
Attachment #749563 - Flags: review+
no longer hardcode firefox as appname for taskbarid inn toolkit code.
Attachment #749563 - Attachment is obsolete: true
Attachment #749832 - Flags: review+
no longer hardcode firefox as appname for taskbarid registry path
Attachment #749530 - Attachment is obsolete: true
Attachment #749833 - Flags: review+
Tests performed on oak before this was landed:
- Verify that an update from before this patch to after this patch works w/ service
- Verify that an update from before this patch to after this patch works w/o service
- Verify that an update from before ths bug to after this patch w/ service succeeds while a second instance is open.
- Verify that an update from before ths bug to after this patch w/o service succeeds while a second instance is open.
- Verify that an update after this patch to after this patch works w/ service
- Verify that an update after this patch to after this patch works w/o service
- Verify that an update after this patch to after this patch gives an error w/o service while a second instance is open.
- Verify that ann update after this patch to after this patch gives an error w/ service (first replace fails then falls back to normal replace which also fails with error 47) while a second instance is open.
- Verified an uninstall clears both old and new update directories
- Verified an install clears both old and new update directories
- Verified an update migrates update files to the new directory
- Verified an update does not migrates update files if they have already been migrated
Depends on: 873037
Depends on: 873039
Depends on: 873041
Depends on: 873043
You need to log in before you can comment on or make changes to this bug.