1221141 - Support loading addon from local directory

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect)

Description

[Alexandre Poirot [:ochameau]]

Thanks to bug 1209341, we are soon going to be able to load (unsigned) addons from a local directory \o/
This is a huge win for addons developers and should add this feature to about:debugging.
We should follow discussion on bug 1209344 which aims to add a UI for this in about:addons.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

Jan, Can you read this patch very briefly?
It looks like we would benefit from having more of react here.
I tend to think we should move all this into a react component dedicated to Addons,
so that I can move the loadAddonFromFile from aboutdebugging.js to a addon-specific file.

   <input id="enable-addon-debugging" type="checkbox" data-pref="devtools.chrome.enabled"/>
+  <button id="load-addon-from-file">&aboutDebugging.addonDebugging.loadFromFile;</button>
   <label for="enable-addon-debugging" title="&aboutDebugging.addonDebugging.tooltip;">&aboutDebugging.addonDebugging.label;</label>
   <div id="addons"></div>

Comment 2

[Jan Keromnes [:janx]]

Comment on attachment 8682535 [details] [diff] [review]
patch v1

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

Very cool idea!

At first I was confused by the already existing "Install Add-on From File" feature in "about:addons", but now I see how this is different.

(In reply to Alexandre Poirot [:ochameau] from comment #1)
> It looks like we would benefit from having more of react here.
> I tend to think we should move all this into a react component dedicated to
> Addons,
> so that I can move the loadAddonFromFile from aboutdebugging.js to a
> addon-specific file.

In general, I try to not use React on things that aren't supposed to be reactive (even though React is a cool hammer!)

Here, I don't see how we would benefit from React. As mentioned in bug 1196785 comment 66 (search for "pref-checkbox"), I tried rewriting the addon-debugging checkbox (and others) as a React component, but that turned out to take 50+ lines of code for no benefit over the non-react 9 lines of code. I feel like the button here doesn't need to become a component either.

Maybe in the future it will make sense to have components like "AddonsToolbar", "AddonsPanel" and "AboutDebugging", but today that would mean more code but no gain.

::: browser/locales/en-US/chrome/browser/devtools/aboutdebugging.dtd
@@ +5,5 @@
>  <!ENTITY aboutDebugging.title                      "about:debugging">
>  <!ENTITY aboutDebugging.addons                     "Add-ons">
>  <!ENTITY aboutDebugging.addonDebugging.label       "Enable add-on debugging">
>  <!ENTITY aboutDebugging.addonDebugging.tooltip     "Turning this on will allow you to debug add-ons and various other parts of the browser chrome">
> +<!ENTITY aboutDebugging.addonDebugging.loadFromFile "Install Add-on temporarily from File">

Nit: I prefer simple keys like "aboutDebugging.loadTemporaryAddon". Also, please make the button say the same thing, i.e. "Load Temporary Add-on" (optionally "From File…", but that makes the button very large).

Anyway, we should have a UX review of this at some point.

::: browser/locales/en-US/chrome/browser/devtools/aboutdebugging.properties
@@ +4,5 @@
>  
>  debug = Debug
>  
>  extensions = Extensions
> +selectAddonFromFile = Select addon directory or xpi file

Nit: Please use title case (i.e. "Select Add-on Directory or XPI File").

::: devtools/client/aboutdebugging/aboutdebugging.js
@@ +8,5 @@
>  
>  "use strict";
>  
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;

Nit: Popular devtools style seems to be `const { classes: Cc, interfaces: Ci, utils: Cu } = Components;`, please use it here as well.

@@ +24,5 @@
>  loader.lazyRequireGetter(this, "WorkersComponent",
>    "devtools/client/aboutdebugging/components/workers", true);
>  loader.lazyRequireGetter(this, "Services");
> +loader.lazyImporter(this, "AddonManager",
> +                    "resource://gre/modules/AddonManager.jsm");

Nit: In about:debugging, I separate `lazyRequireGetter`s from `lazyImporter`s with an empty line.

Nit: Also, please use double-space indent instead of many-space padding.

@@ +86,5 @@
>        Services.prefs.addObserver(pref, updateCheckbox, false);
>        updateCheckbox();
>      });
>  
> +    let loadFromFileBtn = document.getElementById("load-addon-from-file");

Nit: Please add a comment like "// Link buttons to their associated actions."

Nit: A better variable name would be "loadAddonButton" (we're not only loading from an XPI file: we can do folders as well).

@@ +106,5 @@
>    },
>  
> +  loadAddonFromFile() {
> +    let fp = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker);
> +    fp.init(window, Strings.GetStringFromName("selectAddonFromFile"), Ci.nsIFilePicker.modeOpen);

Nit: This line looks longer than 80 chars. I'm not personally bothered by this, but I think as a general tendency we try to make ESLint happy (and for some reason we enforce max-80-char lines).

If it doesn't take you too much time, please run ESLint on the files you touch (note: I recently tried `./mach eslint devtools/client/aboutdebugging/`, but that didn't work so I just gave up).

::: devtools/client/aboutdebugging/aboutdebugging.xhtml
@@ +34,5 @@
>          <div class="header">
>            <h1 class="header-name">&aboutDebugging.addons;</h1>
>          </div>
>          <input id="enable-addon-debugging" type="checkbox" data-pref="devtools.chrome.enabled"/>
> +        <button id="load-addon-from-file">&aboutDebugging.addonDebugging.loadFromFile;</button>

Nit: Placing this in between the "enable-addon-debugging" <input> and its <label> breaks it.

Instead, please style the button so that it lives on the same vertical line as the checkbox, but maybe on the right, e.g. with something like:

> <div style="display: flex; flex-direction: row">
>   <div style="flex: 1">
>     <input type="checkbox"/>
>     <label for=""/>
>   </div>
>   <button/>
> </div>

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Addressed all comments.
Hopefully bug 1209341 is going to land soon, so that we can move forward with this patch!
I would like to add a test before r? this patch.
But I see that there is no tests against about:debugging?!!?

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

With a test \o/

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: test addon uninstall

I had to put this part of the test out of the first patch
as uninstalling temporary addons throws.
At least with the old patch I cherry picked from bug 1209341.
I'll retest with the final patch to see if that still fails.

Comment 6

[Alexandre Poirot [:ochameau]]

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

Comment 7

[Jan Keromnes [:janx]]

> Addressed all comments.

Very cool, thanks!

> Hopefully bug 1209341 is going to land soon, so that we can move forward
> with this patch!

It's checkin-needed so... fingers crossed :)

> Created attachment 8689042 [details] [diff] [review]
> patch v3
> 
> With a test \o/

Wow, thanks a lot Alex!

> https://treeherder.mozilla.org/#/jobs?repo=try&revision=21e0acc5358b

Your test seems to be timing out.

Comment 8

[Jan Keromnes [:janx]]

Comment on attachment 8689042 [details] [diff] [review]
patch v3

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

Works great! Your test works locally, but on treeherder it seems to time out.

A few scary-looking logs I saw while loading/debugging/disabling/removing the test unsigned addon, but they're probably not severe or related to your code:

---

1447872722659	addons.xpi	WARN	Bootstrap state is invalid (missing add-ons: /c/gecko-dev/devtools/client/aboutdebugging/test/addons/unpacked)

JavaScript error: resource://gre/modules/XPCOMUtils.jsm, line 191: TypeError: can't redefine non-configurable property "Services"

Handler function threw an exception: TypeError: threadActor is null
Stack: TabActor.prototype._windowReady@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webbrowser.js:1657:1
TabActor.prototype._changeTopLevelDocument/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webbrowser.js:1594:7
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:87:14

onPacket threw an exception: Error: Server did not specify an actor, dropping packet: {"error":"unknownError","message":"error occurred while processing 'frames: TypeError: generatedLocation.generatedSourceActor is null\nStack: FrameActor.prototype.form@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:3181:9

Full message: TypeError: this._extraPools is null
Full stack: DSC_removeActorPool@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1385:9
ThreadActor.prototype._resumed@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:1573:5
ThreadActor.prototype.onResume/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:1025:20
Handler.prototype.process@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:934:23
this.PromiseWalker.walkerLoop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:813:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:744:11
this.PromiseWalker.schedulePromise@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:776:7
Promise.prototype.then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:451:5
ThreadActor.prototype.onResume@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:1017:1
ThreadActor.prototype.disconnect@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:578:7
AP_remove@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/common.js:263:7
AP_destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/common.js:225:7
DSC_onClosed/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1704:40
DSC_onClosed@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1704:5
DebuggerTransport.prototype.close@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:212:7
DebuggerServerConnection.prototype.close@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1349:5
DS_destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:211:7
BrowserToolboxProcess.prototype.close@resource://devtools/client/framework/ToolboxProcess.jsm:251:7
EventLoop.prototype.enter@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:347:5
ThreadActor.prototype._pushThreadPause@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:543:5
ThreadActor.prototype._pauseAndRespond@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:746:7
BreakpointActor.prototype.hit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:3373:12
@resource://gre/modules/addons/XPIProvider.jsm -> file:///c/gecko-dev/devtools/client/aboutdebugging/test/addons/unpacked/bootstrap.js:7:24
@resource://gre/modules/addons/XPIProvider.jsm:4449:1
XPI_loadBootstrapScope@resource://gre/modules/addons/XPIProvider.jsm:4449:7
XPI_callBootstrapMethod@resource://gre/modules/addons/XPIProvider.jsm:4525:1
XPI_uninstallAddon@resource://gre/modules/addons/XPIProvider.jsm:4824:1
AddonWrapper_uninstall@resource://gre/modules/addons/XPIProvider.jsm:7130:5
doPendingUninstalls@chrome://mozapps/content/extensions/extensions.js:1741:5
gListView_hide@chrome://mozapps/content/extensions/extensions.js:2738:5
gVC_shutdown@chrome://mozapps/content/extensions/extensions.js:660:7
shutdown@chrome://mozapps/content/extensions/extensions.js:182:3
_endRemoveTab@chrome://browser/content/tabbrowser.xml:2372:13
onxbltransitionend@chrome://browser/content/tabbrowser.xml:5364:11

::: devtools/client/aboutdebugging/aboutdebugging.css
@@ +87,5 @@
> +}
> +
> +.addon-options {
> +  flex: 1;
> +}

Nit: Could you please drive-by fix the hard-coded `button { width: 100px }`? Not sure why I did that in the first place, but changing it to `button { padding-left: 20px; padding-right: 20px }` fixes the styling for your larger button.

Comment 9

[Alexandre Poirot [:ochameau]]

The failure is most likely a packaging issue related to how test are run on automation...
It may not be able to get a file path to the test file and just a jar: url.
Test files on automation are something really complex :'(

Comment 10

[Jan Keromnes [:janx]]

Comment on attachment 8689043 [details] [diff] [review]
test addon uninstall

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

> I had to put this part of the test out of the first patch
> as uninstalling temporary addons throws.
> At least with the old patch I cherry picked from bug 1209341.
> I'll retest with the final patch to see if that still fails.

Still seems to fail with the final patch applied, but maybe not for the same reason:

1447874048834	addons.manager	WARN	AddonListener threw exception when calling onUninstalled: ReferenceError: aAddon is not defined (chrome://mochitests/content/browser/devtools/client/aboutdebugging/test/browser_addons_install.js:45:1)
JS Stack trace: listener.onUninstalled@browser_addons_install.js:45:1 < [e-mail]:1802:11 < [e-mail]:2843:5 < [e-mail]:4832:7 < etc.

::: devtools/client/aboutdebugging/test/browser_addons_install.js
@@ +41,5 @@
> +  yield new Promise(done => {
> +    AddonManager.getAddonByID(ADDON_ID, addon => {
> +      let listener = {
> +        onUninstalled: function(aUninstalledAddon) {
> +          if (aUninstalledAddon != aAddon) {

Nit: s/aAddon/addon/ ?

Comment 11

[Jan Keromnes [:janx]]

FYI: Mossop filed bug 1225944 to allow reloading temporary addons.

Maybe eventually we can live-reload a temporary add-on by watching its folder?

Comment 12

[Alexandre Poirot [:ochameau]]

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

Comment 13

[Alexandre Poirot [:ochameau]]

Oh, it really couldn't work on try as I forgot to also push bug 120934.
Here is a try run with lastest patch.

Comment 14

[Alexandre Poirot [:ochameau]]

Ok, it's much better with the platform patch ;)
The platform patch itself has some failure on xpcshell.
And the first about:debugging test highlights some leaks within about:debugging...

I'll try to figure out the leak while the platform patch is being fixed.

Comment 15

[Alexandre Poirot [:ochameau]]

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

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

With the patches from bug 1226185, the leaks should be gone now.
And the addon uninstall test seems to work with latest platform patch,
so I merged it.

Comment 17

[Jan Keromnes [:janx]]

Comment on attachment 8689584 [details] [diff] [review]
patch v4

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

Works great! Thanks again for the first about:debugging test.

Unrelated side-note, I was able to freeze the browser on what looks like a debugger/breakpoint bug:
- Go to about:debugging
- Load the temporary addon from devtools/client/aboutdebugging/test/addons/unpacked/
- Debug it, and set breakpoints on all 4 methods (startup/shutdown/install/uninstall)
- Go to about:addons, and disable the addon
- The remote toolbox is stopped on the "shutdown" breakpoint. Click on Play to unpause the debugger and unfreeze Firefox
- In about:addons, re-enable the addon

Expected: The addon is re-enabled, and your debugger is paused on the "startup" breakpoint

Actual: The addon says it will enable on the next Firefox start, the remote toolbox seems to be paused but not on any visible breakpoints and clicking Play has no effect, Firefox is completely frozen with no way to unfreeze it

Comment 18

[Alexandre Poirot [:ochameau]]

This STR would be better in its own bug report.
There is various major errors around addons debugging, like bug 1227139.

Comment 19

[Alexandre Poirot [:ochameau]]

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

Comment 20

[Alexandre Poirot [:ochameau]]

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

Comment 21

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/91ddf71d401829a08d9ab4650ef4632553cecbd6
Bug 1221141 - Support installing addon from local directory in about:debugging. r=janx

Comment 22

[Alexandre Poirot [:ochameau]]

Attachment: Specify absolute path to each about:debugging test file to please Windows.

http://archive.mozilla.org/pub/firefox/tinderbox-builds/fx-team-win32/1448466932/fx-team-win32-bm71-build1-build6.txt.gz
WindowsError: [Error 2] The system cannot find the file specified: u'_tests\\testing\\mochitest\\browser\\devtools\\client\\aboutdebugging\\test\\addons'

Windows seems to dislike the `addons/` folder passed as support-files in the browser.ini file.
Hopefully, this is going to fix this error.

Comment 23

[Alexandre Poirot [:ochameau]]

Attachment: Specify absolute path to each about:debugging test file to please Windows.

Rebased against fx-team, previous patch was on top of 1225473.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b8775010b736

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91ddf71d4018
https://hg.mozilla.org/mozilla-central/rev/b8775010b736