1277054 - Handle exited add-on actors when reloading via remote debugger

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect)

Description

[Kumar McMillan [:kumar]]

STR
- Start Firefox
- Using the remote debugger, install an add-on temporarily (actor support landed in bug 1273183)
- Trigger a reload with the debug client (actor support landed in bug 1246030)
- Wait about 20 seconds. During this time, some add-ons will be automatically uninstalled due to compatibility (I think).
- Trigger another reload

What should happen: The reload should work just fine. The add-ons that get uninstalled are not needed for any part of the reload process.

What actually happens: You get a stack trace which looks to be caused by the uninstalled add-ons.

Stack trace:

TypeError: this._contextPool is null
Stack: BAA_form@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/addon.js:78:7
RootActor.prototype.onListAddons/</<.addons<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/root.js:359:49
RootActor.prototype.onListAddons/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/root.js:359:19
Handler.prototype.process@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:779:7
this.PromiseWalker.completePromise@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:714:7
BrowserAddonList.prototype.getList/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webbrowser.js:2302:5
safeCall@resource://gre/modules/AddonManager.jsm:179:5
AddonManagerInternal.getAddonsByTypes/<.noMoreObjects@resource://gre/modules/AddonManager.jsm:2495:9
AsyncObjectCaller.prototype.callNext@resource://gre/modules/AddonManager.jsm:367:7
AddonManagerInternal.getAddonsByTypes/<.nextObject/<@resource://gre/modules/AddonManager.jsm:2490:11
this.Experiments.PreviousExperimentProvider.prototype<.getAddonsByTypes@resource:///modules/experiments/Experiments.jsm:2207:5
callProviderAsync@resource://gre/modules/AddonManager.jsm:254:12
AddonManagerInternal.getAddonsByTypes/<.nextObject@resource://gre/modules/AddonManager.jsm:2485:9
AsyncObjectCaller.prototype.callNext@resource://gre/modules/AddonManager.jsm:373:7
AddonManagerInternal.getAddonsByTypes/<.nextObject/<@resource://gre/modules/AddonManager.jsm:2490:11
SocialAddonProvider.getAddonsByTypes@resource://gre/modules/SocialService.jsm:920:5
callProviderAsync@resource://gre/modules/AddonManager.jsm:254:12
AddonManagerInternal.getAddonsByTypes/<.nextObject@resource://gre/modules/AddonManager.jsm:2485:9
AsyncObjectCaller.prototype.callNext@resource://gre/modules/AddonManager.jsm:373:7
AddonManagerInternal.getAddonsByTypes/<.nextObject/<@resource://gre/modules/AddonManager.jsm:2490:11
PluginProvider.getAddonsByTypes@resource://gre/modules/addons/PluginProvider.jsm:154:5
callProviderAsync@resource://gre/modules/AddonManager.jsm:254:12
AddonManagerInternal.getAddonsByTypes/<.nextObject@resource://gre/modules/AddonManager.jsm:2485:9
AsyncObjectCaller.prototype.callNext@resource://gre/modules/AddonManager.jsm:373:7
AddonManagerInternal.getAddonsByTypes/<.nextObject/<@resource://gre/modules/AddonManager.jsm:2490:11
GMPProvider.getAddonsByTypes@resource://gre/modules/addons/GMPProvider.jsm:672:5
callProviderAsync@resource://gre/modules/AddonManager.jsm:254:12
AddonManagerInternal.getAddonsByTypes/<.nextObject@resource://gre/modules/AddonManager.jsm:2485:9
AsyncObjectCaller.prototype.callNext@resource://gre/modules/AddonManager.jsm:373:7
AddonManagerInternal.getAddonsByTypes/<.nextObject/<@resource://gre/modules/AddonManager.jsm:2490:11
this.LightweightThemeManager.getAddonsByTypes@resource://gre/modules/LightweightThemeManager.jsm:454:5
callProviderAsync@resource://gre/modules/AddonManager.jsm:254:12
AddonManagerInternal.getAddonsByTypes/<.nextObject@resource://gre/modules/AddonManager.jsm:2485:9
AsyncObjectCaller.prototype.callNext@resource://gre/modules/AddonManager.jsm:373:7
AddonManagerInternal.getAddonsByTypes/<.nextObject/<@resource://gre/modules/AddonManager.jsm:2490:11
this.XPIProvider.getAddonsByTypes/<@resource://gre/modules/addons/XPIProvider.jsm:4043:7
makeSafe/<@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:168:17
asyncMap_gotValue@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:203:7
asyncMap/</<@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:210:9
completeAddon@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:157:5
this.AddonRepository.getCachedAddonByID<@resource://gre/modules/addons/AddonRepository.jsm:578:7
TaskImpl_run@resource://gre/modules/Task.jsm:319:40
TaskImpl@resource://gre/modules/Task.jsm:280:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14
getRepositoryAddon@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:159:3
asyncMap/<@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:209:7
asyncMap@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:207:3
this.XPIDatabase.getAddonList/<@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:1099:9
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:454:5
this.XPIDatabase.getAddonList@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:1096:5
this.XPIDatabase.getVisibleAddons@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:1179:5
this.XPIProvider.getAddonsByTypes@resource://gre/modules/addons/XPIProvider.jsm:4042:5
callProviderAsync@resource://gre/modules/AddonManager.jsm:254:12
AddonManagerInternal.getAddonsByTypes/<.nextObject@resource://gre/modules/AddonManager.jsm:2485:9
AsyncObjectCaller.prototype.callNext@resource://gre/modules/AddonManager.jsm:373:7
AsyncObjectCaller@resource://gre/modules/AddonManager.jsm:353:3
AddonManagerInternal.getAddonsByTypes@resource://gre/modules/AddonManager.jsm:2483:5
AddonManagerInternal.getAllAddons@resource://gre/modules/AddonManager.jsm:2515:5
this.AddonManager.getAllAddons@resource://gre/modules/AddonManager.jsm:3369:5
BrowserAddonList.prototype.getList@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webbrowser.js:2294:3
RootActor.prototype.onListAddons@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/root.js:340:12
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1648:15
DebuggerTransport.prototype._onJSONObjectReady/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:479:11
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14

Comment 1

[Kumar McMillan [:kumar]]

Attachment: Bug 1277054 - Handle exited add-on actors after reload.

Review commit: https://reviewboard.mozilla.org/r/56650/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56650/

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 8758421 [details]
Bug 1277054 - Handle exited add-on actors after reload.

https://reviewboard.mozilla.org/r/56650/#review53604

It looks like you are working around an issue within BrowserAddonList for the second time.
(I think you worked around that in onInstalled, by removing any previous actor instance)
The actor should no longer be in `_actorByAddonId` Map.
That happens because there is a race when reloading temporary addons,
where `onInstalled` is called first, we call `_onListChanged` which ends up calling `AddonManager.removeAddonListener(this);`,
which, we shouldn't do as soone as `_actorByAddonId` isn't empty. That to ensure cleaning up the map, even if no client called getList() after that.

See `BrowserTabList._checkListening`. We listen for tab close there as soon as the tab actors list isn't empty.

::: devtools/server/tests/unit/test_addon_reload.js:31
(Diff revision 1)
> +        reject(new Error(`${result.error} (see log for details)`));
> +      } else {
> +        resolve(result);
> +      }
> +    });
> +  });

Same here. client.listAddons should return a promise.

::: devtools/server/tests/unit/test_addon_reload.js:48
(Diff revision 1)
> +        } else {
> +          resolve();
> +        }
> +      });
> +  });
> +}

client.request already returns a promise which should be rejected in case of error.

Comment 3

[Kumar McMillan [:kumar]]

Comment on attachment 8758421 [details]
Bug 1277054 - Handle exited add-on actors after reload.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56650/diff/1-2/

Comment 4

[Kumar McMillan [:kumar]]

Comment on attachment 8758421 [details]
Bug 1277054 - Handle exited add-on actors after reload.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56650/diff/2-3/

Comment 5

[Kumar McMillan [:kumar]]

Comment on attachment 8758421 [details]
Bug 1277054 - Handle exited add-on actors after reload.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56650/diff/3-4/

Comment 6

[Kumar McMillan [:kumar]]

Comment on attachment 8758421 [details]
Bug 1277054 - Handle exited add-on actors after reload.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56650/diff/4-5/

Comment 7

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/56650/#review53604

Aha! Thanks. This led me to a more sane fix, let me know what you think. It is still possible for an add-on actor to enter the "exited" state without being uninstalled though -- should I account for that? It is not necessary to fix this reload bug and would be difficult to add a test case for.

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8758421 [details]
Bug 1277054 - Handle exited add-on actors after reload.

https://reviewboard.mozilla.org/r/56650/#review53960

::: devtools/server/actors/webbrowser.js:2325
(Diff revision 5)
>        AddonManager.addAddonListener(this);
>      } else {
> +      if (this._actorByAddonId.size === 0) {
> -      AddonManager.removeAddonListener(this);
> +        AddonManager.removeAddonListener(this);
> -    }
> +      }
> -  }
> +    }

Unfortunately, that isn't that easy.
See checkListener/onListChanged on browsertablist.
Here you won't stop listening, even when uninstalling the last addon.
You would have to move that code calling {add,remove}AddonListener into a seperate function that gets calls on{Un}Installed.

Comment 9

[Kumar McMillan [:kumar]]

Comment on attachment 8758421 [details]
Bug 1277054 - Handle exited add-on actors after reload.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56650/diff/5-6/

Comment 10

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/56650/#review53960

> Unfortunately, that isn't that easy.
> See checkListener/onListChanged on browsertablist.
> Here you won't stop listening, even when uninstalling the last addon.
> You would have to move that code calling {add,remove}AddonListener into a seperate function that gets calls on{Un}Installed.

ok, I see what you mean. The BrowserTabList seems like it has additional complexity we don't need but I have a similar approach now where it adjusts the listener after every listenable action.

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8758421 [details]
Bug 1277054 - Handle exited add-on actors after reload.

https://reviewboard.mozilla.org/r/56650/#review54072

Thanks for addressing this!

::: devtools/server/actors/webbrowser.js:2328
(Diff revision 6)
>  BrowserAddonList.prototype.onInstalled = function (addon) {
>    if (this._actorByAddonId.get(addon.id)) {
>      // 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);
>    }

You may be able to get rid of this.

Comment 12

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/56650/#review54072

Heh, well, it breaks reloading with a stack trace. Thanks for your help figuring out the root cause.

> You may be able to get rid of this.

I tried but it's still necessary. Without it the about:debugging tests fail and I also see the original problem when testing manually without this code. I think the reason is because after a reload, the new add-on gets installed before the old one is uninstalled (as you observed earlier on).

Comment 13

[Kumar McMillan [:kumar]]

Try run looks ok, I think the failure is an unrelated flaky test https://treeherder.mozilla.org/#/jobs?repo=try&revision=05a0f6c69a2e

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/4874ff5d90f8
Handle exited add-on actors after reload. r=ochameau

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4874ff5d90f8