Hi guys, Since yesterday I am unable to update my Firefox nightly - I get "Click to update" in the about box, but it tells me "You are currently in the update channel" and I am unable to restart to update. Numerous people on MozillaZine forums can reproduce this issue, the thread is here: http://forums.mozillazine.org/viewtopic.php?f=23&t=2658755&start=15 The only thing in common is non-default paths. Partial update has worked until now Thanks

Mike and Brian, could you look into this? I'm out sick today and highly suspect this is fallout from bug 793767

Is it windows only ? All platforms ? What is a non-default path ?

(In reply to Mike Hommey [:glandium] from comment #2) > Is it windows only ? All platforms ? What is a non-default path ? I changed the initial install path from Program files, to D:\FirefoxInstall\ Does it for me on Windows 8, some other people can report this with W7

I can reproduce. It fails when number of character of the install folder name <=5 (such as 12, 12345, abc, abcde) And an error in Error console Error: NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService] Source file: resource://gre/modules/XPCOMUtils.jsm Line: 202 Regression window(m-c) Good: http://hg.mozilla.org/mozilla-central/rev/fe1600b22c77 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130208 Firefox/21.0 ID:20130208090713 Bad: http://hg.mozilla.org/mozilla-central/rev/c1ee454506f6 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130208 Firefox/21.0 ID:20130208142414 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fe1600b22c77&tochange=c1ee454506f6 Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/b871cfbc6cd2 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130208 Firefox/21.0 ID:20130208091814 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/d17e8470d7d9 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130208 Firefox/21.0 ID:20130208092216 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b871cfbc6cd2&tochange=d17e8470d7d9 Suspected: 46ee2f9e8f91 Mike Hommey — Bug 793767 - Use the executable file location to derive the update root. r=rstrong

I have a meeting starting now, then dinner but I'll take a look tonight.

I have this issue on Win7, too. Non-default install path : checked number of character of the install folder name <=5 : checked (Actually, it is "the last subdirectory" name <=5. If I modify it to > 5 characters, then update works)

aboutDialog.js calls appUpdater() which calls this.isPending which calls this.um which throws. In particular inside nsUpdateService.js "function UpdateManager()" throws on this line: > var updates = this._loadXMLFileIntoArray(getUpdateFile( > [FILE_UPDATE_ACTIVE])); You can reproduce with just this code in the JS console: > Components.utils.import("resource://gre/modules/FileUtils.jsm"); > const KEY_UPDROOT = "UpdRootD"; > function getUpdateDirCreate(pathArray) { > return FileUtils.getDir(KEY_UPDROOT, pathArray, true); > } > getUpdateDirCreate(["active-update.xml"]).path; This code calls into: nsXREDirProvider::GetUpdateRootDir(nsIFile* *aResult) Which fails on this line: > if (longPath.Length() < programFilesLen) <----------- > return NS_ERROR_FAILURE; So in particular we don't obtain the directory exactly when the dir length is less than the length of the path to your program files directory. On a 64-bit machine (w/ a 32-bit build) this will be strlen("C:\\program files (x86)\\") and on a 32-bit machines (or 64 w/ 64bit build) this would be strlen("C:\\program files\\");

Comment on attachment 712804 [details] [diff] [review] Patch v1. Review of attachment 712804 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/xre/nsXREDirProvider.cpp @@ -994,5 @@ > uint32_t programFilesLen = programFiles.Length(); > > - if (longPath.Length() < programFilesLen) > - return NS_ERROR_FAILURE; > - Considering the intent in the previous code, i think GetUpdateRootDir should be returning the value of updRoot it has at that moment.

As per glandium's suggestion. I'm not sure if this will cause problems with background updates though given the below comment: > // We need the update root directory to live outside of the installation > // 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. Additionally I'm a bit concerned about medium integrity installation dirs (like program files) outside of program files.

If the first patch is preferred I think these lines can also be removed in that function inside the ifdef XP_WIN: > 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); I'll wait for feedback before updating though since I think you want the second patch and that my above concerns have reasons they shouldn't be concerns.

additionally in favor of patchv1, I'm not sure why paths less than the length of program files should return something completely different than paths that are longer or equal to the length of program files.

If I'm reading things correctly, patch v2 would get us back to the behavior before bug 793767. Maybe there was already a problem that would be nice to fix, but I think it would be better to get to something we know works, at least for some reason, than trying to fix things and possibly break others in the process. We can get to fixing things in a followup.

(In reply to Brian R. Bondy [:bbondy] from comment #11) > additionally in favor of patchv1, I'm not sure why paths less than the > length of program files should return something completely different than > paths that are longer or equal to the length of program files. This feels like some sort of an ugly hack that needs to go away.

I think the best fix would be to always use the user's application data directory and use a hash of the install path as the subdirectory directory (whether or not that path is in program files), but we can do that in a different bug if you want. rstrong please let me know about Comment 11 at review time in any case.

I talked to rstrong and he'd like to go with the route of Comment 14. I.e. using a hash appended to the directory name at all times no matter if it is in program files or not. Gerv, we'd like to use the code from nsis\Contrib\CityHash\cityhash inside toolkit/xre, is it OK to copy it somewhere and use there? If so I assume that I'd keep the licensing info on the top the same? Here's a copy of the license inside that file: // Copyright (c) 2011 Google, Inc. // // Permission is hereby granted, free of charge, to any person obtaining a copy // of this software and associated documentation files (the "Software"), to deal // in the Software without restriction, including without limitation the rights // to use, copy, modify, merge, publish, distribute, sublicense, and/or sell // copies of the Software, and to permit persons to whom the Software is // furnished to do so, subject to the following conditions: // // The above copyright notice and this permission notice shall be included in // all copies or substantial portions of the Software. // // THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR // IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, // FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE // AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN // THE SOFTWARE. // // CityHash Version 1, by Geoff Pike and Jyrki Alakuijala // // This file provides CityHash64() and related functions. // // It's probably possible to create even faster hash functions by // writing a program that systematically explores some of the space of // possible hash functions, by using SIMD instructions, or by // compromising on hash quality.

Why not use md5 or some other hash? it's not like we don't have various hashes in the tree already.

I wasn't sure if NSS would be initialized this early on, or if we have a hashing function outside of NSS we could use. I also considered using the win32 crypto API but there are concerns it may be slow to load and initialize crypto API for that. what do you suggest is best? If you know of something please cancel the request to gerv at the same time as you respond.

We avoided using NSS for the shortcut application user model id due to it happening during startup along with cost. I very much doubt the situation has changed though if it has I would have no problem with using NSS.

(In reply to Brian R. Bondy [:bbondy] from comment #11) > additionally in favor of patchv1, I'm not sure why paths less than the > length of program files should return something completely different than > paths that are longer or equal to the length of program files. All it did was return early when the install was obviously not under program files and with other changes it should be removed.

(In reply to Brian R. Bondy [:bbondy] from comment #15) > I talked to rstrong and he'd like to go with the route of Comment 14. I.e. > using a hash appended to the directory name at all times no matter if it is > in program files or not. BTW: That is bug 572162

If computing hashes requires full initialization of NSS then it's one more reason not to go ahead with bug 830880, and use SHA1 from mfbt here.

(In reply to Mike Hommey [:glandium] from comment #21) > If computing hashes requires full initialization of NSS then it's one more > reason not to go ahead with bug 830880, and use SHA1 from mfbt here. We should be careful about that; bsmith expressed unhappiness that things would be using mfbt's SHA1 instead of NSS's. Institutions who care about security want all crypto stuff (no matter how relevant to security) to come from a single location.

After pushing to try I'll land patch v1 here and use bug 572162 for the hash dir name work.

(In reply to Nathan Froyd (:froydnj) from comment #22) > Institutions who care about security want all crypto stuff (no matter how relevant to security) to come from a single location. Too bad for them we have three md5 implementations in use (and another unused one in the tree)

bsmith do you have a recommendation for bug 572162 as to which hashing code? It doesn't matter much to me what we use, we just need a semi-unique hash of an installation directory. Clearing gerv's needinfo until we decide which hashing code to use.

(In reply to Nathan Froyd (:froydnj) from comment #22) > (In reply to Mike Hommey [:glandium] from comment #21) > > If computing hashes requires full initialization of NSS then it's one more > > reason not to go ahead with bug 830880, and use SHA1 from mfbt here. > > We should be careful about that; bsmith expressed unhappiness that things > would be using mfbt's SHA1 instead of NSS's. Institutions who care about > security want all crypto stuff (no matter how relevant to security) to come > from a single location. Technically they only care about the things that are relevant to security, but usually when you decide to use a secure hash function it is because you are relying on some security property (usually guaranteed hash collision avoidance, like in this case). Like I said in bug 830880, NSS has the NSSLOWHASH_* API that can be used without NSS being innitialized. But, it should be fine to use Windows CryptoAPI instead. I don't think that initializing CryptoAPI will be expensive because everything in Windows is already using CryptoAPI so it should already be loaded into memory by other software. So, that's the approach I would recommend, if this is Windows-specific code anyway. NSS loads CAPI so it's unlikely that using CAPI will be slower than using NSS and it may be more work than desired to get the NSSLOWHASH_* functions enabled. I don't know exactly what this bug is about even after reading all the comments above, so I'll just blurt out two points that seem relevant: 1. The elevated installer should generally never read anything (other than the MAR file) that is not guaranteed to be in a secured location. 2. If the intent of the hashed data is to hold the path that will cause us to load some executable code (the omnijar or DLLs or whatever), then it isn't enough that the data hash correctly to some directory. We would also need to double-check that the directory that the hash identifies has the correct files in it. Again, I am not sure what is trying to be accomplished, but I am worried that we're expecting more security properties out of this hashing than we are actually going to get. (In reply to Mike Hommey [:glandium] from comment #24) > Too bad for them we have three md5 implementations in use (and another > unused one in the tree) Nobody cares about which MD5 implementations we use because MD5 is not a secure hash function and so it shouldn't be used for any purpose (it's too slow to be a good insecure hash), especially security-sensitive purposes.

Thanks for the info bsmith. > but I am worried that we're expecting more security properties out > of this hashing than we are actually going to get. No we just want to get a unique directory name in non-elevated user writable location inside a directory here: C:\Users\Brian\AppData\Local\Mozilla\Firefox\ So we just want: I.e. Hash(InstallPath) = __XYZ__ Then we'd use: C:\Users\Brian\AppData\Local\Mozilla\Firefox\firefox-__XYZ__ We could just use the full install path with slashes replaced with underscores or something like that but we're concerned about running into MAX_PATH bugs. The way it is working today is that it uses the dirname if in program files, which causes a clash if the user has both a 32-bit and 64-bit build. And we use the program name otherwise if the user installs outside of program files. This code will be called on every startup by the way.

Reproduced the issue on nightly 21.0a1 2013-02-10. Verified fixed in nightly 21.0a1 2013-02-15, aurora 21.0a2 2013-03-31 (the 21 beta cycle has just begun, no updates available)