Closed Bug 990074 Opened 10 years ago Closed 10 years ago

Sources linked via the optionsURL in install.rdf do not show up in addon debugger.

Categories

(DevTools :: Debugger, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: Optimizer, Assigned: mossop)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

For ex. this addon : https://addons.mozilla.org/en-US/firefox/addon/user-style-manager/
has an options window, and thus a js file linked with that xul window. The addon installs the options window using install.rdf's optionsURL property.

this options.js file is not listed in Addon Debugger.
Blocks: dbg-addon
We need to add DOM windows created from add-on URLs as debuggees. I'm not sure if the best way to do that is in the onNewGlobal event or if we should use a chrome-document-global-created observer. The latter I know will have the location set so we can tie it to add-ons. The former might have through aGlobal.unsafeDereference.location.
Hrm except that chrome documents load about:blank first so in both cases all we get is that. We'll have to use a load listener to spot anything more useful which means we could miss scripts.
Depends on: 990685
Assignee: nobody → dtownsend+bugmail
Attached patch patch (obsolete) — Splinter Review
This listens for new opened windows as long as the AddonThreadActor is alive. Any that map to the add-on get added as debuggees. The notification is necessary because checkGlobal is called very early in the window creation, at a time when it's location is still about:blank for chrome windows. The notification should be sent before any scripts have run though.

This depends on bug 990685.
Attachment #8404283 - Flags: review?(nfitzgerald)
Comment on attachment 8404283 [details] [diff] [review]
patch

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

LGTM, just some comments below.

::: browser/devtools/debugger/test/head.js
@@ +119,5 @@
>        onInstallEnded: function(aAddon, aAddonInstall) {
>          aInstaller.removeListener(listener);
> +        executeSoon(function() {
> +          deferred.resolve(aAddonInstall);
> +        });

:( Why is this necessary?

It shouldn't be necessart after bug 940537, right? If so, add a TODO comment that links the bug number.

::: toolkit/devtools/server/actors/script.js
@@ +4777,5 @@
>    // A constant prefix that will be used to form the actor ID by the server.
>    actorPrefix: "addonThread",
>  
> +  onAttach: function(aRequest) {
> +    if (!this.attached)

Nit: Brackets. Here and throughout.

@@ +4788,5 @@
> +      Services.obs.removeObserver(this, "document-element-inserted");
> +    return ThreadActor.prototype.disconnect.call(this);
> +  },
> +
> +  observe: function(aSubject, aTopic, aData) {

Nit: comment above this function explaining what it is observing and what it does when the observer fires, etc.

@@ +4871,1 @@
>        if (metadata)

Nit from another time: brackets here please.

@@ +4888,2 @@
>        try {
> +        var uri = Services.io.newURI(uridescriptor.value, null, null);

Rather than using |var|, can we just add |let uri;| at a high enough scope that it is visible to both scopes that use it? That looks like right before the try block to me.
Attachment #8404283 - Flags: review?(nfitzgerald) → review+
(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> Comment on attachment 8404283 [details] [diff] [review]
> patch
> 
> Review of attachment 8404283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, just some comments below.
> 
> ::: browser/devtools/debugger/test/head.js
> @@ +119,5 @@
> >        onInstallEnded: function(aAddon, aAddonInstall) {
> >          aInstaller.removeListener(listener);
> > +        executeSoon(function() {
> > +          deferred.resolve(aAddonInstall);
> > +        });
> 
> :( Why is this necessary?
> 
> It shouldn't be necessart after bug 940537, right? If so, add a TODO comment
> that links the bug number.

I've added a comment but this is bug 997408. If bug 940537 causes promise callbacks to be called asynchronously then I guess that will fix this but I think it makes some sense to still use this so we don't even resolve the promise until what we're waiting for has actually happened.
https://hg.mozilla.org/mozilla-central/rev/e666661da03c
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Backed out: https://hg.mozilla.org/integration/fx-team/rev/a9cddc54b6f4
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patchSplinter Review
I could have sworn that when I tested chrome-document-global-created before it didn't have the url set right but here we are...
Attachment #8404283 - Attachment is obsolete: true
Attachment #8412144 - Flags: review?(nfitzgerald)
Comment on attachment 8412144 [details] [diff] [review]
patch

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

::: toolkit/devtools/server/actors/script.js
@@ +4796,5 @@
> +  },
> +
> +  /**
> +   * Called when a new DOM document global is created. Check if the DOM was
> +   * laoded from an add-on and if so make the window a debuggee.

laoded
Attachment #8412144 - Flags: review?(nfitzgerald) → review+
No longer depends on: 990685
https://hg.mozilla.org/mozilla-central/rev/e063eb25827a
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
(In reply to Dave Townsend [:mossop] from comment #9)
> 
> I could have sworn that when I tested chrome-document-global-created before
> it didn't have the url set right but here we are...


This behavior is not guaranteed! For windows like Options or browser.xul, window.location will be empty or about:blank.
(In reply to Jeferson Hultmann from comment #13)
> (In reply to Dave Townsend [:mossop] from comment #9)
> > 
> > I could have sworn that when I tested chrome-document-global-created before
> > it didn't have the url set right but here we are...
> 
> 
> This behavior is not guaranteed! For windows like Options or browser.xul,
> window.location will be empty or about:blank.

What makes you say that and why is this the case (it wasn't in my tests)?
Depends on: 1007756
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.