Gabor Krizsanits (INACTIVE) Reporter
	Description • 9 years ago

Developing add-ons for the new extension API should be simple. For that we need a load extension button and a package extension button from a local directory in the add-on page. Preferably hidden behind some checkbox... We also have to figure out when to enable this, since this is running unsigned extensions essentially.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 1 • 9 years ago

Does this look like the right approach Dave? I suspect there are some cases I'm missing, but I want to make sure I'm not totally off base.

Also, I'm not really sure how we'll address the signing issue. It would be pretty easy to disable signing along this path, but I don't know if it's a good idea :-). I've been testing it by toggling the pref.

	Dave Townsend [:mossop]
	Comment 2 • 9 years ago

Comment on attachment 8646681 [details] [diff] [review]
patch

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

This is looking good

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +2951,5 @@
>            }
>          }
>  
>          try {
> +          // FIXME: Do I need to handle the case where this is a dir add-on?

Yeah you probably do

@@ -5571,5 @@
>     *         XPI is incorrectly signed
>     */
>    loadManifest: Task.async(function* AI_loadManifest(file) {
> -    let zipreader = Cc["@mozilla.org/libjar/zip-reader;1"].
> -                    createInstance(Ci.nsIZipReader);

This change will break things when we try to use the zipreader later

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 3 • 9 years ago

(In reply to Dave Townsend [:mossop] from comment #2)
> ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
> @@ +2951,5 @@
> >            }
> >          }
> >  
> >          try {
> > +          // FIXME: Do I need to handle the case where this is a dir add-on?
> 
> Yeah you probably do

Actually, I looked over this code more carefully and I don't think we need to do anything. I think this code will only be invoked for restartless add-ons if we crash during installation. If that happens, I think we'll end up here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2825

That's maybe not the best outcome. However, I believe that we automatically skip unsigned restartless add-ons in the staging directory anyway:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2883

Given that people are very unlikely to be installing signed add-ons via this method, it seems reasonable to me not to change the current behavior. After all, what are the chances that we crash during installation?

> @@ -5571,5 @@
> >     *         XPI is incorrectly signed
> >     */
> >    loadManifest: Task.async(function* AI_loadManifest(file) {
> > -    let zipreader = Cc["@mozilla.org/libjar/zip-reader;1"].
> > -                    createInstance(Ci.nsIZipReader);
> 
> This change will break things when we try to use the zipreader later

Maybe I missed something, but I tried to remove all uses of zipreader below.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 4 • 9 years ago

(In reply to Bill McCloskey (:billm) from comment #3)
> Actually, I looked over this code more carefully and I don't think we need
> to do anything.

Oh, I forgot that people could install non-restartless add-ons this way :-(.

	Alexandre Poirot [:ochameau] Assignee
	Comment 5 • 9 years ago

Bill, Do you mind if I carry over your patch? I would like to see this feature in Firefox for Devtools needs.
(We are trying to support new development workflow involving an addon to easily reload devtools codebase)

I imagine we mostly miss some test for this?

Also, I was wondering if it make sense to introduce the UI (also or instead) in about:debugging,
which is more developers oriented than about:addons.

	Markus Jaritz [:designakt] (UX)
	Comment 6 • 9 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> Also, I was wondering if it make sense to introduce the UI 
> (also or instead) in about:debugging,
> which is more developers oriented than about:addons.

I did not no about about:debug. Is this used by many add-on developers?

	Blake Winton (:bwinton) (:☕️)
	Comment 7 • 9 years ago

I would suggest "also" instead of "instead", if only for Chrome-parity…  ;)

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 8 • 9 years ago

I was hoping to get to this in the next couple days. I'll assign it over to you if I haven't made any progress by the end of next week.

	Dave Townsend [:mossop]
	Comment 9 • 9 years ago

rhelmer has the backend pieces for this almost done in bug 1209341, we probably only need UI hookups in bug 1209344 so I'm not sure if this bug is useful anymore.

	Alexandre Poirot [:ochameau] Assignee
	Comment 10 • 9 years ago

about:debugging just appeared during FF44 cycle.
It only targets Workers and Addons so far, but it should over time allow you to debug more things and especially contexts running on remote devices like apps and tabs on Fennec/FirefoxOS.
We haven't been communicating about this as it is brand new and certainly needs some polish.

If the backend is already ready, I can certainly add the UI in about:debugging, as I already now this codebase.

(In reply to Bill McCloskey (:billm) from comment #8)
> I was hoping to get to this in the next couple days. I'll assign it over to
> you if I haven't made any progress by the end of next week.

Ok. I won't have much time until Monday, so it sounds like a good plan!

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 11 • 9 years ago

Hmm, I didn't realize bug 1209341 will support unpacked add-ons.

I feel like there's value in being able to load unpacked add-ons in a non-temporary way if you disable the signing pref, especially for non-restartless add-ons. But maybe it's not a high priority.

	Alexandre Poirot [:ochameau] Assignee
	Comment 12 • 9 years ago

Let's try to move the "install from file" UI discussion to bug 1209344.
bill, can you close that bug or rename it to something more specific you have in mind?

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 13 • 9 years ago

OK, I re-titled the bug.

	Alexandre Poirot [:ochameau] Assignee
	Comment 14 • 9 years ago

I would actually like to see that happen as well.
The usecase is quite simple:
 * you may break firefox with your addon and have to restart it often
 * you may want to see how your addon behaves during firefox startup

Mossop, First, are you ok with that?
Robert, You are not already working on that, right?

If yes, how would we implement this?

Codewise:
- should we make it so that existing installTemporaryAddon function install the addon permanently if the signature pref is off?
- or, should we come up with another function?

UI:
 - should the existing "load temporary addon" be permanent if signature is off?
 - should we display another action in about:debugging, if signature is off to load permanently?

	Dave Townsend [:mossop]
	Comment 15 • 9 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #14)
> I would actually like to see that happen as well.
> The usecase is quite simple:
>  * you may break firefox with your addon and have to restart it often
>  * you may want to see how your addon behaves during firefox startup
> 
> Mossop, First, are you ok with that?

Yes I'm fine with this.

> If yes, how would we implement this?
> 
> Codewise:
> - should we make it so that existing installTemporaryAddon function install
> the addon permanently if the signature pref is off?
> - or, should we come up with another function?

I think we should use a different function and here's why. I think the temporary add-ons are something we can make special in ways that I wouldn't want to support for non-temporary add-ons. Things like watching the filesystem and reloading the add-on if something changes. So I'd rather keep them separated and make it possible to still use that even if you're also able to install unsigned add-ons.

	Alexandre Poirot [:ochameau] Assignee
	Comment 16 • 8 years ago

Here is a tentative patch to do that.
It hacks the existing about:debugging feature to install addons permanently.
More work is needed in about:debugging to switch between temporary/permanent.

Note that working in chrome is simplier, local addons are installed permanently.
So I would like to prevent have to dance between temporary and permanent
and keep firefox as simple.

	Alexandre Poirot [:ochameau] Assignee
	Comment 17 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b64ffc993005

	Andy McKay
	Comment 18 • 8 years ago

Just looping in security to check this is ok.

	Andy McKay
	Comment 19 • 8 years ago

Checking in with Kev too on this.

BillM noticing this is assigned to you, make more sense to move to ochameau?

	Alexandre Poirot [:ochameau] Assignee
	Comment 20 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a58414a1041

	Alexandre Poirot [:ochameau] Assignee
	Comment 21 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f629d27fea74

	Alexandre Poirot [:ochameau] Assignee
	Comment 23 • 8 years ago

Dave, I would like to focus on the AddonManager part of this bug. May be I should open a specific bug?
Here, I don't modify the behavior of about:addons nor about:debugging, I would like to keep that for another patch.

Otherwise, what do you think about handling permanently sideloaded addons via a specific InstallLocation, like what we do for temporary install?

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

Comment on attachment 8797157 [details]
Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently.

I'm going to let Rob handle your questions here, he owns the add-ons manager now.

	Robert Helmer [:rhelmer]
	Comment 25 • 8 years ago
mozreview-review

Comment on attachment 8797157 [details]
Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently.

https://reviewboard.mozilla.org/r/82770/#review81438

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4121
(Diff revision 1)
>  
>      addon = XPIDatabase.addAddonMetadata(addon, file.persistentDescriptor);
>  
>      XPIStates.addAddon(addon);
>      XPIDatabase.saveChanges();
> +    //XPIStates.save();

Debugging?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4136
(Diff revision 1)
>      AddonManagerPrivate.callAddonListeners("onInstalled", addon.wrapper);
>  
>      return addon.wrapper;
>    }),
>  
> +/*

This function is commented out, was this an earlier approach to the above replacement of `installTemporaryAddon` (which now calls `installAddonFromSources`?)

	Robert Helmer [:rhelmer]
	Comment 26 • 8 years ago
mozreview-review

Comment on attachment 8797157 [details]
Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently.

https://reviewboard.mozilla.org/r/82770/#review81448

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8846
(Diff revision 1)
>  }
>  
> +const SourceInstallLocation = {
> +  locked: false,
> +  name: KEY_APP_SOURCE,
> +  scope: AddonManager.SCOPE_TEMPORARY,

Hm `SCOPE_TEMPORARY` doesn't seem accurate anymore - not sure if any of the existing ones match exactly what you want to do, `SCOPE_USER` is probably closest.

	Robert Helmer [:rhelmer]
	Comment 27 • 8 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #23)
> Dave, I would like to focus on the AddonManager part of this bug. May be I
> should open a specific bug?
> Here, I don't modify the behavior of about:addons nor about:debugging, I
> would like to keep that for another patch.

I agree, UI should be handled in a separate patch. Temporary add-on loading is only supported via about:debugging and not about:addons, I'm not sure about where this one should go - this might be appropriate for about:addons gear->"Install Add-on From Folder..." or so to go with the existing "From File". I would be OK with it just being in about:debugging if that makes sense from the devtools POV though.

> Otherwise, what do you think about handling permanently sideloaded addons
> via a specific InstallLocation, like what we do for temporary install?

I see why we need a new API to trigger this since AddonManager supports loading extensions from a directory but only if they are present at startup (AFAICT), but I am not sure why a new InstallLocation is needed - wouldn't MutableDirectoryInstallLocation work?

It makes sense to have TemporaryInstallLocation mostly because there is a lot of behavior we don't wish to implement, like supporting updates. Is this the case too for "from source dir" add-ons, or do you want to support features like updates?

Overall I am fine with this approach, just would like to understand better why this should implement a new InstallLocation.

	Alexandre Poirot [:ochameau] Assignee
	Comment 29 • 8 years ago

Just pushed a new patch addressing what you highlighted. I forgot that I kept work-in-progress things in the patch... I introduce a new dedicated scope.

I understand about the new InstallLocation. TBH, the AddonManager code is quite scary, I didn't knew exactly how to proceed and was very happy once I got a working patch!
I'm happy to go with a simplier alternative.
Wouldn't MutableDirectoryInstallLocation end up deleting the source folder on uninstall?
  http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#8387
Also, I know the addon manager does scan the addon folders for modtime or checksum. Do you know if that kicks in with the new InstallLocation? What about the MutableDirectoryInstallLocation?

Regarding the signature, this features forces you to toggle the xpinstall.signatures.required pref to false. The new install location doesn't try to bypass any signature check.

	Robert Helmer [:rhelmer]
	Comment 30 • 8 years ago
mozreview-review

Comment on attachment 8797157 [details]
Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently.

https://reviewboard.mozilla.org/r/82770/#review81540

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4041
(Diff revision 2)
> +   *
>     * @return a Promise that resolves to an Addon object on success, or rejects
>     *         if the add-on is not a valid restartless add-on or if the
> -   *         same ID is already temporarily installed
> +   *         same ID is already installed.
>     */
> -  installTemporaryAddon: Task.async(function*(aFile) {
> +  installAddonFromSources: Task.async(function*(aFile, aInstallLocation = SourceInstallLocation) {

kmag suggested that you could use the existing "proxy file" feature, to avoid having to create a new type of install location:
https://developer.mozilla.org/en-US/Add-ons/Setting_up_extension_development_environment#Firefox_extension_proxy_file

This would require writing a text file to the extension directory in the profile, which contains the path to the addon.

You'd still need a way to activate it without a restart and without having to re-scan the whole extensions dir - I think you could easily modify `AddonInstall.initLocalInstall` (http://searchfox.org/mozilla-central/rev/3411332d29d377bf86405fc3ea72461ea0cb0295/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5411) to work with directories as well as files.

What do you think?

	Alexandre Poirot [:ochameau] Assignee
	Comment 31 • 8 years ago

I tried the MutableDirectoryInstallLocation and yes, it ends up deleting the original source directory:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#8322
installAddon is doing that by trying to move 

I also tried the proxy file route but it forces to modify many things including startInstall:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#6306
which stoped me pursuing this. Modifying initLocationInstall was reasonable but startInstall is much less.

	Alexandre Poirot [:ochameau] Assignee
	Comment 32 • 8 years ago

ALT+Space and bugzilla immediately send your comment even if it takes 10s to display any html :x

So. I'm not sure the proxy file is reasonable, it also sounds like a hack.
But I haven't spent too much time with the MutableDirectoryInstallLocation, I may be able to come up with something by still sharing some code with the temporary addon codepath.

	Alexandre Poirot [:ochameau] Assignee
	Comment 33 • 8 years ago

Here is an alternative patch that works, based on your suggestion to use MutableDirectoryLocation.
Note that I also used the proxy file trick.
f? to see if I should move forward with this patch instead?
If you agree, it may be good to refactor installAddon to accept a dictionnary as argument?
the aExistingId, aCopy, aSymLink are just messy and would be better agregated into an option object.

	Robert Helmer [:rhelmer]
	Comment 34 • 8 years ago

Comment on attachment 8797800 [details] [diff] [review]
alt patch using MutableDirectoryLocation

This lgtm, although maybe I wouldn't use the term "symlink" if it's not an actual symbolic link and actually a text file, but I see what you mean :)

	Robert Helmer [:rhelmer]
	Comment 35 • 8 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #33)
> Created attachment 8797800 [details] [diff] [review]
> alt patch using MutableDirectoryLocation
> 
> Here is an alternative patch that works, based on your suggestion to use
> MutableDirectoryLocation.
> Note that I also used the proxy file trick.
> f? to see if I should move forward with this patch instead?
>
> If you agree, it may be good to refactor installAddon to accept a
> dictionnary as argument?
> the aExistingId, aCopy, aSymLink are just messy and would be better
> agregated into an option object.


Sure.

	Robert Helmer [:rhelmer]
	Comment 38 • 8 years ago
mozreview-review

Comment on attachment 8797157 [details]
Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently.

https://reviewboard.mozilla.org/r/82770/#review82848

::: toolkit/mozapps/extensions/test/xpcshell/test_install_from_sources.js:6
(Diff revision 4)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +const ID = "bootstrap1@tests.mozilla.org";
> +const sampleRDFManifest = {

Is this used anywhere?

	Robert Helmer [:rhelmer]
	Comment 39 • 8 years ago
mozreview-review

Comment on attachment 8797157 [details]
Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently.

https://reviewboard.mozilla.org/r/82770/#review82852

	Alexandre Poirot [:ochameau] Assignee
	Comment 40 • 8 years ago

(In reply to Robert Helmer [:rhelmer] from comment #38)
> Comment on attachment 8797157 [details]
> Bug 1185460 - Implements AddonManager.installAddonFromSources to install
> addon from sources permanently.
> 
> https://reviewboard.mozilla.org/r/82770/#review82848
> 
> ::: toolkit/mozapps/extensions/test/xpcshell/test_install_from_sources.js:6
> (Diff revision 4)
> > +/* Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/publicdomain/zero/1.0/
> > + */
> > +
> > +const ID = "bootstrap1@tests.mozilla.org";
> > +const sampleRDFManifest = {
> 
> Is this used anywhere?

No, I removed it. It's a leftover from a copy paste from test_temporary.js.

	Pulsebot
	Comment 43 • 8 years ago

Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/839c876d0dab
Implements AddonManager.installAddonFromSources to install addon from sources permanently. r=rhelmer

	Phil Ringnalda (:philor)
	Comment 44 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/839c876d0dab