986755 - Add-on debugger should use the XPI hierarchy to group sources

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Dave Townsend [:mossop]]

The debugger uses the URI protocol to split up files from the add-on but I think it would be better to just use the XPI file structure

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch

This adds a localURL and addonID property to the source actor which allows the UI to group add-on code under its ID and extract its path in the add-on. This also makes the Add-on SDK grouping be handled in a way that would allow us to add other groups easily in the future if we want.

The downside is that to keep things efficient getting the source group injects the known source label into the label cache. It isn't ideal but I couldn't see a better way to keep getSourceLabel accept a uri which is needed for other code.

Comment 2

[Dave Townsend [:mossop]]

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

This feels too nasty to me, I think I can come up with something better

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 1

I think this is about as straightforward as I can make it. The source actor looks up if the url is associated with an add-on and if so works out the path inside the add-on's XPI. We then expose that in the actor's properties so the UI can use it instead of the normal URI based groupings.

Comment 4

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

Comment on attachment 8398153 [details] [diff] [review]
patch rev 1

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

r+ with changes mentioned below. However, I'm also asking for a secondary review on just addon manager importing stuff in script.js from ochameau to make sure it doesn't mess up the work he did a little while back. See comment below for details.

Furthermore, you should send a pull request to https://github.com/jimblandy/DebuggerDocs updating the protocol docs on the added properties to source actors.

::: browser/devtools/debugger/test/browser_dbg_addon-modules-unpacked.js
@@ +14,5 @@
> +      case "toolbox-title":
> +        gTitle = json.data.value;
> +        break;
> +    }
> +  } catch(e) { Cu.reportError(e); }

DevToolsUtils.reportException("onMessage", e)

@@ +38,5 @@
> +    gClient.connect(connected.resolve);
> +    yield connected.promise;
> +
> +    yield installAddon();
> +    let debuggerPanel = yield initAddonDebugger(gClient, ADDON5_URL, iframe);

No need to do it in this patch, but everything above this line in the test is pretty much exactly shared with all the other addon thread tests. Please file a bug to consolidate this logic in a helper function in head_dbg.js.

@@ +129,5 @@
> +  gThreadClient = null;
> +  gDebugger = null;
> +  gSources = null;
> +  while (gBrowser.tabs.length > 1)
> +    gBrowser.removeCurrentTab();

brackets

::: toolkit/devtools/server/actors/script.js
@@ +13,5 @@
>  // collections, etc.
>  let OBJECT_PREVIEW_MAX_ITEMS = 10;
>  
> +let addonManager = Cc["@mozilla.org/addons/integration;1"].
> +                   getService(Ci.amIAddonManager);

I believe that ochameau went to great lengths to make sure that we don't load the addon manager in the debugger server for B2G because it was taking up a bunch of memory and was completely pointless on B2G.

We should make sure that this patch doesn't undo his work.

A potential solution is making this a lazy getter and checking what platform we are running on everytime before accessing it.

@@ +2528,5 @@
> +        this._addonPath = path;
> +      }
> +    }
> +  }
> +  catch (e) { }

Silently swallowing every error is pretty scary. First, let's pare this try/catch down so it only wraps the actual calls that will throw. Second, unless you are *absolutely* 100% sure that its ok to swallow a thrown error, we should be logging the error for our future selves who are debugging this code via |DevToolsUtils.reportException("SourceActor", e)|.

If you are really 100% sure that it's ok to swallow one of these errors, please ensure that the error is of the type that you expect to swallow. And if it isn't either throw it again or |DevToolsUtils.reportException|.

Finally, a style nit: This is a lot of code for the constructor. Can you move it into a method that's called from the constructor?

@@ +2551,5 @@
>  
>    get threadActor() this._threadActor,
>    get url() this._url,
> +  get addonID() this._addonID,
> +  get addonPath() this._addonPath,

Nit: please add

    _addonId: null,
    _addonPath: null,

to the prototype above these getters (by the _oldSourceMap and _init properties) so that we continue sharing js shapes between source actors for addon sources and source actors for "normal" sources.

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Addressed review comments. I've added a wrapper around addonManager.mapURIToAddonID so we only get the add-on manager if not on B2G and also take care of any exceptions thrown from that.

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8398592 [details] [diff] [review]
patch rev 2

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

Doesn't break much more b2g than it is already broken!

The fact is that it is quite rare to debug chrome on b2g as everything in app://
so that resolveURIToLocalPath returns null and we will rarely call mapURIToAddonID on b2g.
Today, it looks like we can't debug the toplevel document, the only one using chrome:// URI; it crashes...

So I can't experience this codepath, but it looks good.
Thanks Nick for keeping FxOS in mind!!

Comment 7

[Dave Townsend [:mossop]]

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

Comment 8

[Dave Townsend [:mossop]]

Created a pull request for the docs: https://github.com/jimblandy/DebuggerDocs/pull/21

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/9dba41d6106a