Brian R. Bondy [:bbondy] Reporter
	Description • 12 years ago

If you update <instaslldir>, it will actually update the files into <installdir>\browser\.

That means there will be a second copy of firefox.exe and friends inside the browser subfolder of your installation directory.

I think the fix for this is in the command line arguments passed to updater.exe before launching it in toolkit/xre/nsUpdateDriver.cpp.

	Brian R. Bondy [:bbondy] Reporter
	Comment 1 • 12 years ago

After resource splitting, updates are busted on elm. Updates are getting applied into <installdir>/browser. 
This patch uses the exedir instead. I also think we can remove the gredir and appdir being passed because I think everything we need is in the exedir.  The only things I could see were the updater ini file and the updater.exe which I think would both be in that exe dir.

Please see bug 789461.
I'm not sure if you've been keeping track on the gre/appdir/exedir discussions, but if you want I can request this review from bsmedberg instead.

	Brian R. Bondy [:bbondy] Reporter
	Comment 2 • 12 years ago

Pushed to elm, will sync review comments. 
Also I'm not sure if this patch would be safe on m-c or not.

	Brian R. Bondy [:bbondy] Reporter
	Comment 3 • 12 years ago

http://hg.mozilla.org/projects/elm/rev/0a83ab4cd253

	Brian R. Bondy [:bbondy] Reporter
	Comment 4 • 12 years ago

The original changeset for this on elm is here:
http://hg.mozilla.org/projects/elm/rev/6b638f9ec174

	Jim Mathies [:jimm]
	Comment 5 • 12 years ago

ping - rstrong, this is one of the bugs we need to fix on mc before we can merge.

	Jim Mathies [:jimm]
	Comment 6 • 12 years ago

bbondy - we just enabled mac and linux builds on elm and we have some update test failures similar to the windows failures. Adding the firefox-appdir to the manifest fixed these on windows, but on mac and linux some still show up. Any ideas?

https://tbpl.mozilla.org/php/getParsedLog.php?id=17798682&tree=Elm&full=1#error4

I can file a new bug on this if you like.

	Brian R. Bondy [:bbondy] Reporter
	Comment 7 • 12 years ago

I'll file a bug and will take it.

	Brian R. Bondy [:bbondy] Reporter
	Comment 8 • 12 years ago

(In reply to Brian R. Bondy [:bbondy] from comment #7)
> I'll file a bug and will take it.

I filed that bug at bug 820232. It's different failures from the last ones by the way.

> Adding the firefox-appdir to the manifest fixed these on windows, but on 
> mac and linux some still show up.

That particular fix was in the end reverted in favor of adding the needed prefs to the tests by the way.  That way non firefox programs that run the tests won't get failures.  As mentioned this new failure is not related though.

	Mike Hommey [:glandium] Assignee
	Comment 9 • 12 years ago

Comment on attachment 665114 [details] [diff] [review]
Patch v1

I have a different patch for this.

	Brian R. Bondy [:bbondy] Reporter
	Comment 13 • 12 years ago

Where is the patch? Is this work being reverted from elm in that case?

	Mike Hommey [:glandium] Assignee
	Comment 14 • 12 years ago

This is what I landed on elm in replacement for the previous patch(es). This simplifies the problem by using UpdRootD on all platforms. It needs more code cleanup and comment changes, but I would like comments on the approach. This fixes all updater-related oranges on the elm tree (and I also validated it doesn't break anything on current m-c)

	Mike Hommey [:glandium] Assignee
	Comment 16 • 12 years ago

Updated with the fix from bug 826565

	Robert Strong (they/them - no direct email)
	Comment 17 • 12 years ago

I will hopefully have this review done this evening. I have one important question atm

>+function getAppBaseDir() {
>+  return Services.dirsvc.get("XREExeF", Ci.nsIFile).parent;
Which directory is this with XULRunner?

>+}

	Mike Hommey [:glandium] Assignee
	Comment 18 • 12 years ago

(In reply to Robert Strong [:rstrong] (do not email) from comment #17)
> I will hopefully have this review done this evening. I have one important
> question atm
> 
> >+function getAppBaseDir() {
> >+  return Services.dirsvc.get("XREExeF", Ci.nsIFile).parent;
> Which directory is this with XULRunner?

With the stub, it's the application directory. With plain xulrunner... it's the xulrunner directory :-/

	Mike Hommey [:glandium] Assignee
	Comment 19 • 12 years ago

(In reply to Mike Hommey [:glandium] from comment #18)
> With the stub, it's the application directory. With plain xulrunner... it's
> the xulrunner directory :-/

That being said, does the updater currently work when the app is run with xulrunner /path/to/application.ini?

	Robert Strong (they/them - no direct email)
	Comment 20 • 12 years ago

It only works when the XULRunner app is packaged in the same dir with the app being the top dir and XULRunner in a sub dir. For example, sognbird.

	Mike Hommey [:glandium] Assignee
	Comment 21 • 12 years ago

(In reply to Robert Strong [:rstrong] (do not email) from comment #20)
> It only works when the XULRunner app is packaged in the same dir with the
> app being the top dir and XULRunner in a sub dir. For example, sognbird.

Using the xulrunner-stub, right?

	Robert Strong (they/them - no direct email)
	Comment 22 • 12 years ago

Not positive though I would hope so

	Robert Strong (they/them - no direct email)
	Comment 23 • 12 years ago

Just a heads up that I have been reviewing this patch and just need to verify a couple of things locally before finishing the review.

	Jim Mathies [:jimm]
	Comment 24 • 12 years ago

How goes the review work? Hate to be a nag but, with packager on mc now we are down to two bugs including this one.

	Robert Strong (they/them - no direct email)
	Comment 25 • 12 years ago

I've cleared away this afternoon to work on it so hopefully by end of day

	Robert Strong (they/them - no direct email)
	Comment 26 • 12 years ago

Comment on attachment 701766 [details] [diff] [review]
Use the executable file location to derive the update root

I'm still spinning on this review... at this time specifically due to UpdRootD's original purpose being to provide a directory location that is outside of the install dir for writing update files. It seems to me this problem could be solved for Windows only with fewer changes and hence less risk while allowing the original purpose of UpdRootD to remain the same. I don't think I mind the change of UpdRootD though I still need to consider some of the ramifications of this change including the fallback to XCurProcD not being removed.

	Robert Strong (they/them - no direct email)
	Comment 27 • 12 years ago

glandium, what is the main reason you want to make UpdRootD available on all platforms? I'm tempted to go with Brian's approach to lessen the risk of regression on other platforms especially with XULRunner. Also, at some point we will want the ability to have an UpdRootD outside of the install dir for Mac OS X and now that there are a couple of more Mac devs on the platform integration team the liklihood of that happening soon is promising.

	Robert Strong (they/them - no direct email)
	Comment 28 • 12 years ago

Comment on attachment 701766 [details] [diff] [review]
Use the executable file location to derive the update root

With the concerns I've noted I am going to minus this. If you still think this is the better way to go over the previous patch's approach please comment with the reasons and I'll evaluate them.

	Mike Hommey [:glandium] Assignee
	Comment 29 • 12 years ago

Every place where UpdRootD is gotten from the directory service currently does its own (wrong) fallback to the application directory as the root for updates. Making all platforms use UpdRootD simplifies the problem by doing it in one place. Making OSX have an UpdRootD outside of the install dir should not be more work after this patch than before.

	Robert Strong (they/them - no direct email)
	Comment 30 • 12 years ago

In regards to using the wrong fallback are you referring to using XCurProcD instead of XREExeF? If not, what specifically? Also, with UpdRootD being available everywhere why are the fallbacks still present in nsUpdateService.js?

	Robert Strong (they/them - no direct email)
	Comment 31 • 12 years ago

(In reply to Robert Strong [:rstrong] (do not email) from comment #30)
> In regards to using the wrong fallback are you referring to using XCurProcD
> instead of XREExeF?
Specifically, the parent dir for XREExeF

	Robert Strong (they/them - no direct email)
	Comment 32 • 12 years ago

Comment on attachment 701766 [details] [diff] [review]
Use the executable file location to derive the update root

>diff --git a/browser/components/test/browser_bug538331.js b/browser/components/test/browser_bug538331.js
>--- a/browser/components/test/browser_bug538331.js
>+++ b/browser/components/test/browser_bug538331.js
>@@ -410,17 +410,17 @@ function writeUpdatesToXMLFile(aText)
> 
>   const MODE_WRONLY   = 0x02;
>   const MODE_CREATE   = 0x08;
>   const MODE_TRUNCATE = 0x20;
> 
>   const kIsWin = (navigator.platform.indexOf("Win") == 0);
remove the above since it is no longer used

>diff --git a/js/xpconnect/shell/xpcshell.cpp b/js/xpconnect/shell/xpcshell.cpp
>--- a/js/xpconnect/shell/xpcshell.cpp
>+++ b/js/xpconnect/shell/xpcshell.cpp
>@@ -2102,19 +2102,17 @@ XPCShellDirProvider::GetFile(const char 
> #ifdef MOZ_APP_BASENAME
>         localFile->AppendNative(NS_LITERAL_CSTRING(MOZ_APP_BASENAME));
> #endif
>         // However app name is always appended.
>         localFile->AppendNative(NS_LITERAL_CSTRING(MOZ_APP_NAME));
> #endif
>         return localFile->Clone(result);
> #else
>-        // Fail on non-Windows platforms, the caller is supposed to fal back on
>-        // the app dir.
>-        return NS_ERROR_FAILURE;
>+        return mAppFile->GetParent(result);
>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
>@@ -63,29 +63,21 @@ const URI_BRAND_PROPERTIES      = "chrom
> const URI_UPDATES_PROPERTIES    = "chrome://mozapps/locale/update/updates.properties";
> const URI_UPDATE_NS             = "http://www.mozilla.org/2005/app-update";
> 
> const CATEGORY_UPDATE_TIMER               = "update-timer";
> 
> const KEY_APPDIR          = "XCurProcD";
Seems that the current cases where UpdRootD falls back to XCurProcD should either just the fall back. If a fallback is necessary then it should be in nsXREDirProvider.cpp since with this patch it is now always the source for the update root.
For the other cases where XCurProcD is used in this and other files it should use getAppBaseDir or Services.dirsvc.get("XREExeF", Ci.nsIFile).parent;
If that is the case is there any reason not to remove the above and change the files accordingly?

> const KEY_GRED            = "GreD";
> 
>-#ifdef XP_WIN
>-#define USE_UPDROOT
>-#elifdef ANDROID
>-#define USE_UPDROOT
>-#endif
>-
> #ifdef MOZ_WIDGET_GONK
> #define USE_UPDATE_ARCHIVE_DIR
> #endif
> 
>-#ifdef USE_UPDROOT
> const KEY_UPDROOT         = "UpdRootD";
>-#endif
nit: Please move this up to directly below
const KEY_GRED            = "GreD";

> 
> #ifdef USE_UPDATE_ARCHIVE_DIR
> const KEY_UPDATE_ARCHIVE_DIR = "UpdArchD"
> #endif
> 
> #ifdef XP_WIN
> #define SKIP_STAGE_UPDATES_TEST
> #elifdef MOZ_WIDGET_GONK
>@@ -613,35 +605,37 @@ function binaryToHex(input) {
> #  Gets the specified directory at the specified hierarchy under the
> #  update root directory and creates 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 getUpdateDirCreate(pathArray) {
>-#ifdef USE_UPDROOT
>   try {
>     let dir = FileUtils.getDir(KEY_UPDROOT, pathArray, true);
>     return dir;
>   } catch (e) {
>   }
>-#endif
>   return FileUtils.getDir(KEY_APPDIR, pathArray, true);
> }
> 
>+function getAppBaseDir() {
>+  return Services.dirsvc.get("XREExeF", Ci.nsIFile).parent;
Please add const KEY_XRE_EXE_FILE "XREExeF"; where the other keys are declared and use it here.

>+}
>+
> /**
>  * Gets the root of the installation directory which is the application
>  * bundle directory on Mac OS X and the location of the application binary
>  * on all other platforms.
>  *
>  * @return nsIFile object for the directory
>  */
> function getInstallDirRoot() {
>-  var dir = FileUtils.getDir(KEY_APPDIR, [], false);
>+  var dir = getAppBaseDir();
> #ifdef XP_MACOSX
>   // On Mac, we store the Updated.app directory inside the bundle directory.
>   dir = dir.parent.parent;
> #endif
>   return dir;
> }
> 
> /**

>...
>@@ -923,36 +915,26 @@ function cleanUpUpdatesDir(aBackgroundUp
> 
>   var e = updateDir.directoryEntries;
>   while (e.hasMoreElements()) {
>     var f = e.getNext().QueryInterface(Ci.nsIFile);
>     // Preserve the last update log file for debugging purposes
>     if (f.leafName == FILE_UPDATE_LOG) {
>       var dir;
>       try {
>-#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.
>-        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) {
>+        if (aBackgroundUpdate && getUpdateDirCreate([]).equals(getAppBaseDir())) {
I think this should be getUpdateDirNoCreate. Any reason it shouldn't?

>           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 +
>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
>@@ -9,42 +9,32 @@ Components.utils.import("resource://gre/
> 
> const Ci = Components.interfaces;
> 
> const DIR_UPDATES         = "updates";
> const FILE_UPDATE_STATUS  = "update.status";
> 
> const KEY_APPDIR          = "XCurProcD";
> 
nit: remove the now extra empty line

>-#ifdef XP_WIN
>-#define USE_UPDROOT
>-#elifdef ANDROID
>-#define USE_UPDROOT
>-#endif
>-
>-#ifdef USE_UPDROOT
> const KEY_UPDROOT         = "UpdRootD";
>-#endif
> 

Sorry about taking so long. I'd like to see an updated patch which should just take a few minutes to review.

	Mike Hommey [:glandium] Assignee
	Comment 33 • 12 years ago

(In reply to Robert Strong [:rstrong] (do not email) from comment #30)
> In regards to using the wrong fallback are you referring to using XCurProcD
> instead of XREExeF? 

Yes

> Also, with UpdRootD being available everywhere why are the fallbacks still present in
> nsUpdateService.js?

See comment 14 :)

	Mike Hommey [:glandium] Assignee
	Comment 34 • 12 years ago

(In reply to Robert Strong [:rstrong] (do not email) from comment #32)
> >+        if (aBackgroundUpdate && getUpdateDirCreate([]).equals(getAppBaseDir())) {
> I think this should be getUpdateDirNoCreate. Any reason it shouldn't?

No reason other than the fact that the function doesn't exist. I'll add one.

	Robert Strong (they/them - no direct email)
	Comment 36 • 12 years ago

Comment on attachment 710738 [details] [diff] [review]
Use the executable file location to derive the update root

>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
>@@ -613,35 +605,36 @@ function binaryToHex(input) {
> #  Gets the specified directory at the specified hierarchy under the
> #  update root directory and creates 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 getUpdateDirCreate(pathArray) {
>-#ifdef USE_UPDROOT
>-  try {
>-    let dir = FileUtils.getDir(KEY_UPDROOT, pathArray, true);
>-    return dir;
>-  } catch (e) {
>-  }
>-#endif
>-  return FileUtils.getDir(KEY_APPDIR, pathArray, true);
>+  return FileUtils.getDir(KEY_UPDROOT, pathArray, true);
>+}
>+

Please add
/**
#  Gets the specified directory at the specified hierarchy under the
#  update root directory and 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);
>+}
>+

Same here
>+function getAppBaseDir() {
>+  return Services.dirsvc.get(KEY_EXECUTABLE, Ci.nsIFile).parent;
> }

Still a tad nervous that the parent of XREExeF does the right thing for all XULRunner apps in that on Linux and Windows with <appdir>/xulrunner it always returns <appdir> and on Mac with <appbundle.app>/Contents/Mac/xulrunner.app/<etc> it always returns <appbundle.app>/Contents/Mac though that is mainly from my lack of experience with XREExeF and that XREExeF isn't used heavily in tree.

Thanks for cleaning this up!

	Mike Hommey [:glandium] Assignee
	Comment 37 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/46ee2f9e8f91

	Ryan VanderMeulen [:RyanVM]
	Comment 38 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/46ee2f9e8f91