986108 - Make all restartless add-ons debuggable

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

We should make any restartless add-on debuggable by tagging their main bootstrap script with the right metadata.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This makes the add-ons manager set the same metadata for the bootstrap script that the SDK uses to tag its modules so the add-on debugger will use them. With bug 986107 this makes all sandboxes created by restartless add-ons debuggable.

The side effect of showing bootstrap.js for SDK add-ons might be unwanted but if we switch to native SDK support that will go away. We also have to force hide XPIProvider.jsm since the blob that it evals into the bootstrap scope from bug 618101 makes the whole add-ons manager show up and we don't want that.

Unfocused for the add-ons manager bits, fitzgen for the devtools changes.

Comment 2

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

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

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

For the debugger changes.

::: toolkit/devtools/server/actors/script.js
@@ +4654,5 @@
> +    // Hide eval scripts
> +    if (!aSourceURL)
> +      return false;
> +
> +    // XPIProvider.jsm evals some code in every add-on's bootstrap.js. Hide it

Nit: missing period.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Er, so, this depends on the patch in bug 911098, but essentially undoes all the relevant changes that patch does? Feels like bug 911098 should just be rebased ontop of this patch, no?

Comment 4

[Dave Townsend [:mossop]]

(In reply to Blair McBride [:Unfocused] from comment #3)
> Er, so, this depends on the patch in bug 911098, but essentially undoes all
> the relevant changes that patch does? Feels like bug 911098 should just be
> rebased ontop of this patch, no?

Bug 911098 has been sort of a mess with multiple patch authors and reviewers. I didn't really want to start weighing in as well while it was still under development and just get the initial parts of the feature that the intern developed landed. I was hoping that would have already happened but we've hit test failures, the fixes for which apply here too.

The tests from this patch won't work without the test parts from bug 911098 so it's sort of a pain to try to re-order. What I can do if you're concerned with messy history/blame is just merge the two patches and land in one changeset.

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Updated patch without the bits that undoes them things what the other bug did do

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

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

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +4038,5 @@
>      }
>  
>      let uri = getURIForResourceInFile(aFile, "bootstrap.js").spec;
> +    if (aType == "dictionary")
> +      uri = "resource://gre/modules/addons/SpellCheckDictionaryBootstrap.js"

Nit: Can avoid the getURIForResourceInFile call entirely if this is a dictionary.

Comment 7

[Dave Townsend [:mossop]]

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

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e08c2640229