Closed Bug 793767 Opened 7 years ago Closed 7 years ago

Nightly builds after platform splitting upgrades the wrong installation directory, ProcessUpdates should be passed exe dir

Categories

(Toolkit :: Application Update, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: bbondy, Assigned: glandium)

References

(Blocks 1 open bug)

Details

(Whiteboard: [metro-it1][metro-it2][metro-mvp][LOE:1][completed-elm])

Attachments

(2 files, 2 obsolete files)

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.
Blocks: 789461
Summary: Nightly builds after platform splitting upgrades the wrong installation directory → Nightly builds after platform splitting upgrades the wrong installation directory, ProcessUpdates should be passed ExecutableD
Blocks: metro-build
No longer blocks: 789461
Depends on: 789461
Attached patch Patch v1Splinter Review
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.
Attachment #665114 - Flags: review?(robert.bugzilla)
Pushed to elm, will sync review comments. 
Also I'm not sure if this patch would be safe on m-c or not.
Summary: Nightly builds after platform splitting upgrades the wrong installation directory, ProcessUpdates should be passed ExecutableD → Nightly builds after platform splitting upgrades the wrong installation directory, ProcessUpdates should be passed exe dir
Whiteboard: metro-preview → metro-preview completed-elm
The original changeset for this on elm is here:
http://hg.mozilla.org/projects/elm/rev/6b638f9ec174
Whiteboard: metro-preview completed-elm → [metro-preview][metro-it1][metro-mvp]
Whiteboard: [metro-preview][metro-it1][metro-mvp] → [metro-it1][metro-mvp][LOE:1]
ping - rstrong, this is one of the bugs we need to fix on mc before we can merge.
Whiteboard: [metro-it1][metro-mvp][LOE:1] → [metro-it1][metro-it2][metro-mvp][LOE:1]
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.
I'll file a bug and will take it.
(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.
Comment on attachment 665114 [details] [diff] [review]
Patch v1

I have a different patch for this.
Attachment #665114 - Flags: review?(robert.bugzilla)
Duplicate of this bug: 820232
Duplicate of this bug: 820201
Duplicate of this bug: 821038
Where is the patch? Is this work being reverted from elm in that case?
Assignee: netzen → mh+mozilla
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)
Attachment #692558 - Flags: review?(robert.bugzilla)
Whiteboard: [metro-it1][metro-it2][metro-mvp][LOE:1] → [metro-it1][metro-it2][metro-mvp][LOE:1][completed-elm]
Duplicate of this bug: 826565
Updated with the fix from bug 826565
Attachment #701766 - Flags: review?(robert.bugzilla)
Attachment #692558 - Attachment is obsolete: true
Attachment #692558 - Flags: review?(robert.bugzilla)
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?

>+}
(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 :-/
(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?
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.
(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?
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.
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.
I've cleared away this afternoon to work on it so hopefully by end of day
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.
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.
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.
Attachment #701766 - Flags: review?(robert.bugzilla) → review-
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.
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?
(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
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.
(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 :)
(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.
Attachment #701766 - Attachment is obsolete: true
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!
Attachment #710738 - Flags: review?(robert.bugzilla) → review+
https://hg.mozilla.org/mozilla-central/rev/46ee2f9e8f91
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Depends on: 840195
Blocks: 848283
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.