The packaging guidelines for XULRunner apps are posted here: http://developer.mozilla.org/en/docs/XULRunner:Deploying_XULRunner_1.8 Following these guidelines means that, at least on Windows and Linux (others?), Software Update will only apply patches to the xulrunner subdirectory. These patches should instead be applied to the main xulapp directory (the parent directory that contains application.ini) so that the xulapp's chrome, etc., can be updated. After chatting with darin and bsmedberg it has become clear that this issue will be resolved in a wholly different manner for Gecko1.9/Firefox3 because XULRunner may reside in an arbitrary location. The proposed fix is for the 1.8/Firefox2 branch ONLY and assumes that any XULRunner app is bundling a copy of xulrunner. In particular, this fix is needed by Songbird before the update service can be enabled.

This is one approach we could take to fix this bug. It relies on the nsXREDirProvider's 'resource:app' key. This value is set if we're running a XULRunner app. This dir then gets passed to the update driver which sets it as the working directory for the updater.

Comment on attachment 211628 [details] [diff] [review] patch v1.0 >Index: toolkit/xre/nsAppRunner.cpp > #if defined(MOZ_UPDATER) >+ // If this is a XULRunner app then the updater needs to know the base >+ // directory that contains the application.ini file. This should be the >+ // parent of the xulrunner directory on Windows/Linux and it should be the >+ // Contents directory on MacOSX. Just in case someone packaged their app >+ // incorrectly we'll pass the directory here. >+ nsCOMPtr<nsIFile> xulAppDir; >+ PRBool dummy; >+ rv = dirProvider.GetFile("resource:app", >+ &dummy, >+ getter_AddRefs(xulAppDir)); >+ if (NS_FAILED(rv)) >+ xulAppDir = nsnull; Is this patch against trunk or branch? On trunk, nsXREDirProvider::GetAppDir will return the xulapp directory (change from bug 321237), and this hunk is mostly meaningless. (And you have to pass .GetGREDir() as the first arg of ProcessUpdates() below. > static void > ApplyUpdate(nsIFile *appDir, nsIFile *updateDir, nsILocalFile *statusFile, >- int appArgc, char **appArgv) >+ nsIFile *xulAppDir, int appArgc, char **appArgv) Can we change this signature to be more clear: ApplyUpdate(nsIFile *greDir, nsIFile *updateDir, nsILocalFile *statusFile, nsIFile *appDir, int appArgv, char **appArgv) Then, appDir is never null (in Firefox greDir and appDir would be the same directory). >+ // Get the directory to which the update will be applied. On Mac OSX we need >+ // to apply the update to the Contents directory which is the parent of the >+ // parent of the appDir. On other platforms we will probably apply to the You don't mean Contents directory, you mean the Foo.app directory. The original comment was wrong nd should be fixed. >+ // appDir. However, if this is a XULRunner app then we need to apply the >+ // update to the directory that contains the application.ini file (which >+ // should be the parent of the appDir if the app was packaged correctly, but >+ // we'll use the dir passed to XRE_main just in case someone goofed). In XULRunner apps on Mac OS X the update will be applied from what directory? It should still be the *.app directory, no?

(In reply to comment #2) > Is this patch against trunk or branch? On trunk, nsXREDirProvider::GetAppDir > will return the xulapp directory (change from bug 321237), and this hunk is > mostly meaningless. No, this is for the 1.8 branch. I was under the impression that we would have a more sophisticated solution for the trunk. I'd just like to have something in place for 1.8.1. > Can we change this signature to be more clear: Sure. > Then, appDir is never null (in Firefox greDir and appDir would be the same > directory). dirProvider.GetFile("resource:app") currently returns NS_FAILED when called from firefox. Are you saying that we should pass the same dir twice rather than passing in nsnull? > You don't mean Contents directory, you mean the Foo.app directory. The original > comment was wrong nd should be fixed. I was basing those comments on the info provided at http://developer.mozilla.org/en/docs/XULRunner:Deploying_XULRunner_1.8 and it *looks* like the parent of the parent of the xulrunner dir (which is the directory used by the code) is Contents. I don't have a mac so I can't verify this myself. If this is not what was intended then the docs, code, and comments are all wrong and should be fixed. > In XULRunner apps on Mac OS X the update will be applied from what directory? > It should still be the *.app directory, no? Well, based on the structure outlined by the link above, I would think Contents would be sufficient. Are there more files in the foo.app dir that aren't listed on that doc?

> No, this is for the 1.8 branch. I was under the impression that we would have a > more sophisticated solution for the trunk. I'd just like to have something in > place for 1.8.1. We will, but I want to get this tested on trunk before we backport to branches. > dirProvider.GetFile("resource:app") currently returns NS_FAILED when called > from firefox. Are you saying that we should pass the same dir twice rather than > passing in nsnull? yes > > > You don't mean Contents directory, you mean the Foo.app directory. The original > > comment was wrong nd should be fixed. > > I was basing those comments on the info provided at > http://developer.mozilla.org/en/docs/XULRunner:Deploying_XULRunner_1.8 and it > *looks* like the parent of the parent of the xulrunner dir (which is the > directory used by the code) is Contents. I don't have a mac so I can't verify > this myself. If this is not what was intended then the docs, code, and comments > are all wrong and should be fixed. The complete and partial MARs from mozilla are applied from the Foo.app directory (which is the parent of the parent of the "bin" directory on mac). AFAIK the same directory should be used for mac updates in this scenario. > Well, based on the structure outlined by the link above, I would think Contents > would be sufficient. Are there more files in the foo.app dir that aren't listed > on that doc? no, but it would match the system used by the existing Firefox update mechanism.

An updated 1.8 branch patch based on previous suggestions. Trunk patch to follow shortly.

Here is v1.1 for the trunk.

Comment on attachment 211892 [details] [diff] [review] patch v1.1 for the 1.8 branch in nsAppRunner.cpp, you don't need to clone greDir or appDir, because they aren't modified... just assign them directly. Otherwise it looks fine.

With those changes.

Comment on attachment 212623 [details] [diff] [review] patch v1.2 for the 1.8 branch Kindly land the trunk version first and lets make sure there aren't obvious regressions, and after a couple days this can land on the branch. It's not 1.8.0.2-worthy but it can probably make 1.8.0.3 safely if there aren't reported regressions.

Checking in toolkit/xre/nsAppRunner.cpp; /cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v <-- nsAppRunner.cpp new revision: 1.133; previous revision: 1.132 done Checking in toolkit/xre/nsUpdateDriver.cpp; /cvsroot/mozilla/toolkit/xre/nsUpdateDriver.cpp,v <-- nsUpdateDriver.cpp new revision: 1.16; previous revision: 1.15 done Checking in toolkit/xre/nsUpdateDriver.h; /cvsroot/mozilla/toolkit/xre/nsUpdateDriver.h,v <-- nsUpdateDriver.h new revision: 1.3; previous revision: 1.2 done

This isn't a Firefox bug, and doesn't affect Firefox 2... clearing nomination. (We really need a generic Software Update component somewhere)

I've been watching the update bugs for the last few days and tipped off QA. So far I haven't heard of any regressions that this introduced on the trunk, so I'm going to find someone to land this on the branch in the near future.

For MOZILLA_1_8_BRANCH: Checking in mozilla/toolkit/xre/nsAppRunner.cpp; /cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v <-- nsAppRunner.cpp new revision: 1.113.2.8; previous revision: 1.113.2.7 done Checking in mozilla/toolkit/xre/nsUpdateDriver.cpp; /cvsroot/mozilla/toolkit/xre/nsUpdateDriver.cpp,v <-- nsUpdateDriver.cpp new revision: 1.11.2.5; previous revision: 1.11.2.4 done Checking in mozilla/toolkit/xre/nsUpdateDriver.h; /cvsroot/mozilla/toolkit/xre/nsUpdateDriver.h,v <-- nsUpdateDriver.h new revision: 1.2.4.1; previous revision: 1.2 done

(In reply to comment #9) > (From update of attachment 212623 [details] [diff] [review] [edit]) > it can probably make 1.8.0.3 safely if there aren't reported regressions. Let's not forget this for 1.8.0.3.

Comment on attachment 212623 [details] [diff] [review] patch v1.2 for the 1.8 branch approved for 1.8.0 branch, a=dveditz for drivers

fixed1.8.0.3