911098 - Implement the UI part of the Add-on Debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Comment 1

[Mike Hordecki [:mhordecki] (UTC+2)]

Attachment: 0001-Add-the-debug-button-to-the-add-on-manager.patch

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 810954 [details] [diff] [review]
0001-Add-the-debug-button-to-the-add-on-manager.patch

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

Looks pretty straightforward enough, one logic error here though

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +6305,5 @@
>    });
>  
> +  this.__defineGetter__("isDebuggable", function AddonWrapper_isDebuggable() {
> +    return this.isActive &&
> +             this.operationsRequiringRestart == AddonManager.OP_NEEDS_RESTART_NONE;

My understanding is that only SDK based add-ons are debuggable right now so this isn't quite right.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +922,5 @@
> +      },
> +
> +      isEnabled: function cmd_debugItem_isEnabled(aAddon) {
> +        return Services.prefs.getBoolPref('devtools.debugger.addon-enabled')
> +                 && aAddon && aAddon.isDebuggable;

Put the initial && on the previous line and line up aAddon with Service.

::: toolkit/mozapps/extensions/test/browser/Makefile.in
@@ +34,4 @@
>    browser_bug679604.js \
>    browser_bug714593.js \
>    browser_bug590347.js \
> +	browser_debug_button.js \

Nit: bad indentation

Comment 3

[Eddy Bruel [:ejpbruel]]

(In reply to Dave Townsend (:Mossop) from comment #2)
> Comment on attachment 810954 [details] [diff] [review]
> 0001-Add-the-debug-button-to-the-add-on-manager.patch
> 
> Review of attachment 810954 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks pretty straightforward enough, one logic error here though
> 
> ::: toolkit/mozapps/extensions/XPIProvider.jsm
> @@ +6305,5 @@
> >    });
> >  
> > +  this.__defineGetter__("isDebuggable", function AddonWrapper_isDebuggable() {
> > +    return this.isActive &&
> > +             this.operationsRequiringRestart == AddonManager.OP_NEEDS_RESTART_NONE;
> 
> My understanding is that only SDK based add-ons are debuggable right now so
> this isn't quite right.
> 
> ::: toolkit/mozapps/extensions/content/extensions.js
> @@ +922,5 @@
> > +      },
> > +
> > +      isEnabled: function cmd_debugItem_isEnabled(aAddon) {
> > +        return Services.prefs.getBoolPref('devtools.debugger.addon-enabled')
> > +                 && aAddon && aAddon.isDebuggable;
> 
> Put the initial && on the previous line and line up aAddon with Service.
> 
> ::: toolkit/mozapps/extensions/test/browser/Makefile.in
> @@ +34,4 @@
> >    browser_bug679604.js \
> >    browser_bug714593.js \
> >    browser_bug590347.js \
> > +	browser_debug_button.js \
> 
> Nit: bad indentation

This patch never landed. Review comments haven't been addressed, and as far as I can tell, the add-ons used by test_isDebuggable.js are missing from the patch. It will take me some time to piece things back together.

Comment 4

[Jeff Griffiths (:canuckistani) (:⚡︎)]

*bump* status?

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Address review comments

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8359931 [details] [review]
Address review comments

I'd like to avoid exposing anything more than the necessary isDebuggable in the API, otherwise this looks good.

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

This bitrotted quite a bit already, fixing this up for today

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 911098-addon-debugger-ui.patch

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

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Updated try push: https://tbpl.mozilla.org/?tree=Try&rev=dee5181169ae

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 911098-addon-debugger-ui.patch

Fixed xpcshell tests

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

try for xpcshell and extensions mochitests https://tbpl.mozilla.org/?tree=Try&rev=d9bedf2a7303

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

XPCshell tests passing, but seeing some failures on bc tests, pushing without patch to see if it was caused by these changes: https://tbpl.mozilla.org/?tree=Try&rev=5563c625b8a2

Comment 13

[Jordan Santell [:jsantell] [@jsantell]]

Looks like there was an observer that never cleaned up, hopefully the cause of window leaks:
https://tbpl.mozilla.org/?tree=Try&rev=d07d09dc4dab

Comment 14

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I ran the above try build, after flipping the pref I see the debug button but I see these errors:

JavaScript error: chrome://mozapps/content/extensions/extensions.js, line 20: NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]
JavaScript error: chrome://mozapps/content/extensions/extensions.js, line 957: BrowserDebuggerProcess is not defined

Comment 15

[Jordan Santell [:jsantell] [@jsantell]]

Looks like the tests aren't comprehensive -- good catch, I'll add some more

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

Not crashing for me, but OTOH, not doing much of anything else either.

Comment 17

[Jordan Santell [:jsantell] [@jsantell]]

Everything seems to be working, pushing to try
https://tbpl.mozilla.org/?tree=Try&rev=ab7bc56b604d

Comment 18

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 911098-addon-debugger-ui.patch

Comment 19

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 911098-addon-debugger-ui.patch

Got communication to debug only addons over the process working, added more tests, fixed bitrotting, pushed to try: r?ing Nick and Eddy -- not sure which one of you guys should review. Thanks!

Comment 20

[Jordan Santell [:jsantell] [@jsantell]]

New try patch, made a dumb mistake
https://tbpl.mozilla.org/?tree=Try&rev=f956dadab2ec

Comment 21

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 911098-addon-debugger-ui.patch

Comment 22

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 911098-addon-debugger-ui.patch

Now with -U8

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8393789 [details] [diff] [review]
911098-addon-debugger-ui.patch

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

I only reviewed the debugger changes. You'll need to find someone else for the framework stuff (:harth volunteered) and another person for the addon stuff.

LGTM just a few comments below.

::: browser/devtools/debugger/debugger-panes.js
@@ +130,5 @@
>      let label = SourceUtils.getSourceLabel(url.split(" -> ").pop());
>      let group = SourceUtils.getSourceGroup(url.split(" -> ").pop());
>      let unicodeUrl = NetworkHelper.convertToUnicode(unescape(url));
>  
> +    dump("addSource: " + url + "\n");

rm debug stuff

@@ +134,5 @@
> +    dump("addSource: " + url + "\n");
> +
> +    // Do some parsing to see if it's a part of an Addon
> +    let lastChunk = url.substring(url.lastIndexOf("->")).trim();
> +    let match = /resource:\/\/gre\/modules\/commonjs\/(.*)/.exec(lastChunk);

Should use |nsIURI| to parse this information.

  const uri = Services.io.newURI(lastChunk);
  if (uri.scheme == "resource" && ...) ...

https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIURI

While you're here, can you define |lastChunk| underneath |let url = ...| and replace the |url.split(...).pop()| calls with references to |lastChunk|?

::: browser/devtools/debugger/test/browser_dbg_addon-sources.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Make sure we can attach to addon actors.

Update this comment to better reflect what this test is doing.

::: browser/devtools/debugger/test/head.js
@@ +500,5 @@
>      return deferred.promise;
>    });
>  }
>  
> +function initAddonDebugger(aClient, aUrl, frame) {

aFrame to match style :(

@@ +525,5 @@
> +    let debuggerPanel = aToolbox.getCurrentPanel();
> +    let panelWin = debuggerPanel.panelWin;
> +
> +    // Wait for the initial resume...
> +    panelWin.gClient.addOneTimeListener("resumed", () => {

You can use |waitForClientEvents("resumed")| instead of reimplementing it here.

Comment 24

[Victor Porof [:vporof][:vp]]

Comment on attachment 8393789 [details] [diff] [review]
911098-addon-debugger-ui.patch

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

::: browser/devtools/debugger/debugger-panes.js
@@ +137,5 @@
> +    let lastChunk = url.substring(url.lastIndexOf("->")).trim();
> +    let match = /resource:\/\/gre\/modules\/commonjs\/(.*)/.exec(lastChunk);
> +    if (match) {
> +      label = match[1];
> +      group = "Add-on SDK";

Should this be localized?

Comment 25

[Jordan Santell [:jsantell] [@jsantell]]

ni?ing L10N on Victor's question -- listing addon SDK sources in the addon debugger labels them under the group "Add-on SDK" -- should this be localized, or is the "Add-on SDK" considered a brand/product/proper noun?

Comment 26

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8393789 [details] [diff] [review]
911098-addon-debugger-ui.patch

r?ing Mossop for XPI/extension changes, Harth for Framework changes -- thanks!

Comment 27

[Samuel Bronson]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #25)
> ni?ing L10N on Victor's question -- listing addon SDK sources in the addon
> debugger labels them under the group "Add-on SDK" -- should this be
> localized, or is the "Add-on SDK" considered a brand/product/proper noun?

Is the name localized in, say, the documentation?

Comment 28

[Axel Hecht [:Pike]]

I already said on irc that it's not the l10n folks that decide what's a band or a proper noun.

Please ask the branding or copy guys for that.

A screenshot of the string in use might be helpful to decide if there are l12y problems if the Add-on SDK happens to be a proper noun.

Comment 29

[Dave Townsend [:mossop]]

Comment on attachment 8393789 [details] [diff] [review]
911098-addon-debugger-ui.patch

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

Looks good

::: toolkit/mozapps/extensions/content/extensions.js
@@ +3428,5 @@
>      if (aProperties.indexOf("applyBackgroundUpdates") != -1)
>        this.updateAvailableCount();
>    }
>  };
> +  

Nit: whitespace

@@ +3429,5 @@
>        this.updateAvailableCount();
>    }
>  };
> +  
> +let addonDebuggingObs = {

This can just be a function, call it debuggingPrefChanged

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +6450,5 @@
>      return ops;
>    });
>  
> +  this.__defineGetter__("isBootstrapped", function AddonWrapper_isBootstrapped() {
> +    return this.operationsRequiringRestart === AddonManager.OP_NEEDS_RESTART_NONE;

return aAddon.bootstrap;

@@ +6453,5 @@
> +  this.__defineGetter__("isBootstrapped", function AddonWrapper_isBootstrapped() {
> +    return this.operationsRequiringRestart === AddonManager.OP_NEEDS_RESTART_NONE;
> +  });
> +
> +  this.__defineGetter__("isJetpack", function AddonWrapper_isJetpack() {

Let's use isCommonJS instead

::: toolkit/mozapps/extensions/test/xpcshell/test_isDebuggable.js
@@ +16,5 @@
> +];
> +
> +function run_test() {
> +  do_test_pending();
> +  

Nit: whitespace

Comment 30

[Dave Townsend [:mossop]]

Comment on attachment 8393789 [details] [diff] [review]
911098-addon-debugger-ui.patch

Thinking about it, I don't think we should add the isJetpack or isBootstrapped properties to this API. We don't need them here so let's keep it simple and only expose isDebuggable

Comment 31

[Dave Townsend [:mossop]]

Comment on attachment 8393789 [details] [diff] [review]
911098-addon-debugger-ui.patch

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

::: toolkit/mozapps/extensions/content/extensions.js
@@ +958,5 @@
> +      },
> +
> +      isEnabled: function cmd_debugItem_isEnabled(aAddon) {
> +        let debuggerEnabled = Services.prefs.
> +                              getBoolPref("devtools.debugger.addon-enabled");

You need to check that chrome debugging and remote debugging are enabled too or the add-on debugger won't work

Comment 32

[Jordan Santell [:jsantell] [@jsantell]]

Added 'debug' button to only display once remote and addon debugging is enabled (chrome isn't required) and added tests for this. Fixed Dave and Nick's comments.

Pushing changes to try: https://tbpl.mozilla.org/?tree=Try&rev=77c90f8fc14b

Comment 33

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 911098-addon-debugger-ui.patch

Comment 34

[Heather Arthur [:harth]]

Comment on attachment 8394590 [details] [diff] [review]
911098-addon-debugger-ui.patch

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

Looks good for the framework pieces.

Comment 35

[Dave Townsend [:mossop]]

Comment on attachment 8394590 [details] [diff] [review]
911098-addon-debugger-ui.patch

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

Strangely this patch is still failing try but only on Windows 7. When I add my patches it starts failing in a similar way on other platforms too :(

::: browser/devtools/debugger/test/browser_dbg_addon-sources.js
@@ +5,5 @@
> +// addon itself, or the SDK, with proper groups and labels.
> +
> +const ADDON3_URL = EXAMPLE_URL + "addon3.xpi";
> +const TAB_URL = EXAMPLE_URL + "doc_iframes.html";
> +const DBG_XUL = "chrome://browser/content/devtools/framework/toolbox-process-window.xul";

These two variables seem to be unused.

Comment 36

[Dave Townsend [:mossop]]

Comment on attachment 8394590 [details] [diff] [review]
911098-addon-debugger-ui.patch

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

::: browser/devtools/debugger/test/browser_dbg_addon-sources.js
@@ +36,5 @@
> +
> +    yield testSources();
> +    yield uninstallAddon();
> +    yield closeConnection();
> +    finish();

Mentioned on IRC but you need to iframe.remove() before calling finish()

Comment 37

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 911098-addon-debugger-ui.patch

Comment 38

[Dave Townsend [:mossop]]

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

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/f353b6a98a3d for causing a few leaks:
https://tbpl.mozilla.org/php/getParsedLog.php?id=36636961&tree=Fx-Team


Your try push would need to include debug builds if you want to catch the leaks.

Comment 40

[Dave Townsend [:mossop]]

Odd since we saw some leaks in non-debug builds too. Ah well. Jordan I think you said you had this. Feel free to ping me if you need help.

Comment 41

[Jordan Santell [:jsantell] [@jsantell]]

Pushed a debug try build with the toolbox closing
https://tbpl.mozilla.org/?tree=Try&rev=4dca4b693eba

Comment 42

[Jordan Santell [:jsantell] [@jsantell]]

Try fixed, https://tbpl.mozilla.org/?tree=Try&rev=5922670c85ec

Comment 43

[Dave Townsend [:mossop]]

Attachment: Make restartless add-ons debuggable

Jordan, can you merge this patch into yours, it just makes all restartless add-ons debuggable straight away (though they won't show anything until the other bug lands).

Comment 44

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 911098-addon-debugger-ui.patch

Attaching updated, try-passing, restartless-addon-debuggable-merged, patch

Comment 45

[Dave Townsend [:mossop]]

Attachment: leak fix

Spinning a try run (https://tbpl.mozilla.org/?tree=Try&rev=c60f1767b35c) but I think this is the leak fix we're looking for.

Comment 46

[Dave Townsend [:mossop]]

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

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0c4f13342090

Comment 48

[u279076]

Kind of late in the Beta cycle but does this need any testing before we release Firefox 31?

Comment 49

[Dave Townsend [:mossop]]

Probably not a lot, it's a disabled by default feature in its early stages.

Comment 50

[u279076]

(In reply to Dave Townsend [:mossop] from comment #49)
> Probably not a lot, it's a disabled by default feature in its early stages.

Okay, thanks. Please needinfo me if/when that changes.