Currently the XPInstall engine uses the process directory to determine installation paths. This is too inflexible in the embedding space where embeddors may want to target other directories. Perhaps nsIDirectoryServiceProvider could be used for directory locations? Because that iface is frozen though, we may need to create a new one?

taking, nominating, setting target milestone

A related issue is that different apps can embed Gecko (Netscape, Beonex, Mozilla) and folks may need the executable name not just the directory (e.g. to create a shortcut launching the new XUL app they just installed). That may fall out of this. In the likely case it's slightly different I'll open a new bug when I get there.

Is there anything missing from directory service? If XPInstall uses the directory NS_XPCOM_CURRENT_PROCESS_DIR, that directory is settable by the embedding app when calling NS_InitEmbedding. Also, the locations in NS_APP_PLUGINS_DIR_LIST are also settable by the embedding app.

No, everything's there, we just have to convert XPInstall over to use it. We do use NS_XPCOM_CURRENT_PROCESS_DIR, but then hang everything off that because this code is exercised in the native install (standalone) case when the app directory provider isn't present. To get embedding working we'll get rid of the hacks and switch to doing the right thing, and then put a directory service provider into the xpistub.dll glue layer used by the native installers.

Ccing Grace.

DPRICEFIX

According to what I understand about the bug this looks good to me. r=dbragg

Comment on attachment 69641 [details] [diff] [review] proposed fix >Index: nsInstallFolder.cpp looks good. What tests did you run to make sure nothing broke? >Index: nsRegisterItem.cpp >+ NS_ASSERTION(dirService,"directory service lied to us"); >+ rv = dirService->Get(NS_APP_CHROME_DIR, >+ NS_GET_IID(nsIFile), getter_AddRefs(startupFile)); This won't work in the wizard case because there will be no provider for NS_APP_CHROME_DIR, yet this is the most common situation where we need to use the installed-chrome.txt file. You'll have to check whether nsSoftwareUpdate::GetProgramDirectory() exists, if so use the old code to the chrome directory, if not then use your new code. The existing code hardcoding "chrome" was kinda bad, it should be a constant at the top of the file #ifdef XP_MAC to "Chrome". When you play with low-level directory-changing stuff like this in the install engine it's good to test both running the browser and running the native install. You can build an installer by running "perl build.pl" in mozilla/xpinstall/wizard/windows/builder. You'll want to have lots of free space on your build drive :-) The other thing that should get changed is the location of the xpicleanup.dat file. It's currently hardcoded to NS_OS_CURRENT_PROCESS_DIR, but embedding clients may want the flexibility to move it to a subdirectory. Talk to ccarlen -- I think it should go in nsAppDirectoryServiceDefs.h The file is written to in ScheduledTasks.cpp and looked for in nsSoftwareUpdate.cpp and mozilla/xpfe/bootstrap/nsAppRunner.cpp. Jimmy can point you at some test cases for this. Both XPInstall files need to watch for the wizard case, and use nsSoftwareUpdate::GetProgramDirectory() if it's set.

yea, embeddors need that flexibility. note: ccarlen is out until at best next Monday (maybe longer). cc'ing adam lock who *might* be able to provide some insight on that key.

The new patch includes the generic location for the XPI cleanup dir. It adds NS_APP_INSTALL_CLEANUP_DIR to the nsAppFileLocationProvider, chages nsAppRunner.cpp and xpi cleanup to use the new mechanism

Comment on attachment 70873 [details] [diff] [review] patch with suggestions Dont add a new location: NS_APP_INSTALL_CLEANUP_DIR, if all it really is is the moz bin directory. Are these defined somewhere else: INSTALL_PLUGINS_DIR "Plug-ins" +#define INSTALL_COMPONENTS_DIR "Components" +#define INSTALL_CHROME_DIR

if (nsSoftwareUpdate::GetProgramDirectory()) // In the stub installer ... else ... Why is there this duality all over the place? I see comment 9, but I don't see how that can be. Whenever NS_InitXPCOM has been called, the provider in nsAppFileLocationProvider.cpp has been registered.

cclaren - The duality exists for the stub installer. It is pretty much saying if we're in the wizard we do things the way they've always been done.

But, in the wizard, isn't NS_InitXPCOM called? It must since services are being used. Is there something I'm missing?

Yes, XPCOM has been initialized. This is fairly old code, long predating the direectory service provider. I suppose we could add a directory service provider to xpistub.dll, but this is a far easier hack since all that code was there already. The reason for cloning the XPCOM directory for the cleanup dir is so we have a hook for embeddors to override. Jud suggested it would be better to provide that flexibility rather than to assume the XPCOM dir would always work. dprice will add a comment to that effect so people don't "fix" us.

Oh, I see the confusion. In the wizard case we're running in a temporary directory, with a local temporary components dir etc. But for purposes of installing we want our install targets relative to the final destination directory, not where the currently running (and soon to be deleted) XPCOM thinks things are.

Comment on attachment 70873 [details] [diff] [review] patch with suggestions Add a comment in nsAppFileLocationsProvider which describes what NS_APP_INSTALL_CLEANUP_DIR is. Assuming this builds and you tested it r=dougt.

Comment on attachment 70873 [details] [diff] [review] patch with suggestions Never got an answer about what tests you've run on this... >Index: mozilla/xpinstall/src/ScheduledTasks.cpp >=================================================================== >- directoryService->Get(NS_OS_CURRENT_PROCESS_DIR, >- NS_GET_IID(nsIFile), >- getter_AddRefs(iFileUtilityPath)); >- if (!iFileUtilityPath) >+ if (nsSoftwareUpdate::GetProgramDirectory()) // In the stub installer >+ { >+ rv = directoryService->Get(NS_OS_CURRENT_PROCESS_DIR, >+ NS_GET_IID(nsIFile), >+ getter_AddRefs(iFileUtilityPath)); >+ } If we're in the stub installer we want to use the stub-supplied nsSoftwareUpdate::GetProgramDirectory() -- the existing code is broken. In the wizard case NS_OS_CURRENT_PROCESS_DIR puts the cleanup file in the temp directory, where it promptly gets deleted instead of processed. If we were doing it this way it should have been NS_XPCOM_CURRENT_PROCESS_DIR >Index: mozilla/xpinstall/src/nsInstall.h >=================================================================== >+#ifdef XP_MAC >+#define INSTALL_PLUGINS_DIR "Plug-ins" [...] These should go in nsInstallFolder.h >Index: mozilla/xpinstall/src/nsRegisterItem.cpp >=================================================================== >RCS file: /cvsroot/mozilla/xpinstall/src/nsRegisterItem.cpp,v >retrieving revision 1.24 >diff -u -1 -0 -r1.24 nsRegisterItem.cpp >--- mozilla/xpinstall/src/nsRegisterItem.cpp 19 Feb 2002 16:01:45 -0000 1.24 >+++ mozilla/xpinstall/src/nsRegisterItem.cpp 22 Feb 2002 07:04:08 -0000 >@@ -26,20 +26,21 @@ > #include "nsRegisterItem.h" > #include "nsInstallResources.h" > #include "nsNetUtil.h" > #include "nsIFileChannel.h" > #include "nsXPIDLString.h" > #include "nsReadableUtils.h" > #include "nsInstallTrigger.h" > #include "nsIChromeRegistry.h" > #include "nsIDirectoryService.h" > #include "nsDirectoryServiceDefs.h" >+#include "nsAppDirectoryServiceDefs.h" > > MOZ_DECL_CTOR_COUNTER(nsRegisterItem) > > nsRegisterItem:: nsRegisterItem( nsInstall* inInstall, > nsIFile* chrome, > PRUint32 chromeType, > const char* path ) > : nsInstallObject(inInstall), mChrome(chrome), mChromeType(chromeType), mPath(path) > { > MOZ_COUNT_CTOR(nsRegisterItem); >@@ -261,40 +262,61 @@ > // the chrome registry to do it now. > NS_ASSERTION(mProgDir, "this.Prepare() failed to set mProgDir"); > > // construct a reference to the magic file > PRFileDesc* fd = nsnull; > nsCOMPtr<nsIFile> tmp; > PRBool bExists = PR_FALSE; > rv = mProgDir->Clone(getter_AddRefs(tmp)); > if (NS_SUCCEEDED(rv)) > { >+ if (!nsSoftwareUpdate::GetProgramDirectory()) // not in the stub installer >+ { >+ nsCOMPtr<nsIProperties> directoryService = >+ do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID, &rv); >+ if (NS_SUCCEEDED(rv) && directoryService) >+ { >+ rv = directoryService->Get(NS_APP_CHROME_DIR, >+ NS_GET_IID(nsIFile), >+ getter_AddRefs(tmp)); >+ if(NS_FAILED(rv)) >+ { >+ result = nsInstall::CHROME_REGISTRY_ERROR; >+ return result; >+ } >+ } >+ } >+ else >+ { >+ rv = nsSoftwareUpdate::GetProgramDirectory()->Clone(getter_AddRefs(tmp)); >+ >+ if (NS_SUCCEEDED(rv)) >+ { >+ tmp->Append(INSTALL_CHROME_DIR); >+ } >+ } > nsCOMPtr<nsILocalFile> startupFile( do_QueryInterface(tmp, &rv) ); > > if (NS_SUCCEEDED(rv)) > { >- rv = startupFile->Append("chrome"); >+ rv = startupFile->Exists(&bExists); >+ if (NS_SUCCEEDED(rv) && !bExists) >+ rv = startupFile->Create(nsIFile::DIRECTORY_TYPE, 0755); > if (NS_SUCCEEDED(rv)) > { >- rv = startupFile->Exists(&bExists); >- if (NS_SUCCEEDED(rv) && !bExists) >- rv = startupFile->Create(nsIFile::DIRECTORY_TYPE, 0755); >+ rv = startupFile->Append("installed-chrome.txt"); > if (NS_SUCCEEDED(rv)) > { >- rv = startupFile->Append("installed-chrome.txt"); >- if (NS_SUCCEEDED(rv)) >- { >- rv = startupFile->OpenNSPRFileDesc( >- PR_CREATE_FILE | PR_WRONLY, >- 0744, >- &fd); >- } >+ rv = startupFile->OpenNSPRFileDesc( >+ PR_CREATE_FILE | PR_WRONLY, >+ 0744, >+ &fd); > } > } > } > } > > if ( NS_SUCCEEDED(rv) && fd ) > { > PR_Seek(fd, 0, PR_SEEK_END); > const char* location = (mChromeType & CHROME_PROFILE) ? "profile" : "install"; > nsSoftwareUpdate::Shutdown() [...] > nsCOMPtr<nsIProperties> directoryService = > do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID, &rv); >- directoryService->Get(NS_OS_CURRENT_PROCESS_DIR, >- NS_GET_IID(nsIFile), >- getter_AddRefs(pathToCleanupUtility)); >+ if (NS_FAILED(rv)) >+ return; Shutdown is a tricky time in the life of the app and the rules sometimes change about when you can get services and when not. If we can't get the directory service I want to know about it immediately because that means we're leaving people in an indeterminant, half-installed state. The old code would nicely crash if someone changed the rules, this added return hides that fact. I could reluctantly accept a big screaming assertion, but honestly I'd prefer the potential crash. An assertion gets ignored in release builds but partial installs should be a smoketest blocker. >+ if (nsSoftwareUpdate::GetProgramDirectory()) // In the stub installer >+ { >+ rv = directoryService->Get(NS_OS_CURRENT_PROCESS_DIR, As before, just use the wizard-supplied program directory. >+ if (NS_FAILED(rv) || !pathToCleanupUtility) >+ return; As above I don't like the early, silent return. >Index: mozilla/xpfe/bootstrap/nsAppRunner.cpp >=================================================================== > nsCOMPtr<nsIProperties> directoryService = > do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID, &rv); > if (NS_FAILED(rv)) > return NS_OK; >- rv = directoryService->Get(NS_OS_CURRENT_PROCESS_DIR, >+ rv = directoryService->Get(NS_APP_INSTALL_CLEANUP_DIR, > NS_GET_IID(nsIFile), > getter_AddRefs(registryFile)); > if (NS_FAILED(rv) || !registryFile) > return NS_OK; Not your code originally, but should we be returning OK when we can't get basic things like these? Again, at least assertions, and failure return codes will abort the app and make someone figure it out. > #ifdef XP_MAC > registryFile->Append(ESSENTIAL_FILES); > #endif This mac-ishness should be hidden inside the directory service entry you added. >Index: mozilla/xpcom/io/nsAppFileLocationProvider.cpp >=================================================================== >- >+ else if (nsCRT::strcmp(prop, NS_APP_INSTALL_CLEANUP_DIR) == 0) >+ { >+ rv = CloneMozBinDirectory(getter_AddRefs(localFile)); >+ } Add the #ifdef XP_MAC Essential Files thing here, and the comment doug asked for. Maybe two comments, one in the .h saying what it is, and one here saying why we're just duplicating an already-known location (so embeddors can override).

For testing I... built and ran native mozilla and netscape installers on Linux and Windows ran update.html for mac, linux and windows ran a test install script that called getFolder for each of the folders I changed installed a theme set and hit a breakpoint in nsAppRunner and verified it has the right directory Did that hit everything?

update milestone

Since we normally do not want the commercial installers to cause a reboot simply building and running the default installers isn't a good test since that won't exercise the changed code involving the XPICleanup directory. One fairly small easy change for testing purposes would be to alter xpcom.jst to always install the msvcrt dll's instead of doing so only if they don't exist. Similarly the Mozilla install scripts in the tree don't use the Plugin, Chrome or Components targets you altered so those didn't get tested in the wizard case either. Sorry to hammer on the tests, but thinking about how to test will often expose design problems and bugs before they happen. Actually running the tests often turns up a few more "D'oh!" bugs. Keep in mind that QA on Mozilla is spread thin and largely a black-box affair, programmers need to do their own white-box testing.

Comment on attachment 72689 [details] [diff] [review] another patch >-#ifdef XP_MAC >- registryFile->Append(ESSENTIAL_FILES); >-#endif Good, but you forgot to remove this in the other places that use the new directory service entry. You will still have to Append this in the cases that use the stub-set directory, but only in those cases. >+ if (nsSoftwareUpdate::GetProgramDirectory()) // In the stub installer >+ { >+ nsCOMPtr<nsIFile> tmp = do_QueryInterface(iFileUtilityPath); >+ rv = nsSoftwareUpdate::GetProgramDirectory()->Clone(getter_AddRefs(tmp)); >+ } >+ else >+ { >+ rv = directoryService->Get(NS_APP_INSTALL_CLEANUP_DIR, >+ NS_GET_IID(nsIFile), >+ getter_AddRefs(iFileUtilityPath)); >+ } The state after these two blocks is not equivalent. (hint: what's the lifetime of tmp?)

I think this should go it. I added a new test for the installers. I build the installer, then edited the install.js in borwser.xpi I had it call GetFolder on all of the changed targets. It dumped the correct information to the log files.

adding + to topembed per adt triage and dan veditz

the patch appears to be damaged, I cannot apply it in my tree. Maybe bugzilla messed it up, would you mind mailing me a copy? Did you run the altered-xpcom.xpi test I suggested or an equivalent? The browser.xpi modifications you mention would catch the nsInstallFolder changes, but not the xpicleanup-related modifications

Comment on attachment 75033 [details] [diff] [review] another patch Dunno why I can't apply this, patch (two different versions) complains about line 80 being invalid (that's the Index: [...]nsInstallFolder.h line) >+ if (nsSoftwareUpdate::GetProgramDirectory()) // In the stub installer >+ { >+ nsCOMPtr<nsIFile> tmp = do_QueryInterface(iFileUtilityPath); >+ rv = nsSoftwareUpdate::GetProgramDirectory()->Clone(getter_AddRefs(tmp)); >+ >+ if (NS_FAILED(rv) || !iFileUtilityPath) >+ return nsnull; > > #if defined (XP_MAC) >- iFileUtilityPath->Append(ESSENTIAL_FILES); >+ iFileUtilityPath->Append(ESSENTIAL_FILES); > #endif >+ } I still think this use of tmp is wrong, and I'd like to hear that it worked in your tests. >Index: mozilla/xpinstall/src/nsJSInstall.cpp This looks like it's part of another bug >Index: mozilla/xpinstall/src/nsRegisterItem.cpp >=================================================================== > rv = mProgDir->Clone(getter_AddRefs(tmp)); > if (NS_SUCCEEDED(rv)) DOn't think you need to clone this if you're just going to stomp on tmp immediately after. In fact this code pretty much replaces the reason for keeping mProgDir around, so maybe we don't need that member anymore and it can just be a local in the ::Prepare() routine. >Index: mozilla/xpinstall/src/nsSoftwareUpdate.cpp >=================================================================== > do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID, &rv); >- directoryService->Get(NS_OS_CURRENT_PROCESS_DIR, >- NS_GET_IID(nsIFile), >- getter_AddRefs(pathToCleanupUtility)); >+ >+ if (nsSoftwareUpdate::GetProgramDirectory()) // In the stub installer >+ { >+ nsCOMPtr<nsIFile> tmp = do_QueryInterface(pathToCleanupUtility); >+ rv = nsSoftwareUpdate::GetProgramDirectory()->Clone(getter_AddRefs(tmp)); > #if defined (XP_MAC) > pathToCleanupUtility->Append(ESSENTIAL_FILES); > #endif Please fix the whitespace. Note that you're appending the name to a different object than the clone you just created -- I think you'd better get a mac build buddy so you can test this there. >Index: mozilla/xpcom/io/nsAppFileLocationProvider.cpp >=================================================================== >+ else if (nsCRT::strcmp(prop, NS_APP_INSTALL_CLEANUP_DIR) == 0) >+ { >+ // This is cloned so that embeddors will have a hook to override >+ // with their own cleanup dir. See bugzilla bug #105087 >+ rv = CloneMozBinDirectory(getter_AddRefs(localFile)); >+#ifdef XP_MAC >+ if (NS_SUCCEEDED(rv)) >+ rv = registryFile->Append(ESSENTIAL_FILES); >+#endif This isn't even going to compile on Mac, definitely better get that build buddy.

You forgot to fix up nsSoftwareUpdate.cpp -- once you fix it you can attach that as a separate one-file diff so I don't have to look at the rest of it again :-)

Comment on attachment 76544 [details] [diff] [review] another patch r=dveditz except for the diffs for nsSoftwareUpdate.cpp (see attachment 76847 [details] [diff] [review] for those)

Comment on attachment 76847 [details] [diff] [review] diff not the file >+ if (nsSoftwareUpdate::GetProgramDirectory()) // In the stub installer >+ { >+ nsCOMPtr<nsIFile> tmp; >+ rv = nsSoftwareUpdate::GetProgramDirectory()->Clone(getter_AddRefs(tmp)); > #if defined (XP_MAC) >- pathToCleanupUtility->Append(ESSENTIAL_FILES); >+ tmp->Append(ESSENTIAL_FILES); > #endif >+ pathToCleanupUtility = do_QueryInterface(tmp); >+ >+ } >+ else >+ { >+ rv = directoryService->Get(NS_APP_INSTALL_CLEANUP_DIR, >+ NS_GET_IID(nsIFile), >+ getter_AddRefs(pathToCleanupUtility)); >+ } >+ > //Create the Process framework > pathToCleanupUtility->Append(CLEANUP_UTIL); You should check this !null before using since potentially lots of things up there could go wrong. Should check the rv before appending the mac Essential files, too. r=dveditz with those changes, you probably should attach a new diff for sr=

You could just assert that pathToCleanupUtility was not null, that'd be enough to let embeddors know they forgot to implement NS_APP_CLEANUP_DIR

Comment on attachment 76855 [details] [diff] [review] Full patch with final change sr=mscott

requesting adt1.0.0+

from syd ADT2

adt1.0.0+ for checkin.

Comment on attachment 76855 [details] [diff] [review] Full patch with final change a=asa (on behalf of drivers) for checkin to the 1.0 trunk

checked in

Build: 2002-04-19-08-1.0.0(WIN), 2002-04-19-05-1.0.0(MAC 9.x/10.x), 2002-04-19-09-1.0.0(LINUX), This fixed checked-in before the 1.0.0 branch. Regressions tests for browser passed. getFolder() paths show no change for all platforms as expected. Embedding QA should verify that their directory service is supported for completeness. Ccing Michael. Marking Verified!

Adding keyword verified1.0.0. Fix was checked in before 1.0.0 branch. Still works.