Closed Bug 1268773 Opened 8 years ago Closed 8 years ago

After reloading an addon with the addon debugger opened, the debugging global should be updated

Categories

(DevTools :: about:debugging, defect, P3)

Product:
DevTools
Component:
about:debugging
Type:
defect

Tracking

(firefox50 verified, firefox51 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 50
Iteration:
50.4 - Aug 1
Tracking Flags:
Tracking Status
firefox50 --- verified
firefox51 --- verified

People

(Reporter: rpl, Assigned: rpl)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1268773 - Update the Addon Wrapper for cached addon actors on reload.
58 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1268773 - Test addon wrapper updated in cached addon actor on reload and upgrade.
58 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1268773 - Notify Addon Listeners when the preferred addon debug global is changed.
58 bytes, text/x-review-board-request
aswan
: review+
Details 
STR:
- from the about:debugging page, install an webextension addon with a background page
- open the addon debugger on the installed webextension addon
- the console panel has the background page as the current active global (e.g. the chrome/browser object gives access to the expected webextensions APIs)
- press the reload button in the about:debugging page
  EXPECTED behavior:
  - the console panel has the updated background page as the current active global (e.g. the chrome/browser object is still available, any added/removed permission affects the APIs available accordingly)
  ACTUAL behavior:
  - the console panel has not the background page as the current active global anymore
  WORKAROUND:
  - closing and reopening the addon debugger seems to solve the issue)
Blocks: 1226743
Alex, is this something you'd be interested in addressing or mentoring?
Flags: needinfo?(poirot.alex)
Priority: -- → P3
Too busy to address. Looks too hard to mentor. I don't know myself how to reparent a console.
But I could certainly review patches.

Otherwise I was wondering if we should automatically close the addon toolbox when an addon is uninstalled/disabled? Which should happen when a reload one.
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #2)
> Otherwise I was wondering if we should automatically close the addon toolbox
> when an addon is uninstalled/disabled? Which should happen when a reload one.

Actually this seems like a saner approach.

Luca, would this solve your problem? If so, would you be interested in implementing it?
Flags: needinfo?(lgreco)
(In reply to Jan Keromnes [:janx] (away until July 22) from comment #3)
> (In reply to Alexandre Poirot [:ochameau] from comment #2)
> > Otherwise I was wondering if we should automatically close the addon toolbox
> > when an addon is uninstalled/disabled? Which should happen when a reload one.
> 
> Actually this seems like a saner approach.
> 
> Luca, would this solve your problem? If so, would you be interested in
> implementing it?

I'm looking into the various issues that we have around the addon debugging, and this issue is one of those that I'm already looking into.

I'm not exactly sure if closing the entire toolbox is what I'd like to get (I would prefer to keep the toolbox opened if possible)

Anyway to get a better picture, I took a deeper look into what is happening with the current implementation and it seems that:

- if we uninstall the addon, then the addon actor destroy itself and disconnect the client, and on the client side the toolbox is closed automatically

- if we disable the addon, then the addon actor receive a "onDisabled" notification from the addon manager and the addon actor clears its global, and the webconsole stop to work correctly (and enabling the addon again doesn't help)

- if we press the reload button in the "about:debugging" page, then the addon actor doesn't receive any notification from the addon manager related to a disable/uninstall

I'm experimenting on how to refresh the addon global without closing the toolbox and I managed to make the console working correctly across reloads (but before I can call it "a proposal", I'd like to check if there is a reasonable solution to refresh the threadActor as well, because it is obviously suffering of similar issues after an addon reload)

I'm going to put together my proposal, but if it is not feasible with minimal changes, I'm ok with tweaking it to close the toolbox when the addon actor looses its global (which it is definitely feasible with minimal changes).
Flags: needinfo?(lgreco)
Luca, it might be simplier if you first switch to inherit from the TabActor. The tab actor already does support switching between frames. That may help you supporting context switching.
But we never had to "freeze" the toolbox. That would be an universe of unexpected!

Just thinking out load: would it be a good plan to do a quick fix here by detaching the actor on addon reload/disable. Then focus on tab actor support, before getting back to the reload polish ?
Blocks: 1285557
The attached patches do not fix the issue on the current AddonActor, but they contains a set of small proposed changes that make it possible to fix it in the WebExtensionAddonActor which is going to be attached to Bug 1285557 (anyway, these changes can be used to apply the same fix to the AddonActor, and fix this issue for the legacy add-ons types as well)

The first two patches introduce a small change on the devtools actors side:

- when an addon has been reloaded or upgraded, instead of removing the related addon actor, update the addon wrapper (e.g. used to retrieve the addon metadata the name, the icon etc.) on the currently cached actor
- by refreshing the existent actor instead of removing it, we make it possible to use the same addon actor across addon reloads, which means that we can use the same toolbox window attached to the same addon actor id across reloads.

The third patch uses an existent AddonListener event ("onPropertyChanged") to notify the AddonListeners when the preferred debug global for an addon has been changed, this event wil be used by the WebExtensionAddonActor in Bug 1285557 to detect when the preferred debug global has been changed and a new method to retrieve the new debug global and act accordingly (e.g. when it is set to null because the addon is shutting down or when it is set to the new background window when the addon has been started again)
Attachment #8769208 - Flags: review?(aswan) → review+
Comment on attachment 8769208 [details]
Bug 1268773 - Notify Addon Listeners when the preferred addon debug global is changed.

https://reviewboard.mozilla.org/r/63172/#review60044

The implementation looks good, my initial reaction is that this seems like a strange thing to use onPropertyChanged for, since the AddonListener interface is meant to be used for tracking the overall state of installed/enabled addons, not details about individual addons.  But this does seem much more expedient than adding a new interface just for this...

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_background_debug_global.html:74
(Diff revision 1)
> +    return new Promise((resolve) => {
> +      AddonManager.addAddonListener({
> +        count: 0,
> +        notNullGlobalsCount: 0,
> +        onPropertyChanged(newAddon, aChangedPropNames) {
> +          if (newAddon.id != addon.id &&

i think you want || instead of && ?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_background_debug_global.html:115
(Diff revision 1)
>    yield extension.unload();
>    info("extension unloaded");
> +
> +  let unloadCounters = yield waitForDebugGlobalChangesOnShutdown;
> +  isDeeply(unloadCounters, {count: 1, notNullGlobalsCount: 0},
> +           "Got the expected nuber of onPropertyChanged calls on reload");

s/reload/unload/ (or shutdown)

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7553
(Diff revision 1)
>   * The PrivateWrapper is used to expose certain functionality only when being
>   * called with the add-on instanceID, disallowing other add-ons to access it.
>   */
>  function PrivateWrapper(aAddon) {
>    AddonWrapper.call(this, aAddon);
> +  this.publicWrapper = new AddonWrapper(aAddon);

why do you create a new wrapper instead of just using addonfor() ?
Comment on attachment 8769208 [details]
Bug 1268773 - Notify Addon Listeners when the preferred addon debug global is changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63172/diff/1-2/
Assignee: nobody → lgreco
(In reply to Andrew Swan [:aswan] from comment #10)

Thanks Andrew, for both the very fast review and the proposed tweaks.

All the suggestions have been applied to the last version of the third patch (attachment 8769208 [details]).
Status: NEW → ASSIGNED
Attachment #8769207 - Flags: review?(poirot.alex) → review+
Comment on attachment 8769207 [details]
Bug 1268773 - Test addon wrapper updated in cached addon actor on reload and upgrade.

https://reviewboard.mozilla.org/r/63170/#review60258

We should already cover all this in devtools/client/aboutdebugging/test/browser_addons_reload.js
If not, we should tweak that higher level test.
Feel free to land your lower level test if that helps you working in this codebase, otherwise the high level test can be enough.

::: devtools/client/debugger/test/mochitest/browser_dbg_listaddons_reload.js:66
(Diff revision 1)
> +    let addonActorForm2 = reply2.addons.filter((form) => form.id == ADDON_ID).pop();
> +
> +    is(addonActorForm2.actor, addonActorForm.actor,
> +       "the addon actor has the same actor id after reload");
> +
> +    let addonUpdated = yield addTemporaryAddon(ADDON_URL_UPDATED);

In parallel of the call to addTemporaryAddon, you may also assert that you receive `addonListChanged` event.

You should be able to listen for it with something like this:
```
gClient.addListener("addonListChanged", function listener() {
  gClient.removeListener("addonListChanged", listener);
  resolve();
})
```
Comment on attachment 8769206 [details]
Bug 1268773 - Update the Addon Wrapper for cached addon actors on reload.

https://reviewboard.mozilla.org/r/63168/#review60256

::: devtools/server/actors/webbrowser.js:2333
(Diff revision 1)
> -  if (this._actorByAddonId.get(addon.id)) {
> +  let addonActor = this._actorByAddonId.get(addon.id);
> +  if (addonActor) {
>      // When an add-on gets upgraded or reloaded, it will not be uninstalled
> -    // so this step is necessary to clear the cache.
> -    this._actorByAddonId.delete(addon.id);
> +    // so this step is necessary to update the addon wrapper in the cached actor.
> +    addonActor.updateAddonWrapper(addon);
>    }

Instead of exposing such `updateAddonWrapper`, you could listen for onInstalled from BrowserAddonActor. It also listen for AddonManager events.
I think it will be clear to do it from there!
But put a comment about this code there saying that the addon list will also be updated from BrowserAddonList.
Attachment #8769206 - Flags: review?(poirot.alex)
The assertions in browser_dbg_listaddons_reload.js look like maybe they could be merged into https://dxr.mozilla.org/mozilla-central/source/devtools/server/tests/unit/test_addon_reload.js
Comment on attachment 8769206 [details]
Bug 1268773 - Update the Addon Wrapper for cached addon actors on reload.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63168/diff/1-2/
Attachment #8769206 - Flags: review?(poirot.alex)
Comment on attachment 8769207 [details]
Bug 1268773 - Test addon wrapper updated in cached addon actor on reload and upgrade.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63170/diff/1-2/
Comment on attachment 8769208 [details]
Bug 1268773 - Notify Addon Listeners when the preferred addon debug global is changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63172/diff/2-3/
https://reviewboard.mozilla.org/r/63168/#review60256

> Instead of exposing such `updateAddonWrapper`, you could listen for onInstalled from BrowserAddonActor. It also listen for AddonManager events.
> I think it will be clear to do it from there!
> But put a comment about this code there saying that the addon list will also be updated from BrowserAddonList.

I like the idea, in the updated version I added the onInstalled listener in the Addon Actor and the AddonWrapper is updated from there.
https://reviewboard.mozilla.org/r/63170/#review60258

The high level test is useful to confirm that with this low level changes applied, nothing is changes from an higher level point of view.

But given that the high level test shouldn't probably look into any actor IDs, I've merged my additional low level test assertions with the `devtools/server/tests/unit/test_addon_reload.js`.

> In parallel of the call to addTemporaryAddon, you may also assert that you receive `addonListChanged` event.
> 
> You should be able to listen for it with something like this:
> ```
> gClient.addListener("addonListChanged", function listener() {
>   gClient.removeListener("addonListChanged", listener);
>   resolve();
> })
> ```

Added (along with the additional assertions) in the test unit `devtools/server/tests/unit/test_addon_reload.js`.
Comment on attachment 8769206 [details]
Bug 1268773 - Update the Addon Wrapper for cached addon actors on reload.

https://reviewboard.mozilla.org/r/63168/#review60628

::: devtools/server/actors/addon.js:119
(Diff revision 2)
> +    if (aAddon.id != this._addon.id) {
> +      return;
> +    }
> +
> +    // Update the AddonWrapper in the existent addon actor
> +    // on addon reload/upgrade.

I think the following sentence is enoug:
"Update the AddonManager's addon object on reload/update."
Attachment #8769206 - Flags: review?(poirot.alex) → review+
https://reviewboard.mozilla.org/r/63170/#review60632

::: devtools/server/tests/unit/test_addon_reload.js:78
(Diff revision 2)
> +  const onAddonListChanged = new Promise((resolve) => {
> +    client.addListener("addonListChanged", function listener() {
> +      client.removeListener("addonListChanged", listener);
> +      resolve();
> +    });
> +  });

I recently learned that there is a powerful helper in devtools:
https://dxr.mozilla.org/mozilla-central/source/devtools/server/tests/browser/head.js#92
In case you have access to it from this test (it may only be available in browser mochitests.
You can write this as:
const onAddonListChanged = once(client, "addonListchanged");
https://reviewboard.mozilla.org/r/63172/#review60636

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7544
(Diff revision 3)
>  PrivateWrapper.prototype = Object.create(AddonWrapper.prototype);
>  Object.assign(PrivateWrapper.prototype, {
> -
>    addonId() {
>      return this.id;
>    },
>  
>    /**
> +   * Retrieves the preferred global context to be used from the
> +   * add-on debugging window.
> +   *
> +   * @returns  global
> +   *         The object set as global context. Must be a window object.
> +   */
> +  getDebugGlobal(global) {
> +    let activeAddon = XPIProvider.activeAddons.get(this.id);
> +    if (activeAddon) {
> +      return activeAddon.debugGlobal;
> +    }
> +
> +    return null;
> +  },

Hi Andrew,
sorry if I bother you with this patch again, but I take a look at it again and I think that it would be better to put the new getDebugGlobal helper into the PrivateWrapper, because it gives direct access to the debug global to the caller, and so it is something that should be added to the private wrapper.

It is a bit more annoying to retrieve the preferred debug global, because first you need to get the addon instanceID and then ask for a private wrapper, but I think it will be more safe and it worth the additional code needed to retrieve it.
Comment on attachment 8769206 [details]
Bug 1268773 - Update the Addon Wrapper for cached addon actors on reload.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63168/diff/2-3/
Comment on attachment 8769207 [details]
Bug 1268773 - Test addon wrapper updated in cached addon actor on reload and upgrade.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63170/diff/2-3/
Comment on attachment 8769208 [details]
Bug 1268773 - Notify Addon Listeners when the preferred addon debug global is changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63172/diff/3-4/
https://reviewboard.mozilla.org/r/63168/#review60628

> I think the following sentence is enoug:
> "Update the AddonManager's addon object on reload/update."

Inline comment changed as suggested.
https://reviewboard.mozilla.org/r/63170/#review60632

> I recently learned that there is a powerful helper in devtools:
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/tests/browser/head.js#92
> In case you have access to it from this test (it may only be available in browser mochitests.
> You can write this as:
> const onAddonListChanged = once(client, "addonListchanged");

With the above helper this test would be definitely nicer, unfortunately it is currently not available in the xpcshell tests :-(
(but I would be happy to define it for the xpcshell test helpers as well in one of the follow-ups)
Comment on attachment 8769206 [details]
Bug 1268773 - Update the Addon Wrapper for cached addon actors on reload.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63168/diff/3-4/
Comment on attachment 8769207 [details]
Bug 1268773 - Test addon wrapper updated in cached addon actor on reload and upgrade.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63170/diff/3-4/
Comment on attachment 8769208 [details]
Bug 1268773 - Notify Addon Listeners when the preferred addon debug global is changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63172/diff/4-5/
(In reply to Luca Greco [:rpl] from comment #31)
> Comment on attachment 8769208 [details]
> Bug 1268773 - Notify Addon Listeners when the preferred addon debug global
> is changed.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/63172/diff/4-5/

Fixed a couple of eslint warnings/errors on the test `toolkit/components/extensions/test/mochitest/test_chrome_ext_background_debug_global.html`
Comment on attachment 8769206 [details]
Bug 1268773 - Update the Addon Wrapper for cached addon actors on reload.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63168/diff/4-5/
Comment on attachment 8769207 [details]
Bug 1268773 - Test addon wrapper updated in cached addon actor on reload and upgrade.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63170/diff/4-5/
Comment on attachment 8769208 [details]
Bug 1268773 - Notify Addon Listeners when the preferred addon debug global is changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63172/diff/5-6/
Patch rebased and fixed small merge conflicts with the patch from Bug 1274775 (recently landed on fx-team).
Comment on attachment 8769206 [details]
Bug 1268773 - Update the Addon Wrapper for cached addon actors on reload.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63168/diff/5-6/
Comment on attachment 8769207 [details]
Bug 1268773 - Test addon wrapper updated in cached addon actor on reload and upgrade.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63170/diff/5-6/
Comment on attachment 8769208 [details]
Bug 1268773 - Notify Addon Listeners when the preferred addon debug global is changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63172/diff/6-7/
(In reply to Luca Greco [:rpl] from comment #39)
> Comment on attachment 8769208 [details]
> Bug 1268773 - Notify Addon Listeners when the preferred addon debug global
> is changed.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/63172/diff/6-7/

Patch rebased on a recent fx-team tip.
Rebased patches pushed to try:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=793665237699
Iteration: --- → 50.4 - Aug 1
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/b212c88a2944
Update the Addon Wrapper for cached addon actors on reload. r=ochameau
https://hg.mozilla.org/integration/fx-team/rev/4b4425636a87
Test addon wrapper updated in cached addon actor on reload and upgrade. r=ochameau
https://hg.mozilla.org/integration/fx-team/rev/14fd1e2c9b00
Notify Addon Listeners when the preferred addon debug global is changed. r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b212c88a2944
https://hg.mozilla.org/mozilla-central/rev/4b4425636a87
https://hg.mozilla.org/mozilla-central/rev/14fd1e2c9b00
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
I could reproduce this issue on Firefox 50.0a1 (20160726080520)

This issue is verified as fixed on Firefox 50.0a1 (20160726080520) and Firefox 51.0a1 (2016-09-15), I can confirm that the the console panel has the updated background page as the current active global after pressing the reload button in the about:debugging page
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
status-firefox51: --- → verified
Sorry I have mistaken the build, the correct build where it is fixed is Firefox 50.0a1(20160730030204) not Firefox 50.0a1 (20160726080520).
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: