550936 - Make InstallTrigger support cross-process communication

Status: RESOLVED FIXED

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

Description

[Dave Townsend [:mossop]]

Content webpages currently have an InstallTrigger object used to test whether installing extensions is enabled and to start new installs. This will need some love to make it work between processes.

The InstallTrigger API is synchronous to content callers in that most of the methods return true or false depending on the state of the software installation preferences however starting an extension install is asynchronous with an optional callback. Chrome can call the callback asynchronously.

Comment 1

[Alon Zakai (:azakai)]

In bug 566024 I have tried to make the case for a 'global messageManager' approach which would have made this bug fixable with almost no changes to existing code and almost no new code. However, there are apparently some problems with that idea and it has not convinced people.

Instead, there are 2 ways to go from here, advice on which makes more sense is welcome:

1. The usual ipdl approach. This would add several new files to /toolkit/mozapps/extensions, and insert some changes to existing code, namely #ifdefs around some InstallTrigger code to make it work differently assuming e10s, and various Makefile changes in various places.

2. Implement the InstallTrigger object in a JavaScript function, which in the non-e10s case would call the addonManager code, and in the e10s case would use the messageManager fix planned in bug 566024. Not sure how feasible this is or how many changes it would require to existing code, but potentially it would require far less new code than approach #1.

Comment 2

[Dave Townsend [:mossop]]

(In reply to comment #1)
> 2. Implement the InstallTrigger object in a JavaScript function, which in the
> non-e10s case would call the addonManager code, and in the e10s case would use
> the messageManager fix planned in bug 566024. Not sure how feasible this is or
> how many changes it would require to existing code, but potentially it would
> require far less new code than approach #1.

I attempted to implement InstallTrigger as a JS XPCOM component (if that is what you are talking about) but fell down because the InstallTrigger API is kind of bizarre and xpconnect can't passing through named properties on arrays properly.

Comment 3

[Alon Zakai (:azakai)]

(In reply to comment #2)
> (In reply to comment #1)
> > 2. Implement the InstallTrigger object in a JavaScript function, which in the
> > non-e10s case would call the addonManager code, and in the e10s case would use
> > the messageManager fix planned in bug 566024. Not sure how feasible this is or
> > how many changes it would require to existing code, but potentially it would
> > require far less new code than approach #1.
> 
> I attempted to implement InstallTrigger as a JS XPCOM component (if that is
> what you are talking about) but fell down because the InstallTrigger API is
> kind of bizarre and xpconnect can't passing through named properties on arrays
> properly.

Hmm, I meant something like this:

1. 'Rename' the existing InstallTrigger to (say) InstallTriggerInternal. Do not alter that code at all.
2. In JS, create an InstallTrigger object, with small stubs that do the following:
2.1. In non-e10s, simply call InstallTriggerInternal.
2.2. In e10s, send a cross-process message with the parameters we received.
2.2.1. Also in e10s, in the parent process set up listening for those cross-process messages.

This doesn't implement the entire InstallTrigger in JS, but just wraps around it using JS. But it still needs to pass all the parameters properly etc. - would this fall prey to the same problems you mentioned?

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > 2. Implement the InstallTrigger object in a JavaScript function, which in the
> > > non-e10s case would call the addonManager code, and in the e10s case would use
> > > the messageManager fix planned in bug 566024. Not sure how feasible this is or
> > > how many changes it would require to existing code, but potentially it would
> > > require far less new code than approach #1.
> > 
> > I attempted to implement InstallTrigger as a JS XPCOM component (if that is
> > what you are talking about) but fell down because the InstallTrigger API is
> > kind of bizarre and xpconnect can't passing through named properties on arrays
> > properly.
> 
> Hmm, I meant something like this:
> 
> 1. 'Rename' the existing InstallTrigger to (say) InstallTriggerInternal. Do not
> alter that code at all.
> 2. In JS, create an InstallTrigger object, with small stubs that do the
> following:
> 2.1. In non-e10s, simply call InstallTriggerInternal.
> 2.2. In e10s, send a cross-process message with the parameters we received.
> 2.2.1. Also in e10s, in the parent process set up listening for those
> cross-process messages.
> 
> This doesn't implement the entire InstallTrigger in JS, but just wraps around
> it using JS. But it still needs to pass all the parameters properly etc. -
> would this fall prey to the same problems you mentioned?

I guess I don't know how you would expose that JS InstallTrigger object to the content webpages, the only method I know of is to use JS XPCOM components but perhaps there are other ways I do not know of.

Comment 5

[Alon Zakai (:azakai)]

(In reply to comment #4)
> I guess I don't know how you would expose that JS InstallTrigger object to the
> content webpages, the only method I know of is to use JS XPCOM components but
> perhaps there are other ways I do not know of.

Yeah, it might not be possible I guess. I will try to find out.

Comment 6

[Alon Zakai (:azakai)]

Attachment: e10s patch (1/2)

Part 1 of 2, WIP patch. This applies to e10s.

It assumes patches from bug 566024 and bug 567357, and includes a minor fix to make them work.

Comment 7

[Alon Zakai (:azakai)]

Attachment: mobile-browser patch (2/2)

Part 2 of 2, WIP patch. This applies to mobile-browser.

It assumes patches from bug 566024 and bug 567357.

This patch mostly works: Install dialogs are shown, and installation callbacks are called. However despite the callbacks indicating success, the addon does not appear in the addon manager. Not sure yet if that is a GUI issue or something more deep.

Feedback is needed about some stuff:

1. Should new files be created for the code in browser.js and frame-content.js (instead of adding to those existing files)?

2. In the patch the real InstallTrigger is renamed to InstallTriggerInternal. However that means every window has that property, so perhaps this should be avoided?

3. All child windows set up to listen for callback messages. Consequently, when a callback is sent, more than one window receives it. The current code will work, though, as if a callback has not been prepared, none will be called (so only the relevant window will actually do something). But perhaps this should be done in a more secure way (UUIDs?). Or, is there a proper way to only send the message to the calling window?

4. Only install() and enabled() messages are sent from child->parent; the deprecated API calls are implemented through them. The code to do that, however, is duplicated in this patch and in the previously-existing code, as I couldn't see an easy way to share that code without major changes. Perhaps though there is an elegant way to do that?

Comment 8

[Alon Zakai (:azakai)]

Attachment: HTML file used to test WIP patch

Comment 9

[Alon Zakai (:azakai)]

It was indeed just a GUI issue. Updating to the latest mobile-browser now shows that the WIP patch does in fact install addons properly.

So, there remain the 4 questions I mentioned 2 comments ago, aside from that this seems mostly done.

Comment 10

[Doug Turner (:dougt)]

maybe the mobile-browser piece should be in toolkit.

Either way, Dave can you take a pass at reviewing these patches?

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 449904 [details] [diff] [review]
e10s patch (1/2)

Sure, should have time to get to it sometime this week

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 449904 [details] [diff] [review]
e10s patch (1/2)

There are lots of pieces here that I don't fully understand (are there docs o nthe message manager anywhere?). But here are my initial thoughts on the two patches.

>diff --git a/toolkit/mozapps/extensions/amInstallTrigger.cpp b/toolkit/mozapps/extensions/amInstallTrigger.cpp
>--- a/toolkit/mozapps/extensions/amInstallTrigger.cpp
>+++ b/toolkit/mozapps/extensions/amInstallTrigger.cpp
>@@ -369,7 +369,7 @@
> 
>   nsXPIDLCString previous;
>   rv = catman->AddCategoryEntry(JAVASCRIPT_GLOBAL_PROPERTY_CATEGORY,
>-                                "InstallTrigger",
>+                                "InstallTriggerInternal",

This breaks usage of InstallTrigger in the chrome process, I'd rather not do that, is there any way it can be avoided?

Comment 13

[Josh Matthews [:jdm]]

The most complete docs are here: https://wiki.mozilla.org/Content_Process_Event_Handlers

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 449905 [details] [diff] [review]
mobile-browser patch (2/2)

>+/**
>+ * Listens to requests from child processes for InstallTrigger
>+ * activity, and sends back callbacks.
>+ * TODO: Check if this works in an iframe
>+ */
>+function UpdateTriggerParent() {
>+  messageManager.addMessageListener("InstallTriggerInstall", this);
>+  messageManager.addMessageListener("InstallTriggerEnabled", this);
>+}
>+
>+UpdateTriggerParent.prototype = {
>+  receiveMessage: Util.receiveMessage,
>+
>+  receiveInstallTriggerEnabled: function receiveInstallTriggerEnabled(message) {
>+    return window.InstallTriggerInternal.enabled();
>+  },
>+
>+  receiveInstallTriggerInstall: function receiveInstallTriggerInstall(message, payload) {
>+    var args = payload.args;
>+    var callbackId = payload.callbackId;
>+    return window.InstallTriggerInternal.install(args, {
>+      onInstallEnded: function ITI_callback(url, status) {
>+        messageManager.sendAsyncMessage('InstallTriggerCallback', {
>+          callbackId: callbackId,
>+          url: url,
>+          status: status
>+        });

This breaks the security checks and URI resolution in the InstallTrigger code.

mfinkle tells me that you should be using message.target.messageManager.sendAsyncMessage instead.

>+      }
>+    });
>+  },
>+};
>+var updateTriggerParent = new UpdateTriggerParent();

I think all this should be in mozilla-central somewhere so that all apps can use it as they switch to e10s. Maybe as a jsm in toolkit/mozapps/extensions.

>+/**
>+ * 'Fake' InstallTrigger object that just functions as an
>+ * interface that looks like the real thing, and relays
>+ * messages forward properly
>+ */
>+InstallTrigger = function(window) {
>+  window.wrappedJSObject.InstallTrigger = this;

Is this safe? I was under the impression that we hit all sorts of security issues when allowing content to directly call an object like this? If that isn't the case then I'm half inclined to say that amInstallTrigger.cpp should be implemented in JS and we just remote its calls to the add-ons manager rather than adding this additional level.

>+InstallTrigger.prototype = {
>+  receiveMessage: Util.receiveMessage,
>+
>+  addCallback: function(callback, urls) {
>+    if (!callback) return -1;
>+    var callbackId = 0;
>+    while (true) {
>+      if (!this.callbacks[callbackId]) {
>+        break;
>+      }
>+      callbackId += 1;
>+    }
>+    this.callbacks[callbackId] = {
>+      callback: callback,
>+      urls: urls, // TODO: JSON.parse(JSON.stringify(urls)), // Clone the urls

urls.slice(0) is probably a quicker way to clone assuming it is just an array.

>+    };
>+    return callbackId;
>+  },
>+
>+  // Also functions as removeCallback to parallel addCallback. When
>+  // all URLs are exhausted, free the callbackId and linked stuff
>+  receiveInstallTriggerCallback: function(message, payload) {
>+    var callbackId = payload.callbackId;
>+    var url = payload.url;
>+    var status = payload.status;
>+    this.callbacks[callbackId].callback(url, status);
>+    delete this.callbacks[callbackId].urls[url];
>+    if (JSON.stringify(this.callbacks[callbackId].urls[url]) === '{}') {
>+      this.callbacks[callbackId] = null;
>+    }
>+  },

Seems like web content will be able to call these methods, can't see why anyone would ever want to but for safety they should not be present on the public InstallTrigger object.

>+  enabled: function() {
>+    return sendSyncMessage("InstallTriggerEnabled");

Isn't this returning an array?

>+  },
>+
>+  updateEnabled: function() {
>+    return this.enabled();
>+  },
>+
>+  install: function(args, callback) {
>+    var callbackId = this.addCallback(callback);

You need to pass the urls here.

>+    var ret = sendSyncMessage("InstallTriggerInstall", { args: args, callbackId: callbackId });
>+    return ret[0]['message'];

Where is the 'message' coming from?

>+  installChrome: function(type, url, skin) {
>+    return this.install({
>+      'installChromeAddon': {
>+        'URL': url
>+      }

This just needs to be |'name': url| but please come up with a better name, maybe just use the filename part of the url.

>+  startSoftwareUpdate: function(url, flags) {
>+    return this.install({
>+      'startSoftwareUpdateAddon': {
>+        'URL': url
>+      }

Ditto.

Comment 15

[Alon Zakai (:azakai)]

Attachment: V2 of mobile-browser patch (2/2)

Attached is a patch with fixes following the comments. Responses to comments are below.

> > 
> >   nsXPIDLCString previous;
> >   rv = catman->AddCategoryEntry(JAVASCRIPT_GLOBAL_PROPERTY_CATEGORY,
> >-                                "InstallTrigger",
> >+                                "InstallTriggerInternal",
>
> This breaks usage of InstallTrigger in the chrome process, I'd rather not do
> that, is there any way it can be avoided?

It turns out that this is not necessary, that can simply be removed from the e10s patch (1/2). Since the other part of that patch is just a temporary fix for something that hasn't landed yet, that patch is no longer really necessary.

> >+/**
> >+ * Listens to requests from child processes for InstallTrigger
> >+ * activity, and sends back callbacks.
> >+ * TODO: Check if this works in an iframe
> >+ */
> >+function UpdateTriggerParent() {
> >+  messageManager.addMessageListener("InstallTriggerInstall", this);
> >+  messageManager.addMessageListener("InstallTriggerEnabled", this);
> >+}
> >+
> >+UpdateTriggerParent.prototype = {
> >+  receiveMessage: Util.receiveMessage,
> >+
> >+  receiveInstallTriggerEnabled: function receiveInstallTriggerEnabled(message) {
> >+    return window.InstallTriggerInternal.enabled();
> >+  },
> >+
> >+  receiveInstallTriggerInstall: function receiveInstallTriggerInstall(message, payload) {
> >+    var args = payload.args;
> >+    var callbackId = payload.callbackId;
> >+    return window.InstallTriggerInternal.install(args, {
> >+      onInstallEnded: function ITI_callback(url, status) {
> >+        messageManager.sendAsyncMessage('InstallTriggerCallback', {
> >+          callbackId: callbackId,
> >+          url: url,
> >+          status: status
> >+        });
> 
> This breaks the security checks and URI resolution in the InstallTrigger code.

I don't understand what you mean here.

> 
> >+      }
> >+    });
> >+  },
> >+};
> >+var updateTriggerParent = new UpdateTriggerParent();
> 
> I think all this should be in mozilla-central somewhere so that all apps can
> use it as they switch to e10s. Maybe as a jsm in toolkit/mozapps/extensions.

Makes sense. How about

toolkit/mozapps/extensions/InstallTriggerChild.jsm
toolkit/mozapps/extensions/InstallTriggerParent.jsm

?

> 
> >+/**
> >+ * 'Fake' InstallTrigger object that just functions as an
> >+ * interface that looks like the real thing, and relays
> >+ * messages forward properly
> >+ */
> >+InstallTrigger = function(window) {
> >+  window.wrappedJSObject.InstallTrigger = this;
> 
> Is this safe? I was under the impression that we hit all sorts of security
> issues when allowing content to directly call an object like this? If that
> isn't the case then I'm half inclined to say that amInstallTrigger.cpp should
> be implemented in JS and we just remote its calls to the add-ons manager rather
> than adding this additional level.
> 

In implementing it this way, I was following bsmedberg's outline (mentioned in the linked bugs this is blocked by) - I don't understand all the security implications here myself.

> 
> Seems like web content will be able to call these methods, can't see why anyone
> would ever want to but for safety they should not be present on the public
> InstallTrigger object.
> 

Very good point, I fixed this in the updated patch.

> >+  enabled: function() {
> >+    return sendSyncMessage("InstallTriggerEnabled");
> 
> Isn't this returning an array?
>

Yes, fixed.
 
> >+  },
> >+
> >+  updateEnabled: function() {
> >+    return this.enabled();
> >+  },
> >+
> >+  install: function(args, callback) {
> >+    var callbackId = this.addCallback(callback);
> 
> You need to pass the urls here.
> 

Fixed.

> >+    var ret = sendSyncMessage("InstallTriggerInstall", { args: args, callbackId: callbackId });
> >+    return ret[0]['message'];
> 
> Where is the 'message' coming from?

That is the form in which the message manager returns sync responses (sendSyncMessage).

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #15)
> > >+UpdateTriggerParent.prototype = {
> > >+  receiveMessage: Util.receiveMessage,
> > >+
> > >+  receiveInstallTriggerEnabled: function receiveInstallTriggerEnabled(message) {
> > >+    return window.InstallTriggerInternal.enabled();
> > >+  },
> > >+
> > >+  receiveInstallTriggerInstall: function receiveInstallTriggerInstall(message, payload) {
> > >+    var args = payload.args;
> > >+    var callbackId = payload.callbackId;
> > >+    return window.InstallTriggerInternal.install(args, {
> > >+      onInstallEnded: function ITI_callback(url, status) {
> > >+        messageManager.sendAsyncMessage('InstallTriggerCallback', {
> > >+          callbackId: callbackId,
> > >+          url: url,
> > >+          status: status
> > >+        });
> > 
> > This breaks the security checks and URI resolution in the InstallTrigger code.
> 
> I don't understand what you mean here.

Calls to InstallTrigger can include relative URLs for the XPI and icon. They are meant to be resolved relative to the content URI of course however since this code calls InstallTrigger from a chrome URL the InstallTrigger code will attempt to resolve them relative to that. Additionally we do security checks to test whether the content URL that called InstallTrigger is allowed to access the URLs it passes for the XPI or icon, again with this change we'll do those security checks assuming the chrome url is the caller which will always pass.

> > >+    var ret = sendSyncMessage("InstallTriggerInstall", { args: args, callbackId: callbackId });
> > >+    return ret[0]['message'];
> > 
> > Where is the 'message' coming from?
> 
> That is the form in which the message manager returns sync responses
> (sendSyncMessage).

Are there docs on this somewhere? The only ones I have been able to find are these: https://wiki.mozilla.org/Content_Process_Event_Handlers#Content_Process

Comment 17

[Alon Zakai (:azakai)]

(In reply to comment #16)
> Calls to InstallTrigger can include relative URLs for the XPI and icon. They
> are meant to be resolved relative to the content URI of course however since
> this code calls InstallTrigger from a chrome URL the InstallTrigger code will
> attempt to resolve them relative to that. Additionally we do security checks to
> test whether the content URL that called InstallTrigger is allowed to access
> the URLs it passes for the XPI or icon, again with this change we'll do those
> security checks assuming the chrome url is the caller which will always pass.

Got it. So I think the only way to do this would be to duplicate (or move) the existing code in amInstallTrigger that does this already? Which I guess brings us back to your comment, "If that isn't the case [security issues] then I'm half inclined to say that amInstallTrigger.cpp should be implemented in JS", which brings us back to the security issue.

I talked about the security issue today with mrbkap, and it turns out that on trunk (and electrolysis) it *can* be safe to do this, by using a new JS feature (__exposedProps__), which amounts to adding this to the exposed InstallTrigger object in the patch:

      __exposedProps__: {
        enabled: 'r',
        updateEnabled: 'r',
        install: 'r',
        installChrome: 'r',
        startSoftwareUpdate: 'r',
      },


Basically, with __exposedProps__ it is possible to make an object have only a small set of read-only properties, and nothing else. That way the InstallTrigger object that is exposed to web scripts will not let them modify it (which would in turn have let them run privileged code in it, which would be bad).

> 
> > > >+    var ret = sendSyncMessage("InstallTriggerInstall", { args: args, callbackId: callbackId });
> > > >+    return ret[0]['message'];
> > > 
> > > Where is the 'message' coming from?
> > 
> > That is the form in which the message manager returns sync responses
> > (sendSyncMessage).
> 
> Are there docs on this somewhere? The only ones I have been able to find are
> these: https://wiki.mozilla.org/Content_Process_Event_Handlers#Content_Process

I am afraid those are the best docs I am aware of. This stuff is also changing fairly fast it seems.

Comment 18

[Dave Townsend [:mossop]]

(In reply to comment #17)
> (In reply to comment #16)
> > Calls to InstallTrigger can include relative URLs for the XPI and icon. They
> > are meant to be resolved relative to the content URI of course however since
> > this code calls InstallTrigger from a chrome URL the InstallTrigger code will
> > attempt to resolve them relative to that. Additionally we do security checks to
> > test whether the content URL that called InstallTrigger is allowed to access
> > the URLs it passes for the XPI or icon, again with this change we'll do those
> > security checks assuming the chrome url is the caller which will always pass.
> 
> Got it. So I think the only way to do this would be to duplicate (or move) the
> existing code in amInstallTrigger that does this already? Which I guess brings
> us back to your comment, "If that isn't the case [security issues] then I'm
> half inclined to say that amInstallTrigger.cpp should be implemented in JS",
> which brings us back to the security issue.
> 
> I talked about the security issue today with mrbkap, and it turns out that on
> trunk (and electrolysis) it *can* be safe to do this, by using a new JS feature
> (__exposedProps__), which amounts to adding this to the exposed InstallTrigger
> object in the patch:
> 
>       __exposedProps__: {
>         enabled: 'r',
>         updateEnabled: 'r',
>         install: 'r',
>         installChrome: 'r',
>         startSoftwareUpdate: 'r',
>       },
> 
> 
> Basically, with __exposedProps__ it is possible to make an object have only a
> small set of read-only properties, and nothing else. That way the
> InstallTrigger object that is exposed to web scripts will not let them modify
> it (which would in turn have let them run privileged code in it, which would be
> bad).

So my biggest concern is less about being able to stop content from overwriting properties on the InstallTrigger object and more about being able to inject script that is then called with chrome privileges. Take this example:

InstallTrigger.install({
  foo: {
    get URL() {
      // Attempt to do something nasty
    }
  }
});

This is a perfectly valid call to InstallTrigger that should succeed. The question is whether the URL getter runs with content or chrome privileges.

Comment 19

[Alon Zakai (:azakai)]

(In reply to comment #18)
> 
> So my biggest concern is less about being able to stop content from overwriting
> properties on the InstallTrigger object and more about being able to inject
> script that is then called with chrome privileges. Take this example:
> 
> InstallTrigger.install({
>   foo: {
>     get URL() {
>       // Attempt to do something nasty
>     }
>   }
> });
> 
> This is a perfectly valid call to InstallTrigger that should succeed. The
> question is whether the URL getter runs with content or chrome privileges.

Hmm, I hadn't thought of that, but I believe this is not a problem, because URL() here (and the object it is on) was compiled as part of the normal web page script (so without elevated privileges), and that remains when it is run later on (even if run from chrome scripts). But to make sure, I ran a test of this code, and as expected you get an exception if you do something only chrome can do in this untrusted code. And without anything nasty there, it succeeds properly.

Comment 20

[Dave Townsend [:mossop]]

(In reply to comment #19)
> (In reply to comment #18)
> > 
> > So my biggest concern is less about being able to stop content from overwriting
> > properties on the InstallTrigger object and more about being able to inject
> > script that is then called with chrome privileges. Take this example:
> > 
> > InstallTrigger.install({
> >   foo: {
> >     get URL() {
> >       // Attempt to do something nasty
> >     }
> >   }
> > });
> > 
> > This is a perfectly valid call to InstallTrigger that should succeed. The
> > question is whether the URL getter runs with content or chrome privileges.
> 
> Hmm, I hadn't thought of that, but I believe this is not a problem, because
> URL() here (and the object it is on) was compiled as part of the normal web
> page script (so without elevated privileges), and that remains when it is run
> later on (even if run from chrome scripts). But to make sure, I ran a test of
> this code, and as expected you get an exception if you do something only chrome
> can do in this untrusted code. And without anything nasty there, it succeeds
> properly.

Sounds good, we should make sure we have a test case verifying that. Had I known this was the case I would probably have implemented the current InstallTrigger as a JS object in the first place.

Comment 21

[Alon Zakai (:azakai)]

Attachment: e10s patch v3

WIP (see below for the problem) new version of e10s patch (will attach mobile-browser patch next).

Changes:

1. Fixes to security issues, with __exposedProps__
2. Rewritten as two jsm's in toolkit/mozapps/extensions
3. Handles url resolving and security checks. This duplicates some code from amInstallTrigger, which I see no way around. But perhaps when e10s lands on m-c, this can be done without redundancy.

Regarding the security checks, I am having a problem with the security manager. After wasting some time on it I can't figure it out, but perhaps it's something obvious. The code is fairly simple, and I believe I am passing the right parameters, but the security manager returns false (even though it should return true).

Comment 22

[Alon Zakai (:azakai)]

Attachment: mobile-browser patch v3

mobile-browser part of the last patch. Note that it is now very small as the code has been moved to jsm's in the main code.

Comment 23

[Josh Matthews [:jdm]]

Have you tried tracing nsSecurityScriptManager::CheckLoadURIWithPrincipal to see where it's failing?

Comment 24

[Alon Zakai (:azakai)]

Attachment: e10s patch v3.1

Yep, tracing revealed that it was something small - I checked the return value of the script security manager in JavaScript instead of catching an exception - my brain must have still been in C++ mode from reading the security checking code in amInstallTrigger.cpp ;)

With the attached patch things work ok.

Comment 25

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

Comment on attachment 451320 [details] [diff] [review]
e10s patch v3

Just a drive by review

>+InstallTriggerChild.prototype = {
>+  observe: function UTM_observe(subject, topic, data) {

>+    subject.wrappedJSObject.InstallTrigger = {
>+      __exposedProps__: {
>+        enabled: 'r',
>+        updateEnabled: 'r',
>+        install: 'r',
>+        installChrome: 'r',
>+        startSoftwareUpdate: 'r',

Use " instead of ' for strings

>+      },
>+
>+      enabled: function() {
>+        return messageManager.sendSyncMessage("InstallTriggerEnabled")[0]['message'];

You need to be careful with sending sync messages. If this call doesn't display a UI in the chrome then sync is probably OK.

I assume it will be used like:

if (InstallTrigger.enabled()) { /* do something */ }

Then sync is appropriate.

>+      install: function(args, callback) {

>+        return messageManager.sendSyncMessage("InstallTriggerInstall", { args: args, callbackId: callbackId })[0]['message'];

This call is likely to cause a UI to be displayed and should not be sync as the content won't repaint while the chrome UI is displayed. Also, isn't an async message used to send stuff back? "InstallTriggerCallback" or is the return value really needed?

>+      installChrome: function(type, url, skin) {
>+        return this.install({ url : { 'URL': url } });
>+      },
>+
>+      startSoftwareUpdate: function(url, flags) {
>+        return this.install({ url : { 'URL': url } });
>+      },

Use " instead of '

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

>+      case 'InstallTriggerEnabled':

>+      case 'InstallTriggerInstall':

>+            message.target.messageManager.sendAsyncMessage('InstallTriggerCallback', {

Use " instead of '

Comment 26

[Dave Townsend [:mossop]]

(In reply to comment #25)
> (From update of attachment 451320 [details] [diff] [review])
> >+      install: function(args, callback) {
> 
> >+        return messageManager.sendSyncMessage("InstallTriggerInstall", { args: args, callbackId: callbackId })[0]['message'];
> 
> This call is likely to cause a UI to be displayed and should not be sync as the
> content won't repaint while the chrome UI is displayed. Also, isn't an async
> message used to send stuff back? "InstallTriggerCallback" or is the return
> value really needed?

This call will not cause UI to be displayed synchronously.
The return value is a part of the InstallTrigger API that websites use, I'd rather not break that.

Comment 27

[Alon Zakai (:azakai)]

Attachment: e10s patch v3.2

Ok, fixed the 's to "s.

Comment 28

[Dave Townsend [:mossop]]

Comment on attachment 451352 [details] [diff] [review]
e10s patch v3.2

Only really skimmed this for now because there are important changes that I think need to be made before I can review it in more depth. Also I would love it if we could just do this on regular trunk too so we could at least see some test results from it.

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -1918,12 +1918,13 @@
>                                                 nsnull,
>                                                 SendAsyncMessageToChild,
>                                                 LoadScript,
>-                                                this,
>+                                                nsnull,
>                                                 static_cast<nsFrameMessageManager*>(parentManager.get()),
>                                                 cx);
>     NS_ENSURE_TRUE(mMessageManager, NS_ERROR_OUT_OF_MEMORY);
>     mChildMessageManager =
>       new nsInProcessTabChildGlobal(mDocShell, mOwnerContent, mMessageManager);
>+    mMessageManager->SetCallbackData(this);

I don't know what this is so I cannot review it.

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

>+var EXPORTED_SYMBOLS = [ "InstallTriggerChild" ];
>+
>+let Cc = Components.classes;
>+let Ci = Components.interfaces;
>+
>+let gIOService = Cc["@mozilla.org/network/io-service;1"]
>+  .getService(Ci.nsIIOService);
>+let gObserverService = Cc["@mozilla.org/observer-service;1"]
>+  .getService(Ci.nsIObserverService);

Can we use Services.jsm in the content process? If so drop these lines and use Services.io and Services.obs throughout.

>+/**
>+ * Child part of InstallTrigger e10s handling.
>+ *
>+ * Sets up InstallTriggers that will relay messages properly.
>+ */
>+function InstallTriggerChild(messageManager) {
>+  this.messageManager = messageManager;
>+  gObserverService.addObserver(this, "content-document-global-created", false);
>+}
>+
>+InstallTriggerChild.prototype = {
>+  observe: function UTM_observe(subject, topic, data) {
>+    var messageManager = this.messageManager;
>+    var hidden = new InstallTriggerHidden(subject, messageManager);
>+
>+    // The public interface, which normal scripts can access. Accesses
>+    // the hidden elements through closures here.
>+    subject.wrappedJSObject.InstallTrigger = {
>+      __exposedProps__: {
>+        enabled: "r",
>+        updateEnabled: "r",
>+        install: "r",
>+        installChrome: "r",
>+        startSoftwareUpdate: "r",
>+      },
>+
>+      enabled: function() {
>+        return messageManager.sendSyncMessage("InstallTriggerEnabled")[0]["message"];

Please just call through to amIWebInstaller.isInstallEnabled rather than using InstallTrigger in the parent process.

>+      },
>+
>+      updateEnabled: function() {
>+        return this.enabled();
>+      }    ,

Nit: bad spaces.

>+
>+      install: function(args, callback) {
>+        // Resolve, validate and collect urls
>+        urls = [];
>+        for (var itemId in args) {
>+          var item = args[itemId];
>+          item.URL = hidden.resolveURL(item.URL);

You must support item being a plain string instead of an object.

>+          if (!hidden.checkLoadURIFromScript(item.URL))
>+            return false;
>+          item.icon = hidden.resolveURL(item.icon);

The property is actually IconURL and you need to support it not existing.

>+          if (!hidden.checkLoadURIFromScript(item.icon))
>+            item.icon = null; // If page can't load the icon, just ignore it
>+          urls.push(item.URL);
>+        }
>+        var callbackId = hidden.addCallback(callback, urls);
>+        return messageManager.sendSyncMessage("InstallTriggerInstall", { args: args, callbackId: callbackId })[0]["message"];
>+      },

Please just call through to amIWebInstaller.installAddonsFromWebpage rather than using InstallTrigger in the parent process.

>+      installChrome: function(type, url, skin) {
>+        return this.install({ url : { "URL": url } });
>+      },
>+
>+      startSoftwareUpdate: function(url, flags) {
>+        return this.install({ url : { "URL": url } });
>+      },

Please name these appropriately as I asked in the original review.

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

>+function InstallTriggerParent(window, messageManager) {
>+  this.window = window;
>+  messageManager.addMessageListener("InstallTriggerInstall", this);
>+  messageManager.addMessageListener("InstallTriggerEnabled", this);
>+}
>+
>+InstallTriggerParent.prototype = {
>+  receiveMessage: function(message) {
>+    var payload = message.json;
>+    switch (message.name) {
>+      // .enabled()
>+      case "InstallTriggerEnabled":
>+        return this.window.InstallTrigger.enabled();
>+      // .install()
>+      case "InstallTriggerInstall":
>+        var args = payload.args;
>+        var callbackId = payload.callbackId;
>+        return this.window.InstallTrigger.install(args, {
>+          onInstallEnded: function ITI_callback(url, status) {
>+            message.target.messageManager.sendAsyncMessage("InstallTriggerCallback", {
>+              callbackId: callbackId,
>+              url: url,
>+              status: status
>+            });
>+          }
>+        });
>+    }
>+  },
>+};

I think routing messages from the child process through to InstallTrigger in the parent is a waste of time. We might as well bypass it entirely and talk directly to the add-ons manager

Comment 29

[Alon Zakai (:azakai)]

I'm working on the other comments, but I have a question about these:

> 
> Please just call through to amIWebInstaller.isInstallEnabled rather than using
> InstallTrigger in the parent process.
> 
> Please just call through to amIWebInstaller.installAddonsFromWebpage rather
> than using InstallTrigger in the parent process.
> 
> I think routing messages from the child process through to InstallTrigger in
> the parent is a waste of time. We might as well bypass it entirely and talk
> directly to the add-ons manager

I am not sure I understand what you mean by these. I think you are saying that we should do as follows:

1. The receives a call to (for example) install()
2. It should forward that to the parent
3. The parent should convert the parameters to install() to parameters to installAddonsFromWebpage() (or the child could do the conversion)
4. The parent should call amIWebInstaller.installAddonsFromWebpage()
5. If necessary, the output is converted to the proper output for install() (or the child could do it)
6. The output is sent back to the child.

If so, then compared to the patch, the patch saves us needing to do step 3 (and maybe 5, if necessary), since in the patch we receive data for install() and relay it as blindly as possible, with no conversion. The patch only depends on 1 interface (the public InstallTrigger) and not 2 (InstallTrigger and amIWebInstaller). So I would tend to prefer that approach.

Or have I misunderstood something here?

Comment 30

[Dave Townsend [:mossop]]

(In reply to comment #29)
> I'm working on the other comments, but I have a question about these:
> 
> > 
> > Please just call through to amIWebInstaller.isInstallEnabled rather than using
> > InstallTrigger in the parent process.
> > 
> > Please just call through to amIWebInstaller.installAddonsFromWebpage rather
> > than using InstallTrigger in the parent process.
> > 
> > I think routing messages from the child process through to InstallTrigger in
> > the parent is a waste of time. We might as well bypass it entirely and talk
> > directly to the add-ons manager
> 
> I am not sure I understand what you mean by these. I think you are saying that
> we should do as follows:
> 
> 1. The receives a call to (for example) install()
> 2. It should forward that to the parent
> 3. The parent should convert the parameters to install() to parameters to
> installAddonsFromWebpage() (or the child could do the conversion)
> 4. The parent should call amIWebInstaller.installAddonsFromWebpage()
> 5. If necessary, the output is converted to the proper output for install() (or
> the child could do it)
> 6. The output is sent back to the child.

No, the child has to do the conversion of the parameters (as your patch does currently) and then relay them to the parent which then calls installAddonsFromWebpage directly rather than going through InstallTrigger. The idea would be that we can use this approach for e10s and non-e10s alike and I can remove the InstallTrigger C++ component.

Comment 31

[Alon Zakai (:azakai)]

(In reply to comment #30)
> No, the child has to do the conversion of the parameters (as your patch does
> currently) and then relay them to the parent which then calls
> installAddonsFromWebpage directly rather than going through InstallTrigger. The
> idea would be that we can use this approach for e10s and non-e10s alike and I
> can remove the InstallTrigger C++ component.

Oh, ok, now I understand. Makes perfect sense, I'll do it that way.

Comment 32

[Alon Zakai (:azakai)]

Attachment: e10s patch v4

Version 4 of patch, should address all the comments thus far.

Note: Patch includes a tiny fix to toolkit/mozapps/extensions/XPIProvider.jsm - a local variable was missing 'let'(/var). I happened to notice this while debugging for this patch. Not sure what's usually done for such small stuff, should I open a new bug instead?

Comment 33

[Dave Townsend [:mossop]]

Comment on attachment 454165 [details] [diff] [review]
e10s patch v4

So there is still some work to do here. I've suggested some restructuring of the child jsm and there are a bunch of comments that are mostly style related, very few actual code issues. Once those are taken care of I'd expect the next pass to be an r+ and it would be able to land.

That said, now I've understood more about how the message manager works there are some changes to the parent side that we should do. I'd take them in a follow-up bug but it may be as easy to do them now, I leave it up to you.

What I think we will want to do is ditch the parent jsm entirely. Instead make amManager in addonManager.js register with the global message manager to listen for the messages from the child process. It removes another unnecessary layer between the child and add-ons manager and means applications don't have to include the parent stuff manually.

You could also make a chrome script to do the child installing and load it with loadFrameScript from amManager so that applications don't have to worry about doing that either.

On with the main review:

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

The objects you are using here are overly confusing. I think you need to merge InstallTriggerHidden with the InstallTrigger you set on the content window. The __exposedProps__ will make sure that the bits you don't want content to know about remain hidden right?

So I would have:

function InstallTrigger(aWindow, aMessageManager) {
  // initialisation
}

InstallTrigger.prototype = {
  __exposedProps__: {
    // ...
  }

  // InstallTrigger stuff
  // InstallTriggerHidden stuff
}

And I think you then just want a very simple object observing "content-document-global-created" that creates a new InstallTrigger for each window.

>+let Cc = Components.classes;
>+let Ci = Components.interfaces;
>+let Cu = Components.utils;

Use consts for these please.

>+/**
>+ * The hidden part of an InstallTrigger object, which does the
>+ * necessary message-passing stuff. The properties and functions
>+ * of this hidden object are not viewable by normal scripts.
>+ * The public InstallTrigger object can access them through a
>+ * closure, for the things we do need, see below.
>+ */

The rest of the extensions code uses javadoc style comments to describe the parameters and returns types of all main functions and functions within prototypes.

>+InstallTriggerHidden = function(window, messageManager) {

Use "a" prefix on arguments.

>+  this.window = window;
>+  this.callbacks = {};
>+  messageManager.addMessageListener("WebInstallerInstallCallback", this);
>+}
>+
>+InstallTriggerHidden.prototype = {
>+  addCallback: function(callback, urls) {
>+    if (!callback) return -1;

Put the return on the next line.

>+    var callbackId = 0;
>+    while (true) {
>+      if (!this.callbacks[callbackId]) {
>+        break;
>+      }
>+      callbackId += 1;
>+    }

while (callbackId in this.callbacks)
  callbackId++;

>+    this.callbacks[callbackId] = {
>+      callback: callback,
>+      urls: urls.slice(0), // Clone the urls for our own use (it lets
>+                           // us know when no further callbacks will
>+                           // occur)
>+    };
>+    return callbackId;
>+  },
>+
>+  // receiveMessage
>+  // Also functions as removeCallback to parallel addCallback. When
>+  // all URLs are exhausted, free the callbackId and linked stuff
>+  receiveMessage: function(message) {
>+    var payload = message.json;
>+    var callbackId = payload.callbackId;
>+    var url = payload.url;
>+    var status = payload.status;
>+    this.callbacks[callbackId].callback(url, status);
>+    delete this.callbacks[callbackId].urls[url];
>+    if (JSON.stringify(this.callbacks[callbackId].urls[url]) === JSON.stringify({})) {
>+      this.callbacks[callbackId] = null;
>+    }

This is ugly and also doesn't work. .urls is an array of strings yet you are using it as a dictionary here. Use indexOf to find the url to remove and splice to remove it. Then just check the array length to see whether you can kill the callback.

>+  },
>+
>+  // Utilities
>+
>+  resolveURL: function(url) {
>+    return Services.io.newURI(this.window.location.href, this.window.document.characterSet, null).resolve(url);

This line is too long. Is there really no way to get the nsIURI direct from the window, I thought there was.
I think this method should return an nsIURI to save you having to re-parse the result of it in checkLoadURIFromScript.

>+  },
>+
>+  // Parallels amInstallTrigger.cpp
>+  // TODO: When possible (e10s lands on m-c?) merge that into here?

Add a bug reference.

>+  checkLoadURIFromScript: function(url) {
>+    let secman = Cc["@mozilla.org/scriptsecuritymanager;1"]
>+                   .getService(Ci.nsIScriptSecurityManager);

The general rule here is to put the "." at the end of the first line and line the get up with Cc.

>+    // get the script principal
>+    var principal = this.window.content.document.nodePrincipal;

You chop and change between using let and var in the same scope here, pick one and stick with it throughout the file please.

>+    // convert the requested URL string to a URI
>+    // Note that we use a null base URI here, since that's what we use when we
>+    // actually convert the string into a URI to load.
>+    var uri = Services.io.newURI(url, null, null);
>+    try {
>+      secman.checkLoadURIWithPrincipal(
>+        principal,
>+        uri,
>+        Ci.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL
>+      );

Please put arguments in as few lines as possible.

>+      return true;
>+    } catch(e) {

catch on a new line after the closing brace.

>+      install: function(args, callback) {
>+        // Convert data to amIWebInstaller format
>+        // TODO: Remove amInstallTrigger.cpp when possible, so this is the only
>+        //       place where we do this

You refer to this in multiple places, I'm not sure it really needs to be in the file at all tbh but if it does just do it at the top and reference a bug to do it.

>+        var params = {
>+          mimetype: "application/x-xpinstall",
>+          referer: subject.location.href,
>+          uris: [],
>+          hashes: [],
>+          names: [],
>+          icons: [],
>+        };
>+
>+        for (var name_ in args) {

Drop the underscore please.

>+          var item = args[name_];
>+          if (typeof item === 'string') {
>+            item = { URL: item };
>+          }

Don't use braces around single line blocks.

>+          // Resolve and validate urls
>+          item.URL = hidden.resolveURL(item.URL);

You need to handle the case where URL is not a property of item.

>+          if (!hidden.checkLoadURIFromScript(item.URL))
>+            return false;

You should throw an exception in the event of this failing I believe.

>+          if (item.IconURL) {

Use |if ("IconURL" in item)|

>+            item.IconURL = hidden.resolveURL(item.IconURL);
>+            if (!hidden.checkLoadURIFromScript(item.IconURL)) {
>+              item.IconURL = null; // If page can't load the icon, just ignore it
>+            }
>+          }
>+          params.uris.push(item.URL);
>+          params.hashes.push(item.Hash ? item.Hash : null);

Use |"Hash" in item|.

>+          params.names.push(name_);
>+          params.icons.push(item.IconURL);

Need some kind of test to avoid accessing a property that may not exist.

>+      startSoftwareUpdate: function(url, flags) {
>+        var object = {};
>+        var uri = Services.io.newURI(url, null, null);
>+        var path = uri.path;
>+        // Use filename part of path for the name
>+        object[path.length > 0 ? (
>+            path[path.length-1] != '/' ? path.substr(path.lastIndexOf('/')+1) : path
>+          ) : ( // else, path.length == 0
>+            uri.spec
>+        )] = { "URL": url };

Ouch. Please just QI uri to nsIURL and use its filename property.

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

>+var EXPORTED_SYMBOLS = [ "InstallTriggerParent" ];

I think this would be better named as InstallTriggerListener.

>+Components.utils.import("resource://gre/modules/Services.jsm");
>+
>+let installer = Components.classes["@mozilla.org/addons/integration;1"].
>+                getService(Components.interfaces.amIWebInstaller);

For global variables like this use a "g" prefix, like gInstaller. Also if you're using the long forms put the "." on the second line lined up with the . in Components.classes.

>+/**
>+ * Parent part of InstallTrigger e10s handling.
>+ *
>+ * Listens to requests from child processes for InstallTrigger
>+ * activity, and sends back callbacks.
>+ * TODO: Check if this works in an iframe

Please put a bug reference here

>+ */
>+function InstallTriggerParent(window, messageManager) {
>+  this.window = window;

The window property seems to be unused. I don't see why you can't remove it.

>+  messageManager.addMessageListener("WebInstallerIsInstallEnabled", this);
>+  messageManager.addMessageListener("WebInstallerInstallAddonsFromWebpage", this);
>+}
>+
>+InstallTriggerParent.prototype = {
>+  receiveMessage: function(message) {
>+    var payload = message.json;
>+    switch (message.name) {
>+
>+      case "WebInstallerIsInstallEnabled":
>+        return installer.isInstallEnabled(
>+          payload.mimetype,
>+          Services.io.newURI(payload.referer, null, null)
>+        );

Regardless of the message you have to convert the referer to an nsIURI so do it outside the switch statement.
As a rule, only wrap when an argument will take you past 80 characters. Put the closing bracket on the previous line. 

>+      case "WebInstallerInstallAddonsFromWebpage":
>+        var ret = installer.installAddonsFromWebpage(

Just do return installer.installAddonsFromWebpage.

>+          payload.mimetype,
>+          null, // XXX TODO: use message.target, a XULElement, when the interface changes to allow that instead of the window it wants now

Please put a bug reference here.

>+          Services.io.newURI(payload.referer, null, null),
>+          payload.uris,
>+          payload.hashes,
>+          payload.names,
>+          payload.icons,
>+          function ITP_callback(url, status) {
>+            message.target.messageManager.sendAsyncMessage("WebInstallerInstallCallback", {
>+              callbackId: payload.callbackId,
>+              url: url,
>+              status: status
>+            });
>+          },

We save a bunch of work by not passing the callback function here if the callbackId is -1.

>+          payload.uris.length
>+        );

The normal style is to put arguments on the same line as much as possible, only wrapping at 80 characters.

Comment 34

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

(In reply to comment #33)

Drive by...

> What I think we will want to do is ditch the parent jsm entirely. Instead make
> amManager in addonManager.js register with the global message manager to listen
> for the messages from the child process. It removes another unnecessary layer
> between the child and add-ons manager and means applications don't have to
> include the parent stuff manually.
> 
> You could also make a chrome script to do the child installing and load it with
> loadFrameScript from amManager so that applications don't have to worry about
> doing that either.

Yes. This is very similar to how components like login manager and form history are being refactored for e10s.


> And I think you then just want a very simple object observing
> "content-document-global-created" that creates a new InstallTrigger for each
> window.

I don't think the observer notifications make it inot the content process, but bsmedberg created a DOMWindowCreated event which can be used the same way:

addEventListener("DOMWindowCreated", handler, false);

in a content script is all you should need.

> >+  resolveURL: function(url) {
> >+    return Services.io.newURI(this.window.location.href, this.window.document.characterSet, null).resolve(url);
> 
> This line is too long. Is there really no way to get the nsIURI direct from the
> window, I thought there was.

window.document.documentURIObject ?

Comment 35

[Dave Townsend [:mossop]]

(In reply to comment #34)
> > And I think you then just want a very simple object observing
> > "content-document-global-created" that creates a new InstallTrigger for each
> > window.
> 
> I don't think the observer notifications make it inot the content process, but
> bsmedberg created a DOMWindowCreated event which can be used the same way:
> 
> addEventListener("DOMWindowCreated", handler, false);
> 
> in a content script is all you should need.

I'm not sure how this patch worked then since it seems to use the observer service notification.

Comment 36

[Alon Zakai (:azakai)]

Attachment: e10s patch v5

Updated patch, following the latest batch of comments. Responses to some of them are below (ones not responded to have been fixed).

(This is a patch to e10s; no patch to mobile-browser is needed anymore, so obsoleted that.)

> 
> That said, now I've understood more about how the message manager works there
> are some changes to the parent side that we should do. I'd take them in a
> follow-up bug but it may be as easy to do them now, I leave it up to you.

Might as well do it in this bug.

> 
> What I think we will want to do is ditch the parent jsm entirely. Instead make
> amManager in addonManager.js register with the global message manager to listen
> for the messages from the child process. It removes another unnecessary layer
> between the child and add-ons manager and means applications don't have to
> include the parent stuff manually.

Yep, makes a lot of sense. Done.

> 
> You could also make a chrome script to do the child installing and load it with
> loadFrameScript from amManager so that applications don't have to worry about
> doing that either.
> 

Ditto, done.

> On with the main review:
> 
> >diff --git a/toolkit/mozapps/extensions/InstallTriggerChild.jsm b/toolkit/mozapps/extensions/InstallTriggerChild.jsm
> 
> The objects you are using here are overly confusing. I think you need to merge
> InstallTriggerHidden with the InstallTrigger you set on the content window. The
> __exposedProps__ will make sure that the bits you don't want content to know
> about remain hidden right?

Yes, that would be fine with __exposedProps__. I sort of liked the separation of 2 classes actually, but things always look different when you are the person writing it ;) So, changed to how you suggested.

> 
> >+  resolveURL: function(url) {
> >+    return Services.io.newURI(this.window.location.href, this.window.document.characterSet, null).resolve(url);
> 
> [..]
> I think this method should return an nsIURI to save you having to re-parse the
> result of it in checkLoadURIFromScript.
> 

Not sure I follow. We anyhow get the string version from resolve(), and we need that string version (we send that over IPC). If we converted to a URI here, we would have an additional conversion overall.

> >+          if (typeof item === 'string') {
> >+            item = { URL: item };
> >+          }
> [...]
> >+          // Resolve and validate urls
> >+          item.URL = hidden.resolveURL(item.URL);
> 
> You need to handle the case where URL is not a property of item.

I thought there were two cases:

1. item is a string
2. item is an object of form { URL: ... , ... }

The current code handles these two, I believe, as quoted here. Is there a problem with one of those, or is there a third case?

[mfinkle]
> I don't think the observer notifications make it inot the content process, but
bsmedberg created a DOMWindowCreated event which can be used the same way:

Hmm, the patch works as is, with the observer service. It also works in an iframe, which I was told might require the DOMWindowCreated event. So, DOMWindowCreated does not seem to be needed in this case, unless I am missing something.

Comment 37

[Dave Townsend [:mossop]]

Some answers to your questions below. I would actually prefer to keep the InstallTrigger object in a jsm and have the child script import it and just construct new InstallTrigger objects as necessary. This will make it easy for me to wire it up for webpages in the non-e10s case as well. Call the module InstallTrigger.jsm.

(In reply to comment #36)> > >+  resolveURL: function(url) {
> > >+    return Services.io.newURI(this.window.location.href, this.window.document.characterSet, null).resolve(url);
> > 
> > [..]
> > I think this method should return an nsIURI to save you having to re-parse the
> > result of it in checkLoadURIFromScript.
> > 
> 
> Not sure I follow. We anyhow get the string version from resolve(), and we need
> that string version (we send that over IPC). If we converted to a URI here, we
> would have an additional conversion overall.

You have to convert them to nsIURIs anyway in checkLoadURIFromScript, I think it would be better to just return an nsIURI from resolveURL (using Services.io.newURI to do the resolve and parse would be the cheapest way probably). That way we can be more confident that we don't accidentally add more string -> URI conversions in the future.

> > >+          if (typeof item === 'string') {
> > >+            item = { URL: item };
> > >+          }
> > [...]
> > >+          // Resolve and validate urls
> > >+          item.URL = hidden.resolveURL(item.URL);
> > 
> > You need to handle the case where URL is not a property of item.
> 
> I thought there were two cases:
> 
> 1. item is a string
> 2. item is an object of form { URL: ... , ... }
> 
> The current code handles these two, I believe, as quoted here. Is there a
> problem with one of those, or is there a third case?

Well there are the two correct cases you listed, then there are a load of cases where the website is badly written and gives us bad data and we have to handle it and throw exceptions or behave accordingly. It looks like the current code will throw an NS_ERROR_FAILURE in the event of missing URL, but it'd be better to explicitly check for it to make sure that that doesn't change.

Comment 38

[Dave Townsend [:mossop]]

Comment on attachment 454730 [details] [diff] [review]
e10s patch v5

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -3853,7 +3853,7 @@
>       bis.init(zis, 4096);
> 
>       try {
>-        uri = buildJarURI(this.file, FILE_INSTALL_MANIFEST);
>+        let uri = buildJarURI(this.file, FILE_INSTALL_MANIFEST);

This is actually being taken care of in another bug where this is getting moved around so I wouldn't worry about it.

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

>+  /**
>+   * Parent part of InstallTrigger e10s handling.
>+   *
>+   * Listens to requests from child processes for InstallTrigger
>+   * activity, and sends back callbacks.
>+   */
>+  function InstallTriggerParent(aMessageManager) {
>+    aMessageManager.addMessageListener("WebInstallerIsInstallEnabled", this);
>+    aMessageManager.addMessageListener("WebInstallerInstallAddonsFromWebpage", this);

Please define these message names as constants at the top of the file, MSG_INSTALL_ENABLED, MSG_INSTALL_ADDONS and MSG_INSTALL_CALLBACK I think. Have the same defines in the InstallTrigger.jsm

>+    aMessageManager.loadFrameScript("chrome://mozapps/content/extensions/child-content.js", true);
>+  }
>+
>+  InstallTriggerParent.prototype = {
>+    receiveMessage: function(aMessage) {
>+      var payload = aMessage.json;
>+      var referer = Services.io.newURI(payload.referer, null, null);
>+      switch (aMessage.name) {
>+

Drop this newline.

>+        case "WebInstallerIsInstallEnabled":
>+          return installer.isInstallEnabled(payload.mimetype, referer);
>+
>+        case "WebInstallerInstallAddonsFromWebpage":
>+          var callback = null;
>+          if (payload.callbackId != -1) {
>+            callback = {
>+              onInstallEnded: function ITP_callback(url, status) {
>+                aMessage.target.messageManager.sendAsyncMessage(
>+                  "WebInstallerInstallCallback",
>+                  { callbackId: payload.callbackId, url: url, status: status }
>+                );
>+              },
>+            };
>+          }
>+          return installer.installAddonsFromWebpage(
>+            payload.mimetype,
>+            null, // XXX TODO: see bug 565389 - send a window or element here
>+            referer, payload.uris, payload.hashes, payload.names,
>+            payload.icons, callback, payload.uris.length);
>+      }
>+    },
>+  };
>+
>+  new InstallTriggerParent(
>+    Components.classes["@mozilla.org/globalmessagemanager;1"].
>+    getService(Components.interfaces.nsIChromeFrameMessageManager));

There is really no reason for this to be a new object like this. Just add the receiveMessage method to amManager.prototype.

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

>+      /**
>+       * @see amIInstallTriggerInstaller.idl
>+       */
>+      startSoftwareUpdate: function(aUrl, aFlags) {
>+        var url = Services.io.newURI(aUrl, null, null).QueryInterface(Ci.nsIURL).filename;

Please wrap lines that are longer than 80 characters.

>+      receiveMessage: function(aMessage) {
>+        var payload = aMessage.json;
>+        var callbackId = payload.callbackId;
>+        var url = payload.url;
>+        var status = payload.status;
>+        var details = this.callbacks[callbackId];

How about callbackObj rather than details?

>+        details.callback(url, status);

Wrap that in a try...catch block to protect you from the web content.

>+      checkLoadURIFromScript: function(aUrl) {
>+        var secman = Cc["@mozilla.org/scriptsecuritymanager;1"].
>+                     getService(Ci.nsIScriptSecurityManager);
>+
>+        // get the script principal
>+        var principal = window.content.document.nodePrincipal;
>+
>+        // convert the requested URL string to a URI
>+        // Note that we use a null base URI here, since that's what we use when we
>+        // actually convert the string into a URI to load.
>+        var uri = Services.io.newURI(aUrl, null, null);
>+        try {
>+          secman.checkLoadURIWithPrincipal(
>+            principal, uri,
>+            Ci.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);

Please put arguments on the first line when possible. Same goes throughout.

Comment 39

[Alon Zakai (:azakai)]

Attachment: e10s patch v6

Fixed all the stuff mentioned, but for the following two issues:

(In reply to comment #37)
> Some answers to your questions below. I would actually prefer to keep the
> InstallTrigger object in a jsm and have the child script import it and just
> construct new InstallTrigger objects as necessary. This will make it easy for
> me to wire it up for webpages in the non-e10s case as well. Call the module
> InstallTrigger.jsm.

Actually the code will work as-is in non-e10s builds as well - even non-e10s builds have messageManager code (it just doesn't do IPC stuff). So no need to do anything more to get this to work in the non-e10s case, and for that reason I don't see a need for a separate jsm (but I don't feel very strongly either way).

(In reply to comment #38)
> 
> >+        case "WebInstallerIsInstallEnabled":
> >+          return installer.isInstallEnabled(payload.mimetype, referer);
> >+
> >+        case "WebInstallerInstallAddonsFromWebpage":
> >+          var callback = null;
> >+          if (payload.callbackId != -1) {
> >+            callback = {
> >+              onInstallEnded: function ITP_callback(url, status) {
> >+                aMessage.target.messageManager.sendAsyncMessage(
> >+                  "WebInstallerInstallCallback",
> >+                  { callbackId: payload.callbackId, url: url, status: status }
> >+                );
> >+              },
> >+            };
> >+          }
> >+          return installer.installAddonsFromWebpage(
> >+            payload.mimetype,
> >+            null, // XXX TODO: see bug 565389 - send a window or element here
> >+            referer, payload.uris, payload.hashes, payload.names,
> >+            payload.icons, callback, payload.uris.length);
> >+      }
> >+    },
> >+  };
> >+
> >+  new InstallTriggerParent(
> >+    Components.classes["@mozilla.org/globalmessagemanager;1"].
> >+    getService(Components.interfaces.nsIChromeFrameMessageManager));
> 
> There is really no reason for this to be a new object like this. Just add the
> receiveMessage method to amManager.prototype.

Well, there is no technical reason, but I think it is cleaner this way, with all the messaging stuff concentrated in this object. It's sort of like a plugin for the amManager, instead of having a single big 'flat' object with lots of different methods, and it is a clear parallel for the InstallTriggerChild object. But, as in the previous issue, I don't feel too strongly about it, let me know what you want me to do.

Comment 40

[Dave Townsend [:mossop]]

(In reply to comment #39)
> Created an attachment (id=454914) [details]
> e10s patch v6
> 
> Fixed all the stuff mentioned, but for the following two issues:
> 
> (In reply to comment #37)
> > Some answers to your questions below. I would actually prefer to keep the
> > InstallTrigger object in a jsm and have the child script import it and just
> > construct new InstallTrigger objects as necessary. This will make it easy for
> > me to wire it up for webpages in the non-e10s case as well. Call the module
> > InstallTrigger.jsm.
> 
> Actually the code will work as-is in non-e10s builds as well - even non-e10s
> builds have messageManager code (it just doesn't do IPC stuff). So no need to
> do anything more to get this to work in the non-e10s case, and for that reason
> I don't see a need for a separate jsm (but I don't feel very strongly either
> way).

If that is the case then that is great. Presumably you'll need to disable building amInstallTrigger.cpp to avoid conflicts but after that I could apply this patch Firefox and see how the browser chrome tests fare?

> (In reply to comment #38)
> > 
> > >+        case "WebInstallerIsInstallEnabled":
> > >+          return installer.isInstallEnabled(payload.mimetype, referer);
> > >+
> > >+        case "WebInstallerInstallAddonsFromWebpage":
> > >+          var callback = null;
> > >+          if (payload.callbackId != -1) {
> > >+            callback = {
> > >+              onInstallEnded: function ITP_callback(url, status) {
> > >+                aMessage.target.messageManager.sendAsyncMessage(
> > >+                  "WebInstallerInstallCallback",
> > >+                  { callbackId: payload.callbackId, url: url, status: status }
> > >+                );
> > >+              },
> > >+            };
> > >+          }
> > >+          return installer.installAddonsFromWebpage(
> > >+            payload.mimetype,
> > >+            null, // XXX TODO: see bug 565389 - send a window or element here
> > >+            referer, payload.uris, payload.hashes, payload.names,
> > >+            payload.icons, callback, payload.uris.length);
> > >+      }
> > >+    },
> > >+  };
> > >+
> > >+  new InstallTriggerParent(
> > >+    Components.classes["@mozilla.org/globalmessagemanager;1"].
> > >+    getService(Components.interfaces.nsIChromeFrameMessageManager));
> > 
> > There is really no reason for this to be a new object like this. Just add the
> > receiveMessage method to amManager.prototype.
> 
> Well, there is no technical reason, but I think it is cleaner this way, with
> all the messaging stuff concentrated in this object. It's sort of like a plugin
> for the amManager, instead of having a single big 'flat' object with lots of
> different methods, and it is a clear parallel for the InstallTriggerChild
> object. But, as in the previous issue, I don't feel too strongly about it, let
> me know what you want me to do.

I disagree. It can be just as clear that this is a separate part by using appropriate comments in the amManager prototype and it saves us some additional work (the object instantiation) in the constructor which is in the startup path.

Comment 41

[Alon Zakai (:azakai)]

Attachment: e10s patch v7

(In reply to comment #40)
> (In reply to comment #39)
> > Created an attachment (id=454914) [details] [details]
> > e10s patch v6
> > 
> > Fixed all the stuff mentioned, but for the following two issues:
> > 
> > (In reply to comment #37)
> > > Some answers to your questions below. I would actually prefer to keep the
> > > InstallTrigger object in a jsm and have the child script import it and just
> > > construct new InstallTrigger objects as necessary. This will make it easy for
> > > me to wire it up for webpages in the non-e10s case as well. Call the module
> > > InstallTrigger.jsm.
> > 
> > Actually the code will work as-is in non-e10s builds as well - even non-e10s
> > builds have messageManager code (it just doesn't do IPC stuff). So no need to
> > do anything more to get this to work in the non-e10s case, and for that reason
> > I don't see a need for a separate jsm (but I don't feel very strongly either
> > way).
> 
> If that is the case then that is great. Presumably you'll need to disable
> building amInstallTrigger.cpp to avoid conflicts but after that I could apply
> this patch Firefox and see how the browser chrome tests fare?

I actually think it would work (clumsily) with amInstallTrigger.cpp - it would override it at runtime. But, not sure, and another question is whether such testing needs to wait for e10s to merge into mozilla-central.

> 
> > (In reply to comment #38)
> > > 
> > > >+        case "WebInstallerIsInstallEnabled":
> > > >+          return installer.isInstallEnabled(payload.mimetype, referer);
> > > >+
> > > >+        case "WebInstallerInstallAddonsFromWebpage":
> > > >+          var callback = null;
> > > >+          if (payload.callbackId != -1) {
> > > >+            callback = {
> > > >+              onInstallEnded: function ITP_callback(url, status) {
> > > >+                aMessage.target.messageManager.sendAsyncMessage(
> > > >+                  "WebInstallerInstallCallback",
> > > >+                  { callbackId: payload.callbackId, url: url, status: status }
> > > >+                );
> > > >+              },
> > > >+            };
> > > >+          }
> > > >+          return installer.installAddonsFromWebpage(
> > > >+            payload.mimetype,
> > > >+            null, // XXX TODO: see bug 565389 - send a window or element here
> > > >+            referer, payload.uris, payload.hashes, payload.names,
> > > >+            payload.icons, callback, payload.uris.length);
> > > >+      }
> > > >+    },
> > > >+  };
> > > >+
> > > >+  new InstallTriggerParent(
> > > >+    Components.classes["@mozilla.org/globalmessagemanager;1"].
> > > >+    getService(Components.interfaces.nsIChromeFrameMessageManager));
> > > 
> > > There is really no reason for this to be a new object like this. Just add the
> > > receiveMessage method to amManager.prototype.
> > 
> > Well, there is no technical reason, but I think it is cleaner this way, with
> > all the messaging stuff concentrated in this object. It's sort of like a plugin
> > for the amManager, instead of having a single big 'flat' object with lots of
> > different methods, and it is a clear parallel for the InstallTriggerChild
> > object. But, as in the previous issue, I don't feel too strongly about it, let
> > me know what you want me to do.
> 
> I disagree. It can be just as clear that this is a separate part by using
> appropriate comments in the amManager prototype and it saves us some additional
> work (the object instantiation) in the constructor which is in the startup
> path.

Fair enough, I changed it to that way.

Comment 42

[Dave Townsend [:mossop]]

Comment on attachment 454987 [details] [diff] [review]
e10s patch v7

This is basically right but I just spotted one problem that needs to be fixed first. The install method currently makes changes to the object that the webpage passes in which is something that would be unexpected. I expect the next pass to be q pretty quick r+.

If this works in non-e10s builds too do you need to disable building amInstallTrigger.cpp?

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

>+var gMessageManager =
>+  Components.classes["@mozilla.org/globalmessagemanager;1"].
>+  getService(Components.interfaces.nsIChromeFrameMessageManager);

Please move this into amManager's constructor, doesn't seem to be any need to hold it for the lifetime of the app.

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

>+/**
>+ * Child part of InstallTrigger e10s handling.
>+ *
>+ * Sets up InstallTriggers on newly-created windows,
>+ * that will relay messages for InstallTrigger
>+ * activity. We also process the parameters for
>+ * the InstallTrigger to proper parameters for
>+ * amIWebInstaller.
>+ */
>+function InstallTriggerChild(aMessageManager) {
>+  this.messageManager = aMessageManager;

I'm fine either way, but assuming this is getting loaded into a message manager do you really need to keep a reference to it like this? Can't you just make direct calls to "sendSyncMessage"?

>+      install: function(aArgs, aCallback) {
>+        var params = {
>+          mimetype: "application/x-xpinstall",
>+          referer: window.location.href,
>+          uris: [],
>+          hashes: [],
>+          names: [],
>+          icons: [],
>+        };
>+
>+        for (var name in aArgs) {
>+          var item = aArgs[name];
>+          if (typeof item === 'string') {
>+            item = { URL: item };
>+          } else if (!("URL" in item)) {
>+            throw new Error("Missing .URL for :" + name);

Use the message "Missing URL property for <name>"

>+          }
>+
>+          // Resolve and validate urls
>+          item.URL = this.resolveURL(item.URL);

Sorry, just spotted that this can be modifying the web-page's object. I would just stash this in a temporary variable then push it into the params.uris array.

>+          if (!this.checkLoadURIFromScript(item.URL))
>+            throw new Error("insufficient permissions to install: " + item.URL);
>+
>+          if ("IconURL" in item) {
>+            item.IconURL = this.resolveURL(item.IconURL);
>+            if (!this.checkLoadURIFromScript(item.IconURL)) {
>+              item.IconURL = null; // If page can't load the icon, just ignore it
>+            }
>+          } else {
>+            item.IconURL = null;
>+          }

Same here.

>+      /**
>+       * Resolves a URL in the context of our current window. We need to do
>+       * this before sending URLs to the parent process.
>+       *
>+       * @param  aUrl
>+       *         The url to resolve.
>+       *
>+       * @return A resolved, absolute nsURI object.
>+       */
>+      resolveURL: function(aUrl) {
>+        return Services.io.newURI(
>+          window.document.documentURIObject.resolve(aUrl), null, null);

Please use Services.ui.newURI(aURL, null, window.document.documentURIObject).

Comment 43

[Alon Zakai (:azakai)]

Attachment: e10s patch v8

Ok, fixed all the issues just mentioned.

Comment 44

[Dave Townsend [:mossop]]

I presume that now e10s has been merged with trunk the plan would be to just land this on trunk? I just did a test build on trunk with this patch applied and it fails a number of browser-chrome tests and also spews errors into the error console:

[JavaScript Error: "uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIFrameMessageManager.addMessageListener]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: chrome://mozapps/content/extensions/child-content.js :: ITC_observe :: line 254"  data: no]"]
[JavaScript Error: "uncaught exception: [Exception... "Security Manager vetoed action"  nsresult: "0x80570027 (NS_ERROR_XPC_SECURITY_MANAGER_VETO)"  location: "JS frame :: http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/installchrome.html?%20http%3A%2F%2Fexample.com%2Fbrowser%2Ftoolkit%2Fmozapps%2Fextensions%2Ftest%2Fxpinstall%2Funsigned.xpi :: startInstall :: line 12"  data: no]"]

Comment 45

[Alon Zakai (:azakai)]

Attachment: Patch v9

I started to go over the test failures.

One issue was bug 578172. I modified the code so it works now and refers to the bug.

Another issue is bug 565389. The current code passes null instead of a window, but the window is actually used for various things. In particular browser.js:gXPInstallObserver assumes the window is not null, and fails with an exception. So, unless there is a simple solution here, it seems this bug should wait on bug 565389.

Comment 46

[Alon Zakai (:azakai)]

Attachment: Patch v10

Slightly improved patch, after running all the automated tests (possible since e10s landed on m-c), and while working on bug 565389. The combination of this patch and that one now pass all tests. Changes:

1. Send XULElement and not null, as the origin of the network message. Needs the other patch (the two can best be tested together; might be convenient to use patch queue here: http://hg.mozilla.org/users/azakai_mozilla.com/patches )
2. Remove some warnings on cases where there is no wrappedJSObject (we don't want to have InstallTriggers there anyhow).
3. Expose the constants from amIInstallTrigger.

Comment 47

[Dave Townsend [:mossop]]

Blocking since Fennec needs it.

Comment 48

[Dave Townsend [:mossop]]

(In reply to comment #46)
> 2. Remove some warnings on cases where there is no wrappedJSObject (we don't
> want to have InstallTriggers there anyhow).

What cases are those and why don't we want to have InstallTriggers on them?

Comment 49

[Dave Townsend [:mossop]]

Comment on attachment 458010 [details] [diff] [review]
Patch v10

>diff --git a/toolkit/mozapps/extensions/addonManager.js b/toolkit/mozapps/extensions/addonManager.js
>--- a/toolkit/mozapps/extensions/addonManager.js
>+++ b/toolkit/mozapps/extensions/addonManager.js
>@@ -57,12 +57,26 @@
> const UNSUPPORTED_TYPE  = -244;
> const SUCCESS = 0;
> 
>+const MSG_INSTALL_ENABLED  = "WebInstallerIsInstallEnabled";
>+const MSG_INSTALL_ADDONS   = "WebInstallerInstallAddonsFromWebpage";
>+const MSG_INSTALL_CALLBACK = "WebInstallerInstallCallback";
>+
> Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>+Components.utils.import("resource://gre/modules/Services.jsm");
> 
> var gSingleton = null;
> 
> function amManager() {
>   Components.utils.import("resource://gre/modules/AddonManager.jsm");
>+
>+  var messageManager =
>+    Components.classes["@mozilla.org/globalmessagemanager;1"].
>+    getService(Components.interfaces.nsIChromeFrameMessageManager);

Use Cc and Ci to shorten this and line it up as elsewhere in this file.

>+  messageManager.addMessageListener(MSG_INSTALL_ENABLED, this);
>+  messageManager.addMessageListener(MSG_INSTALL_ADDONS, this);
>+  messageManager.loadFrameScript(
>+    "chrome://mozapps/content/extensions/child-content.js", true);

Put the url into a constant.

>+      case MSG_INSTALL_ADDONS:
>+        var callback = null;
>+        if (payload.callbackId != -1) {
>+          callback = {
>+            onInstallEnded: function ITP_callback(url, status) {
>+              // Doing it this way, instead of aMessage.target.messageManager,
>+              // ensures it works in Firefox and not only Fennec. See bug
>+              // 578172. TODO: Clean up this code once that bug is fixed
>+              var returnMessageManager = aMessage.target.QueryInterface(
>+                Components.interfaces.nsIFrameLoaderOwner).frameLoader.
>+                messageManager;

Split this into a few statements please and make use of Ci to shorten things. The code style here never ends a line with an open bracket.

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

>+          params.uris.push(url.spec);
>+          params.hashes.push("Hash" in item ? item.Hash : null);
>+          params.names.push(name);
>+          params.icons.push(iconUrl !== null ? iconUrl.spec : null);

You should just need:

params.icons.push(iconUrl ? iconUrl.spec : null);

Comment 50

[Alon Zakai (:azakai)]

Attachment: Patch v11

Updated patch:

* Fixes for the last batch of review comments.

* Grabbed a bit of the no-loadgroup patch (bug 565389), specifically the part that removes using the loadgroup itself for the request callbacks (but a window is still passed around). As a result, this patch (1) passes all automated tests on non-e10s m-c, and (2) passes basic manual tests with e10s fennec.

My thinking is that, since we lack proper automated tests for e10s fennec anyhow, this is good enough for the short term for fennec, while not breaking any tests in m-c. So, bug 565389 might be delayed until we figure out the proper way to handle the XUL elements that the message manager gives us, relating them to non-e10s windows, etc.

Comment 51

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

"chrome://mozapps/content/extensions/child-content.js" is too generic. Other toolkit code will need to add content.js files too.

How about "extensions-content.js"

Comment 52

[Dave Townsend [:mossop]]

(In reply to comment #51)
> "chrome://mozapps/content/extensions/child-content.js" is too generic. Other
> toolkit code will need to add content.js files too.
> 
> How about "extensions-content.js"

Sounds good to me.

(In reply to comment #50)
> * Grabbed a bit of the no-loadgroup patch (bug 565389), specifically the part
> that removes using the loadgroup itself for the request callbacks (but a window
> is still passed around). As a result, this patch (1) passes all automated tests
> on non-e10s m-c, and (2) passes basic manual tests with e10s fennec.

I'm a little surprised that this passes the cookie tests, but I guess I'll dig into why when I review, however all of the tests are spewing these errors into the console:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIFrameMessageManager.addMessageListener]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: chrome://mozapps/content/extensions/child-content.js :: ITC_observe :: line 266"  data: no]

This needs to be fixed.

> My thinking is that, since we lack proper automated tests for e10s fennec
> anyhow, this is good enough for the short term for fennec, while not breaking
> any tests in m-c. So, bug 565389 might be delayed until we figure out the
> proper way to handle the XUL elements that the message manager gives us,
> relating them to non-e10s windows, etc.

Bug 565389 changes APIs which will break other applications and possibly extensions so if we want to take it at all in Gecko 2.0 then I think it needs to block beta3. The question is what do we lose by taking this patch without it?

Comment 53

[Alon Zakai (:azakai)]

(In reply to comment #52)
> (In reply to comment #50)
> > * Grabbed a bit of the no-loadgroup patch (bug 565389), specifically the part
> > that removes using the loadgroup itself for the request callbacks (but a window
> > is still passed around). As a result, this patch (1) passes all automated tests
> > on non-e10s m-c, and (2) passes basic manual tests with e10s fennec.
> 
> I'm a little surprised that this passes the cookie tests, but I guess I'll dig
> into why when I review, however all of the tests are spewing these errors into
> the console:
> 
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80004003 (NS_ERROR_INVALID_POINTER)
> [nsIFrameMessageManager.addMessageListener]"  nsresult: "0x80004003
> (NS_ERROR_INVALID_POINTER)"  location: "JS frame ::
> chrome://mozapps/content/extensions/child-content.js :: ITC_observe :: line
> 266"  data: no]
> 
> This needs to be fixed.

I spent several hours delving into this. It is bizarre. My current guess is that this is some bug in either xpconnect or the test harness. Symptoms:

1. The problem only occurs when running all tests - NOT when running individual tests (!) (In particular this makes debugging this very slow - need to run the full test suite each time.)
2. The problem happens when our observer for window creation is called in extensions-content.js. We have several such observers (separate instances for several windows). What happens is the first of these runs fine. The second is not even started - it returns an error message immediately (however, the same function was called earlier, successfully). The error message complains about a problem at the *end* of the function, as if it had happened in the previous observer (which completed successfully, all the way through). My only guess at this point is that an error is being erroneously logged, in a way that (a) lets the first function call complete successfully - the error is not detected, but (b) it is detected as the 2nd function call is being prepared.

Unless someone has an idea for how to continue to investigate this, I suggest that this be delayed to another bug, and not prevent this one from being completed.

> 
> > My thinking is that, since we lack proper automated tests for e10s fennec
> > anyhow, this is good enough for the short term for fennec, while not breaking
> > any tests in m-c. So, bug 565389 might be delayed until we figure out the
> > proper way to handle the XUL elements that the message manager gives us,
> > relating them to non-e10s windows, etc.
> 
> Bug 565389 changes APIs which will break other applications and possibly
> extensions so if we want to take it at all in Gecko 2.0 then I think it needs
> to block beta3. The question is what do we lose by taking this patch without
> it?

To be honest, I do not see the need for bug 565389 anymore. The latest patch for this bug stops using the loadgroup from the window that is passed along; the window being passed is just used for purposes of knowing what window to pop up and possibly other GUI stuff (correct?). So, might as well keep passing a window (and not a XULElement). Aside from that, there is some loadgroup cruft *internally* in the XPI installer code, which can be removed for aesthetic purposes, but it seems very low priority.

Comment 54

[Dave Townsend [:mossop]]

(In reply to comment #53)
> 2. The problem happens when our observer for window creation is called in
> extensions-content.js. We have several such observers (separate instances for
> several windows).

extensions-content.js is run for every window? That is a problem, I was under the impression that it only ran once per process. This means it is going to be adding multiple observers and for every new content window it will be calling addMessageListener multiple times (I'm going to bet that this is the problem, what does the message manager do if you try to add a listener for the same message multiple times?)

Comment 55

[Alon Zakai (:azakai)]

(In reply to comment #54)
> (In reply to comment #53)
> > 2. The problem happens when our observer for window creation is called in
> > extensions-content.js. We have several such observers (separate instances for
> > several windows).
> 
> extensions-content.js is run for every window? That is a problem, I was under
> the impression that it only ran once per process. This means it is going to be
> adding multiple observers and for every new content window it will be calling
> addMessageListener multiple times (I'm going to bet that this is the problem,
> what does the message manager do if you try to add a listener for the same
> message multiple times?)

extensions-content.js is run once per frame, but observes content-document-global-created, which is called several times (once for each window global created). We then call addMessageListener on the InstallTrigger object that we create for each such window.

This is not the problem, it is valid to add multiple listeners for the same message, and this is done in other places. All listeners for a message get called (much like with observing/observerService). Also more specifically, in the output I see that almost all the time these multiple listeners are called properly, except for the bizarre cases where the 2nd one fails as described above.

Comment 56

[Dave Townsend [:mossop]]

(In reply to comment #55)
> (In reply to comment #54)
> > (In reply to comment #53)
> > > 2. The problem happens when our observer for window creation is called in
> > > extensions-content.js. We have several such observers (separate instances for
> > > several windows).
> > 
> > extensions-content.js is run for every window? That is a problem, I was under
> > the impression that it only ran once per process. This means it is going to be
> > adding multiple observers and for every new content window it will be calling
> > addMessageListener multiple times (I'm going to bet that this is the problem,
> > what does the message manager do if you try to add a listener for the same
> > message multiple times?)
> 
> extensions-content.js is run once per frame, but observes
> content-document-global-created, which is called several times (once for each
> window global created). We then call addMessageListener on the InstallTrigger
> object that we create for each such window.

What do you mean by "frame" here?

If we're adding the observer multiple times then it will get called multiple times for a single content window opening which needs to be stopped.

I've also spotted that we never removeMessageListener. How does the message manager know not to pass messages to InstallTrigger objects that were created for long-dead content windows?

Comment 57

[Alon Zakai (:azakai)]

(In reply to comment #56)
> (In reply to comment #55)
> > (In reply to comment #54)
> > > (In reply to comment #53)
> > > > 2. The problem happens when our observer for window creation is called in
> > > > extensions-content.js. We have several such observers (separate instances for
> > > > several windows).
> > > 
> > > extensions-content.js is run for every window? That is a problem, I was under
> > > the impression that it only ran once per process. This means it is going to be
> > > adding multiple observers and for every new content window it will be calling
> > > addMessageListener multiple times (I'm going to bet that this is the problem,
> > > what does the message manager do if you try to add a listener for the same
> > > message multiple times?)
> > 
> > extensions-content.js is run once per frame, but observes
> > content-document-global-created, which is called several times (once for each
> > window global created). We then call addMessageListener on the InstallTrigger
> > object that we create for each such window.
> 
> What do you mean by "frame" here?

In the sense of nsIFrameMessageManager.idl : nsIChromeFrameMessageManager.loadFrameScript(). Basically, a tab.

> 
> If we're adding the observer multiple times then it will get called multiple
> times for a single content window opening which needs to be stopped.

We add one InstallTrigger to each window global, and one addMessageListener() call is made for each of those. This seems proper to me, or I do not understand what you mean.

> 
> I've also spotted that we never removeMessageListener. How does the message
> manager know not to pass messages to InstallTrigger objects that were created
> for long-dead content windows?

removeMessageListener is basically never used anywhere else either, because it isn't really needed. We want to listen to the message for as long as the window exists, and when it stops existing, the listening stops anyhow (as the messageManager for that window is gone).

Comment 58

[Dave Townsend [:mossop]]

(In reply to comment #57)
> (In reply to comment #56)
> > (In reply to comment #55)
> > > (In reply to comment #54)
> > > > (In reply to comment #53)
> > > > > 2. The problem happens when our observer for window creation is called in
> > > > > extensions-content.js. We have several such observers (separate instances for
> > > > > several windows).
> > > > 
> > > > extensions-content.js is run for every window? That is a problem, I was under
> > > > the impression that it only ran once per process. This means it is going to be
> > > > adding multiple observers and for every new content window it will be calling
> > > > addMessageListener multiple times (I'm going to bet that this is the problem,
> > > > what does the message manager do if you try to add a listener for the same
> > > > message multiple times?)
> > > 
> > > extensions-content.js is run once per frame, but observes
> > > content-document-global-created, which is called several times (once for each
> > > window global created). We then call addMessageListener on the InstallTrigger
> > > object that we create for each such window.
> > 
> > What do you mean by "frame" here?
> 
> In the sense of nsIFrameMessageManager.idl :
> nsIChromeFrameMessageManager.loadFrameScript(). Basically, a tab.
> 
> > 
> > If we're adding the observer multiple times then it will get called multiple
> > times for a single content window opening which needs to be stopped.
> 
> We add one InstallTrigger to each window global, and one addMessageListener()
> call is made for each of those. This seems proper to me, or I do not understand
> what you mean.

So, if extensions-content.js is executed for every tab opened it will add a content window observer for every tab opened. But since all the tabs are in the same process every window observer will hear about content windows opened in any tab. So if I open two tabs two observers are there and when a page loads in one tab both observers will hear about it, attempt to attach an InstallTrigger to it and register it to receive callback messages.

> > I've also spotted that we never removeMessageListener. How does the message
> > manager know not to pass messages to InstallTrigger objects that were created
> > for long-dead content windows?
> 
> removeMessageListener is basically never used anywhere else either, because it
> isn't really needed. We want to listen to the message for as long as the window
> exists, and when it stops existing, the listening stops anyhow (as the
> messageManager for that window is gone).

Does the message manager go away when the window stops existing or when the "frame" stops existing. If it is the latter then the window that the callback is destined for can have died long before the message manager does. Even if it is the former then I suspect you need some checks before attempting to pass the callback to the now-non-existent message manager.

Comment 59

[Dave Townsend [:mossop]]

Comment on attachment 458861 [details] [diff] [review]
Patch v11

Need to work out whether there is really a problem based on the previous comments in the bug or not before this could land.

>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 DOM Element related to the request

You are (I think) still passing a window,so name it appropriately.

>+ * @param  aNeedBadCertHandling
>+ *         Whether we should handle bad certs or not
>+ */
>+function XPINotificationCallbacks(aElement, aNeedBadCertHandling) {
>+  this.window = aElement; // && "contentWindow" in aElement) ? aElement.contentWindow :
>+    null;

This comment is meaningless at this point, also almost became a syntax error with the line wrap there.

>+XPINotificationCallbacks.prototype = {
>+  QueryInterface: function(iid) {
>+    if (iid.equals(Ci.nsISupports) || iid.equals(Ci.nsIInterfaceRequestor))
>+      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);

Dolske says the better code to use here would be the JS equivalent of this: http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#1073

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

this.element doesn't seem to exist, perhaps you mean this.window.

>+      this.channel.QueryInterface(Ci.nsIHttpChannelInternal)
>+                  .forceAllowThirdPartyCookie = true;
>       this.channel.asyncOpen(listener, null);
> 
>       Services.obs.addObserver(this, "network:offline-about-to-go-offline", false);
>@@ -4209,9 +4247,6 @@
>   onStartRequest: function AI_onStartRequest(aRequest, aContext) {
>     // We must remove the request from the load group otherwise if the user
>     // closes the page that triggered it the download will be cancelled
>-    if (this.loadGroup)
>-      this.loadGroup.removeRequest(aRequest, null, Cr.NS_BINDING_RETARGETED);
>-

If you're removing the code remove the comment that describes the code too.

>diff --git a/toolkit/mozapps/extensions/addonManager.js b/toolkit/mozapps/extensions/addonManager.js
>--- a/toolkit/mozapps/extensions/addonManager.js
>+++ b/toolkit/mozapps/extensions/addonManager.js
>@@ -57,12 +57,28 @@
> const UNSUPPORTED_TYPE  = -244;
> const SUCCESS = 0;
> 
>+const MSG_INSTALL_ENABLED  = "WebInstallerIsInstallEnabled";
>+const MSG_INSTALL_ADDONS   = "WebInstallerInstallAddonsFromWebpage";
>+const MSG_INSTALL_CALLBACK = "WebInstallerInstallCallback";
>+
>+const ADDONMANAGER_CHILD_SCRIPT =
>+  "chrome://mozapps/content/extensions/child-content.js";

Just call it CHILD_SCRIPT, we're already in add-on manager code. Also as Mark suggested go with extensions-content.js.

> 
> var gSingleton = null;
> 
> function amManager() {
>   Components.utils.import("resource://gre/modules/AddonManager.jsm");
>+
>+  var messageManager = Cc["@mozilla.org/globalmessagemanager;1"].
>+                       getService(Ci.nsIChromeFrameMessageManager);

Change this to how I've shown it.

>+  receiveMessage: function(aMessage) {
>+    var payload = aMessage.json;
>+    var referer = Services.io.newURI(payload.referer, null, null);
>+    switch (aMessage.name) {
>+      case MSG_INSTALL_ENABLED:
>+        return this.isInstallEnabled(payload.mimetype, referer);
>+
>+      case MSG_INSTALL_ADDONS:
>+        var callback = null;
>+        if (payload.callbackId != -1) {
>+          callback = {
>+            onInstallEnded: function ITP_callback(url, status) {
>+              // Doing it this way, instead of aMessage.target.messageManager, g
>+              // ensures it works in Firefox and not only Fennec. See bug
>+              // 578172. TODO: Clean up this code once that bug is fixed
>+              var flo = aMessage.target.QueryInterface(Ci.nsIFrameLoaderOwner);
>+              var returnMessageManager = flo.frameLoader.messageManager;
>+              returnMessageManager.sendAsyncMessage(MSG_INSTALL_CALLBACK,
>+                { callbackId: payload.callbackId, url: url, status: status }
>+              );

Change this to how I've shown it, also make sure to handle errors appropriately like the case where the message manager has gone away already.

>+        var window;
>+        try {
>+          // Normal approach for single-process mode
>+          window = aMessage.target.docShell
>+                           .QueryInterface(Ci.nsIInterfaceRequestor)
>+                           .getInterface(Ci.nsIDOMWindow).content;
>+        } catch (e) {
>+          // Fallback for multiprocess (e10s) mode. Appears to work but has
>+          // not had a full suite of automated tests run on it.
>+          window = aMessage.target.parentNode.parentNode.parentNode
>+                           .parentNode.parentNode.defaultView;

Ick. Use aMessage.target.ownerDocument.defaultView.

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

>+InstallTriggerChild.prototype = {
>+  observe: function ITC_observe(aSubject, aTopic, aData) {
>+    var window = aSubject;
>+
>+    if (!window.wrappedJSObject)
>+      return;

I still want to know what case this is handling.

>+       * @see amIInstallTriggerInstaller.idl
>+       */
>+      startSoftwareUpdate: function(aUrl, aFlags) {
>+        var url = Services.io.newURI(aUrl, null, null)
>+                          .QueryInterface(Ci.nsIURL).filename;

Line this up like so.

>+       * Resolves a URL in the context of our current window. We need to do
>+       * this before sending URLs to the parent process.
>+       *
>+       * @param  aUrl
>+       *         The url to resolve.
>+       *
>+       * @return A resolved, absolute nsURI object.
>+       */
>+      resolveURL: function(aUrl) {
>+        return Services.io.newURI(aUrl, null,
>+                                  window.document.documentURIObject);

Line this up like so.

Comment 60

[Alon Zakai (:azakai)]

Attachment: Patch v12

(In reply to comment #59)
> 
> Need to work out whether there is really a problem based on the previous
> comments in the bug or not before this could land.

You were right about that - avoiding duplicate calls to addMessageListener prevents the bizarre failure notices. I still have no explanation for why it doesn't happen in check-one, nor for why a problem happens at all (looking in the code in addMessageListener, it should handle the duplicates fine...), or in the way that it does fail (failure on the *next* call). There is probably some subtle xpconnect issue I don't fully understand. In any case, works perfectly now.

You were also right about the existence of the duplicates in the first place - I was thinking about a different kind of duplicate, my mistake.

As part of avoiding the duplicates, I cleaned up the code, removing amInstallTrigger.cpp, which is no longer used anyhow.

> >+      case MSG_INSTALL_ADDONS:
> >+        var callback = null;
> >+        if (payload.callbackId != -1) {
> >+          callback = {
> >+            onInstallEnded: function ITP_callback(url, status) {
> >+              // Doing it this way, instead of aMessage.target.messageManager, g
> >+              // ensures it works in Firefox and not only Fennec. See bug
> >+              // 578172. TODO: Clean up this code once that bug is fixed
> >+              var flo = aMessage.target.QueryInterface(Ci.nsIFrameLoaderOwner);
> >+              var returnMessageManager = flo.frameLoader.messageManager;
> >+              returnMessageManager.sendAsyncMessage(MSG_INSTALL_CALLBACK,
> >+                { callbackId: payload.callbackId, url: url, status: status }
> >+              );
> 
> Change this to how I've shown it, also make sure to handle errors appropriately
> like the case where the message manager has gone away already.

Changed, but not sure if it is possible to handle such errors on this side, or how such checking would look. bsmedberg wasn't sure if we have a policy yet about such things; following his advice I also asked in the platform mailing list,

http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/b2c7f463e4419150#

but meanwhile I think handling such cases should be set aside (at some point we will need to look at all the code using the messageManager, none of which yet properly handles this).

> 
> >diff --git a/toolkit/mozapps/extensions/content/child-content.js b/toolkit/mozapps/extensions/content/child-content.js
> 
> >+InstallTriggerChild.prototype = {
> >+  observe: function ITC_observe(aSubject, aTopic, aData) {
> >+    var window = aSubject;
> >+
> >+    if (!window.wrappedJSObject)
> >+      return;
> 
> I still want to know what case this is handling.
> 

Ok, I investigated this more in depth. The issue is that ChromeWindows also get called about (since we listen for the creation of any window), and they lack wrappedJSObject. But anyhow we do not need to add InstallTriggers on them. So I replaced the hackish check for wrappedJSObject with testing QueryInterface on nsIDOMChromeWindow.

Comment 61

[Dave Townsend [:mossop]]

(In reply to comment #60)
> Created attachment 459609 [details] [diff] [review]
> Patch v12
> 
> (In reply to comment #59)
> > 
> > Need to work out whether there is really a problem based on the previous
> > comments in the bug or not before this could land.
> 
> You were right about that - avoiding duplicate calls to addMessageListener
> prevents the bizarre failure notices. I still have no explanation for why it
> doesn't happen in check-one, nor for why a problem happens at all (looking in
> the code in addMessageListener, it should handle the duplicates fine...), or in
> the way that it does fail (failure on the *next* call). There is probably some
> subtle xpconnect issue I don't fully understand. In any case, works perfectly
> now.
> 
> You were also right about the existence of the duplicates in the first place -
> I was thinking about a different kind of duplicate, my mistake.
> 
> As part of avoiding the duplicates, I cleaned up the code, removing
> amInstallTrigger.cpp, which is no longer used anyhow.

This is a start however you're still adding multiple observers for content-document-global-created. One per tab ever opened in fact. By the time I've been using Firefox for a while and have worked through say 100 tabs, then next webpage I load will call those 100 observers. The time spent in each is small, but cumulative and so will hurt our pageload time.

> > >+InstallTriggerChild.prototype = {
> > >+  observe: function ITC_observe(aSubject, aTopic, aData) {
> > >+    var window = aSubject;
> > >+
> > >+    if (!window.wrappedJSObject)
> > >+      return;
> > 
> > I still want to know what case this is handling.
> > 
> 
> Ok, I investigated this more in depth. The issue is that ChromeWindows also get
> called about (since we listen for the creation of any window), and they lack
> wrappedJSObject. But anyhow we do not need to add InstallTriggers on them. So I
> replaced the hackish check for wrappedJSObject with testing QueryInterface on
> nsIDOMChromeWindow.

I just chatted with Jonas who implemented the content-document-global-created notification and according to him it should never be firing for chrome windows. Can you figure out what URL's it is firing for? We might need to file a bug to get that fixed.

Comment 62

[Alon Zakai (:azakai)]

Attachment: Patch v13

> 
> This is a start however you're still adding multiple observers for
> content-document-global-created. One per tab ever opened in fact. By the time
> I've been using Firefox for a while and have worked through say 100 tabs, then
> next webpage I load will call those 100 observers. The time spent in each is
> small, but cumulative and so will hurt our pageload time.

Good point.

Ok, moved from the global observer service and content-document-global-created to listening for the event DOMWindowCreated on the tab itself. So each tab just listens for new windows inside it. No scaling problem anymore.

(Minor annoyance is this is called once per new HTMLDocument, so it is a few times per actual Window. But after reading the code in nsGlobalWindow.cpp carefully, seems that was also the case with content-document-global-created anyhow...)

> 
> > > >+InstallTriggerChild.prototype = {
> > > >+  observe: function ITC_observe(aSubject, aTopic, aData) {
> > > >+    var window = aSubject;
> > > >+
> > > >+    if (!window.wrappedJSObject)
> > > >+      return;
> > > 
> > > I still want to know what case this is handling.
> > > 
> > 
> > Ok, I investigated this more in depth. The issue is that ChromeWindows also get
> > called about (since we listen for the creation of any window), and they lack
> > wrappedJSObject. But anyhow we do not need to add InstallTriggers on them. So I
> > replaced the hackish check for wrappedJSObject with testing QueryInterface on
> > nsIDOMChromeWindow.
> 
> I just chatted with Jonas who implemented the content-document-global-created
> notification and according to him it should never be firing for chrome windows.
> Can you figure out what URL's it is firing for? We might need to file a bug to
> get that fixed.

This happened in about:blank. (But, moved to DOMWindowCreated anyhow as mentioned above, so we don't need to wait on that for this bug.)

Comment 63

[Dave Townsend [:mossop]]

Comment on attachment 459695 [details] [diff] [review]
Patch v13

This looks basically perfect and it would be an r+ except for the fact that it doesn't compile. It is still trying to build and link the extensions binary component, you'll need to remove most of the LIBRARY, COMPONENT, GRE etc. declarations in Makefile.in and remove the other references to the extensions module in toolkit/library. Please also remove amInstallTrigger.h while you're here. I think we could remove the definition of amIInstallTrigger in the IDL but I think keeping that around for documentation makes a little sense.

Comment 64

[Alon Zakai (:azakai)]

Attachment: Patch v14

Sorry about the compilation errors - fixed in this version. I didn't notice because on my system it still built ok (due to having the old library there).

This patch also fixes some minor reported errors about chrome windows, as DOMWindowCreated does notify about such windows and not just content windows.

Meanwhile I have been tracking down some odd failures in jsreftests that this patch has. Turns out it's an xpconnect problem, filed as bug 582100 (and marked this bug as depending on that), should be resolved soon.

Comment 65

[Dave Townsend [:mossop]]

Comment on attachment 460380 [details] [diff] [review]
Patch v14

Looks good, sorry there was so much back and forth here. messageManager is pretty new to me so it took me a while to see what the best path was here.

Comment 66

[Mike Beltzner [:beltzner, not reading bugmail]]

This is ready to check in but depends on bug 582100 which causes a test to fail: what's the way forward?

Comment 67

[Alon Zakai (:azakai)]

Attachment: Workaround version of patch

mrbkap has suggested a workaround for bug 582100, which is blocking this from landing. Here is a patch with that workaround, which is just adding this line:

+        toSource: "r", // XXX workaround for bug 582100

I am sending to the try server now to test.

Comment 68

[:Gavin Sharp [email: [e-mail]]]

Bug 582100 is FIXED. Does that mean that "Patch v14" is ready to land?

Comment 69

[Blake Kaplan (:mrbkap) (inactive)]

I think with the patch in comment 67, this should be good to land. Let me know if that isn't the case, though.

Comment 70

[:Gavin Sharp [email: [e-mail]]]

We ended up trying to land this without the comment 67 hack - it triggered the jsreftest failures again, in addition to apparently being responsible for leaks on mochitest/browser-chrome. It doesn't seem like this needs to hard-block b3 at this stage, so I'm going to push it out to blocking2.0-beta4+.

Comment 71

[Alon Zakai (:azakai)]

Ah, I guess I misunderstood bug 582100. The patch there *combined* with the workaround also mentioned there is enough to pass the jsreftests. (I thought the patch fixed them all, and the workaround was a temporary fix until it lands.)

So, the "Workaround version of patch" attachment here would pass all the jsreftests if pushed now. *However*, bz mentioned that a Txul regression might be related to this bug, so I will investigate that first.

Comment 72

[Alon Zakai (:azakai)]

It does look like there are performance issues with this patch.

Just loading the global messageManager in the parent script (addonManager.js) adds 3% to Txul(!), 84.12 (+- 0.86) vs 81.66 (+- 0.87) on my machine (mean of 100 runs in Txul).

Aside from that there are some additional minor slowdowns as well. As a first step, will try to figure out what is going on with the global messageManager.

Comment 73

[Alon Zakai (:azakai)]

Figuring out the speed issue is turning out to be very difficult.

My current setup is to run Txul 1,000 times in order to lower the randomness, which is very high. At 1,000 times, the standard deviation is around 0.26ms, but there are apparently significant additional sources of randomness that make this very hard to do, as running the exact same code twice in this setup can often show differences of 1ms or more. I am trying to control everything else on this machine - not run anything heavy at the same time, etc. - but perhaps it is just not easy to do properly.

The range in question is around 75.29ms without this patch and 80.95/82.36m with it (two separate runs - with quite different results, but both high). Some example data:

* Without the loadFrameScript in the parent (so no child script is run, and the only new thing the parent does is get the global messageManager: 74.64ms

* With an empty child script (that is loaded): 76.28ms, 76.58ms

* Child script never subscribes to DOMWindowCreated (so really, the child script does close to nothing): 79.07ms

* Don't even create an InstallTriggerChild in the child (so even less is done): 79.42ms

* Remove the bulk of the child script code, leaving just the first 6 lines (constants and Cc/Ci): 77.84ms

* Empty out the prototype and function in the child script: 77.77ms, 78.48ms

All these runs take a great deal of time, and I am not sure how useful the information is. I can't say that I have a much better idea of where the slowdown is, except that it seems to be mostly in the child script, and there doesn't seem to be a single source of slowdown. Or, the randomness is just making it too hard to tell.

Questions:

1. Is there a better procedure for debugging slowdowns?

2. Is there anything obvious from this data that I am overlooking?

3. There might be unavoidable slowdowns here - looks like even a few lines in the child script (that do practically nothing) add a few ms, which means a couple of % slowdown in Txul. So there might not be a way to get this patch to not have any slowdown at all. Is there some amount of slowdown that is acceptable? If not, would we consider scrapping this entire approach and starting work on the bug from scratch in a different way?

Comment 74

[Alon Zakai (:azakai)]

More detailed line-by-line profiling of C++ and JS shows that most of the slowdown is due to nsInProcessTabChildGlobal::LoadFrameScript, half of it due to setting up the channel, another half due to compiling&parsing JS (but *not* running the JS). smaug is looking into caching LoadFrameScript (which will speed things up in general, in particular for Fennec, which uses a lot of that).

Also odd is that loadFrameScript is called multiple times per opened window, which I will look into.

Comment 75

[Alon Zakai (:azakai)]

The speed issues have been investigated, and the 'Workaround' version of this patch (not the other one which is r+'d!) will be ready to land once two issues with the Message Manager are resolved, bug 584864 and bug 584866.

Comment 76

[Alon Zakai (:azakai)]

This patch causes the following on Mochitest 5 on the try server, all platforms:

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 16090 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of AtomImpl with size 20 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 24 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of Connection with size 100 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of DirectoryProvider with size 12 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of GenericFactory with size 16 bytes

Removing all the C++ part of this patch, so it is pure JS, leaves this C++ leak in place - so this is somewhat bizarre. Must be something that is called from JS (but wasn't noticed until now?).

I cannot reproduce this locally, so over the last 3 days have been bisecting with the try server. It appears to be caused when the handleEvent() is called in the child - even if it quits immediately with a return;. If the addEventListener("DOMWindowCreated", this, false) call is commented out (so the handleEvent will never be called), then the failure does not happen.

I have no idea why this would occur. Only suspicion is that DOMWindowCreated is not used by any Firefox code - this patch is the first user (it is used in Fennec, but we wouldn't notice leaks like this there).

Comment 78

[Benjamin Smedberg]

Comment on attachment 462279 [details] [diff] [review]
Workaround version of patch

I think that the globals and message handling here is probably incorrect.

Firstly, you're calling addMessageListener more than once: technically you're calling it once per window, so you could end up with a bunch of message listeners. Secondly, you're closing over "window" for all the installtrigger methods. I think you want one InstallTrigger prototype and then you want to set this._window on particular instances. Because we lack handles right now, you should emulate them using integer IDs that you pass in and back to get the correct install callback:

// for lack of a .createHandle API, keep a map of pending installs
var gPendingInstalls = [];
var gPendingInstallID = 0;

function InstallTrigger(contentWindow)
{
  this._window = contentWindow;
}
InstallTrigger.prototype = {
  __exposedProps__: { ... },
  ...
  install: function(aArgs, aCallback) {
    ...
    // add ourself to the pending install list
    var installID = gPendingInstallID++;
    gPendingInstalls[installID] = aCallback;
    return sendSyncMessage(MSG_INSTALL_ADDONS, installID, params);
  }
};

addEventListener("DOMWindowCreated", function(aEvent) {
  var window = aEvent.originalTarget.defaultView.content;
  window.wrappedJSObject.InstallTrigger = new InstallTrigger(window);
}, false);

addMessageListener(MSG_INSTALL_CALLBACK, function(name, installID, result) {
  gPendingInstalls[installID](result);
  delete gPendingInstalls[installID];
});

Comment 79

[Alon Zakai (:azakai)]

Attachment: Patch v15

Updated patch, with the following:

1. Fix an issue bsmedberg found, with not properly handling the case of multiple InstallTriggers in one tab. To fix that, installer IDs have been added, and we listen to messages just on the Manager object (regarding the name, see below).
2. Following bsmedberg's guidance, no longer using a closure for the InstallTrigger, instead it is moved to the top level.
3. Renamed InstallTriggerChild to InstallTriggerManager, to prevent confusion now that InstallTrigger is top level as well.
4. Stopped using Services.jsm, instead just use gIoService in the child (the only service we need). This was found to be related to GC issues, not entirely sure why (dougt is investigating the underlying issues).

Comment 80

[Dave Townsend [:mossop]]

Comment on attachment 465902 [details] [diff] [review]
Patch v15

A few minor bits here but otherwise this looks ok. Has bsmedberg looked at the new patch yet? It would probably be good to get a follow-up look over from him on it but I'd be ok with that happening after the landing if needs be.

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

>+function InstallTriggerManager() {
>+  addMessageListener(MSG_INSTALL_CALLBACK, this);
>+
>+  addEventListener("DOMWindowCreated", this, false);
>+
>+  var that = this;

Nitty nit, we use "self" as a reference to this when needed.

>+  addEventListener("unload", function() {
>+    // Clean up all references, to help gc work quickly
>+    for (var installerId in that.installerIds) {
>+      that.installerIds[installerId].callbacks = null;
>+      that.installerIds[installerId] = null;
>+    }
>+    that.installerIds = null;
>+  }, false);
>+}
>+
>+InstallTriggerManager.prototype = {
>+  installerIds: [],

Initialise the array in the constructor, it doesn't matter so much here but this sort of thing will break if for some reason we end up instantiating InstallTriggerManager multiple times in this scope.

>+    // This event happens for each HTMLDocument, so it can happen more than
>+    // once per Window. We only need to work once per Window though.
>+    if (window.wrappedJSObject.InstallTrigger)
>+        return;
>+
>+    // The public object which web scripts can see (limited by
>+    // the __exposedProps__ defined below)

This comment is no longer accurate, drop the part about __exposedProps__.

>+    var installTrigger = new InstallTrigger(window);
>+
>+    window.wrappedJSObject.InstallTrigger = installTrigger;
>+
>+    var installerId = this.nextInstallerId ++;
>+    installTrigger.installerId = installerId;

I'd prefer you to pass the installerId to InstallTrigger's constructor.

>+    this.installerIds[installerId] = installTrigger;

If I'm reading this right we will never drop the reference to the InstallTriggers for this tab until the app is closed. That could be a problem, it might be worth only holding a weak reference to it to allow it to die when the window goes away. I'd take that as a follow-up patch though.

Comment 81

[Alon Zakai (:azakai)]

Attachment: Patch v16

Ok, fixed the issues mentioned. Regarding:

> 
> >+    this.installerIds[installerId] = installTrigger;
> 
> If I'm reading this right we will never drop the reference to the
> InstallTriggers for this tab until the app is closed. That could be a problem,
> it might be worth only holding a weak reference to it to allow it to die when
> the window goes away. I'd take that as a follow-up patch though.

The recently-created 'unload' event listened to here is called when this tab/frame is closed. So we won't hold on to anything after the tab is gone. This isn't as good as doing that when the window goes away, but is much better than when the entire app closes. But I agree, would be worth it to address this in a followup bug.

Comment 82

[Benjamin Smedberg]

The cleanup *should* happen with delete gPendingInstalls[installID]; Did that bit get removed/overlooked?

Comment 83

[Alon Zakai (:azakai)]

(In reply to comment #82)
> The cleanup *should* happen with delete gPendingInstalls[installID]; Did that
> bit get removed/overlooked?

A single call to install() can lead to several callbacks, so this can't be done on the first one, but when possible it is, in receiveMessage the patch will do

+    if (callbackObj.urls.length == 0)
+      installer.callbacks[callbackId] = null;

The InstallTrigger object itself (and any remaining callbacks on it) will be removed in the "unload" event, which is called when the tab is closed.

Comment 84

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/c8dc4dd369ee

Comment 85

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Backed out for potentially being the cause of Dromaeo regressions.

Comment 86

[Mike Beltzner [:beltzner, not reading bugmail]]

--> beta5 for Firefox 4.0 so we can tag and get the beta out there.

Also note that this apparently caused a SunSpider regression on Windows XP: http://mzl.la/b2XSji

Comment 87

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

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/bcac114bdace501a#

which has other patches too.

Comment 88

[Brian Crowder]

http://hg.mozilla.org/mozilla-central/rev/3d4ba021c4dd

Comment 89

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 466040 [details] [diff] [review]
Patch v16


>+function InstallTriggerManager() {
>+  this.installerIds = [];
>+  this.nextInstallerId = 0;
>+
>+  addMessageListener(MSG_INSTALL_CALLBACK, this);
>+
>+  addEventListener("DOMWindowCreated", this, false);
>+
>+  var self = this;
>+  addEventListener("unload", function() {
>+    // Clean up all references, to help gc work quickly
>+    for (var installerId in self.installerIds) {
>+      self.installerIds[installerId].callbacks = null;
>+      self.installerIds[installerId] = null;
>+    }
>+    self.installerIds = null;
>+  }, false);
>+}
Does the "unload" listener actually do something? unload event does not bubble, yet
the listener is for bubble phase. Also, which "unload" it should be handling?
All? including all the iframes?

Comment 90

[Olli Pettay [:smaug][[e-mail]]]

Ah, nevermind. I forgot that messageManager has its own unload.