Closed Bug 593025 Opened 14 years ago Closed 14 years ago

Installing addons from webpages does not work

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: mfinkle, Assigned: wesj)

References

Details

Attachments

(1 file, 8 obsolete files)

Trying to install an add-on from a webpage fails. I see errors in the console when the amManager component tries to create the global process message manager:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/addonManager.js#75

The amContentHandler tries to create the amIWebInstaller:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/amContentHandler.js#86
tracking-fennec: --- → 2.0b1+
This is not a regression. Using InstallTrigger does still work. Clicking on a XPI link, does not
tracking-fennec: 2.0b1+ → 2.0+
Summary: [Regression] Installing addons from webpages does not work → Installing addons from webpages does not work
The problem here is in amContentHandler.js

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/amContentHandler.js#88

which is calling into the addons manager code from the content process. Seems to me that we want to fix this in amContentHandler, but thought it might be best to discuss if there was a different change we wanted before I dove in?
I suspect this can be fixed in the following way. Instead of getting the manager and calling manager.installAddonsFromWebpage, replace that with a call to the InstallTrigger object on the window,

  window.wrappedJSObject.InstallTrigger.install(..params..);

The InstallTrigger object already has code for remoting to the parent process properly. This would also shorten the code.

However, InstallTrigger and installAddonsFromWebpage are close but not identical in the params they get, so I'm not certain this will work.
(In reply to comment #4)
> I suspect this can be fixed in the following way. Instead of getting the
> manager and calling manager.installAddonsFromWebpage, replace that with a call
> to the InstallTrigger object on the window,

Let's try this approach. It was my first reaction to the problem too.
Assignee: nobody → wjohnston
Attached patch Call InstallTrigger (obsolete) — Splinter Review
Attachment #472914 - Flags: review?(mark.finkle)
Comment on attachment 472914 [details] [diff] [review]
Call InstallTrigger

We need Dave to review this.

My questions for Dave would be: Should we check to see if we are running in the child process and only then use the InstallTrigger? Or is it OK to always use InstallTrigger?
Attachment #472914 - Flags: review?(mark.finkle) → review?(dtownsend)
Why is this running in the content process, don't content type handlers run in the chrome process?
(In reply to comment #8)
> Why is this running in the content process, don't content type handlers run in
> the chrome process?

It seems like content handlers are running in the child process. CC'ing Jason to let us know for sure.
> It seems like content handlers are running in the child process.

cc-ing bz/crowder/biesi, who've followed this much more than I have.
Comment on attachment 472914 [details] [diff] [review]
Call InstallTrigger

Based on the discussion in IRC mfinkle will provide an alternative way to do this
Attachment #472914 - Flags: review?(dtownsend) → review-
> It seems like content handlers are running in the child process.

You mean nsIContentHandler?  Yes, they are, as is the rest of uriloader dispatch.  I think that's broken, and we want to change that long-term, but that's where we are right now....
Wes - OK, here is the plan. We already do something very similar for InstallTrigger, as you have seen, but instead of just trying to call InstallTrigger, we can send a message back to the parent process in the same way that InstallTrigger is implemented.

See:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions-content.js#96

This code implements InstallTrigger.install by packaging up the data and sending it to the addonsManager.installAddonsFromWebpage:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/addonManager.js#207

Which is exactly what we want to do for the content handler.

So, instead of making a patch that calls the InstallTrigger.install method, let's make a patch that packages up the data and calls addonsManager.installAddonsFromWebpage via a message.

Make sense?
That sounds good to me, assuming we don't end up with significant code duplication in the two places that construct the proper params to send through IPC to the listener in the parent. But I guess all the checks done in InstallTrigger won't be needed. So should be fine in that respect.

I presume there is an automatic test (Firefox, not Fennec) that checks that this works, but we should confirm that.
(In reply to comment #14)
> I presume there is an automatic test (Firefox, not Fennec) that checks that
> this works, but we should confirm that.

Multiple tests in fact, in toolkit/mozapps/extensions/test/xpinstall.
Attached patch WIP: Patch (obsolete) — Splinter Review
This is a WIP for feedback. There are two issues with this approach.

1.) addonsManager.js currently only receives messages from the globalmessagemanager. The childprocessmessagemanager I'm using to pass things, sends messages through the parentprocessmessagemanager. addonsManager  has to be modified to listen on both services. I did that in this patch.

2.) Once the message is received, addonsManager.js expects a window or browser to be associated with message.target. If we make the call through the content handler, there is no target and addonsManager fails.

We could adjust addonsManager to include yet another fallback when there is no target, or we could just try to call InstallTrigger when we detect that we're in content. Wanted some feedback before I moved on.
Attachment #472914 - Attachment is obsolete: true
Attachment #473646 - Flags: feedback?(mark.finkle)
(In reply to comment #16)
> 2.) Once the message is received, addonsManager.js expects a window or browser
> to be associated with message.target. If we make the call through the content
> handler, there is no target and addonsManager fails.

Where is it that this is required? We should support handling the case with no window.
Attached patch Window IDs - mozilla-central (obsolete) — Splinter Review
OK. Another pass at this. Asking for feedback one more time, just to make sure this is the approach we want.

This uses the message manager if possible, but adds the windowID as a param. Then, if addonManager.js receives a message without a browser as its target, it uses the ID to find it. To do that I added new method(s) to nsIBrowserDOMWindow. There's a mobile-browser patch as well that implements those. 

addonManager.js still has to use two services as the parentprocessmessagemanager doesn't implement nsIChromeFrameMessageManager.
Attachment #473646 - Attachment is obsolete: true
Attachment #473811 - Flags: feedback?(mark.finkle)
Attachment #473646 - Flags: feedback?(mark.finkle)
Attached patch mozilla-central (obsolete) — Splinter Review
Whoops. Wrong guy.
Attachment #473811 - Attachment is obsolete: true
Attachment #473815 - Flags: feedback?(mark.finkle)
Attachment #473811 - Flags: feedback?(mark.finkle)
Attached patch mozilla-central (obsolete) — Splinter Review
Really sorry for the bugspam. Had to pull some cruft out of this patch, and removed the interface stuff on accident. This should be right. I'll hold off on the mobile-browser patch to avoid bugspam unless you want it mfinkle.
Attachment #473816 - Flags: feedback?(mark.finkle)
Attachment #473815 - Flags: feedback?(mark.finkle)
In an effort to keep things from getting too complicated, and potentially obsolete, I want to suggest a more conservative approach:
* Boris mentioned in comment 12 that content handlers running in the child process is not the long-term behavior, so we may see this problem "go away" in the future.
* Calling InstallTrigger from the content handler works fine as long as we have a window. We should have a window for nearly all e10s cases I can think of, but I could be wrong
* The content handler should work fine when used in a non-windowed case from the chrome process

Therefore, I am recommending we go with the "InstallTrigger" fix for now. It should work in most cases. We can work toward a better fix in the future.

This approach should not affect Firefox at all, and should work well enough in Fennec - it would definitely fix the most common use-case of "I clicked on an XPI link".

Thoughts?
(In reply to comment #21)
> In an effort to keep things from getting too complicated, and potentially
> obsolete, I want to suggest a more conservative approach:
> * Boris mentioned in comment 12 that content handlers running in the child
> process is not the long-term behavior, so we may see this problem "go away" in
> the future.
> * Calling InstallTrigger from the content handler works fine as long as we have
> a window. We should have a window for nearly all e10s cases I can think of, but
> I could be wrong
> * The content handler should work fine when used in a non-windowed case from
> the chrome process
> 
> Therefore, I am recommending we go with the "InstallTrigger" fix for now. It
> should work in most cases. We can work toward a better fix in the future.
> 
> This approach should not affect Firefox at all, and should work well enough in
> Fennec - it would definitely fix the most common use-case of "I clicked on an
> XPI link".
> 
> Thoughts?

Sounds fair, the one thing I would test is whether an XMLHttpRequest initiated from a webpage to an XPI is caught and handled correctly by this, that is the only odd case that I have seen used to install extensions.
Blocks: 593187
Attached patch Install Trigger again (obsolete) — Splinter Review
Mossop, I'm not sure exactly what you mean by XMLHttpRequest. I've tried testing a few ways, and talked to the AMO people, but I'm not sure how to trigger an install using it.

There is a case where:
1.) The user loads MyPage.php
2.) MyPhp sends back the xpi in the response body with
    Content-Disposition: attachment; filename=name.xpi
    Content-Type:        application/x-xpinstall
3.) Fennec should download the file and execute it using itself (nsIFile.run()), installing it

Currently Fennec ignores the suggested filename here, and for some reason it never copies the downloaded data into the incorrectly named file (I'll file those bugs tomorrow). As a result, any file it does open is usually empty.

I was going to test a little further in that by checking what nsIFile.run() does to an xpi, but I have to force Fennec to be the default handler for xpis on a device and I'm not really sure how at the moment. That's for tomorrow... unless we consider that a pretty rare use case. In which case here is the patch for review.
Attachment #473815 - Attachment is obsolete: true
Attachment #473816 - Attachment is obsolete: true
Attachment #473816 - Flags: feedback?(mark.finkle)
Comment on attachment 474615 [details] [diff] [review]
Install Trigger again

Looks OK to me. Let's make sure this is what Dave had in mind.
Attachment #474615 - Flags: review?(dtownsend)
(In reply to comment #23)
> Created attachment 474615 [details] [diff] [review]
> Install Trigger again
> 
> Mossop, I'm not sure exactly what you mean by XMLHttpRequest. I've tried
> testing a few ways, and talked to the AMO people, but I'm not sure how to
> trigger an install using it.

What I meant was a webpage's JavaScript code can create an XMLHttpRequest and open it to the url of an XPI, previously this would cause an install prompt, same as if it had navigated directly to the XPI.

> There is a case where:
> 1.) The user loads MyPage.php
> 2.) MyPhp sends back the xpi in the response body with
>     Content-Disposition: attachment; filename=name.xpi
>     Content-Type:        application/x-xpinstall
> 3.) Fennec should download the file and execute it using itself
> (nsIFile.run()), installing it

I'm not really sure what this case should do but I don't think it is what you suggest in 3. It probably should just download it and save it.
Mossop, Testing with something simple like:

var req = new XMLHttpRequest();
req.open('GET', "myfile.xpi", true);
req.onreadystatechange = function (aEvt) { ... }
req.send(null);

no install is triggered in Firefox and Fennec. I set the "Content-type" header, but its possible theres some other field which I'm unaware of. As such, I think this patch matches Firefox's current behavior.
Comment on attachment 474615 [details] [diff] [review]
Install Trigger again

Strange, I guess the person I remember talking to about triggering installs with an XHR doesn't remember anything about it so maybe I led you on a wild goose chase, sorry about that.

Still not really keen on this approach but it seems like the best we can do for now so ok.

>diff --git a/toolkit/mozapps/extensions/amContentHandler.js b/toolkit/mozapps/extensions/amContentHandler.js
>--- a/toolkit/mozapps/extensions/amContentHandler.js
>+++ b/toolkit/mozapps/extensions/amContentHandler.js
>@@ -63,34 +63,44 @@ amContentHandler.prototype = {
>     if (aMimetype != XPI_CONTENT_TYPE)
>       throw Cr.NS_ERROR_WONT_HANDLE_CONTENT;
> 
>     if (!(aRequest instanceof Ci.nsIChannel))
>       throw Cr.NS_ERROR_WONT_HANDLE_CONTENT;
> 
>     let uri = aRequest.URI;
> 
>-    let referer = null;
>-    if (aRequest instanceof Ci.nsIPropertyBag2) {
>-      referer = aRequest.getPropertyAsInterface("docshell.internalReferrer",
>-                                                Ci.nsIURI);
>-    }
>-
>     let window = null;
>     let callbacks = aRequest.notificationCallbacks ?
>                     aRequest.notificationCallbacks :
>                     aRequest.loadGroup.notificationCallbacks;
>     if (callbacks)
>       window = callbacks.getInterface(Ci.nsIDOMWindow);
> 
>     aRequest.cancel(Cr.NS_BINDING_ABORTED);
> 
>-    let manager = Cc["@mozilla.org/addons/integration;1"].
>-                  getService(Ci.amIWebInstaller);
>-    manager.installAddonsFromWebpage(aMimetype, window, referer, [uri.spec],
>-                                     [null], [null], [null], null, 1);
>+    let appInfo = Cc["@mozilla.org/xre/app-info;1"];
>+    if (appInfo && appInfo.getService(Ci.nsIXULRuntime).processType == Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT) {

Wrap this line so it doesn't go over 80 chars. When is appInfo ever going to be undefined? Could probably also just use Services.jsm to simplify this.

>+
>+      if (!window.InstallTrigger) 
>+        window = window.wrappedJSObject;
>+
>+      if(window.InstallTrigger)

Space after the if

>+        window.InstallTrigger.install({URL:uri.spec});

Use the startSoftwareUpdate method, although deprecated for websites it acts a bit nicer for plain urls for our purposes.

>+    } else {

New line after the closing brace

>+      let referer = null;
>+      if (aRequest instanceof Ci.nsIPropertyBag2) {
>+        referer = aRequest.getPropertyAsInterface("docshell.internalReferrer",
>+                                                Ci.nsIURI);
>+      }
>+
>+      let manager = Cc["@mozilla.org/addons/integration;1"].
>+                          getService(Ci.amIWebInstaller);

Make getService line up with the Cc

>+      manager.installAddonsFromWebpage(aMimetype, window, referer, [uri.spec],
>+                                      [null], [null], [null], null, 1);

Line up the first argument on the new line with the first argument from the previous line.
Attachment #474615 - Flags: review?(dtownsend) → review-
(In reply to comment #27)
> When is appInfo ever going to be
> undefined?

It is undefined in non-libxul builds (like seamonkey).
(In reply to comment #28)
> (In reply to comment #27)
> > When is appInfo ever going to be
> > undefined?
> 
> It is undefined in non-libxul builds (like seamonkey).

Only in the content process?
(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > When is appInfo ever going to be
> > > undefined?
> > 
> > It is undefined in non-libxul builds (like seamonkey).
> 
> Only in the content process?

They build without MOZ_IPC, so no content process at all.
(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > (In reply to comment #27)
> > > > When is appInfo ever going to be
> > > > undefined?
> > > 
> > > It is undefined in non-libxul builds (like seamonkey).
> > 
> > Only in the content process?
> 
> They build without MOZ_IPC, so no content process at all.

Then I don't think you are correct, quite important parts of the platform rely on it existing to work and some Seamonkey code uses it itself.
Oh, shoot, I gave Alon the wrong advice.  The appInfo is fine, it's the .getService(Ci.nsIXULRuntime) which fails in Seamonkey.
(In reply to comment #32)
> Oh, shoot, I gave Alon the wrong advice.  The appInfo is fine, it's the
> .getService(Ci.nsIXULRuntime) which fails in Seamonkey.

That still surprises me since the add-ons manager would fail without this, as would the update service, and some seamonkey specific tests use it.
I'm still mis-remembering, it turns out.  Apparently the configuration I'm thinking of is Thunderbird, which has previously failed on the appInfo line.  See http://hg.mozilla.org/mozilla-central/rev/883651094520 for an example.
(In reply to comment #34)
> I'm still mis-remembering, it turns out.  Apparently the configuration I'm
> thinking of is Thunderbird, which has previously failed on the appInfo line. 
> See http://hg.mozilla.org/mozilla-central/rev/883651094520 for an example.

That's in an xpcshell test where I agree, app-info cannot be reliably had (which is why many tests create a mock implementation of it to allow the components to work). We can rely on this existing in core code though.
Attached patch cleanup (obsolete) — Splinter Review
Final cleanup. Services.appinfo fails in content. I'm don't know why, but here I just left in the other Service code.
Attachment #474615 - Attachment is obsolete: true
Attachment #474970 - Flags: review?
No longer blocks: 593187
Flags: in-litmus+
Attachment #474970 - Flags: review? → review?(dtownsend)
Comment on attachment 474970 [details] [diff] [review]
cleanup

>diff --git a/toolkit/mozapps/extensions/amContentHandler.js b/toolkit/mozapps/extensions/amContentHandler.js
>--- a/toolkit/mozapps/extensions/amContentHandler.js
>+++ b/toolkit/mozapps/extensions/amContentHandler.js
>@@ -63,34 +63,46 @@ amContentHandler.prototype = {
>     if (aMimetype != XPI_CONTENT_TYPE)
>       throw Cr.NS_ERROR_WONT_HANDLE_CONTENT;
> 
>     if (!(aRequest instanceof Ci.nsIChannel))
>       throw Cr.NS_ERROR_WONT_HANDLE_CONTENT;
> 
>     let uri = aRequest.URI;
> 
>-    let referer = null;
>-    if (aRequest instanceof Ci.nsIPropertyBag2) {
>-      referer = aRequest.getPropertyAsInterface("docshell.internalReferrer",
>-                                                Ci.nsIURI);
>-    }
>-
>     let window = null;
>     let callbacks = aRequest.notificationCallbacks ?
>                     aRequest.notificationCallbacks :
>                     aRequest.loadGroup.notificationCallbacks;
>     if (callbacks)
>       window = callbacks.getInterface(Ci.nsIDOMWindow);
> 
>     aRequest.cancel(Cr.NS_BINDING_ABORTED);
> 
>-    let manager = Cc["@mozilla.org/addons/integration;1"].
>-                  getService(Ci.amIWebInstaller);
>-    manager.installAddonsFromWebpage(aMimetype, window, referer, [uri.spec],
>-                                     [null], [null], [null], null, 1);
>+    let appinfo = Cc["@mozilla.org/xre/app-info;1"].
>+                  getService(Ci.nsIXULRuntime);
>+    if (appinfo.processType == Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT) {
>+

No newline at the start of the block

>+      if (!window.InstallTrigger) 
>+        window = window.wrappedJSObject;
>+
>+      if (window.InstallTrigger)
>+        window.InstallTrigger.startSoftwareUpdate(uri.spec);

It might be worth logging something here so we can track down cases where this might not work.

>+    } else {
>+

Ditto, and put a newline after the closing brace.

>+      let referer = null;
>+      if (aRequest instanceof Ci.nsIPropertyBag2) {
>+        referer = aRequest.getPropertyAsInterface("docshell.internalReferrer",
>+                                                Ci.nsIURI);
>+      }
>+
>+      let manager = Cc["@mozilla.org/addons/integration;1"].
>+                    getService(Ci.amIWebInstaller);
>+      manager.installAddonsFromWebpage(aMimetype, window, referer, [uri.spec],
>+                                       [null], [null], [null], null, 1);
>+    }
>   },
> 
>   classID: Components.ID("{7beb3ba8-6ec3-41b4-b67c-da89b8518922}"),
>   QueryInterface: XPCOMUtils.generateQI([Ci.nsIContentHandler])
> };
> 
> var NSGetFactory = XPCOMUtils.generateNSGetFactory([amContentHandler]);
Attachment #474970 - Flags: review?(dtownsend) → review+
Attached patch Patch for checkin (obsolete) — Splinter Review
Sorry. I lost this in my queue. Here is the final patch for checkin. I still need to run this through try.
Attachment #474970 - Attachment is obsolete: true
Attachment #482050 - Flags: review+
And that wasn't the right patch...
Attachment #482050 - Attachment is obsolete: true
Attachment #482052 - Flags: review+
pushed:
http://hg.mozilla.org/mozilla-central/rev/12771f51d64a
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
verified FIXED on builds:
Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101014 Namoroka/4.0b8pre Fennec/4.0b2pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101014 Namoroka/4.0b8pre Fennec/4.0b2pre
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
I can verify it as fixed in fennec_4.0~b2~20101014005046_armel.deb for Maemo, too. Thanks for the fix.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: