Closed
Bug 1277054
Opened 6 years ago
Closed 6 years ago
Handle exited add-on actors when reloading via remote debugger
Categories
(DevTools :: Shared Components, defect)
DevTools
Shared Components
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 | ||
Comment 1•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/56650/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56650/
Attachment #8758421 -
Flags: review?(poirot.alex)
Comment 2•6 years ago
|
||
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)
Assignee | ||
Comment 3•6 years ago
|
||
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)
Assignee | ||
Comment 4•6 years ago
|
||
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/
Assignee | ||
Comment 5•6 years ago
|
||
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/
Assignee | ||
Comment 6•6 years ago
|
||
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/
Assignee | ||
Comment 7•6 years ago
|
||
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.
Updated•6 years ago
|
Attachment #8758421 -
Flags: review?(poirot.alex)
Comment 8•6 years ago
|
||
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.
Assignee | ||
Comment 9•6 years ago
|
||
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)
Assignee | ||
Comment 10•6 years ago
|
||
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•6 years ago
|
||
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+
Assignee | ||
Comment 12•6 years ago
|
||
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).
Assignee | ||
Comment 13•6 years ago
|
||
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
Comment 14•6 years ago
|
||
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
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4874ff5d90f8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•4 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•