the work will be done on UNINSTALLER_WORK_BRANCH The spec lives at http://blues/users/dprice/publish/uninstall/uninstall.html

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

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

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.

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.

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

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.

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

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

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

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

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

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

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

cleaned up some badness that I missed in PerformUninstallCommand

new patch that adds a prepare uninstall phase to the uninstaller

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.

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

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

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

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

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.

http://blues/users/dprice/publish/uninstall/uninstall.html cant we put the specs out in the open?

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

taking this (per email exchange with Syd Logan)

nsbeta1- for buffy

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?

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

reassigning

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?

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.

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!

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

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

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.

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.

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

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

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

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

jasonb@dante.com: actually it is...

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

(In reply to comment #48) > jasonb@dante.com: 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).

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

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.

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.

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.

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.

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.

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.

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.

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