105087 - XPInstall needs to use generic directory locating mechanism.

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Judson Valeski]

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?

Comment 1

[Daniel Veditz [:dveditz]]

taking, nominating, setting target milestone

Comment 2

[Daniel Veditz [:dveditz]]

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.

Comment 3

[Conrad Carlen (not reading bugmail)]

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. 

Comment 4

[Daniel Veditz [:dveditz]]

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.

Comment 5

[Jimmy Lee]

Ccing Grace.

Comment 6

[Syd Logan]

DPRICEFIX

Comment 7

[dprice (gone)]

Attachment: proposed fix

Comment 8

[dbragg]

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

Comment 9

[Daniel Veditz [:dveditz]]

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.

Comment 10

[Judson Valeski]

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.

Comment 11

[dprice (gone)]

Attachment: patch with suggestions

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 12

[Doug Turner (:dougt)]

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

Comment 13

[Conrad Carlen (not reading bugmail)]

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.

Comment 14

[dprice (gone)]

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.

Comment 15

[Conrad Carlen (not reading bugmail)]

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

Comment 16

[Daniel Veditz [:dveditz]]

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.

Comment 17

[Daniel Veditz [:dveditz]]

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 18

[Doug Turner (:dougt)]

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 19

[Daniel Veditz [:dveditz]]

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).

Comment 20

[dprice (gone)]

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?

Comment 21

[dprice (gone)]

update milestone

Comment 22

[dprice (gone)]

Attachment: another patch

Comment 23

[Daniel Veditz [:dveditz]]

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 24

[Daniel Veditz [:dveditz]]

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?)

Comment 25

[dprice (gone)]

Attachment: another patch

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.

Comment 26

[Syd Logan]

adding + to topembed per adt triage and dan veditz

Comment 27

[Daniel Veditz [:dveditz]]

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 28

[Daniel Veditz [:dveditz]]

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.

Comment 29

[dprice (gone)]

Attachment: another patch

Comment 30

[dprice (gone)]

Attachment: diff with -w to hide whitespace changes

Comment 31

[Daniel Veditz [:dveditz]]

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 32

[dprice (gone)]

Attachment: change to nsSoftwareUpdate.cpp

Comment 33

[dprice (gone)]

Attachment: diff not the file

Comment 34

[Daniel Veditz [:dveditz]]

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 35

[Daniel Veditz [:dveditz]]

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=

Comment 36

[Daniel Veditz [:dveditz]]

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 37

[dprice (gone)]

Attachment: Full patch with final change

Comment 38

[Scott MacGregor]

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

sr=mscott

Comment 39

[dprice (gone)]

requesting adt1.0.0+

Comment 40

[dprice (gone)]

from syd ADT2

Comment 41

[Jaime Rodriguez, Jr.]

adt1.0.0+ for checkin.

Comment 42

[Asa Dotzler [:asa]]

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

Comment 43

[dprice (gone)]

checked in

Comment 44

[Jimmy Lee]

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!

Comment 45

[Jimmy Lee]

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