Closed Bug 242529 Opened 19 years ago Closed 14 years ago

XPInstall hooks for Extension Manager

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.7final

People

(Reporter: bugs, Assigned: bugs)

References

Details

(Keywords: fixed-aviary1.0)

Attachments

(1 file, 7 obsolete files)

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.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.7final
Attached patch patch to fully implement (obsolete) — Splinter Review
Description coming shortly.
Attachment #147608 - Attachment is obsolete: true
Attachment #147609 - Attachment is obsolete: true
Attached patch better patch, works in seamonkey (obsolete) — Splinter Review
Attachment #148291 - Attachment is obsolete: true
Attached patch oops, wrong patch (obsolete) — Splinter Review
Attachment #148340 - Attachment is obsolete: true
Attached patch SIGH (obsolete) — Splinter Review
Attachment #148341 - Attachment is obsolete: true
Attached patch LKJSFDLKDJFLLKJ!LK!JL:!KJ!! (obsolete) — Splinter Review
Attachment #148342 - Attachment is obsolete: true
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)

Attachment #148343 - Flags: superreview?(brendan)
Attachment #148343 - Flags: review?(dveditz)
Whiteboard: fixed-aviary1.0
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.
Attachment #148343 - Flags: review?(dveditz) → review-
I'm going to return to this bug this week after we have shipped 0.9
Attachment #148343 - Attachment is obsolete: true
filed 248218 on the cert issue. 
Attachment #151478 - Flags: review?(dveditz)
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
Attachment #151478 - Flags: review?(dveditz) → review+
plussing this so it doesn't get lost. 
Flags: blocking-aviary1.0RC1+
Attachment #151478 - Flags: superreview?(brendan)
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
Keywords: fixed-aviary1.0
Whiteboard: fixed-aviary1.0
Blocks: 272688
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.
Attachment #151478 - Flags: superreview?(brendan)
Attachment #148343 - Flags: superreview?(brendan)
Can this be closed?
QA Contact: xpi-engine
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.