Closed Bug 1277054 Opened 3 years ago Closed 3 years ago

Handle exited add-on actors when reloading via remote debugger

Categories

(DevTools :: Shared Components, defect)

defect
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: kumar, Assigned: kumar)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
Assignee: nobody → kumar.mcmillan
Blocks: 1226743
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.
Attachment #8758421 - Flags: review?(poirot.alex)
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/
Attachment #8758421 - Attachment description: MozReview Request: Bug 1277054 - Handle exited add-on actors after reload. r?ochameau → MozReview Request: Bug 1277054 - Handle exited add-on actors after reload. r=ochameau
Attachment #8758421 - Flags: review?(poirot.alex)
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 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 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/
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.
Attachment #8758421 - Flags: review?(poirot.alex)
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 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/
Attachment #8758421 - Attachment description: MozReview Request: Bug 1277054 - Handle exited add-on actors after reload. r=ochameau → Bug 1277054 - Handle exited add-on actors after reload.
Attachment #8758421 - Flags: review?(poirot.alex)
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 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.
Attachment #8758421 - Flags: review?(poirot.alex) → review+
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).
Try run looks ok, I think the failure is an unrelated flaky test https://treeherder.mozilla.org/#/jobs?repo=try&revision=05a0f6c69a2e
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/4874ff5d90f8
Handle exited add-on actors after reload. r=ochameau
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4874ff5d90f8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.