The XPInstall engine needs to check for a toplevel extension.rdf file in XPI files when installing and if one exists invoke the extension manager's install API.

Of the two, I prefer the first one despite the ifdefs because it a) presents less short term risk (the latter may conflict with Seamonkey install packages that have a file called "extension.rdf" at the top level), and b) it does not introduce unnecessary extra stub components to the seamonkey build.

Description coming shortly.

OK, (now that I've learned how to use a file picker) this is ready for review. Here's a description of what it does, file by file: browser-prefs.js: - default prefs for XPI download status window chrome (Seamonkey only) xpfe/components/extensions/*: - a stub extension manager implementation for Seamonkey that throws not implemented when invoked to force Seamonkey to only recognize install.js scripts at the top level of xpi files, and ignore extension install.rdf manifests. Ignore the xpfe/components/killAll thing it's an error I've fixed it in my tree xpinstall/packager/*: - add the new files to Seamonkey's packager lists so this works in installer builds. xpinstall/public/nsIXPInstallManager.idl, et al: - interface from 241922 xpinstall/src/nsInstall.cpp: xpinstall/src/nsInstall.h: xpinstall/src/nsSoftwareUpdate.cpp: - Make the nsInstallInfo objects take a nsIExtensionManager object and create a proxy object for the install thread so that they can invoke the extension/theme installation process. xpinstall/src/nsSoftwareUpdateRun.cpp: - add a function ("CanInstallFromExtensionManifest") to check for an install.rdf file at the top level of a xpi file, and adapt the RunChromeInstallOnThread function to call this function, if it returns successfully attempt to install via the Extension Manager. Seamonkey will throw not implemented here and then attempt to install via the install.js method. Firefox extensions will invoke the EM's install process, or if they don't provide an install.rdf file, use the traditional install.js method. The diff makes it look like there are a lot of changes here but it's just me indenting the function by one indent level. - the same methodology applies to the Theme case further down in the patch, although there's not a specialized helper function since themes aren't signed etc. So the code just creates a zip reader with the JAR file and uses nsIZipReader::Test to check for install.rdf. If one exists it uses the new API, if one doesn't it just registers with the chrome reg. xpinstall/src/nsXPInstallManager.cpp: - the patch from 241922 updated to meet dveditz & brendan's comments (now checks xpinstall.enabled pref) - also before opening the download window it checks the chrome type being handled by this manager's transaction and opens one UI for skin jars, one for xpis. For Seamonkey both of these UIs are the same dialog (that's why the prefs at the start of the patch point at the same URL) - for Firefox they're different (Theme Manager handles theme downloads, Extension manager handles XPI downloads)

Comment on attachment 148343 [details] [diff] [review] LKJSFDLKDJFLLKJ!LK!JL:!KJ!! >Index: xpfe/components/extensions/public/nsIExtensionManager.idl >=================================================================== >+ const unsigned long FLAG_INSTALL_PROFILE = 0x01; >+ const unsigned long FLAG_INSTALL_GLOBAL = 0x02; >+ >+ void installExtension(in nsIFile aXPIFile, in unsigned long aFlags); >+ void installTheme(in nsIFile aJARFile, in unsigned long aFlags); As flags go this would appear to let someone install both profile AND global by passing x03, or neither by passing 0. Pick one as your preferred default (profile?) and use just one flag. Simplifies error handling in your function and potential trouble for the caller. Should these be void? A status code returned to XPInstall would be handy for reporting to users whether it worked or not. Package developers already suffer enough mysterious XPInstall failures as it is because of the taciturn nsChromeRegistry. >Index: xpfe/components/extensions/src/nsExtensionManager.js >=================================================================== >+ // nsIClassInfo >+ getInterfaces: function (aCount) >+ { >+ var interfaces = [Components.interfaces.nsIExtensionManager, >+ Components.interfaces.nsIXPIProgressDialog, >+ Components.interfaces.nsIObserver]; >+ aCount.value = interfaces.length; >+ return interfaces; >+ }, >+ QueryInterface: function (aIID) >+ { >+ if (!aIID.equals(Components.interfaces.nsIExtensionManager) && >+ !aIID.equals(Components.interfaces.nsIObserver) && >+ !aIID.equals(Components.interfaces.nsISupports)) >+ throw Components.results.NS_ERROR_NO_INTERFACE; >+ return this; Should these match? I also don't see any methods from nsIObserver or nsIXPIProgressDialog defined, but maybe this is a work-in-progress check-in? >Index: xpfe/components/killAll/Makefile.in >=================================================================== >-libs:: >- $(INSTALL) $(srcdir)/nsKillAll.js $(DIST)/bin/components >- >-clean:: >- rm -f $(DIST)/bin/components/nsKillAll.js >+EXTRA_COMPONENTS = nsExtensionManager.js This appears to be a mistake, nsExtensionManager.js isn't in this directory, and you haven't removed the code that calls the killAll component or adds it to a packaged install. >Index: xpinstall/packager/packages-static-unix >Index: xpinstall/packager/packages-static-win >Index: xpinstall/packager/packages-unix >Index: xpinstall/packager/packages-win You don't support Mac? >Index: xpinstall/src/nsSoftwareUpdate.cpp >=================================================================== > nsIXULChromeRegistry* chromeRegistry = nsnull; > NS_WITH_ALWAYS_PROXIED_SERVICE( nsIXULChromeRegistry, >- tmpReg, >+ tmpRegCR, > NS_CHROMEREGISTRY_CONTRACTID, > NS_UI_THREAD_EVENTQ, &rv); > if (NS_SUCCEEDED(rv)) >- chromeRegistry = tmpReg; >+ chromeRegistry = tmpRegCR; >+ >+ >+ NS_WITH_ALWAYS_PROXIED_SERVICE( nsIExtensionManager, >+ extensionManager, >+ "@mozilla.org/extensions/manager;1", >+ NS_UI_THREAD_EVENTQ, &rv); >+ if (NS_FAILED(rv)) >+ return rv; > > // we want to call this with or without a chrome registry > nsInstallInfo *info = new nsInstallInfo( 0, aLocalFile, aURL, aArguments, aPrincipal, we MUST NOT fail if you can't get the proxy -- that's guaranteed to be the case during the initial install when only a bare-bones xpcom engine is running. You'll have to do the same kind of checks as for the chrome registry and pass null if the proxy fails, and be prepared to deal with a null extension manager later. I think I used the temp variable because there were issues with it not being null in some cases where the proxy call failed, but I suppose you could just change your if-failed statement to "extensionManager = nsnull;" instead of the return. >@@ -352,24 +362,32 @@ nsSoftwareUpdate::InstallChrome( PRUint3 >+ NS_WITH_ALWAYS_PROXIED_SERVICE( nsIExtensionManager, >+ extensionManager, >+ "@mozilla.org/extensions/manager;1", >+ NS_UI_THREAD_EVENTQ, &rv); >+ if (NS_FAILED(rv)) >+ return rv; This is OK here because we don't support InstallChrome() in the native installs. >@@ -630,17 +695,53 @@ extern "C" void RunChromeInstallOnThread >+ hZip->Open(); >+ >+ nsIExtensionManager* em = info->GetExtensionManager(); >+ rv = hZip->Test("install.rdf"); The Open() could fail if it's an invalid archive, corrupted on download, etc. -- that's when the zip directory structure is parsed. It looks like you might get away with it, the nsZipArchive hashtable is memset to 0 in the constructor so this will return ZIP_ERR_FNF without noticing the archive was not initialized. Still, someone might come along later and "fix" something (e.g. I see someone added arena allocation at some point) and break this implementation reliance. Best to just check the Open() status. >Index: xpinstall/src/nsXPInstallManager.cpp >=================================================================== >+nsXPInstallManager::InitManagerFromChrome(const PRUnichar **aURLs, PRUint32 aURLCount, >+ nsIXPIProgressDialog* aListener) >+{ >+ // If Software Installation is not enabled, we don't want to proceed with >+ // update. >+ PRBool xpinstallEnabled = PR_TRUE; In case of error I'd prefer a fail-safe PR_FALSE default. >+ if (!xpinstallEnabled) >+ return NS_OK; Do you need an error return for cleanup on the calling end? NS_ERROR_NOT_AVAILABLE might be appropriate. You could shorten this a bit, doing cleanup only once: + for (PRInt32 i = 0; i < aURLCount; ++i) + { + nsXPITriggerItem* item = new nsXPITriggerItem(0, aURLs[i], nsnull); + if (item) + mTriggers->Add(item); + } + + mInstallSvc = do_GetService(nsSoftwareUpdate::GetCID()); + if (mTriggers->Size() != aURLCount || !mInstallSvc ) + { + delete mTriggers; + mTriggers = nsnull; + return NS_ERROR_OUT_OF_MEMORY; + } (I suppose there might be other reasons for the service failure, but memory would be the most likely cause since we're in the same chunk of code.) This version bypasses the CertReader, so signed extensions are not supported. Probably not what we want to do. You could pass the listener from InitManagerFromChrome() to InitManager() (use null in the existing cases) and let it go through the CertReader business, then suppress the UI in InitManagerInternal() if the listener is present. That's just off the top of my head, you might find another way you like better. I haven't fully completed the nsSoftwareUpdateRun.cpp review, but I'll plug these comments into the bug to start. My initial impression of the unreviewed code is that I don't like having to CRC check the entire archive twice in the non-extension case. There's a bunch of now-common code in the two paths that seems like could be coalesced.

I'm going to return to this bug this week after we have shipped 0.9

filed 248218 on the cert issue.

Comment on attachment 151478 [details] [diff] [review] patch, addressing dan's comments >Index: xpinstall/src/nsSoftwareUpdateRun.cpp >=================================================================== >+/////////////////////////////////////////////////////////////////////////////////////////////// >+// Function name : CanInstallFromExtensionManifest >+// Description : Returns a stream to an extension manifest file from a passed jar file. Doesn't seem to return a stream. >+// Return type : PRInt32 Not entirely clear how you interpret the result. Most of it returns nsInstall error codes, but at the end you return a nsIZipReader nsresult, and when called you treat it as an nsresult with NS_SUCCEEDED. The "finalresult" returned here wouldn't be the actual status returned unless it succeeded, but at the end this is used as an nsInstall error code. Maybe you just need a boolean return code since any failures falls into the old code. Should add a comment for the new CRCCheck argument >+ // Verify that install.rdf exists >+ return hZip->Test("install.rdf"); hZip->GetEntry() would avoid a redundant CRC recalculation. Assuming you change to return a boolean, try: nsCOMPtr<nsIZipEntry> tmpEntry; return NS_SUCCEEDED(hZip->GetEntry("install.rdf",getter_AddRefs(tmpEntry))); >- NS_ASSERTION(0, "CRC check of archive failed!"); ... >+ NS_ASSERTION(0, "CRC check of archive failed!"); If you don't mind, please change my old NS_ASSERTION(0,..) to NS_ERROR() > nsCOMPtr<nsIFile> jarpath = installInfo->GetFile(); > > if (NS_SUCCEEDED(rv)) > { >+ finalStatus = CanInstallFromExtensionManifest( hZip, Consider boolean return value as mentioned above. rv will always be success there. Probably want to check jarpath for non-null instead >+ rv = em->InstallExtension(jarpath, nsIExtensionManager::FLAG_INSTALL_PROFILE); >+ if (NS_SUCCEEDED(rv)) >+ installed = PR_TRUE; Here I think you do want to make sure finalStatus is set, especially in the error case so that a false success doesn't get sent to the listener's OnInstallDone(), but also in the success case if you follow my advice to change CanInstallFromExtensionManifest to return a boolean. >+ nsIExtensionManager* em = info->GetExtensionManager(); >+ rv = hZip->Test("install.rdf"); This is installChrome -- use GetEntry() instead of Test() here too. r=dveditz with those minor changes

plussing this so it doesn't get lost.

brendan, can you review? also wondering if all/parts of this patch was landed landed? whiteboard to keyword comment change is needed if it did...

Didn't this land a while ago? If not, how about an up-to-date patch including changes per dveditz's comments? /be

This have added +xpinstall/src/nsInstallTrigger.cpp:184 + `nsIScriptGlobalObject*globalObject' might be used uninitialized in this function on brad TBox. Indeed, for some reason the line 184 was changed from "nsIScriptGlobalObject* globalObject = nsnull;" to just "nsIScriptGlobalObject* globalObject;", causing the code to become: ... 184 nsIScriptGlobalObject* globalObject; 185 nsCOMPtr<nsIScriptGlobalObjectOwner> globalObjectOwner = 186 do_QueryInterface(aWindowContext); 187 if ( globalObjectOwner ) 188 { 189 globalObject = globalObjectOwner->GetScriptGlobalObject(); 190 } 191 if ( !globalObject ) 192 return NS_ERROR_INVALID_ARG; ... This way if globalObjectOwner is null, then random things might happen.

So... this patch broke XPI install in SeaMonkey. See bug 272764 comment 90.

Can this be closed?