Closed Bug 1221141 Opened 8 years ago Closed 7 years ago

Support loading addon from local directory


(DevTools :: about:debugging, defect)

Not set


(firefox45 fixed)

Firefox 45
Tracking Status
firefox45 --- fixed


(Reporter: ochameau, Assigned: ochameau)




(2 files, 5 obsolete files)

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.
See Also: → 1209344
Depends on: 1209341
Attached patch patch v1 (obsolete) — Splinter Review
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=""/>
+  <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>
Attachment #8682535 - Flags: feedback?(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/
@@ +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[";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=""/>
> +        <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>
Attachment #8682535 - Flags: feedback?(janx) → feedback+
Blocks: 1225108
Attached patch patch v2 (obsolete) — Splinter Review
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?!!?
Attachment #8682535 - Attachment is obsolete: true
Attached patch patch v3 (obsolete) — Splinter Review
With a test \o/
Attachment #8689042 - Flags: review?(janx)
Attached patch test addon uninstall (obsolete) — Splinter 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.
Attachment #8688497 - Attachment is obsolete: true
> 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!


Your test seems to be timing out.
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
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

::: 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.
Attachment #8689042 - Flags: review?(janx) → review+
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 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 < AMI_callAddonListeners@AddonManager.jsm:1802:11 < AMP_callAddonListeners@AddonManager.jsm:2843:5 < XPI_uninstallAddon@XPIProvider.jsm: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/ ?
FYI: Mossop filed bug 1225944 to allow reloading temporary addons.

Maybe eventually we can live-reload a temporary add-on by watching its folder?
Oh, it really couldn't work on try as I forgot to also push bug 120934.
Here is a try run with lastest patch.
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.
Depends on: 1226185
Attached patch patch v4Splinter Review
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.
Attachment #8689584 - Flags: review?(janx)
Attachment #8689042 - Attachment is obsolete: true
Attachment #8689043 - Attachment is obsolete: true
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
Attachment #8689584 - Flags: review?(janx) → review+
This STR would be better in its own bug report.
There is various major errors around addons debugging, like bug 1227139.
Bug 1221141 - Support installing addon from local directory in about:debugging. r=janx
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.
Attachment #8692035 - Flags: review+
Rebased against fx-team, previous patch was on top of 1225473.
Attachment #8692043 - Flags: review+
Attachment #8692035 - Attachment is obsolete: true
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.