Blair McBride [:Unfocused] (UNAVAILABLE) Reporter
	Description • 11 years ago

InstallTrigger is currently exposed to web content as a JS object with __exposeProps__ defined. Switching to using WebIDL is the way to do this going forward, and bring with it numerous guarantees such as safer handling of arguments.

	Blair McBride [:Unfocused] (UNAVAILABLE) Reporter
	Comment 1 • 11 years ago

Still trying to figure out some details of WebIDL...

	Peter Van der Beken [:peterv]
	Comment 2 • 11 years ago

It turns out that bug 863952 is going to take a while, and won't be easy to port to any branches. Instead you should do something like this for now, it'll make InstallTrigger work but without any static methods. After bug 863952 is fixed we can make it work with real static methods.

	Blair McBride [:Unfocused] (UNAVAILABLE) Reporter
	Comment 3 • 11 years ago

Update: Progressing, but its more involved than I expected. 

peterv's method for dealing with the WebIDL interface at least seem to be working nicely. Thanks :)

	Blair McBride [:Unfocused] (UNAVAILABLE) Reporter
	Comment 4 • 11 years ago

This is done in the sense that it works well on desktop, works exactly the same as far as content is concerned, passes all tests, and doesn't regress e10s. All of which felt like walking on glass in the dark.

However, there's the small side issue of it completely not working on mobile due to it thinking the factory isn't registered... and I have no idea why. But lets not hold up reviews any further on tiny details like one of our products not working!


(I'm totally gonna ask someone for any ideas on the mobile issue.)

	Masatoshi Kimura [:emk]
	Comment 5 • 11 years ago

(In reply to Blair McBride [:Unfocused] from comment #4)
> However, there's the small side issue of it completely not working on mobile
> due to it thinking the factory isn't registered... and I have no idea why.

InstallTrigger isn't exposed on b2g in the first place. Dunno about Android.

	Blair McBride [:Unfocused] (UNAVAILABLE) Reporter
	Comment 6 • 11 years ago

(In reply to Masatoshi Kimura [:emk] from comment #5)
> InstallTrigger isn't exposed on b2g in the first place. Dunno about Android.

Yea, sorry, when I say "mobile", I mean Android - where it is meant to work.

But now you mention it, there is a couple of fixups I need to do to ensure it's never exposed on b2g.

	Dave Townsend [:mossop]
	Comment 7 • 11 years ago

Comment on attachment 8346463 [details] [diff] [review]
Patch v0.99

Review of attachment 8346463 [details] [diff] [review]:
-----------------------------------------------------------------

Got far enough into this to spot a few problems with this patch. I may have an incomplete understanding of the latest state of e10s though so you may just shout me down.

I believe that e10s will have multiple DOM windows per process (even if we're one process per tab then that is still multiple DOM windows). So you're going to get callback collisions if two windows in the same process call InstallTrigger at the same time, each InstallTrigger will use the same callbackID for them. I'd actually really like to see a couple of tests added for simultaneous calls to InstallTrigger from two windows in the same tab and two different tabs regardless.

There isn't any code here that stops RemoteMediator listening for messages after the window goes away, sounds like a memory leak unless there is some weak referencing going on here I can't see.

InstallTriggerProperty won't be called until a page actually attempts to access InstallTrigger. This means that only processes where at least one window has accessed InstallTrigger will be listening for MSG_JAR_FLUSH, also there will be as many listeners for that message as there are windows that accessed InstallTrigger.

On the whole I'm not a fan of the two different mediators. It used to be that all the e10s APIs worked even in non-e10s. Has this changed? I'd rather see us dogfooding the message manager approach even in non-e10s, it reduces the code paths and so gives us better test coverage with nightly users and on tinderbox. I'm assuming there isn't a big perf cost to that.

::: toolkit/mozapps/extensions/addonManager.js
@@ +39,3 @@
>  
> +  // let messageManager = Cc["@mozilla.org/parentprocessmessagemanager;1"]
> +  //                        .getService(Ci.nsIMessageListenerManager);

No longer needed?

@@ +157,5 @@
>     * Listens to requests from child processes for InstallTrigger
>     * activity, and sends back callbacks.
>     */
>    receiveMessage: function AMC_receiveMessage(aMessage) {
> +    let payload = aMessage.data;

Yay for stable APIs!

::: toolkit/mozapps/extensions/amInstallTrigger.js
@@ +78,5 @@
> +RemoteMediator.prototype = {
> +  enabled: function() {
> +    return this.messageManager.sendSyncMessage(MSG_INSTALL_ENABLED, {
> +      mimetype: XPINSTALL_MIMETYPE,
> +      referer: this._url.spec

this._url doesn't seem to be defined anywhere

@@ +241,5 @@
> +      this._callbacks.delete(callbackID);
> +  },
> +
> +  classID: Components.ID("{9df8ef2b-94da-45c9-ab9f-132eb55fddf1}"),
> +  contractID: "@mozilla.org/addons/installtrigger;1",

Is this object even usable through XPCOM as-is?

@@ +252,5 @@
> +InstallTriggerProperty.prototype = {
> +  init: function(window) {
> +    return window.InstallTriggerImpl._create(window,
> +                                            new InstallTrigger(window));
> +  },

Seems odd to create an object per window to create another object per window. Can the property initialisation be merged into InstallTrigger itself?

	:Gijs (he/him) Assignee
	Comment 8 • 10 years ago

Unbitrot

	:Gijs (he/him) Assignee
	Comment 9 • 10 years ago

As I'm going through this...

(In reply to Dave Townsend [:mossop] from comment #7) 
> @@ +252,5 @@
> > +InstallTriggerProperty.prototype = {
> > +  init: function(window) {
> > +    return window.InstallTriggerImpl._create(window,
> > +                                            new InstallTrigger(window));
> > +  },
> 
> Seems odd to create an object per window to create another object per
> window. Can the property initialisation be merged into InstallTrigger itself?

Which do you propose merging with InstallTrigger, the InstallTriggerProperty object, or the InstallTriggerImpl (webidl) object?

AFAICT the latter isn't possible because of how static things and webidl work, but I /think/ the former is an option. Is that what you want, though?

	:Gijs (he/him) Assignee
	Comment 10 • 10 years ago

I've attempted to address a bunch of the feedback. Replies follow inline.

(In reply to Dave Townsend [:mossop] from comment #7)
> Comment on attachment 8346463 [details] [diff] [review]
> Patch v0.99
> 
> Review of attachment 8346463 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Got far enough into this to spot a few problems with this patch. I may have
> an incomplete understanding of the latest state of e10s though so you may
> just shout me down.
> 
> I believe that e10s will have multiple DOM windows per process (even if
> we're one process per tab then that is still multiple DOM windows). So
> you're going to get callback collisions if two windows in the same process
> call InstallTrigger at the same time, each InstallTrigger will use the same
> callbackID for them. I'd actually really like to see a couple of tests added
> for simultaneous calls to InstallTrigger from two windows in the same tab
> and two different tabs regardless.

Hrm. I'm not sure this is a new problem as the old implementation also used callback IDs. But perhaps you're saying that in the brave new e10s world, the multiple processes/instances might have different counters, and therefore might use the same ID, which would lead to sadness? Not 100% sure.

Also, in terms of tests for this, those would be mochitest-browser tests, or can we do this in some other flavour of tests, or...? :-)

> There isn't any code here that stops RemoteMediator listening for messages
> after the window goes away, sounds like a memory leak unless there is some
> weak referencing going on here I can't see.

I added a QI implementation and use addWeakMessageListener. AFAICT that should be enough. I'll push to try to see how badly this patch fares against the existing tests, also for other reasons...

> InstallTriggerProperty won't be called until a page actually attempts to
> access InstallTrigger. This means that only processes where at least one
> window has accessed InstallTrigger will be listening for MSG_JAR_FLUSH, also
> there will be as many listeners for that message as there are windows that
> accessed InstallTrigger.

I moved this listener to the addonsManager itself. I've not checked if it works correctly. Are there tests for this, and/or how would I do that? :-)

> On the whole I'm not a fan of the two different mediators. It used to be
> that all the e10s APIs worked even in non-e10s. Has this changed? I'd rather
> see us dogfooding the message manager approach even in non-e10s, it reduces
> the code paths and so gives us better test coverage with nightly users and
> on tinderbox. I'm assuming there isn't a big perf cost to that.

I've removed the non-remote version.


> ::: toolkit/mozapps/extensions/addonManager.js
> @@ +39,3 @@
> >  
> > +  // let messageManager = Cc["@mozilla.org/parentprocessmessagemanager;1"]
> > +  //                        .getService(Ci.nsIMessageListenerManager);
> 
> No longer needed?

Removed.

> ::: toolkit/mozapps/extensions/amInstallTrigger.js
> @@ +78,5 @@
> > +RemoteMediator.prototype = {
> > +  enabled: function() {
> > +    return this.messageManager.sendSyncMessage(MSG_INSTALL_ENABLED, {
> > +      mimetype: XPINSTALL_MIMETYPE,
> > +      referer: this._url.spec
> 
> this._url doesn't seem to be defined anywhere

Fixed.

> @@ +241,5 @@
> > +      this._callbacks.delete(callbackID);
> > +  },
> > +
> > +  classID: Components.ID("{9df8ef2b-94da-45c9-ab9f-132eb55fddf1}"),
> > +  contractID: "@mozilla.org/addons/installtrigger;1",
> 
> Is this object even usable through XPCOM as-is?

It is now, ish. See below.

> @@ +252,5 @@
> > +InstallTriggerProperty.prototype = {
> > +  init: function(window) {
> > +    return window.InstallTriggerImpl._create(window,
> > +                                            new InstallTrigger(window));
> > +  },
> 
> Seems odd to create an object per window to create another object per
> window. Can the property initialisation be merged into InstallTrigger itself?

I've merged the InstallTriggerProperty object into InstallTrigger. This means that the JS global property category registration will call the constructor without a window, in which case it gets a small object with an init method that produces the actual value to put on the window, by calling the constructor /with/ the window, and doing the webidl magic. I'm not convinced this is neater/better, but it seems to work in a bit of cursory testing. Can you confirm this is what you meant (or tell me to revert it again? :-) ). Thanks!

Off to try (linux only for now because I assume there will be more issues, and at least linux isn't backlogged for builds):
remote:   https://tbpl.mozilla.org/?tree=Try&rev=b850db83bf84

	Dave Townsend [:mossop]
	Comment 11 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #9)
> As I'm going through this...
> 
> (In reply to Dave Townsend [:mossop] from comment #7) 
> > @@ +252,5 @@
> > > +InstallTriggerProperty.prototype = {
> > > +  init: function(window) {
> > > +    return window.InstallTriggerImpl._create(window,
> > > +                                            new InstallTrigger(window));
> > > +  },
> > 
> > Seems odd to create an object per window to create another object per
> > window. Can the property initialisation be merged into InstallTrigger itself?
> 
> Which do you propose merging with InstallTrigger, the InstallTriggerProperty
> object, or the InstallTriggerImpl (webidl) object?
> 
> AFAICT the latter isn't possible because of how static things and webidl
> work, but I /think/ the former is an option. Is that what you want, though?

Yes. From my understanding for every window the original patch would create a new InstallTriggerProperty instance, init it with a window, which would then create a new InstallTrigger instance to serve as the property. I don't see a reason why that is necessary, instead just have one object.

	:Gijs (he/him) Assignee
	Comment 12 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #10)
> Created attachment 8418029 [details] [diff] [review]
> Patch v1.1

> Off to try (linux only for now because I assume there will be more issues,
> and at least linux isn't backlogged for builds):
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=b850db83bf84

This is breaking pretty epically. Funnily enough, I just tried to run this locally (admittedly on OS X) and it breaks with a different error (NS_ERROR_NOINTERFACE for the frame messagemanager getting from the docshell). Clobbering at the minute to see what's going on here. I wonder if this interface isn't available on non-e10s, and that was why we needed the double mediators... but http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIMessageManager.idl#82 seems to imply that it should be there, and some (but not all...?!) of the mochitest-e10s runs show the same error.

	Dave Townsend [:mossop]
	Comment 13 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #10)
> Created attachment 8418029 [details] [diff] [review]
> Patch v1.1
> 
> I've attempted to address a bunch of the feedback. Replies follow inline.
> 
> (In reply to Dave Townsend [:mossop] from comment #7)
> > Comment on attachment 8346463 [details] [diff] [review]
> > Patch v0.99
> > 
> > Review of attachment 8346463 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Got far enough into this to spot a few problems with this patch. I may have
> > an incomplete understanding of the latest state of e10s though so you may
> > just shout me down.
> > 
> > I believe that e10s will have multiple DOM windows per process (even if
> > we're one process per tab then that is still multiple DOM windows). So
> > you're going to get callback collisions if two windows in the same process
> > call InstallTrigger at the same time, each InstallTrigger will use the same
> > callbackID for them. I'd actually really like to see a couple of tests added
> > for simultaneous calls to InstallTrigger from two windows in the same tab
> > and two different tabs regardless.
> 
> Hrm. I'm not sure this is a new problem as the old implementation also used
> callback IDs. But perhaps you're saying that in the brave new e10s world,
> the multiple processes/instances might have different counters, and
> therefore might use the same ID, which would lead to sadness? Not 100% sure.

You're right, looks like the existing code probably has the same problem.

> Also, in terms of tests for this, those would be mochitest-browser tests, or
> can we do this in some other flavour of tests, or...? :-)

mochitest-browser is likely the easiest option. I'd suggest the two windows being from different domains. I'm not sure what the plan is w.r.t. the number of child processes that e10s will use. It might be just 1 for now in which case we don't need to make this work for more than 1, but I'd still like us to have a test there that will fail if that number increases later on.

> > InstallTriggerProperty won't be called until a page actually attempts to
> > access InstallTrigger. This means that only processes where at least one
> > window has accessed InstallTrigger will be listening for MSG_JAR_FLUSH, also
> > there will be as many listeners for that message as there are windows that
> > accessed InstallTrigger.
> 
> I moved this listener to the addonsManager itself. I've not checked if it
> works correctly. Are there tests for this, and/or how would I do that? :-)

I'd say you'll need to load a frame script into content processes and listen for the observer notification that we expect that message to trigger. Sounds sort of sucky to do :(

> > @@ +252,5 @@
> > > +InstallTriggerProperty.prototype = {
> > > +  init: function(window) {
> > > +    return window.InstallTriggerImpl._create(window,
> > > +                                            new InstallTrigger(window));
> > > +  },
> > 
> > Seems odd to create an object per window to create another object per
> > window. Can the property initialisation be merged into InstallTrigger itself?
> 
> I've merged the InstallTriggerProperty object into InstallTrigger. This
> means that the JS global property category registration will call the
> constructor without a window, in which case it gets a small object with an
> init method that produces the actual value to put on the window, by calling
> the constructor /with/ the window, and doing the webidl magic. I'm not
> convinced this is neater/better, but it seems to work in a bit of cursory
> testing. Can you confirm this is what you meant (or tell me to revert it
> again? :-) ). Thanks!

I'm very confused about why this is necessary. Why doesn't this work:

function InstallTrigger() {
  // stuff
}

InstallTrigger.prototype = {
  init: function(window) {
    this._window = window;
    return window.InstallTriggerImpl._create(window, this);
  },

  // Rest of the implementation
}

	:Gijs (he/him) Assignee
	Comment 14 • 10 years ago

Comment on attachment 8418029 [details] [diff] [review]
Patch v1.1

Thanks for the quick feedback!

(In reply to Dave Townsend [:mossop] from comment #13)
> (In reply to :Gijs Kruitbosch from comment #10)
> > Created attachment 8418029 [details] [diff] [review]
> > Patch v1.1
> > 
> > I've attempted to address a bunch of the feedback. Replies follow inline.
> > 
> > (In reply to Dave Townsend [:mossop] from comment #7)
> > > Comment on attachment 8346463 [details] [diff] [review]
> > > Patch v0.99
> > > 
> > > Review of attachment 8346463 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Got far enough into this to spot a few problems with this patch. I may have
> > > an incomplete understanding of the latest state of e10s though so you may
> > > just shout me down.
> > > 
> > > I believe that e10s will have multiple DOM windows per process (even if
> > > we're one process per tab then that is still multiple DOM windows). So
> > > you're going to get callback collisions if two windows in the same process
> > > call InstallTrigger at the same time, each InstallTrigger will use the same
> > > callbackID for them. I'd actually really like to see a couple of tests added
> > > for simultaneous calls to InstallTrigger from two windows in the same tab
> > > and two different tabs regardless.
> > 
> > Hrm. I'm not sure this is a new problem as the old implementation also used
> > callback IDs. But perhaps you're saying that in the brave new e10s world,
> > the multiple processes/instances might have different counters, and
> > therefore might use the same ID, which would lead to sadness? Not 100% sure.
> 
> You're right, looks like the existing code probably has the same problem.
> 
> > Also, in terms of tests for this, those would be mochitest-browser tests, or
> > can we do this in some other flavour of tests, or...? :-)
> 
> mochitest-browser is likely the easiest option. I'd suggest the two windows
> being from different domains. I'm not sure what the plan is w.r.t. the
> number of child processes that e10s will use. It might be just 1 for now in
> which case we don't need to make this work for more than 1, but I'd still
> like us to have a test there that will fail if that number increases later
> on.

Right. I might need handholding when it gets to this, but if this isn't a new issue, would you mind if I file a followup for that so we can first focus on getting the webidl conversion stuff fixed and shipped?

> > > InstallTriggerProperty won't be called until a page actually attempts to
> > > access InstallTrigger. This means that only processes where at least one
> > > window has accessed InstallTrigger will be listening for MSG_JAR_FLUSH, also
> > > there will be as many listeners for that message as there are windows that
> > > accessed InstallTrigger.
> > 
> > I moved this listener to the addonsManager itself. I've not checked if it
> > works correctly. Are there tests for this, and/or how would I do that? :-)
> 
> I'd say you'll need to load a frame script into content processes and listen
> for the observer notification that we expect that message to trigger. Sounds
> sort of sucky to do :(

I can probably try to make it work, although I wouldn't mind doing a followup. What I really meant though, was, "how can I check I didn't break this bit of the addons manager", ie, what is that message /for/ and is there some way I can (manually or automatically) check that it's still doing its job and I've not just started dropping the messages on the floor? :-)

(I'm guessing this has to do with flushing jar caches when installing updates for add-ons, but I'd like to understand a bit better)

> > > @@ +252,5 @@
> > > > +InstallTriggerProperty.prototype = {
> > > > +  init: function(window) {
> > > > +    return window.InstallTriggerImpl._create(window,
> > > > +                                            new InstallTrigger(window));
> > > > +  },
> > > 
> > > Seems odd to create an object per window to create another object per
> > > window. Can the property initialisation be merged into InstallTrigger itself?
> > 
> > I've merged the InstallTriggerProperty object into InstallTrigger. This
> > means that the JS global property category registration will call the
> > constructor without a window, in which case it gets a small object with an
> > init method that produces the actual value to put on the window, by calling
> > the constructor /with/ the window, and doing the webidl magic. I'm not
> > convinced this is neater/better, but it seems to work in a bit of cursory
> > testing. Can you confirm this is what you meant (or tell me to revert it
> > again? :-) ). Thanks!
> 
> I'm very confused about why this is necessary. Why doesn't this work:
> 
> function InstallTrigger() {
>   // stuff
> }
> 
> InstallTrigger.prototype = {
>   init: function(window) {
>     this._window = window;
>     return window.InstallTriggerImpl._create(window, this);
>   },
> 
>   // Rest of the implementation
> }

I think you're right, it should be possible to do something simpler than this (note to self: bug 983845 comment #10 and its commits might help). I will try to poke at this more and come back with a more straightforward patch.

	juan becerra [:juanb]
	Comment 15 • 10 years ago

I can't think of a specific use case to test this, so qa-'ing this. If there's something you could point us at for testing, let us know.

	:Gijs (he/him) Assignee
	Comment 16 • 10 years ago

So this is what I made up yesterday, but I'm not sure I'm happy with it. The problem with this approach is that I'm pretty sure the callback objects (and therefore the callback functions, and therefore their windows) will leak if the addon manager doesn't reply (it nulls out the callback if add-on installs are not allowed by default from that domain, AFAICT), and probably also if the window goes away before the callback arrives and the cpmm never gets a callback. Try run is here: https://tbpl.mozilla.org/?tree=Try&rev=866453ce3aa6 and still shows a /lot/ of orange, too. Going to try to use the contentframe message manager after all, and (at least for now) just no-op if we're not content (this will break chrome window InstallTrigger, but Dave said that was probably OK when discussed on IRC).

	Peter Van der Beken [:peterv]
	Comment 17 • 10 years ago

(In reply to Dave Townsend [:mossop] from comment #11)
> Yes. From my understanding for every window the original patch would create
> a new InstallTriggerProperty instance, init it with a window, which would
> then create a new InstallTrigger instance to serve as the property. I don't
> see a reason why that is necessary, instead just have one object.

I think I just did it that way so that it'll be cleaner to remove the hack when we do the proper fix for static methods/properties.

	:Gijs (he/him) Assignee
	Comment 18 • 10 years ago

So I managed to cook up a way to use the current listeners which I /think/ works fine and doesn't leak and so on. I also fixed a bunch of test failures from my previous try push(es). Now also checking android/b2g for stuff:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=0a216eabd11f

Blair mentioned that he had an updated patch in his queue, and looking at that and discussing briefly with him, I think we need a separate content script still for the jar flush after all, so I incorporated that.

Peter, I changed test_interfaces.html because this being a webidl thing means it now no longer needs the xbl:false property in there, AIUI. That test has lots of comments I need to check with a DOM peer. Can you clarify if that is correct and/or rs just that change? I think it is inherent in the fact that we're switching to webidl, so I don't expect this necessity to change in later iterations of the patch.

( https://hg.mozilla.org/try/diff/9c5a4d793c07/dom/tests/mochitest/general/test_interfaces.html )


I'm still planning to look at the option of using contentframemessagelisteners and nulling the object on windows that don't have it (which is what Blair did in his newer version of the patch).

	:Gijs (he/him) Assignee
	Comment 19 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #18)
> So I managed to cook up a way to use the current listeners which I /think/
> works fine and doesn't leak and so on. I also fixed a bunch of test failures
> from my previous try push(es). Now also checking android/b2g for stuff:
> 
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=0a216eabd11f

We shouldn't try to have this interface on b2g, repushed making it conditional on non-b2g:

https://tbpl.mozilla.org/?tree=Try&rev=3e1e8da6dbd0

I'll upload the current patch in a second and request feedback on that.

	Dave Townsend [:mossop]
	Comment 21 • 10 years ago

Comment on attachment 8419022 [details] [diff] [review]
Patch v1.3

Review of attachment 8419022 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking pretty close to done, most of the remains are minor.

With WebIDL protecting the property accesses presumably you could also merge the RemoteMediator methods into InstallTrigger but I think there's a clear semantic separation between the two so it seems reasonable to keep them separate.

::: toolkit/mozapps/extensions/addonManager.js
@@ +191,5 @@
> +          payload.icons, callback);
> +      case MSG_JAR_FLUSH:
> +        let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> +        file.initWithPath(payload);
> +        Services.obs.notifyObservers(file, "flush-cache-entry", null);

I don't understand what this section is doing here. This component should only be used in the main process, and XPIProvider.jsm already notifies observers there.

::: toolkit/mozapps/extensions/amInstallTrigger.js
@@ +23,5 @@
> +
> +function CallbackObject(id, callback, urls, mediator) {
> +  this.id = id;
> +  this.callback = callback;
> +  this.urls = new Set(urls);

I don't know why anyone would do it but it is technically possible to pass the same url multiple times so we'd end up not calling the callback for one of those with a set. I'm not sure if I care about that

@@ +44,5 @@
> +  this._lastCallbackID = 0;
> +  this._callbacks = new Map();
> +  this.mm = Cc["@mozilla.org/childprocessmessagemanager;1"]
> +            .getService(Ci.nsISyncMessageSender);
> +  this.mm.addWeakMessageListener(MSG_INSTALL_CALLBACK, this);

I'm wondering what happens if the user navigates away from the page after the install starts but before the callback is called. GC might not have happened and so this mediator might still be alive which leaves us attempting to call a callback in a closed window. Hopefully that just fails but maybe it would be better to be safe and add a "pagehide" listener on the window and clear all the callbacks immediately. I wouldn't be surprised to learn that the current code has similar problems and it can certainly be a follow-up.

@@ +78,5 @@
> +  _addCallback: function(callback, urls) {
> +    if (!callback || typeof callback != "function")
> +      return -1;
> +
> +    let callbackID = this._windowID + "-" + ++this._lastCallbackID;

Nice!

@@ +90,5 @@
> +  ensureRegistered: function() {
> +    if (!this._registered) {
> +      this._registered = true;
> +    }
> +  },

Not sure what this is for

@@ +100,5 @@
> +}
> +
> +InstallTrigger.prototype = {
> +  init: function(window) {
> +    this._lastcallbackID = 0;

Unused now

@@ +108,5 @@
> +    this._url = window.document.documentURIObject;
> +
> +    window.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
> +    let utils = window.getInterface(Components.interfaces.nsIDOMWindowUtils)
> +    this._mediator = new RemoteMediator(utils.outerWindowID);

Do we create one InstallTrigger per outer-window or per inner-window? I suspect the latter in which case you want to use the innerWindowID here.

@@ +163,5 @@
> +      installData.names.push(name);
> +      installData.icons.push(iconUrl ? iconUrl.spec : null);
> +    }
> +
> +    let callbackID = this._mediator._addCallback(callback, installData.uris);

Any reason to call this here instead of just from inside mediator.install?

::: tools/quitter/chrome.manifest
@@ -3,2 @@
>  contract @mozilla.org/quitter-observer;1 {c235a986-5ac1-4f28-ad73-825dae9bad90}
> -category profile-after-change @mozilla.org/quitter-observer;1 @mozilla.org/quitter-observer;1

Not sure what these changes are doing here.

	Dave Townsend [:mossop]
	Comment 22 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #14)
> Comment on attachment 8418029 [details] [diff] [review]
> Patch v1.1
> 
> Thanks for the quick feedback!
> 
> (In reply to Dave Townsend [:mossop] from comment #13)
> > (In reply to :Gijs Kruitbosch from comment #10)
> > > Created attachment 8418029 [details] [diff] [review]
> > > Patch v1.1
> > > 
> > > I've attempted to address a bunch of the feedback. Replies follow inline.
> > > 
> > > (In reply to Dave Townsend [:mossop] from comment #7)
> > > > Comment on attachment 8346463 [details] [diff] [review]
> > > > Patch v0.99
> > > > 
> > > > Review of attachment 8346463 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > Got far enough into this to spot a few problems with this patch. I may have
> > > > an incomplete understanding of the latest state of e10s though so you may
> > > > just shout me down.
> > > > 
> > > > I believe that e10s will have multiple DOM windows per process (even if
> > > > we're one process per tab then that is still multiple DOM windows). So
> > > > you're going to get callback collisions if two windows in the same process
> > > > call InstallTrigger at the same time, each InstallTrigger will use the same
> > > > callbackID for them. I'd actually really like to see a couple of tests added
> > > > for simultaneous calls to InstallTrigger from two windows in the same tab
> > > > and two different tabs regardless.
> > > 
> > > Hrm. I'm not sure this is a new problem as the old implementation also used
> > > callback IDs. But perhaps you're saying that in the brave new e10s world,
> > > the multiple processes/instances might have different counters, and
> > > therefore might use the same ID, which would lead to sadness? Not 100% sure.
> > 
> > You're right, looks like the existing code probably has the same problem.
> > 
> > > Also, in terms of tests for this, those would be mochitest-browser tests, or
> > > can we do this in some other flavour of tests, or...? :-)
> > 
> > mochitest-browser is likely the easiest option. I'd suggest the two windows
> > being from different domains. I'm not sure what the plan is w.r.t. the
> > number of child processes that e10s will use. It might be just 1 for now in
> > which case we don't need to make this work for more than 1, but I'd still
> > like us to have a test there that will fail if that number increases later
> > on.
> 
> Right. I might need handholding when it gets to this, but if this isn't a
> new issue, would you mind if I file a followup for that so we can first
> focus on getting the webidl conversion stuff fixed and shipped?

I'm wary of making this a follow-up bug since they tend to get forgotten over time and I think this is a pretty simple test to write. It just needs to open two tabs from different domains, click a link in each which starts the installs, accepts the confirmation dialogs when they pop-up then after installs have completed check that each page's callback saw the correct url.

> > > > InstallTriggerProperty won't be called until a page actually attempts to
> > > > access InstallTrigger. This means that only processes where at least one
> > > > window has accessed InstallTrigger will be listening for MSG_JAR_FLUSH, also
> > > > there will be as many listeners for that message as there are windows that
> > > > accessed InstallTrigger.
> > > 
> > > I moved this listener to the addonsManager itself. I've not checked if it
> > > works correctly. Are there tests for this, and/or how would I do that? :-)
> > 
> > I'd say you'll need to load a frame script into content processes and listen
> > for the observer notification that we expect that message to trigger. Sounds
> > sort of sucky to do :(
> 
> I can probably try to make it work, although I wouldn't mind doing a
> followup. What I really meant though, was, "how can I check I didn't break
> this bit of the addons manager", ie, what is that message /for/ and is there
> some way I can (manually or automatically) check that it's still doing its
> job and I've not just started dropping the messages on the floor? :-)
> 
> (I'm guessing this has to do with flushing jar caches when installing
> updates for add-ons, but I'd like to understand a bit better)

The jar protocol caches nsIZipReader instances for requested resources, so it doesn't need to open a new one everytime you request a resource from the same file. So if we attempt to load a resource from an extension (a script, an icon etc.), then we install an update to that extension attempting to load the resource again will try to load from the cached nsIZipReader and so get the old version. So when uninstalling an extension we flush the jar cache for its XPI to make sure that doesn't happen.

	:Gijs (he/him) Assignee
	Comment 23 • 10 years ago

(In reply to Dave Townsend [:mossop] from comment #21)
> Comment on attachment 8419022 [details] [diff] [review]
> Patch v1.3
> 
> Review of attachment 8419022 [details] [diff] [review]:

Thanks! Most of this is just stuff I neglected to clean up after refactor upon refactor, sorry. :-\

> 
> This is looking pretty close to done, most of the remains are minor.
> 
> With WebIDL protecting the property accesses presumably you could also merge
> the RemoteMediator methods into InstallTrigger but I think there's a clear
> semantic separation between the two so it seems reasonable to keep them
> separate.

Yes, I'd like to keep them separate, in part because it makes experimenting with other message managers relatively easy, in part because this way, the mediator itself doesn't need to keep a window reference around, which seems saner from a GC perspective (although I suppose that's mostly irrational paranoia as the function refs it keeps in its callback table will likely have a window ref, so meh).

> ::: toolkit/mozapps/extensions/addonManager.js
> @@ +191,5 @@
> > +          payload.icons, callback);
> > +      case MSG_JAR_FLUSH:
> > +        let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> > +        file.initWithPath(payload);
> > +        Services.obs.notifyObservers(file, "flush-cache-entry", null);
> 
> I don't understand what this section is doing here. This component should
> only be used in the main process, and XPIProvider.jsm already notifies
> observers there.

I forgot to remove this. :-\

> ::: toolkit/mozapps/extensions/amInstallTrigger.js
> @@ +23,5 @@
> > +
> > +function CallbackObject(id, callback, urls, mediator) {
> > +  this.id = id;
> > +  this.callback = callback;
> > +  this.urls = new Set(urls);
> 
> I don't know why anyone would do it but it is technically possible to pass
> the same url multiple times so we'd end up not calling the callback for one
> of those with a set. I'm not sure if I care about that

Yeah, I'm going to go and say that that is strange and not supported, if that's OK. indexOf is ugly, Set is the new hotness, whatever. :-)

> @@ +44,5 @@
> > +  this._lastCallbackID = 0;
> > +  this._callbacks = new Map();
> > +  this.mm = Cc["@mozilla.org/childprocessmessagemanager;1"]
> > +            .getService(Ci.nsISyncMessageSender);
> > +  this.mm.addWeakMessageListener(MSG_INSTALL_CALLBACK, this);
> 
> I'm wondering what happens if the user navigates away from the page after
> the install starts but before the callback is called. GC might not have
> happened and so this mediator might still be alive which leaves us
> attempting to call a callback in a closed window. Hopefully that just fails
> but maybe it would be better to be safe and add a "pagehide" listener on the
> window and clear all the callbacks immediately. I wouldn't be surprised to
> learn that the current code has similar problems and it can certainly be a
> follow-up.

I will graciously accept the suggestion of a followup. :-)


> @@ +90,5 @@
> > +  ensureRegistered: function() {
> > +    if (!this._registered) {
> > +      this._registered = true;
> > +    }
> > +  },
> 
> Not sure what this is for

More cleanup I forgot...

> @@ +108,5 @@
> > +    this._url = window.document.documentURIObject;
> > +
> > +    window.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
> > +    let utils = window.getInterface(Components.interfaces.nsIDOMWindowUtils)
> > +    this._mediator = new RemoteMediator(utils.outerWindowID);
> 
> Do we create one InstallTrigger per outer-window or per inner-window? I
> suspect the latter in which case you want to use the innerWindowID here.

Yeah, so, this is where I need help. Where do I find out about this outer/inner window distinction? When are there more inner windows for a single outer window (which seems to be what's taken as a given in your comment). I just read the IDL which said "innerWindowID throws, outerWindowID never throws", and picked the thing that didn't throw! ;-)

(I should note that I debugged some bits locally, and the outerWindowID in the callbackIDs seemed to be different per tab...)

> ::: tools/quitter/chrome.manifest
> @@ -3,2 @@
> >  contract @mozilla.org/quitter-observer;1 {c235a986-5ac1-4f28-ad73-825dae9bad90}
> > -category profile-after-change @mozilla.org/quitter-observer;1 @mozilla.org/quitter-observer;1
> 
> Not sure what these changes are doing here.

bug 1006541 :-(

	Dave Townsend [:mossop]
	Comment 24 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #23)
> (In reply to Dave Townsend [:mossop] from comment #21)
> > @@ +108,5 @@
> > > +    this._url = window.document.documentURIObject;
> > > +
> > > +    window.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
> > > +    let utils = window.getInterface(Components.interfaces.nsIDOMWindowUtils)
> > > +    this._mediator = new RemoteMediator(utils.outerWindowID);
> > 
> > Do we create one InstallTrigger per outer-window or per inner-window? I
> > suspect the latter in which case you want to use the innerWindowID here.
> 
> Yeah, so, this is where I need help. Where do I find out about this
> outer/inner window distinction? When are there more inner windows for a
> single outer window (which seems to be what's taken as a given in your
> comment). I just read the IDL which said "innerWindowID throws,
> outerWindowID never throws", and picked the thing that didn't throw! ;-)

https://developer.mozilla.org/en-US/docs/Inner_and_outer_windows

> (I should note that I debugged some bits locally, and the outerWindowID in
> the callbackIDs seemed to be different per tab...)

Yes, the outerWindowID will be different for each different tab. But that tab has the same outerWindowID for the lifetime of the tab but each page loaded in that tab gets its own innerWindowID.

I would guess that innerWindowID can throw when the outer window doesn't yet have an inner window loaded in it. I don't think that could ever be the case when InstallTrigger is being called.

	:Gijs (he/him) Assignee
	Comment 25 • 10 years ago

(In reply to Dave Townsend [:mossop] from comment #22)
> > > > Also, in terms of tests for this, those would be mochitest-browser tests, or
> > > > can we do this in some other flavour of tests, or...? :-)
> > > 
> > > mochitest-browser is likely the easiest option. I'd suggest the two windows
> > > being from different domains. I'm not sure what the plan is w.r.t. the
> > > number of child processes that e10s will use. It might be just 1 for now in
> > > which case we don't need to make this work for more than 1, but I'd still
> > > like us to have a test there that will fail if that number increases later
> > > on.
> > 
> > Right. I might need handholding when it gets to this, but if this isn't a
> > new issue, would you mind if I file a followup for that so we can first
> > focus on getting the webidl conversion stuff fixed and shipped?
> 
> I'm wary of making this a follow-up bug since they tend to get forgotten
> over time and I think this is a pretty simple test to write. It just needs
> to open two tabs from different domains, click a link in each which starts
> the installs, accepts the confirmation dialogs when they pop-up then after
> installs have completed check that each page's callback saw the correct url.

Do we run these w/ e10s enabled on tinderbox yet? I just see mochitest plain runs with e10s enabled...

	:Gijs (he/him) Assignee
	Comment 26 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #19)
> (In reply to :Gijs Kruitbosch from comment #18)
> > So I managed to cook up a way to use the current listeners which I /think/
> > works fine and doesn't leak and so on. I also fixed a bunch of test failures
> > from my previous try push(es). Now also checking android/b2g for stuff:
> > 
> > remote:   https://tbpl.mozilla.org/?tree=Try&rev=0a216eabd11f
> 
> We shouldn't try to have this interface on b2g, repushed making it
> conditional on non-b2g:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=3e1e8da6dbd0

Ugh. So the widget toolkit == gonk detection here isn't right, because it's still causing failures on b2g desktop builds. I... don't know how to detect this situation correctly in a moz.build file, and 15 minutes of code-spelunking and search-the-webbing hasn't turned up anything useful. Should I use MOZ_B2G instead?

I'm generally kind of confused, because the toolkit bits use "#ifndef MOZ_WIDGET_GONK", and that seems to always have worked. I'm assuming a moz.build file going:

 if MOZ_WIDGET_TOOLKIT != 'gonk':

is the same, so I don't understand why my moz.build change in dom/webidl doesn't seem to have had the desired effect...

Vicamo, can you help? (blame points to bug 920551...)

	Dave Townsend [:mossop]
	Comment 27 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #25)
> (In reply to Dave Townsend [:mossop] from comment #22)
> > > > > Also, in terms of tests for this, those would be mochitest-browser tests, or
> > > > > can we do this in some other flavour of tests, or...? :-)
> > > > 
> > > > mochitest-browser is likely the easiest option. I'd suggest the two windows
> > > > being from different domains. I'm not sure what the plan is w.r.t. the
> > > > number of child processes that e10s will use. It might be just 1 for now in
> > > > which case we don't need to make this work for more than 1, but I'd still
> > > > like us to have a test there that will fail if that number increases later
> > > > on.
> > > 
> > > Right. I might need handholding when it gets to this, but if this isn't a
> > > new issue, would you mind if I file a followup for that so we can first
> > > focus on getting the webidl conversion stuff fixed and shipped?
> > 
> > I'm wary of making this a follow-up bug since they tend to get forgotten
> > over time and I think this is a pretty simple test to write. It just needs
> > to open two tabs from different domains, click a link in each which starts
> > the installs, accepts the confirmation dialogs when they pop-up then after
> > installs have completed check that each page's callback saw the correct url.
> 
> Do we run these w/ e10s enabled on tinderbox yet? I just see mochitest plain
> runs with e10s enabled...

We don't, but when we get serious about shipping e10s we will. But by then we'll have forgotten all about this.

	Vicamo Yang [:vicamo][:vyang]
	Comment 28 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #26)
> Ugh. So the widget toolkit == gonk detection here isn't right, because it's
> still causing failures on b2g desktop builds. I... don't know how to detect
> this situation correctly in a moz.build file, and 15 minutes of
> code-spelunking and search-the-webbing hasn't turned up anything useful.
> Should I use MOZ_B2G instead?

Yes.  B2G runs on two platforms, Gonk and Linux desktop.  MOZ_WIDGET_GONK is only for B2G running on Gonk.

	Bobby Holley (:bholley)
	Comment 29 • 10 years ago

Comment on attachment 8419022 [details] [diff] [review]
Patch v1.3

Talked with Gijs about this on IRC. He's going to finish up the other comments, and then I'll review the binding bits.

	:Gijs (he/him) Assignee
	Comment 30 • 10 years ago

I hope/believe this is ready for review. Try push: https://tbpl.mozilla.org/?tree=Try&rev=f94b509b9567

I'd like to followup the jar flush check (which shouldn't really materially change with this patch because we're still using a frame script the way we were before, albeit with a different name), and checking for closed window callbacks. Note that it seems we already have some tests for the latter still doing installs in http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpinstall/browser_navigateaway.js ...

	:Gijs (he/him) Assignee
	Comment 31 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #30)
> Created attachment 8419798 [details] [diff] [review]
> Patch v1.4
> 
> I hope/believe this is ready for review. Try push:
> https://tbpl.mozilla.org/?tree=Try&rev=f94b509b9567

This is reasonably green, but it still breaks b2g desktop. So it turns out that MOZ_B2G is wrong - I really should be using gonk here, because the add-ons manager is apparently available for the packaged linux builds:

http://mxr.mozilla.org/mozilla-central/source/b2g/installer/package-manifest.in#428

and that's what needs modifying.

	:Gijs (he/him) Assignee
	Comment 32 • 10 years ago

Tweaking b2g config stuff: https://tbpl.mozilla.org/?tree=Try&rev=b597418d36dc

	:Gijs (he/him) Assignee
	Comment 33 • 10 years ago

Comment on attachment 8420066 [details] [diff] [review]
Use WebIDL to expose InstallTrigger,

And r?peterv for the dom test_interfaces changes.

	:Gijs (he/him) Assignee
	Comment 34 • 10 years ago

Comment on attachment 8420066 [details] [diff] [review]
Use WebIDL to expose InstallTrigger,

Err, except bholley is a DOM peer, so, meh! :-)

	Dave Townsend [:mossop]
	Comment 35 • 10 years ago

Comment on attachment 8420066 [details] [diff] [review]
Use WebIDL to expose InstallTrigger,

Review of attachment 8420066 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks good but I think the test needs a couple of changes.

::: toolkit/mozapps/extensions/test/xpinstall/browser_concurrent_installs.js
@@ +22,5 @@
> +  },
> +  onCloseWindow: function(win) {
> +    var window = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindow);
> +    if (window.document.location.href == XPINSTALL_URL && gQueuedForInstall.length > 0) {
> +      installNext();

I think this will trigger the second install after the first has complete so you aren't really testing concurrent installs here.

@@ +88,5 @@
> +  is(gResults.length, 2, "Should have two urls");
> +  isnot(gResults[0].loc, gResults[1].loc, "Should not have results from the same page.");
> +  isnot(gResults[0].xpi, gResults[1].xpi, "Should not have the same XPIs.");
> +  let org = gResults[0].loc.contains("example.org") ? 0 : 1;
> +  let com = org == 0 ? 1 : 0;

I'd prefer it if you verify that the example.com result is actually in the array rather than assuming it.

@@ +89,5 @@
> +  isnot(gResults[0].loc, gResults[1].loc, "Should not have results from the same page.");
> +  isnot(gResults[0].xpi, gResults[1].xpi, "Should not have the same XPIs.");
> +  let org = gResults[0].loc.contains("example.org") ? 0 : 1;
> +  let com = org == 0 ? 1 : 0;
> +  is(/example.com/.test(gResults[com].loc), /example.com/.test(gResults[com].xpi), "Should get .com XPI for .org loc");

"Should get .com XPI for .com loc"

@@ +93,5 @@
> +  is(/example.com/.test(gResults[com].loc), /example.com/.test(gResults[com].xpi), "Should get .com XPI for .org loc");
> +  is(/example.org/.test(gResults[org].loc), /example.org/.test(gResults[org].xpi), "Should get .org XPI for .org loc");
> +
> +  Services.perms.remove("example.com", "install");
> +  Services.perms.remove("example.org", "install");

These probably want to be in the cleanup function

::: toolkit/mozapps/extensions/test/xpinstall/concurrent_installs.html
@@ +7,5 @@
> +  <meta charset="utf-8">
> +<title>Concurrent InstallTrigger tests</title>
> +<script type="text/javascript">
> +function installCallback(url, status) {
> +  debugger;

Still needed?

@@ +13,5 @@
> +  document.getElementById("status").textContent = status;
> +}
> +
> +function startInstall() {
> +  console.error(location.href);

ditto

	:Gijs (he/him) Assignee
	Comment 36 • 10 years ago

(In reply to Dave Townsend [:mossop] from comment #35)
> Comment on attachment 8420066 [details] [diff] [review]
> Use WebIDL to expose InstallTrigger,
> 
> Review of attachment 8420066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code looks good but I think the test needs a couple of changes.
> 
> ::: toolkit/mozapps/extensions/test/xpinstall/browser_concurrent_installs.js
> @@ +22,5 @@
> > +  },
> > +  onCloseWindow: function(win) {
> > +    var window = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindow);
> > +    if (window.document.location.href == XPINSTALL_URL && gQueuedForInstall.length > 0) {
> > +      installNext();
> 
> I think this will trigger the second install after the first has complete so
> you aren't really testing concurrent installs here.

So, in my testing at least, because the callbacks are async, if the IDs conflict, the test still fails as expected, because from the closing of the window, the install hasn't completed yet.

With the dialog functioning, there's no way to have "real" concurrent installs, as far as I can tell - only one of the two dialogs can be open and focused at the same time, and my attempts to run both installs in parallel (effectively just running a .forEach() on the list of tabs and clicking both links 'sync' one after the other) pops up two install permission dialogs one after the other, but the concurrent waitForFocus calls don't work (and even manual concoctions with focusing / clicking didn't work). So I'm unsure how you want me to make the installs "more concurrent" ? :-)

	Dave Townsend [:mossop]
	Comment 37 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #36)
> (In reply to Dave Townsend [:mossop] from comment #35)
> > Comment on attachment 8420066 [details] [diff] [review]
> > Use WebIDL to expose InstallTrigger,
> > 
> > Review of attachment 8420066 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The code looks good but I think the test needs a couple of changes.
> > 
> > ::: toolkit/mozapps/extensions/test/xpinstall/browser_concurrent_installs.js
> > @@ +22,5 @@
> > > +  },
> > > +  onCloseWindow: function(win) {
> > > +    var window = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindow);
> > > +    if (window.document.location.href == XPINSTALL_URL && gQueuedForInstall.length > 0) {
> > > +      installNext();
> > 
> > I think this will trigger the second install after the first has complete so
> > you aren't really testing concurrent installs here.
> 
> So, in my testing at least, because the callbacks are async, if the IDs
> conflict, the test still fails as expected, because from the closing of the
> window, the install hasn't completed yet.

Ah ok that might just cover it then.

> With the dialog functioning, there's no way to have "real" concurrent
> installs, as far as I can tell - only one of the two dialogs can be open and
> focused at the same time, and my attempts to run both installs in parallel
> (effectively just running a .forEach() on the list of tabs and clicking both
> links 'sync' one after the other) pops up two install permission dialogs one
> after the other, but the concurrent waitForFocus calls don't work (and even
> manual concoctions with focusing / clicking didn't work). So I'm unsure how
> you want me to make the installs "more concurrent" ? :-)

Hmm that's what I was expecting you to do. Because the install is really download then install, triggering both to start at the same time then waiting for both to complete is the ideal. But damn focus!

	Dave Townsend [:mossop]
	Comment 38 • 10 years ago

Do you still hit problems if you start the next install immediately before accepting the dialog for the first?

	:Gijs (he/him) Assignee
	Comment 39 • 10 years ago

It seems that does indeed work, and it was not too hard to modify the test accordingly, so here's a new patch with the comments addressed. I pushed to try because I am paranoid: https://tbpl.mozilla.org/?tree=Try&rev=e9c91bdf3353

	Dave Townsend [:mossop]
	Comment 40 • 10 years ago

Comment on attachment 8421181 [details] [diff] [review]
Use WebIDL to expose InstallTrigger,

Review of attachment 8421181 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work

	Bobby Holley (:bholley)
	Comment 41 • 10 years ago

Comment on attachment 8421181 [details] [diff] [review]
Use WebIDL to expose InstallTrigger,

Review of attachment 8421181 [details] [diff] [review]:
-----------------------------------------------------------------

Can you add some detailed comments explaining the static WebIDL trick?

r=bholley with that. Thanks for jumping on this with such tenacity!

	:Gijs (he/him) Assignee
	Comment 42 • 10 years ago

Thanks everyone! :-)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/26bac45c96b9

Comments at https://hg.mozilla.org/integration/mozilla-inbound/rev/26bac45c96b9#l11.102 .

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 43 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/26bac45c96b9

	:Gijs (he/him) Assignee
	Comment 45 • 10 years ago

Comment on attachment 8423785 [details] [diff] [review]
Patch for Aurora uplift

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: n/a
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): reasonably low because of the automated tests. Here's a try push against aurora: https://tbpl.mozilla.org/?tree=Try&rev=19dd618fca20
String or IDL/UUID changes made by this patch: some interfaces were removed and replaced with webidl versions.

	Ryan VanderMeulen [:RyanVM]
	Comment 46 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/731c627e0059