565389 - Stop passing a loadgroup through from InstallTrigger to the add-ons manager

Status: RESOLVED INVALID

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Dave Townsend [:mossop]]

E10S is going to make things difficult to do this anymore. Currently we use the loadgroup to automatically handle authentication dialogs and to fix cookie sending (bug 462739).

We can fix the auth dialogs by handling auth from the notification callbacks ourselves.

We can fix cookies using the forceAllowThirdPartyCookies property on nsIHttpChannelInternal. The slight downside is that means cookies always get sent with the http request for the XPI regardless of the third party cookie setting.

Comment 2

[Alon Zakai (:azakai)]

Attachment: Patch

Patch passes all the automated tests.

Note that I temporarily replaced the window sent from amInstallTrigger.cpp with null. The reason is that anyhow this code will not be used once the InstallTrigger patch (bug 550936) lands, so I didn't spend time figuring out what should properly be sent there. (To test the two patches together, can get my patch queue, http://hg.mozilla.org/users/azakai_mozilla.com/patches )

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 458008 [details] [diff] [review]
Patch

Pretty much there, some small changes are necessary I think though.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -633,11 +633,7 @@
>   {
>     var brandBundle = document.getElementById("bundle_brand");
>     var installInfo = aSubject.QueryInterface(Components.interfaces.amIWebInstallInfo);
>-    var win = installInfo.originatingWindow;
>-    var shell = win.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>-                   .getInterface(Components.interfaces.nsIWebNavigation)
>-                   .QueryInterface(Components.interfaces.nsIDocShell);
>-    var browser = this._getBrowser(shell);
>+    var browser = installInfo.originatingElement;
>     if (!browser)
>       return;
>     const anchorID = "addons-notification-icon";
>diff --git a/toolkit/mozapps/extensions/AddonManager.jsm b/toolkit/mozapps/extensions/AddonManager.jsm
>--- a/toolkit/mozapps/extensions/AddonManager.jsm
>+++ b/toolkit/mozapps/extensions/AddonManager.jsm
>@@ -388,20 +388,20 @@
>    *         An optional placeholder icon URL while the add-on is being downloaded
>    * @param  aVersion
>    *         An optional placeholder version while the add-on is being downloaded
>-   * @param  aLoadGroup
>-   *         An optional nsILoadGroup to associate any network requests with
>+   * @param  aElement
>+   *         An optional XULElement to associate the install with

Minor nit, but I think the backend code here need not care what kind of element it is, so just call it a DOM Element throughout. This applies to the other comments too.

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm

> /**
>+ * Handles callbacks for HTTP channels of XPI downloads. We support
>+ * prompting for auth dialogs and, optionally, to ignore bad certs.
>+ *
>+ * @param  aElement
>+ *         An optional XULElement related to the request
>+ * @param  aNeedBadCertHandling
>+ *         Whether we should handle bad certs or not
>+ */
>+function XPINotificationCallbacks(aElement, aNeedBadCertHandling) {
>+  this.window = (aElement && aElement.contentWindow) ? aElement.contentWindow :
>+    null;

Test (aElement && "contentWindow" in aElement) instead. Presumably this does not work for out of process content, I'm not sure how we can support that case properly in the new world order.

>+  // Verify that we don't end up on an insecure channel if we haven't got a
>+  // hash to verify with (see bug 537761 for discussion)
>+  this.needBadCertHandling = aNeedBadCertHandling;
>+
>+  if (this.needBadCertHandling) {
>+    Components.utils.import("resource://gre/modules/CertUtils.jsm");
>+
>+    this.notifyCertProblem = BadCertHandler.prototype.notifyCertProblem;
>+    this.notifySSLError = BadCertHandler.prototype.notifySSLError;
>+  }
>+}
>+
>+XPINotificationCallbacks.prototype = {
>+  QueryInterface: function(iid) {
>+    if (iid.equals(Ci.nsISupports) ||
>+        iid.equals(Ci.nsIInterfaceRequestor) ||
>+        ((iid.equals(Ci.nsIBadCertListener2) ||
>+          iid.equals(Ci.nsISSLErrorListener) && this.needBadCertHandling)))
>+      return this;
>+    throw Components.results.NS_ERROR_NO_INTERFACE;
>+  },
>+
>+  getInterface: function(iid) {
>+    if (iid.equals(Components.interfaces.nsIAuthPrompt2)) {
>+      var pwmgr = Cc["@mozilla.org/passwordmanager/authpromptfactory;1"].
>+                  getService(Ci.nsIPromptFactory);
>+      return pwmgr.getPrompt(this.window, Ci.nsIAuthPrompt2);
>+    } else if ((iid.equals(Ci.nsIBadCertListener2) ||
>+                iid.equals(Ci.nsISSLErrorListener)) &&
>+               this.needBadCertHandling)
>+      return this;
>+
>+    throw Components.results.NS_ERROR_NO_INTERFACE;
>+  },
>+};

This is going to break some of the special redirection handling which I'm surprised didn't make some tests fail. It will also cause problems if we make changed to BadCertHandler in the future. Instead what you should do is have this object only implement nsIInterfaceRequestor, create a new BadCertHandler in the constructor and then in getInterface return the prompt if nsIAuthPrompt2 was requested, otherwise just call the BadCertHandler's getInterface method.

>     listener.init(this, this.stream);
>     try {
>       this.channel = NetUtil.newChannel(this.sourceURI);
>-      if (this.loadGroup)
>-        this.channel.loadGroup = this.loadGroup;
>-
>-      // Verify that we don't end up on an insecure channel if we haven't got a
>-      // hash to verify with (see bug 537761 for discussion)
>-      if (!this.hash)
>-        this.channel.notificationCallbacks = new BadCertHandler();
>+      this.channel.notificationCallbacks = new XPINotificationCallbacks(
>+        this.element, !this.hash);

Line break before new.

>+      this.channel.QueryInterface(Ci.nsIHttpChannelInternal).
>+        forceAllowThirdPartyCookie = true;

Line up dots etc.

> #endif
> 
> AddonManagerPrivate.registerProvider(XPIProvider);
>+

Trailing empty line.

>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
>@@ -74,19 +74,12 @@
>                                                 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);
>+    manager.installAddonsFromWebpage(aMimetype, null, referer, [uri.spec],
>+      [null], [null], [null], null, 1);

Line these arguments back up as they were.

>diff --git a/toolkit/mozapps/extensions/amIWebInstallListener.idl b/toolkit/mozapps/extensions/amIWebInstallListener.idl
>--- a/toolkit/mozapps/extensions/amIWebInstallListener.idl
>+++ b/toolkit/mozapps/extensions/amIWebInstallListener.idl
>@@ -36,8 +36,8 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsISupports.idl"
>+#include "nsIDOMXULElement.idl"

Just use nsIDOMElement throughout, also move this include after the empty line.

>diff --git a/toolkit/mozapps/extensions/amIWebInstaller.idl b/toolkit/mozapps/extensions/amIWebInstaller.idl
>--- a/toolkit/mozapps/extensions/amIWebInstaller.idl
>+++ b/toolkit/mozapps/extensions/amIWebInstaller.idl
>@@ -36,8 +36,8 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsISupports.idl"
>+#include "nsIDOMXULElement.idl"

Ditto.

>diff --git a/toolkit/mozapps/extensions/amWebInstallListener.js b/toolkit/mozapps/extensions/amWebInstallListener.js

>     args.installs = this.downloads;
>     args.wrappedJSObject = args;
> 
>-    Services.ww.openWindow(this.window, "chrome://mozapps/content/xpinstall/xpinstallConfirm.xul",
>+    var window = this.element && this.element.contentWindow ? this.contentWindow : null;

Same test change as previously.

>diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js
>--- a/toolkit/mozapps/extensions/content/extensions.js
>+++ b/toolkit/mozapps/extensions/content/extensions.js
>@@ -1343,7 +1343,7 @@
>       if (pos == urls.length) {
>         if (installs.length > 0) {
>           // Display the normal install confirmation for the installs
>-          AddonManager.installAddonsFromWebpage("application/x-xpinstall", this,
>+          AddonManager.installAddonsFromWebpage("application/x-xpinstall", this.document.documentElement,

If you're going to be passing this then it probably makes sense for the code that attempts to get a window from an element to also look at element.ownerDocument.defaultView.

>diff --git a/toolkit/mozapps/extensions/test/xpinstall/browser_whitelist.js b/toolkit/mozapps/extensions/test/xpinstall/browser_whitelist.js
>--- a/toolkit/mozapps/extensions/test/xpinstall/browser_whitelist.js
>+++ b/toolkit/mozapps/extensions/test/xpinstall/browser_whitelist.js
>@@ -17,7 +17,7 @@
> }
> 
> function allow_blocked(installInfo) {
>-  is(installInfo.originatingWindow, gBrowser.contentWindow, "Install should have been triggered by the right window");
>+  is(getWindow(installInfo.originatingElement), getWindow(gBrowser), "Install should have been triggered by the right window");

Might as well test that originatingElement is equal to the browser element we expect rather than trying to get a window out of it. Same for all the other tests.

>+function getWindow(node) {
>+  var window = gBrowser;
>+  while (window && window.nodeName != 'window')
>+    window = window.parentNode;
>+  return window;
>+}

This looks like it always returns the same thing regardless of the passed node so the tests that compare its result don't seem very useful. I guess they are going away though.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

>>+function XPINotificationCallbacks(aElement, aNeedBadCertHandling) {
>>+  this.window = (aElement && aElement.contentWindow) ? aElement.contentWindow :
>>+    null;

>Test (aElement && "contentWindow" in aElement) instead. Presumably this does
>not work for out of process content, I'm not sure how we can support that case
>properly in the new world order.

If aElement is a <browser> the contentWindow will always be there. In multi-process cases, .contentWindow is always null - but it's always present.

>+  getInterface: function(iid) {
>+    if (iid.equals(Components.interfaces.nsIAuthPrompt2)) {
>+      var pwmgr = Cc["@mozilla.org/passwordmanager/authpromptfactory;1"].
>+                  getService(Ci.nsIPromptFactory);
>+      return pwmgr.getPrompt(this.window, Ci.nsIAuthPrompt2);

Is this the best way to make an authprompt in the "new prompter world" we live in? Maybe ask Dolske?

Oh and use Ci.nsIAuthPrompt2 in the "if"

Comment 5

[Dave Townsend [:mossop]]

(In reply to comment #4)
> >>+function XPINotificationCallbacks(aElement, aNeedBadCertHandling) {
> >>+  this.window = (aElement && aElement.contentWindow) ? aElement.contentWindow :
> >>+    null;
> 
> >Test (aElement && "contentWindow" in aElement) instead. Presumably this does
> >not work for out of process content, I'm not sure how we can support that case
> >properly in the new world order.
> 
> If aElement is a <browser> the contentWindow will always be there. In
> multi-process cases, .contentWindow is always null - but it's always present.

That is fine, I'm just covering the case where aElement is not a <browser>. If it is we use aElement.contentWindow which will either be null or a useful window.

Comment 6

[Alon Zakai (:azakai)]

Correct me if I am wrong, but I believe this bug is no longer needed/relevant.