161508 - (uninstaller) implement a component uninstaller

Status: RESOLVED WONTFIX

(SeaMonkey :: Installer, enhancement)

Description

[dprice (gone)]

the work will be done on UNINSTALLER_WORK_BRANCH

The spec lives at http://blues/users/dprice/publish/uninstall/uninstall.html

Comment 1

[dprice (gone)]

Attachment: patch to log uninstall information at install time

Comment 2

[dprice (gone)]

Attachment: nsISoftwareUninstall.idl

Comment 3

[dprice (gone)]

Attachment: patch 2

This new patch does two things.  It changes the install process so that it logs
the information we need in installed-components.txt and nsSoftwareUpdate
implements the nsISoftwareUninstall interface  It is to the point where one can
put a break point in nsSoftwareUpdate::RemovePackage() and you'll hit it when
you click on the button in the prefs pannel

Comment 4

[dprice (gone)]

Attachment: patch 3

These are changes to the logging progress notifier to make it support logging
uninstall information to an additional file.

Comment 5

[dprice (gone)]

Attachment: patch 4

These are changes to the existing install methods so they will log the right
information to the uninstall file.  This is includes adding new flags to the
initInstall and addFile methods.

Comment 6

[dprice (gone)]

Attachment: patch 5

this contains the first cut of the work to find a package, delete the files, 
and update our file lists.  It also contains the beginning of work on the 
GetInstalledPackages method.  Most of this work needs to be moved from
nsSoftwareUpdate and into a better home.

Comment 7

[dprice (gone)]

Attachment: patch 6 - idl, xpconnect javascript xul

This patch contains all of the .idl changes for the uninstaller.  It contains
the C++ classes that implement the interfaces.	I snipped a huge part out of
the nsSoftwareUninstall.cpp file because it isn't related.  The patch also
contains the .xul file that calls uses the new interfaces and does all the
wonderful UI

Comment 8

[dprice (gone)]

Attachment: everything so far

I AM THUNDER DIFF! ALL SHALL LOVE ME AND DESPAIR!
;)

This is a snapshot of everything in my tree right now.	I will be breaking it
up into smaller, more manageable diffs so that reviewers won't be swamped with
the size of these changes.  But I'm putting this up in case I miss something
when factoring out the changes.

Comment 9

[dprice (gone)]

patches, 3, 4 and 6 are up to date.

Comment 10

[dprice (gone)]

Attachment: patch 7

These are methods in the nsSoftwareUninstall class that are directly called by
nsSoftwareUninstall.

Comment 11

[dprice (gone)]

Attachment: patch 8

These methods iterate through the list of uninstall commands and fing the right
methods to do the real work.

Comment 12

[dprice (gone)]

Attachment: patch 9

methods that undo the file operations.	Delete file, unregister xpcomp
component etc.

Comment 13

[dprice (gone)]

Attachment: patch 10

this is mostly the .h files, class definitions etc. etc.

Comment 14

[Charles Manske]

Comment on attachment 96015 [details] [diff] [review]
patch 6 - idl, xpconnect javascript xul

r=cmanske for pref-smartupdate.xul.
Just one comment:
Why don't you simply do:
return aSoftwareUninstall.removePackage(regName, prettyName);
in BackendRemove() and drop:
var result = true;  and
return ( result );

Comment 15

[dprice (gone)]

Attachment: everything again

Here's the entire diff again.  This was done against the UNINSTALL_WORK_BASE.

Comment 16

[dprice (gone)]

Attachment: missed the .xul

Comment 17

[dprice (gone)]

Attachment: new PerformUninstallCommand

cleaned up some badness that I missed in PerformUninstallCommand

Comment 18

[dprice (gone)]

Attachment: diff that adds a perpare uninstall step

new patch that adds a prepare uninstall phase to the uninstaller

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 96545 [details] [diff] [review]
everything again

Comments on diff except for the new nsSoftwareUninstall.cpp (working on it)

>Index: cleanup/InstallCleanup.cpp
>===================================================================
>+#ifdef WIN32
>+    unregComplete   = UnregisterScheduledFiles( reg ); 
>+#endif

why win32 only?  (#ifdef WIN32 is discouraged, btw, use an XP_ equiv)


>+int UnregisterScheduledFiles( HREG reg )
>+{
...
>+        // the delete key exists, so we loop through its children
>+        // and try to delete all the listed files

Fix the "cut and past" comment

Instead of cut and paste maybe you could have parameterized this loop
with the registry key to look at.

>+                rv = NativeUnregisterComponent(valbuf);

There's nothing here that's platform specific so why wasn't it the
NativeUnregisterComponent() routine propogated to each platform
and then stubbed out there until you finished?

>\ No newline at end of file

Fix these, breaks some ports

>Index: cleanup/InstallCleanup.h
>@@ -42,8 +42,15 @@
> int PerformScheduledTasks(HREG);
> int DeleteScheduledFiles(HREG);
> int ReplaceScheduledFiles(HREG);
>+int UnregisterScheduledFiles(HREG);
> int NativeReplaceFile(const char* replacementFile, const char* doomedFile );
> int NativeDeleteFile(const char* aFileToDelete);
>+
>+#ifdef WIN32
>+int UnregisterComponent(char *componentPath);
>+int NativeUnregisterComponent(const char *aComponentFile);
>+int PerformUnregisterComponent(const char* aComponentFile);
>+#endif

What's the difference between UnregisterScheduledFiles
and the windows specific ones?
> 
> #endif //INSTALL_CLEANUP_H
> 
>Index: cleanup/InstallCleanupWin.cpp
>===================================================================
> int NativeDeleteFile(const char* aFileToDelete)
> {
>-    if (GetFileAttributes(aFileToDelete) == 0xFFFFFFFF)
>+    DWORD attributes = GetFileAttributes(aFileToDelete);
>+
>+    if (attributes == 0xFFFFFFFF)
>     {
>       return DONE;// No file to delete, do nothing.
>     }
>     else 
>     {
>-        if(!DeleteFile(aFileToDelete))
>-          return TRY_LATER;
>+        if (attributes & FILE_ATTRIBUTE_DIRECTORY)
>+            if(!RemoveDirectory(aFileToDelete))
>+                return TRY_LATER;
>+        else
>+            if(!DeleteFile(aFileToDelete))
>+                return TRY_LATER;

By shoving this into the windows-only file you will leave other platforms
stuck when they also hit the directory problem. Also, by burying it
at this lowest level you really haven't solved the original problem, because
you now have no way to know when only directories are left. Remember that
one of the problems we discussed is what to do if someone has put things
in your directory later, such as plugins. In those cases you will never
be able to delete the directory, and thus you will never more be able to
restart the browser.

*after* all the files are gone if you still can't delete the directories
we said it was ok to forget the directories if they failed. But you've
got no way to track when all the files are gone here.

>+// walling this off because we'll probably have to change the way we unregister
>+// components when MRE comes around.

Seems silly to "wall off" a 7 line routine from a 4 line routine in the same
file, especially when the sole purpose of the 4 line routine is to do what
the walled-off routine is doing. It'd be another thing if the parent
routine was doing lots of things and you were isolating one aspect, but not
when it's a pure wrapper.

>+int PerformUnregisterComponent(const char* aComponentFile)
>+{
>+    int maxCopy = MAXREGNAMELEN - 1;
>+    char unregisterCommand[MAXREGNAMELEN];
>+
>+    strncpy(unregisterCommand, UNREGISTER_COMMAND, maxCopy);
>+    maxCopy-= strlen(unregisterCommand);
>+    strncat(unregisterCommand, aComponentFile, maxCopy);
>+    system(unregisterCommand);

What if there are spaces in the component pathname?

Will strncat copy a null if maxCopy is 0? Doubt it, and in that unlikely case
(impossible here) your string would not be null terminated.

Not that it's going to matter much, but you'd be slightly more efficient
making UNREGISTER_COMMAND a static -- either way the string gets compiled
in.
   const char kUnregisterCommand[] = "regxpcom -u ";
Then use compile-time sizeof(kUnregisterCommand) instead of runtime strlen.

>Index: public/Makefile.in
>===================================================================
> 		nsPIXPIStubHook.idl \
>+    nsISoftwareUninstall.idl \
> 		$(NULL)

Whitespace oddness

>Index: public/makefile.win
>===================================================================
>             .\nsIXPINotifier.idl          \
>             .\nsPIXPIStubHook.idl         \
>             .\nsPIXPIProxy.idl            \
>+            .\nsISoftwareUninstall.idl    \
>+            .\nsIXPIPackageInfo.idl       \
>             $(NULL)

Why do the obsolete makefile.wins get an extra idl? Where are the mac
build changes? I wouldn't bother with makefile.win, they're gone
on the trunk.

>Index: public/nsISoftwareUninstall.idl
>===================================================================
>+ * Version: MPL 1.1

New files get the MPL tri-license.

>+[scriptable,uuid(b0822c5f-e1bd-4ff5-94e3-298f3cd5a1ab)]
>+interface nsISoftwareUninstall : nsISupports
>+{
>+  nsISimpleEnumerator getInstalledPackages();

nit: this will not return all installed packages, only the uninstallable ones.

>+  boolean removePackage(in AUTF8String packageRegName, in AUTF8String pacakgePrettyName);

nit: why not straightforward "uninstall"?

Why does someone need to pass in the pretty name to uninstall something?
Since that could change over time with versions it's a bad key.

>Index: public/nsIXPIPackageInfo.idl
>===================================================================
>+#include "nsISupports.idl"
>+[scriptable,uuid(53308be6-000a-45c7-881b-6892711a0cf8)]
>+interface nsIXPIPackageInfo : nsISupports
>+{
>+	[noscript] void Init(in AUTF8String aPrettyName, in AUTF8String aRegName, in AUTF8String aVersionString);

We talked about this, the init should go in the object (or beter, have
a real constructor), it shouldn't clutter up the public interface

>+%{C++
>+extern nsresult
>+NS_NewIXPIPackageInfo(nsIXPIPackageInfo** aResult);
>+
>+%}

This shouldn't be there either, only your code needs to make these.

>\ No newline at end of file

fix newline problems

>Index: public/nsIXPIPackeageInfo.idl
>===================================================================

What's this one? Looks empty.

>Index: public/nsSoftwareUpdateIIDs.h
>===================================================================
> 
>+#define NS_SoftwareUpdateInstallVersion_CID          \
>+{ /* 18c2f98f-b09f-11d2-bcde-00805f0e1353 */         \
>+    0x18c2f98f,                                      \
>+    0xb09f,                                          \
>+    0x11d2,                                          \
>+    {0xbc, 0xde, 0x00, 0x80, 0x5f, 0x0e, 0x13, 0x53} \
>+}

I think this one is already there? are you adding it again?

>+// {43E6370F-A86E-46bb-82F1-1415B8974751}
>+#define NS_SoftwareUninstall_CID                     \
>+{ /* {43E6370F-A86E-46bb-82F1-1415B8974751} */       \
>+    0x43e6370f,                                      \
>+    0xa86e,                                          \
>+    0x46bb,                                          \
>+    {0x82, 0xf1, 0x14, 0x15, 0xb8, 0x97, 0x47, 0x51} \
>+}
> 
>-#endif /* nsSoftwareUpdateIIDs_h___ */
>+#define NS_XPInstallPackageInfo_CID                  \
>+{ /* {5338E16A-C85F-40ad-B40A-1A8B644C5C42} */       \
>+    0x5338e16a,                                      \
>+    0xc85f,                                          \
>+    0x40ad,                                          \
>+    {0xb4, 0xa, 0x1a, 0x8b, 0x64, 0x4c, 0x5c, 0x42}  \
>+}

You don't need or use these.

>Index: src/ScheduledTasks.h
>===================================================================
>RCS file: /cvsroot/mozilla/xpinstall/src/ScheduledTasks.h,v
>retrieving revision 1.15
>diff -u -r1.15 ScheduledTasks.h
>--- src/ScheduledTasks.h	26 Mar 2002 00:04:03 -0000	1.15
>+++ src/ScheduledTasks.h	24 Aug 2002 09:16:06 -0000
>@@ -37,7 +37,7 @@
> 
> PRInt32 DeleteFileNowOrSchedule(nsIFile* filename);
> PRInt32 ReplaceFileNowOrSchedule(nsIFile* tmpfile, nsIFile* target, PRInt32 aMode);
>-PRInt32 ScheduleFileForDeletion(nsIFile* filename);
>+PRInt32 ScheduleFile(nsIFile* filename, char* key);

I'm not keen on this change. Combining the routines internally, sure,
but now everyone needs to know the specific registry key details in
order to call this routiner.

Please change the module-public name back.

>Index: src/makefile.win
>===================================================================

No equivalent Makefile.in or macbuild changes

>Index: src/nsInstall.cpp
>===================================================================
>+    if (mLogUninstall)
>+    {
>+        rv = nsSoftwareUninstall::OpenUninstallLogStream(&uninstallLogStream);
>+
>+        if (NS_FAILED(rv) || !uninstallLogStream)
>+        {
>+            *aReturn = SaveError( nsInstall::UNEXPECTED_ERROR );
>+            mFinalStatus = *aReturn;
>+            return NS_OK;
>+        }
>+        *uninstallLogStream << "[" << NS_ConvertUCS2toUTF8(mRegistryPackageName).get() << "]" << nsEndl;
>+        *uninstallLogStream << "PrettyName=" << NS_ConvertUCS2toUTF8(mUIName).get() << nsEndl;
>+    }

These details should be hidden in your nsSoftwareUpdate file, all nsInstall
should have to know is if (mLogUninstall) openLog(<args>).

>     if ( mInstalledFiles->Count() > 0 )

You may have wanted to wait for this to open the log, btw

>             result = ie->Complete();
> 
>+            if (mLogUninstall)
>+            {
>+                if (ie->CanUninstall())
>+                    *uninstallLogStream << ie->GetUninstallCommand() << nsEndl;
>+            }

Again, nsInstall doesn't need to know the logwriting details.

	     if (mLogUninstall && ie->CanUninstall())
		 nsSoftwareUninstall::logme(ie);

> 
>+    if ( mLogUninstall )
>+    {
>+        if ( mUninstallCommands != nsnull )

ditto.

>+    if( uninstallLogStream )
>+    {

ditto.

>         return NS_OK;
>     }
> 
>+    if ( mUninstallCommands != nsnull )
>+    {
>+        nsString *str;
>+        for (PRInt32 i = 0; i < mUninstallCommands->Count(); ++i)
>+        {
>+            str = (nsString*)mUninstallCommands->ElementAt(i);
>+            if (str)
>+                delete str;
>+        }
>+        mUninstallCommands->Clear();
>+    }

You probably have to do this multiple places (right?), make a cleanup
function you can call rather than having to duplicate this loop. Then
when you change the format of what's stored you won't miss one.

>+    // initialize item queue
>+    mUninstallCommands = new nsVoidArray();
>+    if (mUninstallCommands == nsnull)
>+    {
>+        *aReturn = SaveError(nsInstall::OUT_OF_MEMORY);
>+        return NS_OK;
>+    }

I'm not sure what allocating this buys you, especially when you create
it each time so you're not saving any optional space. Look at all the
extra work it causes.

>+    if (aFlags & DO_NOT_UNINSTALL) 
>+        mLogUninstall = PR_FALSE;

Where do you reset this? What if there are two initInstall blocks
in the file, and only the first is a Don't Uninstall block. (In fact,
one reason for creating two blocks might be to separate "permanent"
changes from uninstallable ones.

>Index: src/nsInstall.h
>===================================================================
>+#ifdef XP_MAC
>+#define UNINSTALL_LOG NS_LITERAL_CSTRING("Installed Components")
>+#else
>+#define UNINSTALL_LOG NS_LITERAL_CSTRING("installed-components.txt")
>+#endif

nit: these names might invite changing or deleting the file, they
seem like just another log (and .txt especially). Maybe "uninstall.dat"
and "Uninstall Data" would carry the "don't touch me" freight better.

>Index: src/nsInstallObject.h
>===================================================================
>+        virtual char* GetUninstallCommand() = 0;

nit: it's not really a "command"; info or data, perhaps?

>Index: src/nsJSInstall.cpp
>===================================================================
>+    if(argc == 4)
>+    {
>+      if(JSVAL_IS_INT(argv[3]))
>+        flags = JSVAL_TO_INT(argv[3]);
>+    }

So if it's not the type you expect you just ignore it?

JS_ValueToEMCAUint32() might be better. Whatever the arg is gets converted
to an integer value according to javascript rules. (Do not use the
JSVAL_TO_ macros on anything but the specified type, but the JS_ValueTo
functions perform true conversions.)

>+  if(argc >= 1)
>+  {
>+    //  public int RegisterUninstallCommand (File fd, String args, Int flags);  
>+
>+    jsObj = JSVAL_TO_OBJECT(argv[0]);
>+    if (!JS_InstanceOf(cx, jsObj, &FileSpecObjectClass, nsnull))

Requiring these to be fileobjects prevents anyone from using built-in
commands since you wouldn't necessarily know the paths to them.

>+    if(argc == 2)
>+      ConvertJSValToStr(b1, cx, argv[1]);
>+
>+    if(argc == 3)
>+    {
>+      if(JSVAL_IS_INT(argv[2]))
>+        flags = JSVAL_TO_INT(argv[2]);
>+    }

You use either the args or the flags, but never both?

>Index: src/nsSoftwareUpdate.cpp
>===================================================================
>+    rv = nsSoftwareUninstall::GetInstalledPackages(&(*aResult));

Can you explain what &(*aResult) is doing, or were you just making
the compiler happy?

>Index: src/nsSoftwareUpdate.h
>===================================================================
> class nsSoftwareUpdate: public nsISoftwareUpdate,
>                         public nsPIXPIStubHook,
>-                        public nsIObserver
>+                        public nsIObserver,
>+                        public nsISoftwareUninstall

Ultimately having this extra uninstall interface is a hack, but in the
short term it saves you having to convert the ugly nsISoftwareUpdate.h
into a scriptable .idl file at this time. There's a bug on doing that,
maybe you could reference that bug number in a comment here or in 
nsISoftwareUninstall.idl that it's temporary until that bug is fixed.

Comment 20

[dprice (gone)]

Here are responses to some of the issues raised in the review.  My comments
begin with <dprice>
>Index: cleanup/InstallCleanup.cpp
>===================================================================
>+#ifdef WIN32
>+    unregComplete   = UnregisterScheduledFiles( reg ); 
>+#endif

why win32 only?  (#ifdef WIN32 is discouraged, btw, use an XP_ equiv)

<dprice> the immediate short term goal was to get it working on windows.
the ifdef win32 is only here so we can land without breaking other platforms.

>+int UnregisterScheduledFiles( HREG reg )
>+{
...
>+        // the delete key exists, so we loop through its children
>+        // and try to delete all the listed files

Fix the "cut and past" comment

Instead of cut and paste maybe you could have parameterized this loop
with the registry key to look at.

>+                rv = NativeUnregisterComponent(valbuf);

There's nothing here that's platform specific so why wasn't it the
NativeUnregisterComponent() routine propogated to each platform
and then stubbed out there until you finished?

<dprice> I thought GetFileAttributes was a windows only method.
The 

>Index: cleanup/InstallCleanup.h
>@@ -42,8 +42,15 @@
> int PerformScheduledTasks(HREG);
> int DeleteScheduledFiles(HREG);
> int ReplaceScheduledFiles(HREG);
>+int UnregisterScheduledFiles(HREG);
> int NativeReplaceFile(const char* replacementFile, const char* doomedFile );
> int NativeDeleteFile(const char* aFileToDelete);
>+
>+#ifdef WIN32
>+int UnregisterComponent(char *componentPath);
>+int NativeUnregisterComponent(const char *aComponentFile);
>+int PerformUnregisterComponent(const char* aComponentFile);
>+#endif

What's the difference between UnregisterScheduledFiles
and the windows specific ones?


> 
> #endif //INSTALL_CLEANUP_H
> 
>Index: cleanup/InstallCleanupWin.cpp
>===================================================================
> int NativeDeleteFile(const char* aFileToDelete)
> {
>-    if (GetFileAttributes(aFileToDelete) == 0xFFFFFFFF)
>+    DWORD attributes = GetFileAttributes(aFileToDelete);
>+
>+    if (attributes == 0xFFFFFFFF)
>     {
>       return DONE;// No file to delete, do nothing.
>     }
>     else 
>     {
>-        if(!DeleteFile(aFileToDelete))
>-          return TRY_LATER;
>+        if (attributes & FILE_ATTRIBUTE_DIRECTORY)
>+            if(!RemoveDirectory(aFileToDelete))
>+                return TRY_LATER;
>+        else
>+            if(!DeleteFile(aFileToDelete))
>+                return TRY_LATER;

By shoving this into the windows-only file you will leave other platforms
stuck when they also hit the directory problem. Also, by burying it
at this lowest level you really haven't solved the original problem, because
you now have no way to know when only directories are left. Remember that
one of the problems we discussed is what to do if someone has put things
in your directory later, such as plugins. In those cases you will never
be able to delete the directory, and thus you will never more be able to
restart the browser.

*after* all the files are gone if you still can't delete the directories
we said it was ok to forget the directories if they failed. But you've
got no way to track when all the files are gone here.

<dprice>  This hasn't been fixed properly yet.  It was brought up and discussed
after the initial review and wasn't included in the work for cleaning up the first 
patch before this patch.  It is on the to do list of things that need to be 
cleaned up.

>+// walling this off because we'll probably have to change the way we unregister
>+// components when MRE comes around.

Seems silly to "wall off" a 7 line routine from a 4 line routine in the same
file, especially when the sole purpose of the 4 line routine is to do what
the walled-off routine is doing. It'd be another thing if the parent
routine was doing lots of things and you were isolating one aspect, but not
when it's a pure wrapper.

<dprice>
when we discussed the implementation, you mentioned that it would be best to 
wall off the method that does the actualy work into a separate function.  I guess
that meant in the native unregister method.  We were talking about this not being
the right way to solve things on the mac.  So I took that to mean that we had
to move the unregister code into the native methods.  Then when we discussed walling
off the unregister work in a separate function, I put it into the perform method.
but it does seem silly to have this much indirection.

>+int PerformUnregisterComponent(const char* aComponentFile)
>+{
>+    int maxCopy = MAXREGNAMELEN - 1;
>+    char unregisterCommand[MAXREGNAMELEN];
>+
>+    strncpy(unregisterCommand, UNREGISTER_COMMAND, maxCopy);
>+    maxCopy-= strlen(unregisterCommand);
>+    strncat(unregisterCommand, aComponentFile, maxCopy);
>+    system(unregisterCommand);

What if there are spaces in the component pathname?

Will strncat copy a null if maxCopy is 0? Doubt it, and in that unlikely case
(impossible here) your string would not be null terminated.

Not that it's going to matter much, but you'd be slightly more efficient
making UNREGISTER_COMMAND a static -- either way the string gets compiled
in.
   const char kUnregisterCommand[] = "regxpcom -u ";
Then use compile-time sizeof(kUnregisterCommand) instead of runtime strlen.

>Index: public/Makefile.in
>===================================================================
> 		nsPIXPIStubHook.idl \
>+    nsISoftwareUninstall.idl \
> 		$(NULL)

Whitespace oddness

>Index: public/makefile.win
>===================================================================
>             .\nsIXPINotifier.idl          \
>             .\nsPIXPIStubHook.idl         \
>             .\nsPIXPIProxy.idl            \
>+            .\nsISoftwareUninstall.idl    \
>+            .\nsIXPIPackageInfo.idl       \
>             $(NULL)

Why do the obsolete makefile.wins get an extra idl? Where are the mac
build changes? I wouldn't bother with makefile.win, they're gone
on the trunk.


<dprice> the branch was cut before the nmake removal.  There will have
to be some work done getting this to build on the trunk.

>Index: public/nsISoftwareUninstall.idl
>===================================================================
>+ * Version: MPL 1.1

New files get the MPL tri-license.

>+[scriptable,uuid(b0822c5f-e1bd-4ff5-94e3-298f3cd5a1ab)]
>+interface nsISoftwareUninstall : nsISupports
>+{
>+  nsISimpleEnumerator getInstalledPackages();

nit: this will not return all installed packages, only the uninstallable ones.

>+  boolean removePackage(in AUTF8String packageRegName, in AUTF8String
pacakgePrettyName);

nit: why not straightforward "uninstall"?

Why does someone need to pass in the pretty name to uninstall something?
Since that could change over time with versions it's a bad key.

<dprice> that should come out.

>Index: public/nsIXPIPackageInfo.idl
>===================================================================
>+#include "nsISupports.idl"
>+[scriptable,uuid(53308be6-000a-45c7-881b-6892711a0cf8)]
>+interface nsIXPIPackageInfo : nsISupports
>+{
>+	[noscript] void Init(in AUTF8String aPrettyName, in AUTF8String aRegName, in
AUTF8String aVersionString);

We talked about this, the init should go in the object (or beter, have
a real constructor), it shouldn't clutter up the public interface

>+%{C++
>+extern nsresult
>+NS_NewIXPIPackageInfo(nsIXPIPackageInfo** aResult);
>+
>+%}

This shouldn't be there either, only your code needs to make these.

<dprice> right, wasn't this a could go either way, if there's time change it?
It will be fixed in the next patch.

>\ No newline at end of file

fix newline problems

>Index: public/nsIXPIPackeageInfo.idl
>===================================================================

What's this one? Looks empty.

>Index: public/nsSoftwareUpdateIIDs.h
>===================================================================
> 
>+#define NS_SoftwareUpdateInstallVersion_CID          \
>+{ /* 18c2f98f-b09f-11d2-bcde-00805f0e1353 */         \
>+    0x18c2f98f,                                      \
>+    0xb09f,                                          \
>+    0x11d2,                                          \
>+    {0xbc, 0xde, 0x00, 0x80, 0x5f, 0x0e, 0x13, 0x53} \
>+}

I think this one is already there? are you adding it again?

>+// {43E6370F-A86E-46bb-82F1-1415B8974751}
>+#define NS_SoftwareUninstall_CID                     \
>+{ /* {43E6370F-A86E-46bb-82F1-1415B8974751} */       \
>+    0x43e6370f,                                      \
>+    0xa86e,                                          \
>+    0x46bb,                                          \
>+    {0x82, 0xf1, 0x14, 0x15, 0xb8, 0x97, 0x47, 0x51} \
>+}
> 
>-#endif /* nsSoftwareUpdateIIDs_h___ */
>+#define NS_XPInstallPackageInfo_CID                  \
>+{ /* {5338E16A-C85F-40ad-B40A-1A8B644C5C42} */       \
>+    0x5338e16a,                                      \
>+    0xc85f,                                          \
>+    0x40ad,                                          \
>+    {0xb4, 0xa, 0x1a, 0x8b, 0x64, 0x4c, 0x5c, 0x42}  \
>+}

You don't need or use these.

>Index: src/ScheduledTasks.h
>===================================================================
>RCS file: /cvsroot/mozilla/xpinstall/src/ScheduledTasks.h,v
>retrieving revision 1.15
>diff -u -r1.15 ScheduledTasks.h
>--- src/ScheduledTasks.h	26 Mar 2002 00:04:03 -0000	1.15
>+++ src/ScheduledTasks.h	24 Aug 2002 09:16:06 -0000
>@@ -37,7 +37,7 @@
> 
> PRInt32 DeleteFileNowOrSchedule(nsIFile* filename);
> PRInt32 ReplaceFileNowOrSchedule(nsIFile* tmpfile, nsIFile* target, PRInt32
aMode);
>-PRInt32 ScheduleFileForDeletion(nsIFile* filename);
>+PRInt32 ScheduleFile(nsIFile* filename, char* key);

I'm not keen on this change. Combining the routines internally, sure,
but now everyone needs to know the specific registry key details in
order to call this routiner.

Please change the module-public name back.

<dprice> will do

>Index: src/makefile.win
>===================================================================

No equivalent Makefile.in or macbuild changes

>Index: src/nsInstall.cpp
>===================================================================
>+    if (mLogUninstall)
>+    {
>+        rv = nsSoftwareUninstall::OpenUninstallLogStream(&uninstallLogStream);
>+
>+        if (NS_FAILED(rv) || !uninstallLogStream)
>+        {
>+            *aReturn = SaveError( nsInstall::UNEXPECTED_ERROR );
>+            mFinalStatus = *aReturn;
>+            return NS_OK;
>+        }
>+        *uninstallLogStream << "[" <<
NS_ConvertUCS2toUTF8(mRegistryPackageName).get() << "]" << nsEndl;
>+        *uninstallLogStream << "PrettyName=" <<
NS_ConvertUCS2toUTF8(mUIName).get() << nsEndl;
>+    }

These details should be hidden in your nsSoftwareUpdate file, all nsInstall
should have to know is if (mLogUninstall) openLog(<args>).

>     if ( mInstalledFiles->Count() > 0 )

You may have wanted to wait for this to open the log, btw

>             result = ie->Complete();
> 
>+            if (mLogUninstall)
>+            {
>+                if (ie->CanUninstall())
>+                    *uninstallLogStream << ie->GetUninstallCommand() << nsEndl;
>+            }

Again, nsInstall doesn't need to know the logwriting details.

	     if (mLogUninstall && ie->CanUninstall())
		 nsSoftwareUninstall::logme(ie);

> 
>+    if ( mLogUninstall )
>+    {
>+        if ( mUninstallCommands != nsnull )

ditto.

>+    if( uninstallLogStream )
>+    {

ditto.

>         return NS_OK;
>     }
> 
>+    if ( mUninstallCommands != nsnull )
>+    {
>+        nsString *str;
>+        for (PRInt32 i = 0; i < mUninstallCommands->Count(); ++i)
>+        {
>+            str = (nsString*)mUninstallCommands->ElementAt(i);
>+            if (str)
>+                delete str;
>+        }
>+        mUninstallCommands->Clear();
>+    }

You probably have to do this multiple places (right?), make a cleanup
function you can call rather than having to duplicate this loop. Then
when you change the format of what's stored you won't miss one.

>+    // initialize item queue
>+    mUninstallCommands = new nsVoidArray();
>+    if (mUninstallCommands == nsnull)
>+    {
>+        *aReturn = SaveError(nsInstall::OUT_OF_MEMORY);
>+        return NS_OK;
>+    }

I'm not sure what allocating this buys you, especially when you create
it each time so you're not saving any optional space. Look at all the
extra work it causes.

>+    if (aFlags & DO_NOT_UNINSTALL) 
>+        mLogUninstall = PR_FALSE;

Where do you reset this? What if there are two initInstall blocks
in the file, and only the first is a Don't Uninstall block. (In fact,
one reason for creating two blocks might be to separate "permanent"
changes from uninstallable ones.

<dprice> It hasn't come up yet.  I'll add something to reset it.

>Index: src/nsInstall.h
>===================================================================
>+#ifdef XP_MAC
>+#define UNINSTALL_LOG NS_LITERAL_CSTRING("Installed Components")
>+#else
>+#define UNINSTALL_LOG NS_LITERAL_CSTRING("installed-components.txt")
>+#endif

nit: these names might invite changing or deleting the file, they
seem like just another log (and .txt especially). Maybe "uninstall.dat"
and "Uninstall Data" would carry the "don't touch me" freight better.

>Index: src/nsInstallObject.h
>===================================================================
>+        virtual char* GetUninstallCommand() = 0;

nit: it's not really a "command"; info or data, perhaps?

>Index: src/nsJSInstall.cpp
>===================================================================
>+    if(argc == 4)
>+    {
>+      if(JSVAL_IS_INT(argv[3]))
>+        flags = JSVAL_TO_INT(argv[3]);
>+    }

So if it's not the type you expect you just ignore it?

JS_ValueToEMCAUint32() might be better. Whatever the arg is gets converted
to an integer value according to javascript rules. (Do not use the
JSVAL_TO_ macros on anything but the specified type, but the JS_ValueTo
functions perform true conversions.)

>+  if(argc >= 1)
>+  {
>+    //  public int RegisterUninstallCommand (File fd, String args, Int flags);  
>+
>+    jsObj = JSVAL_TO_OBJECT(argv[0]);
>+    if (!JS_InstanceOf(cx, jsObj, &FileSpecObjectClass, nsnull))

Requiring these to be fileobjects prevents anyone from using built-in
commands since you wouldn't necessarily know the paths to them.

>+    if(argc == 2)
>+      ConvertJSValToStr(b1, cx, argv[1]);
>+
>+    if(argc == 3)
>+    {
>+      if(JSVAL_IS_INT(argv[2]))
>+        flags = JSVAL_TO_INT(argv[2]);
>+    }

You use either the args or the flags, but never both?

>Index: src/nsSoftwareUpdate.cpp
>===================================================================
>+    rv = nsSoftwareUninstall::GetInstalledPackages(&(*aResult));

Can you explain what &(*aResult) is doing, or were you just making
the compiler happy?


<dprice>
oops, missed that. It should be.
NS_IMETHODIMP 
nsSoftwareUpdate::GetInstalledPackages(nsISimpleEnumerator **aResult)
{
    nsresult rv;
    nsCOMPtr<nsISimpleEnumerator> tmp;

    rv = nsSoftwareUninstall::GetInstalledPackages(getter_AddRefs(tmp));
    CallQueryInterface(tmp, aResult);

    if (NS_FAILED(rv))
        return NS_ERROR_FAILURE;
    else
        return NS_OK;
}


>Index: src/nsSoftwareUpdate.h
>===================================================================
> class nsSoftwareUpdate: public nsISoftwareUpdate,
>                         public nsPIXPIStubHook,
>-                        public nsIObserver
>+                        public nsIObserver,
>+                        public nsISoftwareUninstall

Ultimately having this extra uninstall interface is a hack, but in the
short term it saves you having to convert the ugly nsISoftwareUpdate.h
into a scriptable .idl file at this time. There's a bug on doing that,
maybe you could reference that bug number in a comment here or in 
nsISoftwareUninstall.idl that it's temporary until that bug is fixed.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 96545 [details] [diff] [review]
everything again

Comments on the rest of the patch:
>Index: src/nsSoftwareUninstall.cpp
>===================================================================
>+
>+#include "nsTopProgressNotifier.h"
>+#include "nsLoggingProgressNotifier.h"

What do you use these for?

>+static NS_DEFINE_CID(kIProcessCID, NS_PROCESS_CID); 

These are deprecated in favor of ContractID's

>+nsresult 
>+nsSoftwareUninstall::GetInstalledPackages(nsISimpleEnumerator **aResult)
>+{ 

Since this routine is public (i.e. called from outside our module)
it wouldn't hurt to do a little more sanity checking on the args such
as making sure it's not null. Others argue why bother, go ahead and crash
and people will learn not to call you with a null. You are checking, it's
just way down at the bottom -- move it to the top and shortcircuit
all the work you do here.

>+    rv = NS_NewISupportsArray(getter_AddRefs(packageArray));
>+    if (NS_FAILED(rv))  
>+        return NS_ERROR_FAILURE; 

Why NS_ERROR_FAILURE? the rv you got from NS_NewiSupportsArray
might be more informative.

>+    rv = GetUninstallLogContents(&buf, &bufSize);
>+    if (NS_FAILED(rv) || !buf || (bufSize == 0))
>+        return NS_ERROR_FAILURE;

Don't do all that checking -- if !buf and a zero size buffer are
unacceptable conditions make that part of the errors returned from
GetUninstallLogContents(). It's your own routine written to serve your
needs, not a public API where you suffer with what you've been given.
And again, pass back the rv you get from there which should be
(could be) more informative than a generic FAILURE.

What happens when the log information gets too big for memory?
How big does the log get when you install all of Mozilla/Netscape
(just as a ball-park figure)? The install_wizard.log is 80-90K,
so it's going to be in that range. Could get pretty big for folks
who keep upgrading, less so for normal customers.

>+            line[PL_strlen(line)-1] = '\0';  // trim the trailing ]

How do you know the last character is a ']'?

>+            for(PRUint32 i = 0; i < PL_strlen(line); i++)

How many times does PL_strlen() get called? How are these kinds
of loops usually written?

>+                if(line[i] == '=' )

Are you sure '=' can't be part of a reg name?

>+            nsCString regName;
>+            nsCString pretty;
>+
>+            regName.Assign(regNamePtr);
>+            pretty.Assign(prettyPtr);

Assignment during construction is often more efficient with strings than
creating and Assigning later. And on the stack AutoStrings are usually
better than the forced allocation in a plain nsCString. In this case, though,
you then pass the strings into the XPIPackageInfo -- why not use
nsDependentString or Substring to wrap it before passing it in?

It looks like your regNamePtr includes the pretty name as part of it,
that's not right.

>+            versionString.Assign(NS_LITERAL_CSTRING("Version String")); // have to get the version info

Put a TODO or FIXME comment so it'll get flagged by syntax highlighting
in popular editors and easy to find later.

>+            nsCOMPtr<nsIXPIPackageInfo> packInfo;
>+            rv = NS_NewIXPIPackageInfo(getter_AddRefs(packInfo));
>+            if (NS_FAILED(rv))
>+                return rv;
>+
>+            rv = packInfo->Init(pretty, regName, versionString);

We talked about making this a plain object constructor, no one
other than your code should ever be creating these so there's no
need for this generality.

Every early return you leak buf, which since it's the size of the file
is going to be a pretty darn big leak.

>+nsresult
>+nsSoftwareUninstall::GetUninstallLogPath(nsILocalFile **aLocalFile)  
>+{                                                       

This routine will be logging in the wrong spot in a stub install.
Check nsSoftwareUpdate::GetProgramDirectory() as in other places
(such as nsInstallFolder). Anything logged in the temp directory
will get thrown away.

>+    /* we'll never be in the stub installer, so don't check. */

Oh, I see. Why do you believe that? Won't people want the ability
to uninstall plugins or other stuff when they find out how 
useless they are?

>+    if (!nsSoftwareUpdate::GetUninstallLogName())
>+        rv = iFile->AppendNative(UNINSTALL_LOG);
>+    else
>+        rv = iFile->AppendNative(nsDependentCString(nsSoftwareUpdate::GetUninstallLogName()));

How would mUninstallLogName ever get set? Why would someone want to set it?
Seems like extra configurability to carry around.

>+    PRBool bExists = PR_FALSE, bTryProfileDir = PR_FALSE, bWritable = PR_FALSE;

Put these on separate lines for readability. I don't care if it's one
statement or three, they'll come out the same once they're compiled.

>+        rv = iFile->Create(nsIFile::NORMAL_FILE_TYPE, 0644);

Is 644 right? Normally it is, but in this case what if a root runs the
initial install and creates the file -- after that users won't be able
to write to it even if they can write to the dir. we'll have to work
this one out.

>+nsSoftwareUninstall::GetUninstallLogContents(char **aBuf, PRInt32 *aSize)

>+    if (aSize == 0)
>+    {
>+        PR_Close(fd);

Why check this late? Check before you bother opening the file.

>+    *aBuf = new char[fileSize+1];

You don't check aBuf for null. Either you trust your caller or you
don't, check or don't check, but don't check some and not others.
Like aSize this check should be first thing for an early bailout.

>+    if (!*aBuf)
>+    {
>+        PR_Close(fd);
>+        return NS_ERROR_FAILURE;    
>+    }

Rather than duplicating this PR_Close(fd) tons of times a nested if
structure wouldn't be unreasonable here.

>+    aBuf[fileSize-1] = '\0';

Off by one.

>+nsresult
>+nsSoftwareUninstall::OpenUninstallLogStream(nsOutputFileStream **aOutStream)

>+    rv = Convert_nsIFile_To_nsFileSpec(localFile, &logFile);
>+    if (NS_FAILED(rv) || !logFile) 
>+        return NS_ERROR_FAILURE;
>+
>+    *aOutStream = new nsOutputFileStream(*logFile, PR_WRONLY | PR_CREATE_FILE | PR_APPEND, 0744 );

Sorry you had such crufty code to copy, but no one's going to let new
nsFileSpec code land in the tree. You'd have to ask dougt (owner of XPCOM)
to verify this, but it looks like getting an nsIOutputStream from an nsIFileIO
(see nsIStreamIO.idl) might do the trick. JSLib from MozDev.org manages
to read and write to arbitrary files from Javascript, so the non-nsFileSpec
is obviously complete enough to use.

>+    if (size > 0)
>+        (*aOutStream)->seek(logFile->GetFileSize());

Do you need to seek if you've opened the file PR_APPEND?

>+void   
>+AppendBackSlash(char *aInput, PRUint32 aInputSize)

This doesn't appear to be used anywhere.

>+        if(*aInput == '\0')
>+        {
>+            PL_strncat(aInput, "\\",(aInputSize - PL_strlen(aInput)));

strcat into an empty string is strange. Calling strlen() on a known empty
string is even stranger.

>+        else if(aInput[PL_strlen(aInput) - 1] != '\\')
>+        {
>+            PL_strncat(aInput, "\\",(aInputSize - PL_strlen(aInput)));

Surely the string isn't going to change length between the if and the
PL_strncat call?

strn... functions don't guarantee a null termination if the 'n' limits
the copy -- you must do that yourself. plstr.h makes this clear even if
you thought somehow the PL_ functions were "improved" versions.

>+GetFirstNonSpace(char *aString)
>+{
>+
>+  iStrLength = PL_strlen(aString);
>+
>+  for(i = 0; i < iStrLength; i++)

In general you expect the first non-space long before the end of
the string, and since you're looking at each character anyway
(so you'll see a null) calling strlen() first is not very efficient.

>+      if(!nsCRT::IsAsciiSpace(aString[i])) 

Avoid nsCRT:: for non-Unicode strings, they just add layers. In this
case the signature takes a PRUnichar and you've got a char, which
should have been a clue this wasn't quite the right thing.

The standard isascii() macro should be fine for these simple purposes.
Since you've already made sure to read a line at a time you don't really
need to compare each character against \n or \r, so you could just look
for the one or two chars you really do what to skip. This is your file
format after all, it's not like there's going to be arbitrary data in it.

>+nsSoftwareUninstall::PerformUninstall(const nsACString &aRegistryName, 
>+                                      const nsACString &aPrettyName)
>+{

The pretty name doesn't seem to do anything here, should only need the
registry name argument.

>+        if ( PL_strcasestr(line,PromiseFlatCString(aRegistryName).get()) )

You call PromiseFlagCString many times, which can be expensive if
it turns out to do more than just wrap the string. Better to make
the arg an nsAFlatCString instead and put the PromiseFlatCString
wrapper in the call to this routine, doing any conversion only once.

Doesn't look like you check that this is a [header] line, just that it
contains the registry name. You might by bad luck or bad naming hit
a file that matches.

>+            line = reader.LinePtr(); // skip the pretty name

Should you double check that the next line really is a pretty name?

>+    for (PRInt32 index = 0; index < flen-1; ++index)
>+      if ( buf[index] == '\0' ) 
>+		  buf[index] = '\n';
>+	buf[flen-1] = '\0';
>+    // undo the damage done by reader

This is a pretty ugly thing to have to do... Please look into a way of
reading the file that's not destructive. Maybe the reader could return
nsSubstrings, for example.

>+    rv = Convert_nsIFile_To_nsFileSpec(localFile, &fileSpec); 

See earlier comments about nsFileSpec.

>+nsSoftwareUninstall::PerformFileDelete(char *anAction, char* aKey, char *aFilename, PRInt32 aFilenameSize)

Why is the aFilename buffer passed in here? doesn't appear to be used outside.

>+    char *tmp = PL_strcasestr(anAction,aKey); // skip the qualifiers before the key

Do you really want to treat each of the types you've combined here the same?
For example, the create folder one you should handle separately as discussed
before (at this point you know it's a folder, later on you lose that
distinction and have to figure it out again). If you're replacing a file
then you probably DO NOT want to remove it because the chances are someone
else is using it (why was it already there).

>+	char *namePtr = GetFirstNonSpace( &(tmp[PL_strlen(aKey)]) ); 

Can you think of a more idiomatic way to write the tmp expression?

>+    PL_strncpy(aFilename, namePtr, aFilenameSize - 1);

If your format is such that a copy works (that is, the string must be
null terminated) then why don't you just pass namePtr and avoid the copy?

>+    else if (PL_strcasestr(anAction,KEY_DO_NOT_UNINSTALL) == nsnull)
>+        PerformFileDoDeleteFile(aFilename);

Why do you assume there is one and only one modifier on a file?
Your logging routine doesn't reject combined flags, and I can
certainly imagine a shared component or a non-uninstallable one.

>+    path = NS_ConvertUTF8toUCS2(aFilename);  // BADNESS INIT WITH PERSISTENT FILE DESCRIPTOR

On this kind of comment please add one of the standard flags TODO, FIXME
or XXX (not keen on XXX, but it's common and easy to grep for w/out false
positives)

>+void 
>+ParseWinRegKeyElements(char* anAction, char **aRoot, char **aKey, char **aName)

I notice all these routines return void, so you have no way to know
if it worked or not. Whether the uninstall succeeded or not you'll
remove all traces of it from the log so the user doesn't have the
chance to try again.

>+    PRInt32 actionLen = PL_strlen(anAction);
>+    PRInt32 actionIndex = 0;
>+    PRInt32 keyLen = 0;
>+    PRInt32 i;
>+    PRBool bFoundName = PR_FALSE;
>+    PRBool bFoundOpenBracket = PR_FALSE;
>+    PRInt32 iBrackets = 0;
>+
>+    if (!actionLen)
>+        return;

actionLen isn't a pointer or a boolean, compare == 0 explicitly.
>+
>+    *aRoot = anAction;
>+    while(anAction[actionIndex] != '\\' && anAction[actionIndex]!= '\0')

Please move the actionIndex declaration/initialization near here. We're
writing C++, not C

>+    *aKey = &(anAction[actionIndex+1]);

If you had used a char* as the index this wouldn't look so ugly,
but even with the index form this is an odd way to write what you mean.

Check over the rest of this routine keeping previous comments in mind.

>+#ifdef WIN32
>+HKEY  ParseRootKey(char* aRootKey)

We're not supporting WIN16 anymore

>+    return(hkRootKey);

hkRootKey might be undefined. Either initialize it or have a final
else /* unknown */ case to set it or return error.

>+void PerformDeleteWinRegKey(char* aRootKey, char *aKey)

>+        if(!totalSubKeys)
>+            RegDeleteKey(hkRootKey, aKey);

You don't do anything different if there are subkeys so why bother
checking for them? Just delete and if it fails it fails. Maybe someone
else added subkeys and in that case we should just leave it.

On the other hand what are script writers supposed to do if the
product, rather than the install, is creating stuff? I think that's
why the stub uninstaller just zaps a whole subtree, but I'd rather
not do that unless a script says it's OK.

>+ParseForCopyFile(char *aString, char *aKeyStr, char *aFile, PRInt32 aFilenameBufSize)

"Undo" for moving a file would be copying it back, not deleting it. But
you can make a really good argument for ignoring all the File object commands,
with a possible exception for shortcuts/aliases/links. If people are doing
low level stuff they can write an extra uninstaller that knows what makes sense
to undo in those cases.

>+nsSoftwareUninstall::PerformUninstallActions(nsVoidArray *aActions)

>+        PL_strncpy(theAction,(char*)aActions->ElementAt(index),MAX_BUF);

Why do you need to copy the action? I guess these are all still pointing
into buf -- OK. Some of the actions may not touch theAction, though, and
you'd be copying it for no reason. You could declare each of your routines
to take it as a const char* and then have each copy as necessary.

>+            PerformFileDelete(theAction, KEY_INSTALL_FILE, filename, MAX_BUF); 

As mentioned previously you don't do anything with the huge filename
buffer you pass into these in most cases. Have the routines get their
own local storage if they need it. Save arguments for cases where they
mean something.

>+        else if (subString = PL_strcasestr(theAction, KEY_COPY_FILE))
>+        else if (subString = PL_strcasestr(theAction, KEY_MOVE_FILE))
>+        {
>+            ParseForCopyFile(theAction,KEY_MOVE_FILE, filename, MAX_BUF);
>+            if (!filename)
>+                return NS_ERROR_FAILURE;
>+            PerformFileDoDeleteFile(filename);

As mentioned above deleting the file isn't a good "undo" for a move,
and the copy makes me a little uncomfortable because we have no mechanism
for script authors to tell us when not to undo it -- maybe they've mucked
with the system in ways that makes the file necessary.

>+        else if (subString = PL_strcasestr(theAction, KEY_CREATE_FOLDER))
>+        {
>+            PerformFileDelete(theAction, KEY_CREATE_FOLDER, filename, MAX_BUF);  

We can't really tell the difference between directories created by
an addFile/addDirectory (which we definitely want to remove) and those
created by File.dirCreate which we could possibly lump in with the "low level"
ones we want to ignore. So let's zap these if they're empty (as you do).
But we *know* it's a directory at this point, so let's not lose that
knowledge by lumping these in with mere files.

>+        else if (subString = PL_strcasestr(theAction, KEY_CREATE_REG_KEY))
>+        {
>+            ParseWinRegKeyElements(GetFirstNonSpace(&(theAction[PL_strlen(KEY_CREATE_REG_KEY)])), 

Is this right? There's got to be a more readable way of doing that.

>+            ParseWinRegKeyElements(GetFirstNonSpace(&(theAction[PL_strlen(KEY_STORE_REG_VALUE_STR)])), 
>+                         &root, &key, &name);
>+            if (!root || !key || !name )
>+                return NS_ERROR_FAILURE;

This is all in a big for loop -- these will only be initialized to nsnull
until the first time you hit a command that uses them. After than your
error checking doesn't work.

>+NS_IMETHODIMP nsXPIPackageInfo::GetVersionString(nsACString & aVersionString)
>+{
>+    aVersionString = NS_LITERAL_CSTRING("version"); 

Stubbing this seems unnecessary since it's stubbed when you create
a nsXPIPackageInfo.

Comment 22

[Daniel Veditz [:dveditz]]

<dprice> the branch was cut before the nmake removal.  There will have
> to be some work done getting this to build on the trunk.

But not cut before win32 gmake was added (last April?). With nmake announced to
go away at the beginning of August you could have saved yourself a bunch of work
by ignoring it from the beginning.

Comment 23

[dprice (gone)]

good point.

how about this:
NS_IMETHODIMP 
nsSoftwareUpdate::GetInstalledPackages(nsISimpleEnumerator **aResult)
{
        return nsSoftwareUninstall::GetInstalledPackages(aResult);
}

Comment 24

[Michael Gabriel]

how can you do this without bug 9590 be fixed and aint this the same as bug 7884 ?

Comment 25

[dprice (gone)]

The patches include a mechanism to log install info to be used when an uninstall
arises.  So you can say it includes a fix for 9590.

Is this a dupe of 7884?  Yeah probably.

Comment 26

[Henrik Gemal]

http://blues/users/dprice/publish/uninstall/uninstall.html

cant we put the specs out in the open?

Comment 27

[Henrik Gemal]

*** Bug 161360 has been marked as a duplicate of this bug. ***

Comment 28

[[e-mail] (away - not reading bugmail)]

Attachment: XUL panel screenshot

Comment 29

[[e-mail] (away - not reading bugmail)]

Attachment: uninstaller spec posted by dprice

Comment 30

[[e-mail] (away - not reading bugmail)]

Attachment: uninstaller spec with corrected image links

Comment 31

[[e-mail] (away - not reading bugmail)]

Attachment: implementation details (dprice)

Comment 32

[[e-mail] (away - not reading bugmail)]

taking this (per email exchange with Syd Logan)

Comment 33

[Syd Logan]

nsbeta1- for buffy

Comment 34

[Nick Cross]

Isn't http://bugzilla.mozilla.org/show_bug.cgi?id=7884 (which has a cylic block
with http://bugzilla.mozilla.org/show_bug.cgi?id=9590) pretty much similar to this?

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 7884 has been marked as a duplicate of this bug. ***

Comment 36

[[e-mail] (away - not reading bugmail)]

reassigning

Comment 37

[Patrick]

We're getting a large number of crash reports with 1.4 in case users upgrade
from 1.4-x caused by 3rd party XPI's.

People don't realize that the XPI's are the cause of their crash, after all,
they DID uninstall their old Mozilla first. 

Wouldn't the approach of bug 210731 solve this problem?

Comment 38

[Benjamin Smedberg]

taking. I am planning on implementing this using my proposal at
http://groups.google.com/groups?dq=&hl=en&lr=&ie=UTF-8&oe=UTF-8&frame=right&th=c280fb0534302d02&seekm=be0262%24rg81%40ripley.netscape.com#link1
to install extensions into their own directories. This will not allow current
XPIs to be uninstalled, only new XPIs that are designed to interact with the new
extension manager.

Comment 39

[K'Trina Medina]

Benjamin, would you mind being the QA contact for this bug as well? I'm no
longer testing installer and much to busy at the time to keep up to date on
this. Thanks!

Comment 40

[WD]

*** Bug 229265 has been marked as a duplicate of this bug. ***

Comment 41

[Benjamin Smedberg]

*** Bug 235180 has been marked as a duplicate of this bug. ***

Comment 42

[Roland Mainz]

From bug 235180:
> RFE: XPInstall engine should support uninstall scripts.
>
> XPI packages should be able to provide uninstall scripts (uninstall.js) which
> should enable Mozilla/FireFox/ThunderBird/etc. to uninstall add-ons.
> There should be an "Uninstall" button when the package provides an
> "uninstall.js" script (the button should be disabled if there is no such
> script).
> 
> ------- Additional Comment #1 From Benjamin Smedberg 2004-02-22 05:19 PST 
> We don't need a separate uninstall.js.  Instead, we need to reverse the 
> actions
> which are saved in the install log.  This is being worked on for firebird, and
> will probably make it back into seamonkey.
>
> *** This bug has been marked as a duplicate of 161508 ***
>
> ------- Additional Comment #2 From Roland Mainz 2004-02-22 06:49 PST [reply] 
> Benjamin Smedberg wrote:
> > We don't need a separate uninstall.js.  Instead, we need to reverse the 
> > actions which are saved in the install log
>
> Such an action will FAIL if the install.js does something which is unexpected 
> by your uninstall engine. Some actions are not "reverseable" (think about 
> package dependicies), therefore such a design will not work.

bsmedberg:
Remember that packages can have dependicies and may make "unexpected" actions.
What if two XPIs install into the same directory or share some files or
dirctories... or if the user makes changes by hand or a later XPI makes
modifications) ... or one package depends on others ? Just reversing the actions
done by the installer is then likely be a gurantee to cause havoc - in the worst
case the mozilla/GRE installation will become unuseable.
It would be usefull to (at least) add support for "preinstall", "postinstall",
"prebackout" and "postbackout" scripts that these issues can be addressed by the
author of the XPI on demand.

Comment 43

[Daniel Veditz [:dveditz]]

We are well aware of the horrors of uninstalling a tangled mess and will make
allowances. If it were easy we'd have done it long ago.

Obvious example problem: install scripts can launch an executable install
binary. How could you possibly uninstall anything it did? We will have to add
support for registering an uninstall binary that would get run at the
appropriate time.

Comment 44

[Jaap A. Haitsma]

Isn't this bug irrelevant after the new theme and extension installer of Firefox
and Thunderbird got implemented

Comment 45

[Jason Bassford]

This bug is targetted against the Mozilla suite - it has nothing (directly) to
do with Firefox or Thunderbird...

Comment 46

[Michael Gabriel]

just how if firefox doing it, when xpi packages dont register any information
about their content ???

Comment 47

[Jason Bassford]

Firefox doesn't have components.  It's not a suite - it's a stand-alone
application.  So there really isn't a valid comparison.  Extensions aren't the
same thing as suite components.  As far as I know, things like the suite's
Mail/News component isn't available as an XPI...

Comment 48

[timeless]

[e-mail]: actually it is...

Comment 49

[Jason Bassford]

Cool.  Well, then - I guess somebody else needs to respond to comment 46...

Comment 50

[Darin Fisher]

(In reply to comment #48)
> [e-mail]: actually it is...

timeless: you were referring to mailnews being available as a firefox extension?

jasonb is correct, there is a big difference between firefox extensions and XPIs
that install into the application directory.  firefox extensions install into
individual directories inside the user's profile directory (by default).  they
can be made to install into the application directory, but even then they are
not installed into the base components directory.  (note: this applies to XPIs
created with an install.rdf manifest file.)  the result is that firefox
extensions can be "easily" inserted and removed without fear of trampling over
other components or other extensions.  this is why firefox requires a restart
after installing an extension (i.e., it must regenerate compreg.dat, xpti.dat,
and chrome.rdf).

Comment 51

[timeless]

> As far as I know, things like the suite's Mail/News component isn't available
as an XPI...

mailnews is available as an xpi:
ftp://ftp34.newaol.com/pub/mozilla.org/mozilla/nightly/2004-11-16-07-trunk/windows-xpi/mail.xpi
the xpi can be installed into a seamonkey navigator only install to add
mailnews. you'd probably restart seamonkey before trying to use it.

i'm not saying that the xpi at all matches the stuff that firefox can do, just
that it exists as an entity (i've used it at times).

Comment 52

[Henrik Gemal]

FYI:
http://jgillick.nettripper.com/extuninstaller/
What is Extension Uninstaller? This extension provides a friendly user interface
for uninstalling any extension installed in Mozilla, Firefox and Thunderbird. 

Comment 53

[Tony Mechelynck [:tonymec]]

In reply to comment #52 from 3½ years ago

Extension Uninstaller is obsolete on Trunk (not on Sm 1.1) now that there is a "decent" addons manager.

Some components (Chatzilla, Venkman, DebugQA), distributed with SeaMonkey, are now being packaged as "extensions" which, with the new add-ons manager, can be easily disabled, re-enabled, even uninstalled.

As for uninstalling "other" components (those not packaged as extensions, like MailNews or BreakPad) I expect a way to uninstall them will eventually be found, but IIUC there's more pressing tasks to be done now.

Comment 54

[Benjamin Smedberg]

We are not going to implement anything like comment 53 for the toolkit. If seamonkey wants it, it will have to be seamonkey-specific code. I recommend WONTFIX.

Comment 55

[Robert Kaiser]

This is actually FIXED on trunk to a certain extent - all components that are packaged as extensions (mainly chatzilla, venkman, DOMI) can easily be uninstalled or deactivated. Everything not packaged as an extension is not planned uninstallable any time, so WONTFIX for that.
Note that in theory, we might some time be able or actually do move to packaging mailnews, addressbook or composer as extensions some time in the future, but that's also not planned at the moment.

Comment 56

[jesse]

this bug might be drifting off-topic.  Perhaps the ability to uninstall things like composer or mailnews should be a separate bug (I'd vote for it).  As I understand this bug, the goal over the past six years has been to implement in Seamonkey something similar to what already exists in firefox - an easy GUI interface for removing user-installed add-ons.  I think this is crucial.  As it stands, unless the add-on itself has an uninstaller, for the average user installing an add-on is essentially a permanent decision - an especially scary proposition when so many add-ons have version specific issues.

Comment 57

[Tony Mechelynck [:tonymec]]

In reply to comment #56:
In present nightly builds of Sm 2.0a1pre (not 1.1.9 or earlier) an add-ons interface quite similar to that in Firefox and Thunderbird is implemented, and works: so that part, concerning user-installed add-ons and also some distributed components like Chatzilla, Venkman, DOM Inspector, and Debug & QA UI, is FIXED (in Trunk; the fix will probably not be ported to the Sm 1.1 Branch which lacks the Toolkit backend). What remains concerns Mail&News, Composer, etc., and that is WONTFIX for the time being.

Comment 58

[Tony Mechelynck [:tonymec]]

P.S. -- and a "temporary uninstall" is possible too -- it's called Disable, like in Fx+Tb. The disabled extension(s) remain listed in the add-ons manager, but in greyed-out, and their code is not used.

Comment 59

[Robert Kaiser]

(In reply to comment #56)
> Perhaps the ability to uninstall things
> like composer or mailnews should be a separate bug (I'd vote for it).

This probably would get WONTFIXed.

> As I
> understand this bug, the goal over the past six years has been to implement in
> Seamonkey something similar to what already exists in firefox - an easy GUI
> interface for removing user-installed add-ons.  I think this is crucial.

Yes, and this has been fixed for trunk (i.e. what will become SeaMonkey 2), like comment #57 has also stated.