One of the next challenges of the ongoing transition to WebExtension addons fully able to run in a separated addon process is going to be make them debuggable. The goal of this issue is identify (and then split in sub-issues) the changes needed to make the WebExtensions Addon Debugging oop-compatible.

Some initial information: - the ext-backgroundPage.js API implementation script, that is responsible of the creation of the background pages, is currently using the AddonManager component to store the background page content window (once it has been created) as the preferred addon debugging global, and then to reset it to null (when the page is going to be destroyed), and this is probably the first part that is currently not OOP compatible (because the AddonManager is going to potentially run in a different process, the main process, from the background page content window, which is going to live in the addon process, once the webext-oop transition has been completed). - the new WebExtensionActor introduced in Firefox 50, which is the backend of the WebExtension Addon Debugger (running on the "debuggee / server side"), extends the TabActor (which is good because it is the same interface exposed to make oop tabs debuggable, and so we can probably model most of our oop-compatible actor using similar strategies), but it is still using some of the integration strategies of the previous AddonActor which are probably going to be oop-incompatible (e.g. it is assuming that the addon is running in the main process, it is currently using the AddonManager to be notified of the addon lifecycle events and to retrieve the preferred debugging global of the target addon) Link to interesting pieces of code for this issue: - current AddonManager usage in ext-backgroundPage.js - http://searchfox.org/mozilla-central/rev/6b94deded39a868f248a492e74e05733d6c4ed48/toolkit/components/extensions/ext-backgroundPage.js#101 - http://searchfox.org/mozilla-central/rev/6b94deded39a868f248a492e74e05733d6c4ed48/toolkit/components/extensions/ext-backgroundPage.js#114 - the WebExtensionActor source file - http://searchfox.org/mozilla-central/rev/6b94deded39a868f248a492e74e05733d6c4ed48/devtools/server/actors/webextension.js - where the WebExtensionActor instances are created: - http://searchfox.org/mozilla-central/rev/6b94deded39a868f248a492e74e05733d6c4ed48/devtools/server/actors/webbrowser.js#2260 - where the regular tab actor is connected to the child process (which is part of what makes the tabs running in the content process debuggable) - http://searchfox.org/mozilla-central/rev/6b94deded39a868f248a492e74e05733d6c4ed48/devtools/server/actors/webbrowser.js#374 - where the debugger server running in the child process is injected: - http://searchfox.org/mozilla-central/rev/6b94deded39a868f248a492e74e05733d6c4ed48/devtools/server/main.js#1007

This is one of the two major blockers for OOP extensions at this point. If we can't get it fixed soon, I think we're going to have to switch extensions between in-process and OOP mode whenever the debugger is enabled or disabled.

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. Hi :ochameau and :jryans, we are preparing the WebExtensions addons to be able to run in their own extension process, and this is now one of the main blockers. The attached draft patch is my current proposal to make the WebExtensions Debugging oop-compatible. This patch has the following goals: - change the actor to be oop-compatible, provide the same features and pass all the existent tests with the new implementation. - use a single implementation for both the oop modes (enabled or disabled) - keep the change clean and as small as possible From a DevTools point of view, the strategy (basically based on how the existent "connect to a tab in a content process" scenario is implemented) includes: - split the existent WebExtensionActor (and its responsabilities) into a WebExtensionParentActor and a WebExtensionChildActor - the WebExtensionParentActor is created on the listTabs RDP request, runs in the main process, subscribes itself as an AddonListener on the AddonManager (to receive, handle and forward the addon lifecycle events), and connect itself to its child counterpart (like the BrowserTabActor and the ContentTabActor do) - a WebExtensionChildActor runs in the extension process, is created using DebuggerServer.connectToChild when the parent's connect method is called, currently when the addon are listed like it happens for the tabs, and it behaves like a ChromeActor/TabActor with the addon as its target. - adapt devtools/server/main.js and devtools/server/child.js to the new "connect to a webextension running in an extension process" scenario Before diving into the rest of the details of the patch and its lines of code, I'd like to get an initial feedback from you on the proposed approach (e.g. if I'm reading the architecture correctly, if the strategy seems reasonable) From a "WebExtensions internals" point of view, there is a bunch of removed code (e.g. background page doesn't need to explicitly update the preferred debugging global, and its related test can be removed as well) and the addition of a new exported helper (e.g. exported by ExtensionParent.jsm): the new helper provides a "browser" element that can be used with the DebuggerServer.connectTo method to connect the parent actor with the extension process (it doesn't need to be a browser element from an existent extension view, e.g. because the particular webextension doesn't have a background page, but it is better to get it from the WebExtensions internals so that it is created with the required oop-mode, the same used for all the webextensions addons).

Push to try of the attached patch (still running): https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c71554897b0ff09149ec2f9b30a868311040fb1&selectedJob=72025940

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. The basic approach seems reasonable to me! I only skimmed for now, but seems logical from what I saw and your description.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6) > Comment on attachment 8830498 [details] > Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. > > The basic approach seems reasonable to me! I only skimmed for now, but > seems logical from what I saw and your description. Thanks for the taking a look at it, an initial feedback was the confirmation that I needed, I'm going to move this asap to its next step.

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. Hi Kris, I asked an initial feedback from the DevTools point of view first, mostly because 99% of the strategy is about changes in the devtools actor and the devtools internals, now that we got a positive initial feedback from the DevTools team of the proposed plan, I would like to ask you the same: an initial feedback on the general plan (and its draft patch as attached to this issue). Here is a summary of the interesting pieces from a "WebExtension internals" point of view: - define a new ExtensionParent.createDebuggingTargetPage utility method which provides a "extension process browser element" (with the oop enabled or disabled accordingly with the webext-oop preference) to the RemoteDebugging Actor which runs in the parent process (used to be able to connect to the "extension process", start its child counterpart and exchange messages with it through the messagemanager) - https://reviewboard.mozilla.org/r/107256/diff/1#index_header - remove everything related to the addon debugging from the ext-backgroundPage.js API script (because it is not needed anymore, the new debugging actor running in the extension process is going to discover and select the available extension page global if any) - https://reviewboard.mozilla.org/r/107256/diff/1#index_header Let me know how it sounds/looks to you, and as usually I would love to have a chat with you if anything is not clear enough.

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. Sorry for the feedback delay... I would like to know a bit more to have a complete point of view. You always mention "the webextension process", singular. Is it going to be just one for all addons? Also, it is just for curiosity, but you seem to mentioned that the AddonManager itsself is going to live in that process?? (It would mess up with your usage of it in webbrowser.js/webextension-parent.js) I'm really wondering what Krys things about createDebuggingTargetPage. It looks a bit hacky to me. Isn't there an already existing interface to connect to the extension process? If there is only one process, there is no need to spawn one browser per addon, but share one instead. One thing to acknowledge, but I imagine you already know. It means that we will start loading a bunch of random stuff in the addon process: devtools module loader, debugger server, actors and tons of dependencies. That shouldn't be an issue, as it is only when devtools are involved, but something to keep in mind if you have any perf issue or memory/cpu limitation (mobile?). It used to be an issue on FirefoxOS on crappy phones where we blew up the memory... I miss the overall gigantic big picture. But are all API definitions live in the addon process? For ex, ext-devtools.js, which uses gDevTools should live in the parent. Do you think some API, like inspectedWindow you are currently working on, will have to live in the addon process? I imagine you will have magicall cross process API replication? About debug:webext_parent_exit/debug:webext_parent_reload that looks so dumb to do such parent>child>parent with all these message manager.... but I can't think about anything else. It would be handy to have hybrid actors being able to have methods defined in the parent! I don't quite fully understand the new logic around preferredTargetWindow, it seems to arbitrary pick the first window that comes. The story around TabActor.docShell getter being overloaded is fuzzy. It has to be overloaded as TabActor.docShell is throwing an error message if not. In webextension.js, it is only overloaded from createFallbackWindow. The naming of this function is at least misleading! As it has to be called otherwise something wrong is going to happen with this throwing docShell getter. It makes me thing this code isn't doing quite exactly what we expect. Otherwise, yes, overall the usage of connectToChilld looks good. We should just confirms bits around the number of processes and what lives where.

Hi Alex, > Sorry for the feedback delay... > I would like to know a bit more to have a complete point of view. sure, I'm happy to provide as much details as needed (and thank you for the detailed comments and questions, that give me the opportunity to add more context) > You always mention "the webextension process", singular. Is it going to be just one for all addons? > Also, it is just for curiosity, but you seem to mentioned that the AddonManager itsself is going to live in that process?? (It would mess up with your usage of it in webbrowser.js/webextension-parent.js) My apologies, I should have detailed this part in my previous comment, follows some additional details on "the extension process" - extensions will (at least currently) run in a single extension process, - there is a "dom.processCount.extension" preference which will be set to 1 (but at the same time it means that it can be changed at some point) - there is a "extensions.webextensions.remote" preference that enable/disable the webext-oop mode Kris can correct me if any of the above details are wrong. Going back to the WebExtensions Addon Actors: The parent actor is the only part that interacts with the AddonsManager (basically main goal of this change is "remove any usage of the AddonsManager from the code running in the extension process"). > I'm really wondering what Krys things about createDebuggingTargetPage. It looks a bit hacky to me. Isn't there an already existing interface to connect to the extension process? If there is only one process, there is no need to spawn one browser per addon, but share one instead. yeah, that part is still pretty "rough", I didn't expanded it too much because I want to hear Kris about it. Sharing the "browser" target between all the instances is not something that I am excluding (e.g. it could be lazily created and shared through the utility method provided by the WebExtensions jsm modules, and destroyed when there are no parent actors instances) but I opted to get an initial feedback before starting to refine the proposal further. > One thing to acknowledge, but I imagine you already know. It means that we will start loading a bunch of random stuff in the addon process: devtools module loader, debugger server, actors and tons of dependencies. That shouldn't be an issue, as it is only when devtools are involved, but something to keep in mind if you have any perf issue or memory/cpu limitation (mobile?). It used to be an issue on FirefoxOS on crappy phones where we blew up the memory... yeah, that's a good point. the most common platform is definitely the desktop browser, but nothing in the implementation is desktop specific (and being able to debug a WebExtension running on Firefox for Android is something that we definitely want). I'm not sure if the extension process will be enabled on both both Desktop and Mobile at the same time, but if it is not, the new implementation is still able to work when the extensions run in the main process, and with the extension process disabled, there shouldn't be a big overhead compared to the current version of the addon debugging actor, am I right? > I miss the overall gigantic big picture. But are all API definitions live in the addon process? For ex, ext-devtools.js, which uses gDevTools should live in the parent. Do you think some API, like inspectedWindow you are currently working on, will have to live in the addon process? I imagine you will have magicall cross process API replication? That is right, the WebExtensions internals have abstractions for the most common scenarios: - if the API method needs to run something in the parent process, you write the parent side of the API, the API methods often return a promise that will be resolved/rejected, on the child extension process side an "auto-generate proxies" will receive the results of the promise resolve/reject (that need to be clonable beacause they are sent over the message manager messages) - for some of API methods the "auto-generated proxies" are not enough and we want to run custom code in the child extension process (e.g. because the method synchronously return a value or it needs to return something that only exists in the child extension process, e.g. the window of an extension page), in that case you explicitly register an API implementation that will run only in the child extension process (and sometimes you will need custom code for both sides of the API method) by taking the devtools API as an example: - the devtools internals are only used from the parent process - the inspectedWindow.tabId is implemented as a "child extension process"-side API module, which synchronously returns the tabId as it has been received (from the parent process) during the extension page creation - the inspectedWindow.eval and reload are completely implemented on the "parent /main process"-side, and on the "child extension process"-side the "auto-generated proxies" are enough - the devtools.panels.create API needs an API module in both the "parent/main" and "child extension" process, the parent part interacts with the devtools internals, the child part provides the panel API object that this method passes as its callback parameter > About debug:webext_parent_exit/debug:webext_parent_reload that looks so dumb to do such parent>child>parent with all these message manager.... but I can't think about anything else. It would be handy to have hybrid actors being able to have methods defined in the parent! I totally agree that this "direct" interactions between the parent and the child actor are pretty "primitive", I looked into the other actors to search other similar scenarios and any recent "best practice", but the only one seems to be the pair "BrowserTabActor/ContentActor" pair which uses the same strategy, and so I deferred any decision on it after the initial feedback. > I don't quite fully understand the new logic around preferredTargetWindow, it seems to arbitrary pick the first window that comes. The story around TabActor.docShell getter being overloaded is fuzzy. It has to be overloaded as TabActor.docShell is throwing an error message if not. In webextension.js, it is only overloaded from createFallbackWindow. The naming of this function is at least misleading! As it has to be called otherwise something wrong is going to happen with this throwing docShell getter. It makes me thing this code isn't doing quite exactly what we expect. The docShell property should be correctly updated using the inherited TabActor method _changeTopLevelDocument(window) (http://searchfox.org/mozilla-central/source/devtools/server/actors/tab.js#1154,1163,1174,1181). The new "auto-selection" of a WebExtensions global implements the following behavior: - it creates a fallback window that is used when no real extension global (an extension page window) has been found - when the auto-discovery of the globals starts, if there is no preferredTargetWindow, the first real window from the target WebExtension becomes the preferredTargetWindow (which basically means that if the WebExtensions has a background page, the background page window will be selected as the preferredTargetWindow) - if the webextension has no persistent extension page (basically: no background page/scripts), then the fallback window will be the default target, until a webextension global has been created (e.g. if the user want to debug a webextension popup, once the popup is opened, the tools switch from the fallback page to the popup window) - when all the real extension pages are destroyed (e.g. the extension has been reloaded or disabled/enabled), the addon debugger windows switch to the fallback window (like in the current version) - when the extension is uninstalled, the addon debugger window is closed (like in the current version) In my opinion this new implementation has the following advantages over the current one: - it doesn't need any integration in the defined Extension Pages (e.g. like the current integrations between the ext-backgroudPage.js API module with the AddonManager to set/unset the preferred global) - it selects the extension popup window automatically if the current target is the fallback window (e.g. the background page) > Otherwise, yes, overall the usage of connectToChilld looks good. We should just confirms bits around the number of processes and what lives where. Thanks again for the detailed feedback, let me know if the above details are enough to complete the picture (and if they are not, I'm happy to add more details).

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. https://reviewboard.mozilla.org/r/107256/#review118900 ::: devtools/server/actors/webextension.js:221 (Diff revision 1) > /** > * Return true if the given source is associated with this addon and should be > * added to the visible sources (retrieved and used by the webbrowser actor module). > */ > -WebExtensionActor.prototype._allowSource = function (source) { > - try { > +WebExtensionChildActor.prototype._allowSource = function (source) { > + if (!source.url.startsWith("moz-extension:")) { I'd rather we try to avoid hard coding this assumption in more places than we need to. ::: toolkit/components/extensions/ExtensionParent.jsm:831 (Diff revision 1) > +function createDebuggingTargetPage(addonId) { > + const fakeDebuggingExtension = { > + id: addonId, > + remote: ExtensionManagement.useRemoteWebExtensions, > + }; > + > + return new HiddenExtensionPage(fakeDebuggingExtension, "addon-debugging-target-page"); > +} I really don't like the idea of this. Why can't we just use the background page?

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. Looks fine, aside from the fakeDebuggingExtension thing.

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. https://reviewboard.mozilla.org/r/107256/#review118900 > I'd rather we try to avoid hard coding this assumption in more places than we need to. would be an utility method exported from one of the WebExtensions JSM files a reasonable alternative? (so that the above becomes something like `if (!isWebExtensionURL(source.url)) { ...}`) > I really don't like the idea of this. Why can't we just use the background page? I totally agree that this is the main point to discuss and to agree on. Like described in a bit more detail in Comment 10, I didn't expanded too much this part because I wanted to discuss about it with you in more detail. I think that a reasonable way to reach an agreement on this, it can be to start by discussing about the requirements of the approach: As described in last part of Comment 4, what we need is a "browser" element that can be used with the DebuggerServer.connectTo method to connect the parent actor (running in the main process) with the child actor (running in the extension process). This browser element cannot be the one from the background page because some addons do not have a background page at all (e.g. an extension can have a popup window associated to a browser or page action or a devtools panel, but no background page) It would be great to use a browser element that is not related to any of the existent view (if there is any, because like described above, it is not always true), because the Addon Debugger should be able (like it does in its current form) to persist across the extension reloads (and on the contrary, it would be even more annoying to use the Addon Debugger if the addon is livereload on every file change like web-ext already does by default). A solution that seems to fit into the above requirements is to create an additional browser element that is created when the addon debugging actor is started (the browser element can be eventually be shared between all the addon debugging actors if we can reasonable assume that we are going to have a single extension process). If you think that we have a reasonable alternatives to this approach, I'm obviously open to discuss and to experiment with the additional proposed approaches, or we can evaluate if, with the necessary changes, we can derive a better approach from the current proposal.

(In reply to Luca Greco [:rpl] from comment #13) > > I'd rather we try to avoid hard coding this assumption in more places than we need to. > > would be an utility method exported from one of the WebExtensions JSM files > a reasonable alternative? > (so that the above becomes something like `if (!isWebExtensionURL(source.url)) { ...}`) I'd rather we just check the addonId property of the principal. > This browser element cannot be the one from the background page because some > addons do not have a background page at all (e.g. an extension can have a > popup window associated to a browser or page action or a devtools panel, but > no background page) Let's just create a background page if one doesn't already exist.

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. https://reviewboard.mozilla.org/r/107256/#review118900 > would be an utility method exported from one of the WebExtensions JSM files a reasonable alternative? > (so that the above becomes something like `if (!isWebExtensionURL(source.url)) { ...}`) We cannot check the source's document principal from the _allowSource method: this method only receive a json object that represents the new source detected and not the document itself. In the updated patches, I opted for filtering out any "resource" and "chrome" urls [1] and then just using extensionURIToAddon on the remaining to detect and allow the sources related to the target extension. [1]: "resource" and "chrome" urls are received from this devtools actor when the Firefox internals are loaded, they should not be visible in the addon debugger and extensionURIToAddon raises and log an expection on non "moz-extension" url schemes, and so I'm filtering them out before retrieving the addonID so that we don't log an huge amount of errors related to this, esplecially given that we already know that are not actual errors)

(In reply to Kris Maglione [:kmag] from comment #14) > > This browser element cannot be the one from the background page because some > > addons do not have a background page at all (e.g. an extension can have a > > popup window associated to a browser or page action or a devtools panel, but > > no background page) > > Let's just create a background page if one doesn't already exist. Like I briefly explained over IRC: if we use the messageManager of the browser element from one of the extension pages (one of the existent or even a new one like suggested above) to connect to the extension process, then that browser element will be destroyed during the extension reload and the addon debugger is going to be closed automatically (because the connection with the child actor is lost). This is not how the Addon Debugger currently works: it is able to stay open and to work correctly across addon reloads (it is automatically closed if the addon has been uninstalled), closing the Addon Debugger automatically when the addon reload would provide a very poor user experience for the addon developer (even more when the Addon Debugger is used from a "web-ext run" session, which will reload the addon automatically on every the source change). Changing the devtools toolbox and devtools panels to make them able to "stay open and to work correctly when the browser element used to connect to the child process is destroyed and then recreated" seems like a change that is going to have a pretty big impact on the devtools internals, in my own opinion it can be a reasonable option only if the DevTools team thinks that is "the only way to go" (obviously I may be wrong, :jryans or :ochameau can correct me if I'm). In the meantime, I've updated and splitted the patches with the following goals: - cleanup and refactor the currently proposed approach (and I also tried to keep an eye on how the approach could evolve if we decide to switch from a single child extension process to multiple extension processes, I'm going to add more details about this in a separate comment) and make the "debugging utils" exported by the ExtensionParent.jsm module much more explicit (and refactoring out the pieces shared with the HiddenExtensionPage into separate HiddenXULWindow class, then reused by both the HiddenExtensionPage and the new ExtensionDebuggingUtils). - split the devtools actor running in the main process (so that we don't connect the parent actor to the child actor for all the addons when the root.listAddons RDP method is called, on the contrary this new proposed version creates the child actor instance only when an Addon Debugger has been opened and only for the target webextension). Anyway, if this is still far from what you would like to, it would be of great help if I can get much more detailed information about what is exactly that you dislike about the proposed approach and why (e.g. any issues that you see and/or you would like to prevent, these details will help me to be sure that I can reply to the explicited doubts and issues that you see with better comments, as well as changing the approach to address them). Thanks in advance!

Comment on attachment 8849613 [details] Bug 1302702 - Provide a DebugUtils object from ExtensionParent.jsm. https://reviewboard.mozilla.org/r/122400/#review124582 ::: toolkit/components/extensions/ExtensionParent.jsm:926 (Diff revision 1) > + // TODO(rpl): if we switch to use more than one extension process, we have to be sure > + // that this browser's frameLoader is associated to the same process using the > + // `sameProcessAsFrameLoader` property. > + // (http://searchfox.org/mozilla-central/source/dom/interfaces/base/nsIBrowser.idl) > + // > + // At that point we are going to do the same for the browser elements related to > + // the extensions page, and we have to keep track of the existent browsers > + // associated to a webextension to ensure that they are all running in the > + // same process (and at that point we can retrieve one of these tracked extensions > + // browser elements and use it to configure this one, and there is already ancestors > + // addon debugging browser associated to the extenson, e.g. because the addon has been > + // reloaded by the user, we can use it to ensure that the new extension pages runs in > + // the same process). I took this note while I was exploring how to change the previous proposal and how switching from a single extension child process to multiple extension child processes could affect the extension debugging if we go for the proposed approach: Basically, if we switch to multiple extension child processes, we have to change the WebExtensions internals to be absolutely sure that all the extension pages related to the same webextension are going to run in the same process (which is absolutely needed to make API methods like `extension.getViews` to work correctly). One reasonable way to do it (at least if we don't have other features that I didn't know about and/or we add other features to achieve this) can probably be to use the browser's `sameProcessAsFrameLoader` property, and in that case we could do the same for the browser elements created by ExtensionDebuggingUtils to be used in the WebExtensionParentActor to connect to the right child process.

As briefly discussed yesterday, we agreed with Kris that we can proceed with the proposed approach: - to connect the webextension debugging actor to the right extension process we are going to use a browser element provided by debugging helpers exported by one of the webextension JSM modules (so that we can keep the connection open across the extension reloads) - "extension debugging" browser element is not used to load any extension page, it is lazily created once the webextension debugging actor only is explicitly connected by an addon debugger toolbox (instead of being connected when the addon are listed as happens for the listTabs method) - "extension debugging" browser element is destroyed when the debugging actor is destroyed (basically when the addon debugger is closed) Now that we agreed on the general approach, we can proceed by moving the patches into the review phase.

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. Hi Alex, In Attachment #8830498 [details] there is the new version of the patch that splits the webextension addon debugging actor into the parent and child actors, besides the changes related to the proposed debugging utils helpers to be exported from the webextension JSM module (which I flagged for review from Kris, now that we agreed on the general approach), there are also some changes related to the actor that I'd like to ask you a preliminary feedback. The pieces of this patch that needs from additional feedback are the ones related to the split between the webextension actor that provides the addon metadata for the root.listAddons RDP method (e.g. are used by the about:debugging to list all the installed addons) and the webextension parent addon actor that provides the actual debugging target of the addon debugger toolbox. The main reason is that I would prefer to defer the connection to the child extension process to the phase when an actual addon debugger is opened instead of connecting every installed addon when the about:debugging page is loaded and lists all the addons. In the meantime I cleared the r? flag on these patches, until I'm sure that it will not change further during the Kris's review of attachment #8849613 [details].

Worth a mention that in Bug 1354614 (and Bug 1354596) we are also planning to migrate away from listTabs for reason pretty similar to the ones that I described in Comment 23 as the reasons for splitting the actor that can provide the addon metadata from the one that is going to connect to the child extension process.

Comment on attachment 8849613 [details] Bug 1302702 - Provide a DebugUtils object from ExtensionParent.jsm. https://reviewboard.mozilla.org/r/122400/#review130606 ::: toolkit/components/extensions/ExtensionParent.jsm:684 (Diff revision 1) > + throw new Error("Unable to shutdown an unloaded HiddenXULWindow instance"); > + } > + > + this.unloaded = true; > + > + if (this._windowlessBrowser) { This check should not be necessary. ::: toolkit/components/extensions/ExtensionParent.jsm:685 (Diff revision 1) > + } > + > + this.unloaded = true; > + > + if (this._windowlessBrowser) { > + this._windowlessBrowser.loadURI("about:blank", 0, null, null, null); Not necessary. ::: toolkit/components/extensions/ExtensionParent.jsm:691 (Diff revision 1) > + get chromeShell() { > + return this._windowlessBrowser.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDocShell) > + .QueryInterface(Ci.nsIWebNavigation); > + } Please just store this rather than using a getter. ::: toolkit/components/extensions/ExtensionParent.jsm:698 (Diff revision 1) > + .getInterface(Ci.nsIDocShell) > + .QueryInterface(Ci.nsIWebNavigation); > + } > + > + get chromeDocument() { > + return this._windowlessBrowser ? this._windowlessBrowser.document : null; No need to check for `this._windowlessBrowser` here. ::: toolkit/components/extensions/ExtensionParent.jsm:708 (Diff revision 1) > + if (this._waitInitialized) { > + return this._waitInitialized; This shouldn't be called more than once, so no need to store this promise. ::: toolkit/components/extensions/ExtensionParent.jsm:719 (Diff revision 1) > + // The windowless browser is a thin wrapper around a docShell that keeps > + // its related resources alive. It implements nsIWebNavigation and > + // forwards its methods to the underlying docShell, but cannot act as a > + // docShell itself. Calling `getInterface(nsIDocShell)` gives us the > + // underlying docShell, and `QueryInterface(nsIWebNavigation)` gives us > + // access to the webNav methods that are already available on the > + // windowless browser, but contrary to appearances, they are not the same > + // object. This comment needs to stay near the relevant code. It has nothing to do with the code directly below it. ::: toolkit/components/extensions/ExtensionParent.jsm:850 (Diff revision 1) > + /** > + * Retrieve a XUL browser element which has been configured to be able to connect > + * the devtools actor with the process where the extension is running. > * > - * @returns {Promise<XULDocument>} > - * a promise which resolves to the newly created XULDocument. > + * @param {WebExtensionParentActor} webExtensionParentActor > + * the devtools actor that is retrieving the browser element. Nit: Capitalize, align with opening brace of type name. ::: toolkit/components/extensions/ExtensionParent.jsm:859 (Diff revision 1) > - // The invisible page is currently wrapped in a XUL window to fix an issue > - // with using the canvas API from a background page (See Bug 1274775). > - let windowlessBrowser = Services.appShell.createWindowlessBrowser(true); > + if (!this.hiddenXULWindow) { > + this.hiddenXULWindow = new HiddenXULWindow(); > + } > - this.windowlessBrowser = windowlessBrowser; > > - // The windowless browser is a thin wrapper around a docShell that keeps > + await this.hiddenXULWindow.initWindowlessBrowser(); Do this once, when you create the hiddenXULWindow, store promise, await on subsequent calls. ::: toolkit/components/extensions/ExtensionParent.jsm:863 (Diff revision 1) > - .getInterface(Ci.nsIDocShell) > - .QueryInterface(Ci.nsIWebNavigation); > > - return this.initParentWindow(chromeShell).then(() => { > - return promiseDocumentLoaded(windowlessBrowser.document); > - }); > + const extension = GlobalManager.getExtension(webExtensionParentActor.addonId); > + if (!extension) { > + throw new Error("Extension not found: `${addonId}`"); You have the backquotes and double quotes mixed up. ::: toolkit/components/extensions/ExtensionParent.jsm:871 (Diff revision 1) > + let isBrowserRemote = browser.getAttribute("remote") === "true"; > + > + // Remove the cached addon debugging browser element if it has not been configured > + // to use the same process mode of the target extension. > + if (isBrowserRemote !== extension.remote) { `extension.remote != browser.isRemoteBrowser` ::: toolkit/components/extensions/ExtensionParent.jsm:877 (Diff revision 1) > + > + // Remove the cached addon debugging browser element if it has not been configured > + // to use the same process mode of the target extension. > + if (isBrowserRemote !== extension.remote) { > + browser.remove(); > + browser = null; Clear the map entry. ::: toolkit/components/extensions/ExtensionParent.jsm:882 (Diff revision 1) > + browser = await this._createExtensionProcessBrowser(extension); > + this.debuggingBrowsersMap.set(webExtensionParentActor, browser); What happens if this is called reentrantly while you're waiting for the browser to be created? If that shouldn't be possible, set the entry in the browser map to null, and assert that it's still null after the new browser is created. If it should, please find some way to handle it. ::: toolkit/components/extensions/ExtensionParent.jsm:895 (Diff revision 1) > - * > - * @param {nsIDocShell} chromeShell > + * it destroys the XUL browser element, and it also destroy the hidden XUL window > + * if it is not currently needed. > - * the docShell related to initialize. > * > - * @returns {Promise<nsIXULDocument>} > - * the initialized parent chrome document. > + * @param {WebExtensionParentActor} webExtensionParentActor > + * the devtools actor that has retrieved an addon debugging browser element. Capitalize and align. ::: toolkit/components/extensions/ExtensionParent.jsm:899 (Diff revision 1) > - * @returns {Promise<nsIXULDocument>} > - * the initialized parent chrome document. > + * @param {WebExtensionParentActor} webExtensionParentActor > + * the devtools actor that has retrieved an addon debugging browser element. > */ > - initParentWindow(chromeShell) { > - if (PrivateBrowsingUtils.permanentPrivateBrowsing) { > - let attrs = chromeShell.getOriginAttributes(); > + releaseExtensionProcessBrowser(webExtensionParentActor) { > + const browser = this.debuggingBrowsersMap.get(webExtensionParentActor); > + this.debuggingBrowsersMap.delete(webExtensionParentActor); Move inside `if`. ::: toolkit/components/extensions/ExtensionParent.jsm:914 (Diff revision 1) > + const browser = chromeDoc.createElement("browser"); > + browser.setAttribute("type", "content"); > + browser.setAttribute("disableglobalhistory", "true"); > + browser.setAttribute("webextension-addon-debugging-target", extension.id); > + > + let awaitFrameLoader = Promise.resolve(); > + > + if (extension.remote) { > + browser.setAttribute("remote", "true"); > + browser.setAttribute("remoteType", extension.remote); > + awaitFrameLoader = promiseEvent(browser, "XULFrameLoaderCreated"); Why are we duplicating all of this rather than just reusing the logic in HiddenExtensionPage? ::: toolkit/components/extensions/ExtensionParent.jsm:926 (Diff revision 1) > + // TODO(rpl): if we switch to use more than one extension process, we have to be sure > + // that this browser's frameLoader is associated to the same process using the > + // `sameProcessAsFrameLoader` property. > + // (http://searchfox.org/mozilla-central/source/dom/interfaces/base/nsIBrowser.idl) > + // > + // At that point we are going to do the same for the browser elements related to > + // the extensions page, and we have to keep track of the existent browsers > + // associated to a webextension to ensure that they are all running in the > + // same process (and at that point we can retrieve one of these tracked extensions > + // browser elements and use it to configure this one, and there is already ancestors > + // addon debugging browser associated to the extenson, e.g. because the addon has been > + // reloaded by the user, we can use it to ensure that the new extension pages runs in > + // the same process). This comment applies everywhere that we use remote browsers, so I'm not sure it makes sense to have it here. Feel free to add a comment to the top of an appropriate file, instead. ::: toolkit/components/extensions/ExtensionParent.jsm:932 (Diff revision 1) > + // that this browser's frameLoader is associated to the same process using the > + // `sameProcessAsFrameLoader` property. > + // (http://searchfox.org/mozilla-central/source/dom/interfaces/base/nsIBrowser.idl) > + // > + // At that point we are going to do the same for the browser elements related to > + // the extensions page, and we have to keep track of the existent browsers s/existent/existing/ ::: toolkit/components/extensions/ExtensionParent.jsm:965 (Diff revision 1) > * the Extension on which we are going to listen for the newly created ExtensionProxyContext. > * @param {string} params.viewType > - * the viewType of the WebExtension page that we are watching (e.g. "background" or "devtools_page"). > + * the viewType of the WebExtension page that we are watching (e.g. "background" or "devtools_page"). > * @param {XULElement} params.browser > - * the browser element of the WebExtension page that we are watching. > + * the browser element of the WebExtension page that we are watching. > * > * @param {Function} onExtensionProxyContextLoaded > - * the callback that is called when a new context has been loaded (as `callback(context)`); > + * the callback that is called when a new context has been loaded (as `callback(context)`); > * > * @returns {Function} > * Unsubscribe the listener. Fix capitalization and indentation.

Comment on attachment 8849615 [details] Bug 1302702 - Remove from ext-backgroundPage any code that uses the AddonManager object. https://reviewboard.mozilla.org/r/122404/#review130608 Please also remove the relevant code from XPIProvider.jsm.

In the updated attachment 8849613 [details] I applied the changes based on the review comments from Comment 25, plus a number of improvements that I was already preparing (e.g. how to handle an extension that has been started before the preference that enable the oop extensions has been switched, and then it is reloaded after the preference switch, in that case the extension changes is remote mode and the addon debugger is closed, because it is connected to the wrong process and it have to be reopened to be reattached to the right process). In the new attachment 8857092 [details] I added an initial version of a new xpcshell test to "test unit" the DebugUtils, in particular it currently tests that that only the expected number of debugging resources has been allocated by the DebugUtils and that they are deallocated as expected when released. (Part of this scenarios is also tested by attachment 8849614 [details], which tests that the Addon Actor releases the allocated DebugUtils resources when it is closed, but the aboutdebugging mochitest-browser tests are pretty slow and I want to explicitly tests more scenarios) I'm going to create more tests in the next updates on the patches attached to this issue. I didn't removed the "addon debugging" related code from the XPIProvider yet in attachment 8849615 [details] (as suggested in Comment 26): the addon.setDebugGlobal can still be theoretically used by bootstrapped addons (system addons, and the other bootstrap/SDK addons at least until Firefox 57) to be able to customize their own preferred debug global during the addon development, but I'm completely open to remove it (maybe in as a follow up issue?).

(In reply to Luca Greco [:rpl] from comment #32) > I didn't removed the "addon debugging" related code from the XPIProvider yet > in attachment 8849615 [details] (as suggested in Comment 26): > the addon.setDebugGlobal can still be theoretically used by bootstrapped > addons (system addons, and the other bootstrap/SDK addons at least until > Firefox 57) to be able to customize their own preferred debug global during > the addon development, but I'm completely open to remove it (maybe in as a > follow up issue?). It's not used outside of WebExtensions code at the moment, and since bootstrapped add-ons are going away, there's not much point to keeping it for the sake of potential uses in the future.

Comment on attachment 8849613 [details] Bug 1302702 - Provide a DebugUtils object from ExtensionParent.jsm. https://reviewboard.mozilla.org/r/122400/#review132146 ::: toolkit/components/extensions/ExtensionParent.jsm:741 (Diff revision 2) > + } > + > + /** > + * Creates the browser XUL element that will contain the WebExtension Page. > + * > + * @param {Object} xulAttributes Nit: Remove extra whitespace. ::: toolkit/components/extensions/ExtensionParent.jsm:761 (Diff revision 2) > + > + const browser = chromeDoc.createElement("browser"); > + browser.setAttribute("type", "content"); > + browser.setAttribute("disableglobalhistory", "true"); > + > + for (const name in xulAttributes) { for (const [name, value] of Object.entries(attributes)) ::: toolkit/components/extensions/ExtensionParent.jsm:762 (Diff revision 2) > + const browser = chromeDoc.createElement("browser"); > + browser.setAttribute("type", "content"); > + browser.setAttribute("disableglobalhistory", "true"); > + > + for (const name in xulAttributes) { > + browser.setAttribute(name, xulAttributes[name]); We need to not call `setAttribute` when this is null. A browser `remote="null"` attribute has undefined behavior. ::: toolkit/components/extensions/ExtensionParent.jsm:767 (Diff revision 2) > + browser.setAttribute(name, xulAttributes[name]); > + } > + > + let awaitFrameLoader = Promise.resolve(); > + > + if (browser.isRemoteBrowser) { This won't work. `isRemoteBrowser` is a binding attribute, and bindings aren't applied until the browser has been inserted into the document and a reflow has occurred. ::: toolkit/components/extensions/ExtensionParent.jsm:896 (Diff revision 2) > - createWindowlessBrowser() { > - // The invisible page is currently wrapped in a XUL window to fix an issue > - // with using the canvas API from a background page (See Bug 1274775). > - let windowlessBrowser = Services.appShell.createWindowlessBrowser(true); > - this.windowlessBrowser = windowlessBrowser; > + async getExtensionProcessBrowser(webExtensionParentActor) { > + const extensionId = webExtensionParentActor.addonId; > + const extension = GlobalManager.getExtension(extensionId); > + if (!extension) { > + throw new Error(`Extension not found: ${extensionId}`); > + } > > - // The windowless browser is a thin wrapper around a docShell that keeps > - // its related resources alive. It implements nsIWebNavigation and > - // forwards its methods to the underlying docShell, but cannot act as a > - // docShell itself. Calling `getInterface(nsIDocShell)` gives us the > - // underlying docShell, and `QueryInterface(nsIWebNavigation)` gives us > - // access to the webNav methods that are already available on the > + const waitForBrowser = () => { > + const promisedBrowser = (async () => { > + if (!this.hiddenXULWindow) { > + this.hiddenXULWindow = new HiddenXULWindow(); > + this.watchExtensionUpdated(); > + } > - // windowless browser, but contrary to appearances, they are not the same > - // object. > - let chromeShell = windowlessBrowser.QueryInterface(Ci.nsIInterfaceRequestor) > - .getInterface(Ci.nsIDocShell) > - .QueryInterface(Ci.nsIWebNavigation); > > - return this.initParentWindow(chromeShell).then(() => { > - return promiseDocumentLoaded(windowlessBrowser.document); > + return this.hiddenXULWindow.createBrowserElement({ > + "webextension-addon-debug-target": extensionId, > + "remote": extension.remote ? "true" : null, > + "remoteType": E10SUtils.EXTENSION_REMOTE_TYPE, > + }); > + })(); > + > + // If another promise has been already stored, > + // remove the browser and reject this call. > + if (this.debugBrowserPromisesMap.get(extensionId)) { > + promisedBrowser.then((browser) => browser.remove()); > + throw new Error(`Unexpected cached browser for ${extensionId}`); > + } > + > + this.debugBrowserPromisesMap.set(extensionId, promisedBrowser); > + promisedBrowser.catch(error => { > + // Remove the cached promise from the map if it has been rejected. > + this.debugBrowserPromisesMap.delete(extensionId); > + throw error; > - }); > + }); > + > + return promisedBrowser; > + }; > + > + let waitBrowser = this.debugBrowserPromisesMap.get(extensionId); > + > + // Create a new promise if there is no cached one in the map. > + if (!waitBrowser) { > + this.debugBrowserPromisesMap.set(extensionId, null); > + waitBrowser = waitForBrowser(); > + } > + > + if (!this.debugActorsWeakMap.has(waitBrowser)) { > + this.debugActorsWeakMap.set(waitBrowser, new Set()); > - } > + } > > + this.debugActorsWeakMap.get(waitBrowser).add(webExtensionParentActor); > + > + return waitBrowser; > + }, debugActors: new DefaultWeakMap(() => new Set()), async getExtensionProcessBrowser(webExtensionParentActor) { const extensionId = webExtensionParentActor.addonId; const extension = GlobalManager.getExtension(extensionId); if (!extension) { throw new Error(`Extension not found: ${extensionId}`); } const createBrowser = () => { if (!this.hiddenXULWindow) { this.hiddenXULWindow = new HiddenXULWindow(); this.watchExtensionUpdated(); } return this.hiddenXULWindow.createBrowserElement({ "webextension-addon-debug-target": extensionId, "remote": extension.remote ? "true" : null, "remoteType": E10SUtils.EXTENSION_REMOTE_TYPE, }); }; let browserPromise = this.debugBrowserPromises.get(extensionId); // Create a new promise if there is no cached one in the map. if (!browserPromise) { browserPromise = createBrowser(); this.debugBrowserPromises.set(extensionId, browserPromise); browserPromise.catch(() => { this.debugBrowserPromises.delete(extensionId); }); } this.debugActors.get(browserPromise).add(webExtensionParentActor); return browserPromise; }, ::: toolkit/components/extensions/ExtensionParent.jsm:996 (Diff revision 2) > * @param {object} params.extension > * the Extension on which we are going to listen for the newly created ExtensionProxyContext. > * @param {string} params.viewType > - * the viewType of the WebExtension page that we are watching (e.g. "background" or "devtools_page"). > + * the viewType of the WebExtension page that we are watching (e.g. "background" or "devtools_page"). > * @param {XULElement} params.browser > - * the browser element of the WebExtension page that we are watching. > + * the browser element of the WebExtension page that we are watching. > * > * @param {Function} onExtensionProxyContextLoaded > - * the callback that is called when a new context has been loaded (as `callback(context)`); > + * the callback that is called when a new context has been loaded (as `callback(context)`); Nit: Fix alignment and indentation ::: toolkit/components/extensions/ExtensionParent.jsm:1002 (Diff revision 2) > * the Extension on which we are going to listen for the newly created ExtensionProxyContext. > * @param {string} params.viewType > - * the viewType of the WebExtension page that we are watching (e.g. "background" or "devtools_page"). > + * the viewType of the WebExtension page that we are watching (e.g. "background" or "devtools_page"). > * @param {XULElement} params.browser > - * the browser element of the WebExtension page that we are watching. > + * the browser element of the WebExtension page that we are watching. > * Nit: Remove empty line. ::: toolkit/components/extensions/ExtensionParent.jsm:1003 (Diff revision 2) > * @param {Function} onExtensionProxyContextLoaded > - * the callback that is called when a new context has been loaded (as `callback(context)`); > + * the callback that is called when a new context has been loaded (as `callback(context)`); > * > * @returns {Function} Nit: s/Function/function/

Comment on attachment 8849613 [details] Bug 1302702 - Provide a DebugUtils object from ExtensionParent.jsm. https://reviewboard.mozilla.org/r/122400/#review133222 ::: toolkit/components/extensions/ExtensionParent.jsm:763 (Diff revision 3) > + // Skip null and undefined values. > + if (value == null) { > + continue; > + } > + > + browser.setAttribute(name, value); if (value != null) { browser.setAttribute(name, value); } No need for the comment.

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. Hi Alex, I'd like to get an initial feedback on the approach proposed in this patch to defer the "connection of the extension process" as much as possible (the new patch connects the extension process only when a "addon debugging" toolbox is going to be opened).

Comment on attachment 8857092 [details] Bug 1302702 - ExtensionParent DebugUtils xpcshell test unit. https://reviewboard.mozilla.org/r/128986/#review133882 ::: toolkit/components/extensions/test/xpcshell/test_ext_debugging_utils.js:48 (Diff revision 3) > + // Switch the remote mode preference to recreate the scenario of an addon debugger > + // opened on a target addon started as non-remote, the reloaded after the oop > + // extension mode has been enabled. > + Preferences.set("extensions.webextensions.remote", true); This isn't something I particularly want to support, so I'd rather we not add tests for it.

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. Re-new feedback requested on Comment 49. (the previous feedback request has been removed when I've updated the xpcshell test included in another patch from this queue, no changes have been applied on this patch by the last pushed update)

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. https://reviewboard.mozilla.org/r/107256/#review134838 Looks good at high level but you should simplify the big picture between WebExtensionActor, WebExtensionParentActor and WebExtensionChildActor. There is only two actors: one in the parent with addon metadata and reload/uninstall actions, and the second one in the child, which is the tabActor required for the toolbox setup. ::: devtools/client/framework/toolbox-process-window.js:56 (Diff revision 7) > + const {requestTypes} = yield gClient.request({ > + to: addonActor.actor, > + type: "requestTypes", > + }); > + > + if (requestTypes.includes("connect")) { This is really weird and looks very complex. I see it very differently. We have two actors the Parent and the Child, they are different and it is better to see them as being different. Parent is just exposing some metadata about the addon and allows to reload it. One last feature of Parent is to be able to fetch the Child actor via the connect() method. Child ends up being a TabActor for the given add-on and doesn't contain much requests/attributes specifics to add-ons. It basically is the TabActor we hand over the toolbox. So here I would do: let { addons } = yield gClient.listAddons(); let addonActor = addons.filter(addon => addon.id === addonID).pop(); if (addonActor.isWebExtension) { // For WebExtension, the add-on may live in a distinct process and we need to fetch the TabActor from this process via the connect request const {form} = yield gClient.request({ to: addonActor.actor, type: "connect", }); addonActor = form; } There may be value in using this intermediate requestTypes request for compatibility against old gecko when debugging remotely, but I'm not sure it is worth it? Do we support debugging WebExtension on fennec? In any case, we never do duck typing like this via requestTypes, instead we put flags on RootActor or TabActor via `traits` object: http://searchfox.org/mozilla-central/source/devtools/server/actors/root.js#117 ::: devtools/server/actors/webbrowser.js:17 (Diff revision 7) > var { DebuggerServer } = require("devtools/server/main"); > var DevToolsUtils = require("devtools/shared/DevToolsUtils"); > > loader.lazyRequireGetter(this, "RootActor", "devtools/server/actors/root", true); > loader.lazyRequireGetter(this, "BrowserAddonActor", "devtools/server/actors/addon", true); > -loader.lazyRequireGetter(this, "WebExtensionActor", "devtools/server/actors/webextension", true); > +loader.lazyRequireGetter(this, "WebExtensionActor", "devtools/server/actors/webextension-parent", true); Exported symbol name should match module name. And it makes sense for me to rename WebExtensionActor to WebExtensionParentActor. (while renaming or getting rid of the existing WebExtensionParentActor) ::: devtools/server/actors/webbrowser.js (Diff revision 7) > - } > + } > - deferred.resolve([...this._actorByAddonId].map(([_, actor]) => actor)); > + > + resolve([...this._actorByAddonId].map(([_, actor]) => actor)); > + }); > }); > - return deferred.promise; It is better to have a dedicated patch for such cleanup. Also we tend to refactor all at once in a given file. Here you are only refactoring one method so that we end up with multiple patterns (promise versus Promise) in the same file. ::: devtools/server/actors/webextension-parent.js:71 (Diff revision 7) > + return false; > + }, > + > + connect() { > + if (!this._addonDebuggingActor) { > + this._addonDebuggingActor = new WebExtensionParentActor(this.conn, this); Naming is wrong for WebExtensionParentActor this isn't an actor, this is just an helper. There is too many things with the same name and too many "actors" here. WebExtensionParentActor is an helper to keep the parent<>child communication code in a distinct class. (I'm wondering if that would be easier to merge it into WebExtensionActor? Or just make it be a single function with a couple of inlined functions for listeners) Anyway, the name should be something else. what about AddonProcessConnect / TabActorFetch / ChildActorConnect / ... ? There is no need to repeat "WebExtension" for an helper in this file that is not exported. We know this is about web extension. Also, if you end up keeping _addonDebuggingActor (I think it could be removed), you can make it shorter. _debuggingActor/_childActor/... would be enough. ::: devtools/server/actors/webextension-parent.js:72 (Diff revision 7) > + }, > + > + connect() { > + if (!this._addonDebuggingActor) { > + this._addonDebuggingActor = new WebExtensionParentActor(this.conn, this); > + this._waitDebuggingActorConnected = this._addonDebuggingActor.connect(); nit: instead of saving connect() returned value, you can save the form and return it after: this._form = this._addonDebuggingActor.connect() .then(() => this._addonDebuggingActor.form()); } return this._form; ::: devtools/server/actors/webextension-parent.js:122 (Diff revision 7) > +/** > + * Creates the actor that represents the addon in the parent process, which connects > + * itself to a WebExtensionChildActor counterpart which is created in the > + * extension process (or in the main process if the WebExtensions OOP mode is disabled). > + * > + * The WebExtensionParentActor subscribes itself as an AddonListener on the AddonManager "subscribes itself as an AddonListener on the AddonManager" This is no longer true ::: devtools/server/actors/webextension-parent.js:149 (Diff revision 7) > + this._browser = null; > + this._childActorID = null; > +} > + > +WebExtensionParentActor.prototype = { > + actorPrefix: "webExtensionParent", This is useless ::: devtools/server/actors/webextension-parent.js:186 (Diff revision 7) > + this._browser.messageManager || > + this._browser.frameLoader.messageManager); > + }, > + > + form() { > + return Object.assign(this._webExtensionActor.form(), this._form); This is really weird, it should be the other way around (from WebExtensionActor.connect()). Also, I wouldn't expose a form function from here, instead I would return `this._form` from WebExtensionParent.connect() (instead of returning `this`) ::: devtools/server/actors/webextension-parent.js:190 (Diff revision 7) > + form() { > + return Object.assign(this._webExtensionActor.form(), this._form); > + }, > + > + exit() { > + AddonManager.removeAddonListener(this); This is useless. ::: devtools/server/actors/webextension-parent.js:199 (Diff revision 7) > + this._mm.sendAsyncMessage("debug:webext_parent_exit", { > + actor: this._childActorID, > + }); > + } > + > + this._webExtensionActor.onDebuggingActorExit(this); Can't we call this._webExtensionActor.exit()? Can we really call onDebuggingActorExit with mixed up instances? ::: devtools/server/actors/webextension.js:107 (Diff revision 7) > - iconURL: this.addon.iconURL, > - debuggable: this.addon.isDebuggable, > - temporarilyInstalled: this.addon.temporarilyInstalled, > - isWebExtension: this.addon.isWebExtension, > }); > }; It looks like we could remove this `form` overload completely as actor and id look useless. actor is useless for sure. id looks redundant with the one set by WebExtensionActor. ::: devtools/server/actors/webextension.js:226 (Diff revision 7) > + // Filter out resource and chrome sources (which are related to the loaded internals). > + if (["resource", "chrome"].includes(uri.scheme)) { > + return false; > + } > + > + let addonID = this.aps.extensionURIToAddonId(uri); nit: you may move `try {` to line 225, just before the call to extensionURIToAddonId as this try/catch is there for just this function call. ::: devtools/server/actors/webextension.js:251 (Diff revision 7) > + global.document.nodePrincipal.addonId == this.id) { > + if (this.attached) { > + this.preferredTargetWindow = global; > + this._changeTopLevelDocument(this.preferredTargetWindow); > + } > + } This looks hacky, but I haven't reviewed the preferredTargetWindow story in detail. Note that if that helps you can iterate over all existing top level window in this child with this: let e = Services.ww.getWindowEnumerator(null); while (e.hasMoreElements()) { let window = e.getNext(); } (Be careful it will return top level xul windows in the parent process!)

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. https://reviewboard.mozilla.org/r/107256/#review134838 Thanks Alex for the detailed feedback, it has been very helpful. I've pushed a round of updates to address the last round of review comments, and I'm going to take another look at the child actor part to check if I can simplify it before your next review (because I didn't touch this part of the changes in the last rounds of updates) > This is really weird and looks very complex. > I see it very differently. > We have two actors the Parent and the Child, they are different and it is better to see them as being different. > Parent is just exposing some metadata about the addon and allows to reload it. One last feature of Parent is to be able to fetch the Child actor via the connect() method. > Child ends up being a TabActor for the given add-on and doesn't contain much requests/attributes specifics to add-ons. It basically is the TabActor we hand over the toolbox. > > So here I would do: > let { addons } = yield gClient.listAddons(); > let addonActor = addons.filter(addon => addon.id === addonID).pop(); > if (addonActor.isWebExtension) { > // For WebExtension, the add-on may live in a distinct process and we need to fetch the TabActor from this process via the connect request > const {form} = yield gClient.request({ > to: addonActor.actor, > type: "connect", > }); > addonActor = form; > } > > There may be value in using this intermediate requestTypes request for compatibility against old gecko when debugging remotely, but I'm not sure it is worth it? Do we support debugging WebExtension on fennec? > In any case, we never do duck typing like this via requestTypes, instead we put flags on RootActor or TabActor via `traits` object: > http://searchfox.org/mozilla-central/source/devtools/server/actors/root.js#117 With the changes to the connect page it is going to be possible to connect a webextension on fennec using the connect page. I wasn't unsure if the traits was a better way to detect it because I noticed that recently we are also using a new actorHasMethod helper (http://searchfox.org/mozilla-central/search?q=actorHasMethod&path=), which checks the provided methods on an actor. In the last updated patch I've changed it to use a new trait instead. > It is better to have a dedicated patch for such cleanup. Also we tend to refactor all at once in a given file. Here you are only refactoring one method so that we end up with multiple patterns (promise versus Promise) in the same file. I agree, it was a leftover from the previous version of this patch, I'm going to revert it in the next update (we can cleanup it in a follow up issue, alongside to the "list tabs" counterpart). > Naming is wrong for WebExtensionParentActor > this isn't an actor, this is just an helper. > There is too many things with the same name and too many "actors" here. > WebExtensionParentActor is an helper to keep the parent<>child communication code in a distinct class. > (I'm wondering if that would be easier to merge it into WebExtensionActor? Or just make it be a single function with a couple of inlined functions for listeners) > Anyway, the name should be something else. what about AddonProcessConnect / TabActorFetch / ChildActorConnect / ... ? > There is no need to repeat "WebExtension" for an helper in this file that is not exported. We know this is about web extension. > > Also, if you end up keeping _addonDebuggingActor (I think it could be removed), > you can make it shorter. _debuggingActor/_childActor/... would be enough. Yeah, I agree, this is more like an object that proxies the child actor running in the extension process. I opted to rename the class to ProxyChildActor, and the instance variable to _childActor (the main reason for tracking it is in the parent actor is to provide a disconnect method as a counterpart of the WebExtensionParentActor connect method). > nit: instead of saving connect() returned value, > you can save the form and return it after: > > this._form = this._addonDebuggingActor.connect() > .then(() => this._addonDebuggingActor.form()); > } > > return this._form; I opted to do something similar: connect now returns a promise which resolves to the form object, the parent actor caches the form promise. > "subscribes itself as an AddonListener on the AddonManager" > > This is no longer true yep, this comment was meant to be moved to the actor that is actually listening to the AddonManager events (moved accordingly in the updated patches). > This is really weird, it should be the other way around (from WebExtensionActor.connect()). > Also, I wouldn't expose a form function from here, instead I would return `this._form` from WebExtensionParent.connect() (instead of returning `this`) +1 (I changed it mostly like suggested above in the updated patch). > Can't we call this._webExtensionActor.exit()? > Can we really call onDebuggingActorExit with mixed up instances? This callback is meant to invalidate the last connected child actor in the parent actor, and not destroying the parent actor completely. I looked again into it and it should not be possible to call this callback with mixed up instances, and so I removed the parameter and renamed it to `onProxyChildActorExit`. > It looks like we could remove this `form` overload completely as actor and id look useless. > actor is useless for sure. > id looks redundant with the one set by WebExtensionActor. Right, I'm pretty sure that they were not needed and I forgot to completely remove it in the first draft patch that has splitted the actor into the parent and child pieces. > nit: you may move `try {` to line 225, just before the call to extensionURIToAddonId as this try/catch is there for just this function call. is it possible for the source.url to be something that is not an URL and make Services.io.newURI to raise an exception? (e.g. something like a fake "debugger eval code" url like here: http://searchfox.org/mozilla-central/rev/d8ac097a1de2544f0e51d186bc850662c5b7430a/devtools/server/actors/source.js#59) > This looks hacky, but I haven't reviewed the preferredTargetWindow story in detail. > Note that if that helps you can iterate over all existing top level window in this child with this: > let e = Services.ww.getWindowEnumerator(null); > while (e.hasMoreElements()) { > let window = e.getNext(); > } > (Be careful it will return top level xul windows in the parent process!) Let me take another look to this part of the child actor, to check if I can simplify it further, I didn't touch this part in the last updates yet.

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. Hi Alex, I cleaned up the "child extension process" part of this patch, and it should be now ready for a final round of feedback. In the last round of changes I removed the preferredTargetWindow property (it is definitely not necessary and confusing), and cleaned up the discovery mechanism (which is still hooked to the this._shouldAddNewGlobalAsDebuggee method, because it is the discovery mechanisms that works on both extension child process mode and main process mode, while iterating over the top level windows only works in oop mode, as the windowless browsers are not part of the iterated windows when the extension is running in the main process). Besides that, there are some fixes (e.g. the target should be recognized as a webextension when the connection prefix is not the one assigned when the extension is running in the main process) and some minor tweaks in preparation for the other patches attached to this issues (e.g. open the toolbox in the same process when the extension is running in the extension process), which I can also move in the related patch once we move to the review stage if we prefer.

Comment on attachment 8860912 [details] Bug 1302702 - Shorter extension urls in addon debugger window title and frames list selector. This patch contains a small set of proposed changes to make the extension pages URLs which are shown in the addon debugger window title and in the frame list selector shorter, by removing everything but the pathname (otherwise the only visible part is the "moz-extension" protocol and the long uuid origin, which is not very helpful to the addon developer). (this patch is not mandatory for this issue and we can move it into a followup issue if we prefer)

Comment on attachment 8860915 [details] Bug 1302702 - No need to use the BrowserToolboxProcess to debug OOP WebExtensions. When the target addon is running in the extension process, there is no need to open the addon debugger toolbox in a different process and we can open a toolbox in the main process, which has a number of nice effects: - we can open more than one addon debugger window at once (which is currently prevented by the BrowserToolboxProcess implementation) - we can open an addon debugger window alongside to a browser toolbox (which is currently doable only by connecting one of the two toolbox from another firefox instance) - the user doesn't need to confirm the new debugging connection explicitly through the confirmation dialog. This patch contains a proposed set of changes to the about:debugging client to allow it. (This change is not mandatory for this issue, but it is "a nice to have" and it would also allow to simplify the about:debugging tests related to the oop webextensions, which are currently using a very hacky trick to coordinate the two processes involved in the test)

Comment on attachment 8860913 [details] Bug 1302702 - Fix inspector panel deadwrapper exceptions on addon reloads. Currently, when the inspector panel is initialized and connected to an extension page and the target addon is reloaded, the inspector panel breaks because the inspector is accessing the old window object (which is a DeadWrapper) while processing the onFrameLoad listener for the new document loaded (which has its new valid window object). The small changes in this patch fix this issue, I'm going to create a proper test case for it, but in the meantime I wanted to double-check this with you: it seems to me that the `this.rootWin` has not been updated in the `onFrameLoad` listener by mistake, but I wanted to ask your opinion on it in case I'm missing something about how it works usually when the target is a regular tab.

Comment on attachment 8860912 [details] Bug 1302702 - Shorter extension urls in addon debugger window title and frames list selector. https://reviewboard.mozilla.org/r/132922/#review138886 ::: devtools/client/framework/target.js:526 (Diff revision 1) > event.title = packet.title; > event.nativeConsoleAPI = packet.nativeConsoleAPI; > event.isFrameSwitching = packet.isFrameSwitching; > > - if (!packet.isFrameSwitching) { > - // Update the title and url unless this is a frame switch. > + // Update the title and url unless this is a frame switch or the target > + // is a WebExtension. Why do you block url/title update here? It would be useful to comment about the why rather than paraphrasing the code. ::: devtools/client/framework/toolbox.js:1853 (Diff revision 1) > let checked = frame.id == this.selectedFrameId; > > + let label = frame.url; > + > + if (this.target.isWebExtension) { > + // Show a shorter url for extensions page in the addon debugger. nit: s/in the addon debugger.//

Comment on attachment 8860913 [details] Bug 1302702 - Fix inspector panel deadwrapper exceptions on addon reloads. https://reviewboard.mozilla.org/r/132924/#review138894 ::: devtools/server/actors/inspector.js:2421 (Diff revision 1) > if (this.rootDoc && !Cu.isDeadWrapper(this.rootDoc) && > this.rootDoc.defaultView) { > this.onFrameUnload({ window: this.rootDoc.defaultView }); > } > + // Update the rootWin and rootDoc. > + this.rootWin = window; Good catch! This is stunning anything work without also updating this property... nit: This comment isn't really helpful. What about: "Update all references to DOM objects to target the new document"?

Comment on attachment 8860915 [details] Bug 1302702 - No need to use the BrowserToolboxProcess to debug OOP WebExtensions. https://reviewboard.mozilla.org/r/132928/#review138898 Looks like a great improvement. May be worth being in its own bug given that it changes significantly addon debugging behavior. ::: devtools/client/aboutdebugging/modules/addon.js:52 (Diff revision 2) > +async function debugWebExtension(target) { > + const client = new DebuggerClient(DebuggerServer.connectPipe()); > + await client.connect(); > + > + const {addons} = await client.listAddons(); > + let addonActor = addons.filter(addon => addon.id === target.addonID).pop(); What about reusing about:debugging client? (target.client) You would only keep the call to connect. ::: devtools/client/aboutdebugging/modules/addon.js:62 (Diff revision 2) > + let options = {form, client, chrome: true, isTabActor: true}; > + const addonTarget = await TargetFactory.forRemoteTab(options); > + > + const hostType = Toolbox.HostType.WINDOW; > + const toolbox = await gDevTools.showToolbox(addonTarget, "webconsole", hostType); > + toolbox.once("destroyed", () => client.close()); (do not forget to remove that if you reuse the same client) ::: devtools/client/aboutdebugging/modules/addon.js:64 (Diff revision 2) > + > + const hostType = Toolbox.HostType.WINDOW; > + const toolbox = await gDevTools.showToolbox(addonTarget, "webconsole", hostType); > + toolbox.once("destroyed", () => client.close()); > + } else { > + client.close(); (Same here)

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. https://reviewboard.mozilla.org/r/107256/#review138788 ::: devtools/client/framework/connect/connect.js:185 (Diff revision 11) > + }); > + > + // Connect the toolbox to the extension process sub-actor. > + addon = form; > + } > + } This code would be better shared in the target file, in this method: http://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#373-452 (rather than duplicated here and in toolbox-process-window) ::: devtools/client/framework/target.js:357 (Diff revision 11) > > get isAddon() { > return !!(this._form && this._form.actor && ( > this._form.actor.match(/conn\d+\.addon\d+/) || > - this._form.actor.match(/conn\d+\.webExtension\d+/) > + this._form.actor.match(/conn\d+\.webExtension\d+/) || > + this._form.actor.match(/child\d+\/webExtension\d+/) It would be easier to maintain by doing: return !!(this._form && this._form.actor && this._form.actor.match(/conn\d+\.addon\d+/)) || this.isWebExtension; ::: devtools/client/framework/toolbox-process-window.js:54 (Diff revision 11) > - openToolbox({ > - form: addonActor, > - chrome: true, > - isTabActor: addonActor.isWebExtension ? true : false > + let isTabActor = false; > + > + if (addonActor.isWebExtension) { > + isTabActor = true; > + > + if (gClient.mainRoot.traits.webExtensionAddonConnect) { I'm still wondering if this trait is needed at all. Does it allow debugging WebExt when connecting on existing Fennec release [53,54,55] (The one that won't have this patch)? If any piece of this patch is needed to debug fennec extension, this trait is not necessary. ::: devtools/server/actors/webextension-parent.js:76 (Diff revision 11) > + name: this.addon.name, > + iconURL: this.addon.iconURL, > + debuggable: this.addon.isDebuggable, > + temporarilyInstalled: this.addon.temporarilyInstalled, > + isWebExtension: true, > + isExtensionProcessConnected: this.isExtensionProcessConnected, This field (isExtensionProcessConnected) doesn't seem to be used anywhere? And so isExtensionProcessConnected getter is also unused. ::: devtools/server/actors/webextension-parent.js:86 (Diff revision 11) > + if (!this._childActor) { > + this._childActor = new ProxyChildActor(this.conn, this); > + this._waitChildActorForm = this._childActor.connect(); > + } > + > + return this._waitChildActorForm.then((form) => { nit: you may omit parentheses for functions with just one argument ::: devtools/server/actors/webextension-parent.js:97 (Diff revision 11) > + iconURL: this.addon.iconURL, > + }); > + }); > + }, > + > + disconnect() { What is your intent here? Is it a request method? Looks like it as I can see it referenced in the spec file. But it collides with the now obsolete "disconnect" function, which is called whenever the connection is closed (on client disconnection). This is now called "destroy": http://searchfox.org/mozilla-central/source/devtools/server/actors/common.js#260-267 But as you do not use this request anywhere, we can drop this, or rename it to destroy and make exit() call destroy(). ::: devtools/server/actors/webextension-parent.js:162 (Diff revision 11) > + > + // Called when the debug browser element has been destroyed > + // (no actor is using it anymore to connect the child extension process). > + const onDestroy = () => { > + this.exit(); > + }; nit: onDestroy = this.exit.bind(this); ::: devtools/server/actors/webextension-parent.js:170 (Diff revision 11) > + > + this._form = await DebuggerServer.connectToChild(this._conn, this._browser, onDestroy, > + {addonId: this.addonId}); > + > + // Set the isOOPExtension attribute on the connected child actor form. > + this._form.isOOPExtension = this._browser.isRemoteBrowser; nit: what about calling this "isOOP"? nit2: Also, can it be done differently from WebExtensionParentActor.connect()? That, to prevent having to define this form object in three different places. It would also prevent having to call connect to know if that's oop. But do not spend too much time on this, this is just if that's as-easy. ::: devtools/server/actors/webextension-parent.js:186 (Diff revision 11) > + return this._browser && ( > + this._browser.messageManager || > + this._browser.frameLoader.messageManager); > + }, > + > + exit() { nit: what about calling this destroy rather than exit? (as it doesn't directly relates to TabActor.exit) ::: devtools/server/actors/webextension-parent.js:202 (Diff revision 11) > + > + ExtensionParent.DebugUtils.releaseExtensionProcessBrowser(this); > + > + this._parentActor = null; > + this._browser = null; > + this._childActorID = null; nit: this._form = null; this._mm = null; ? ::: devtools/server/actors/webextension.js:56 (Diff revision 11) > + * @param {string} addonId > + * the addonId of the target WebExtension. > */ > -function WebExtensionActor(conn, addon) { > +function WebExtensionChildActor(conn, chromeGlobal, prefix, addonId) { > ChromeActor.call(this, conn); > + this.traits.reconfigure = false; Is it any usefull (reconfigure = false) It looks like this is only checked here: http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/netmonitor-controller.js#280 And this "supportsPerfStats" isn't used anywhere. ::: devtools/server/actors/webextension.js:92 (Diff revision 11) > - this.preferredTargetWindow = null; > - this._findAddonPreferredTargetWindow(); > + // URL shown in the window tittle when the addon debugger is opened). > + let extensionWindow = this._discoveryExtensionPage(); > > - AddonManager.addAddonListener(this); > + if (extensionWindow) { > + this._setWindow(extensionWindow); > -} > + } Can't it be done only during _attach? Here you are going to call setWindow twice: during actor instanciation and on attach call. ::: devtools/server/actors/webextension.js:166 (Diff revision 11) > > -/** > - * Discover the preferred debug global and switch to it if the addon has been attached. > - */ > -WebExtensionActor.prototype._findAddonPreferredTargetWindow = function () { > - return new Promise(resolve => { > +// Discovery an extension page to use as a default target window. > +// NOTE: This currently fail to discovery an extension page running in a > +// windowless browser when running in non-oop mode, and the background page > +// is discovered later by the listener of the "new-extension-page" event. > +WebExtensionChildActor.prototype._discoveryExtensionPage = function () { nit: I would name this: _getAnyExtensionPage, _searchForExtensionWindow, ... ::: devtools/server/actors/webextension.js:184 (Diff revision 11) > +// Customized ChromeActor/TabActor hooks. > + > +WebExtensionChildActor.prototype._onDocShellDestroy = function (docShell) { > + if (this.attached && docShell == this.docShell) { > + // Change top level window as a "non-frame switching" navigation. > + this._changeTopLevelDocument(this.fallbackWindow, false); Why do you have to fake a "non-frame switching" event here? Do you reproduce this codepath locally or via a test? It looks like this code is redundant with this one: http://searchfox.org/mozilla-central/source/devtools/server/actors/tab.js#789-808 Which should be called by the next line: ChromeActor.prototype._onDocShellDestroy.call(this, docShell); ::: devtools/server/actors/webextension.js:196 (Diff revision 11) > + // NOTE: we need to be sure that `this.window` can return a > + // window before calling the ChromeActor.onAttach, or the TabActor > + // will not be subscribed to the child doc shell updates. > + > + // Create a fallback window to use during addon reloads. > - this._createFallbackWindow(); > + this._createFallbackWindow(); Shouldn't we call this only if we have to use this.fallbackWindow? ::: devtools/server/actors/webextension.js:206 (Diff revision 11) > + if (extensionWindow) { > + this._setWindow(extensionWindow); > + } else { > + // If no extension page has been found, select the fallback window. > + this._setWindow(this.fallbackWindow); > - } > + } nit: this._setWindow(extensionWindow || this.fallbackWindow); ::: devtools/server/actors/webextension.js:215 (Diff revision 11) > + this.window.document.nodePrincipal.addonId === this.id) { > + return; > + } > + > + // Change top level document as a simulated frame switching > + this._changeTopLevelDocument(window, true); I don't get why you would switch to *any* new addon document being created. I see that you are working around the addon reload. But let's imaging the addon creates a second document lazily, when you click on some UI. It will instanciate a new doc, fire a new event like this one and the toolbox is going to switch to the new document whereas the one you were debugging is still alive. And you would not expect to switch to the new one. ::: devtools/server/actors/webextension.js:283 (Diff revision 11) > - return global.document.nodePrincipal.addonId == this.id; > + const isExtensionWindow = global.document.nodePrincipal.addonId == this.id; > + > + // Emit a new-extension-window event if it is a valid extension window > + // for the target extension. > + if (isExtensionWindow && global.location) { > + events.emit(this, "new-extension-window", global); I don't see the value of this new event. Especially given that this is only listened from this same actor instance. It would be much easier to just call another function, like _watchForExtensionWindow but on WebExtensionChildActor prototype. Actually, you may just inline _watchForExtensionWindow right here as isExtensionWindow is similar to the check being done in _watchForExtensionWindow. ::: devtools/shared/specs/webextension-parent.js:24 (Diff revision 11) > + response: { form: RetVal("json") }, > + }, > + > + disconnect: { > + request: { }, > + }, As said in another comment, this request doesn't seem to be used.

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. Thanks luca, I have a couple of questions but it is getting much simplier! Please r? your next version.

Comment on attachment 8860912 [details] Bug 1302702 - Shorter extension urls in addon debugger window title and frames list selector. https://reviewboard.mozilla.org/r/132922/#review138886 > Why do you block url/title update here? > It would be useful to comment about the why rather than paraphrasing the code. The reason for blocking the url/title update is Bug 1261687 (Detached devtools window title changes when selecting another frame), I've added an inline comment to explain in more detail when and why the url/title are not updated.

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. https://reviewboard.mozilla.org/r/107256/#review138788 > This code would be better shared in the target file, in this method: > http://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#373-452 > (rather than duplicated here and in toolbox-process-window) yeah, that is much better! I moved this code into the target method, and now the change applied to connect.js and toolbox-process-window.js is simpler and cleaner. > It would be easier to maintain by doing: > return !!(this._form && this._form.actor && this._form.actor.match(/conn\d+\.addon\d+/)) || this.isWebExtension; I agree, that is definitely better, updated as suggested. > I'm still wondering if this trait is needed at all. > Does it allow debugging WebExt when connecting on existing Fennec release [53,54,55] (The one that won't have this patch)? > > If any piece of this patch is needed to debug fennec extension, this trait is not necessary. With the change applied to connect.js it is going to be possible to connect to a remote addon on a previous Fennec release that doesn't have any of this change (it was already possible to open a toolbox connected to a remote addon, but the connect page wasn't using the WebExtensions actor as a tab actor, and so only the reduced toolbox was opened, after this changes on connect.js, the connect page will be able to open the complete toolbox for a remote webextension). > This field (isExtensionProcessConnected) doesn't seem to be used anywhere? > And so isExtensionProcessConnected getter is also unused. I was thinking about including this field (as well as the disconnect method) for a programmatic usage of the webextension addon actor from an external RDP client (e.g. I'd like to make web-ext, the CLI tool, able to connect the addon actor and print the filtered console messages and errors in a follow up) and test them in an additional test in 'devtools/server/tests'. Anyway, these are not mandatory changes for this bugzilla issue, and it would be better to handle them separately in a follow up bugzilla issue. I've removed this field (and the disconnect method) in the meantime. > What is your intent here? > Is it a request method? Looks like it as I can see it referenced in the spec file. > But it collides with the now obsolete "disconnect" function, which is called whenever the connection is closed (on client disconnection). > This is now called "destroy": > http://searchfox.org/mozilla-central/source/devtools/server/actors/common.js#260-267 > > But as you do not use this request anywhere, we can drop this, or rename it to destroy and make exit() call destroy(). Like described above, I was including the disconnect method as a method to use from an external RDP client to disconnect a connected webextension parent actor without closing the RDP connection. Like for the isExtensionProcessConnected field I opted to remove it completely now, and handle both f them in a follow up bugzilla issue. > nit: what about calling this "isOOP"? > > nit2: Also, can it be done differently from WebExtensionParentActor.connect()? > That, to prevent having to define this form object in three different places. It would also prevent having to call connect to know if that's oop. > But do not spend too much time on this, this is just if that's as-easy. I would like to make it a property of the parent actor form, unfortunately when it is requested for the first time by the about:debugging page, the addon has been installed, but it is still inizializating and it has not yet been defined in the ExtensionParent.jsm. But I renamed it into isOOP as suggested, and I'm not setting the form properties all in one place in the WebExtensionParentActor.connect method (based on a new isOOP getter defined by the child actor proxy). > nit: what about calling this destroy rather than exit? (as it doesn't directly relates to TabActor.exit) agree, renamed it to destroy. > nit: this._form = null; this._mm = null; ? I added `this._form = null` (I haven't added `this._mm`, because it is a getter and it is going to return undefined as soon as `this._browser` is set to null) > Is it any usefull (reconfigure = false) > It looks like this is only checked here: > http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/netmonitor-controller.js#280 > And this "supportsPerfStats" isn't used anywhere. Removed (it was part of the previous version, but I totally agree that if the only usage of it is unused, there is no reason to set that trait). > Can't it be done only during _attach? > Here you are going to call setWindow twice: during actor instanciation and on attach call. The main reason for doing it here is to be able to return the first url and title which are going to be rendered in the window title of the addon debugging toolbox. > nit: I would name this: _getAnyExtensionPage, _searchForExtensionWindow, ... renamed to _searchForExtensionWindow > Why do you have to fake a "non-frame switching" event here? > Do you reproduce this codepath locally or via a test? > It looks like this code is redundant with this one: > http://searchfox.org/mozilla-central/source/devtools/server/actors/tab.js#789-808 > Which should be called by the next line: > ChromeActor.prototype._onDocShellDestroy.call(this, docShell); I was thinking about make it more explicit when we are changing the top level document because the current selected frame has been destroyed (vs. changing the top level document while the previous one is still alive), but it is not really needed, and so I've reverted this part of the change in the updated version of this patch. The redefined `_onDocShellDestroy` is still needed, and I've added an inline comment to make it clear its behavior: - the call to `ChromeActor.prototype._onDocShellDestroy` will notify the client that a frame has been destroyed and it will always change the top level document to an empty about:blank document related to the browser XUL element used by the parent actor to connect to the extension process - after that call, if needed (basically if the destroyed docshell was the currently selected top level frame), we create and switch the actor to the fallback page. > Shouldn't we call this only if we have to use this.fallbackWindow? yep, patch updated to only create the fallbackWindow when it is needed. > I don't get why you would switch to *any* new addon document being created. I see that you are working around the addon reload. But let's imaging the addon creates a second document lazily, when you click on some UI. It will instanciate a new doc, fire a new event like this one and the toolbox is going to switch to the new document whereas the one you were debugging is still alive. And you would not expect to switch to the new one. yeah, the previous version wasn't implementing the correct behavior, I've updated it and its behavior should be: When a new extension window has been detected, switch the actor to it only if there is not current window set, or if the current set window is the fallback window, while the top level document is not changes in all the other cases. > I don't see the value of this new event. Especially given that this is only listened from this same actor instance. > It would be much easier to just call another function, like _watchForExtensionWindow but on WebExtensionChildActor prototype. > Actually, you may just inline _watchForExtensionWindow right here as isExtensionWindow is similar to the check being done in _watchForExtensionWindow. Removed the event and opted for a call to a method (named _onNewExtensionWindow). > As said in another comment, this request doesn't seem to be used. disconnect method has been removed (I'm going to move it into a follow up as described in more details in one of the previous comments)

Hi Alex, besides the change described in the above mozreview comments, another important change that is part of the updated patch is the strategy used to recognize the globals and sources to list in the addon debugging toolbox: While investigating Bug 1361575 I noticed that by filtering the allowed globals and sources using the document principal addonId we are currently filtering out any iframe with a "non extension url" added into an extension window (which is allowed, e.g. when the addon specify the http/https url into the addon permission and then adds an iframe pointed to it into a background page, a popup window etc.) Given that this issue is already changing this part of the actor, I thought that it was better to immediately check how to change the actor to make these globals and sources accessible from the addon debugging toolbox: In the updated version of the actor, both the `_allowSource` and `_shouldAddNewGlobalAsDebuggee` methods are now checking if the related global is an extension window or a descendent of an extension window, using the two new helper methods `isExtensionWindow` and `isExtensionWindowDescendent`.

Comment on attachment 8866437 [details] Bug 1302702 - Add devtools webextension actor mochitest-chrome unit tests. https://reviewboard.mozilla.org/r/138054/#review141228 ::: devtools/server/tests/mochitest/test_webextension-addon-debugging-connect.html:78 (Diff revision 1) > +add_task(function* test_webextension_addon_debugging_connect_inprocess() { > + yield setWebExtensionOOPMode(false); > + yield test_connect_addon(false); > +}); > + > +add_task(function* test_webextension_addon_debugging_connect_oop() { > + yield setWebExtensionOOPMode(true); > + yield test_connect_addon(true); > +}); The current webextension debugging tests are part of the aboutdebugging test suite, which is useful because they test that everything works from a user point of view, by using the exact same components that the user is going to interact with (e.g. the about:debugging page, the spawned browser toolbox process and the real developer tools clients running in the toolbox panels). On the other hand, there are also a number of cons: - the about:debugging webextensions debugging tests are painfully slow and prone to intermittency (mostly because they load everything and they run in the browser toolbox process that has to be recreated for every test) - these tests are currently running the webextension only in non-oop mode and the hack used to orchestrate the test code running in the two different process would need some tweaks now that there are three process involved, but running these tests twice (in oop and non-oop mode) could make the issue with these tests even worse For these reason, and to be able to achieve a better test coverage (both from a "covered lines" and from a "covered scenarios" point of views), I would like to add an additional layer of tests that do not involve the toolbox, but only a test webextension and the RDP actors, the DebuggerClient and the actors Fronts. This patch contains an initial set composed of 2 new test files added to the devtools/server/test/mochitest dir as chrome mochitests for an initial feedback. Both the tests runs in oop and oop-mode (in the webextensions test suite most of the tests in both the modes and so we solve this differently at a directory level, we can use the same approach if we prefer, but given that only a small subset of this directory of tests will need to run twice I though to propose a file based approach first).

Comment on attachment 8866437 [details] Bug 1302702 - Add devtools webextension actor mochitest-chrome unit tests. https://reviewboard.mozilla.org/r/138054/#review141486 Yes, having actor tests is a great idea! I'm just wondering if that can be written using xpcshell? Does WebExtension works on xpcshell? Not a big deal, it is mostly because it is faster and server actors are tested using xpcshell whenever we can. Please move tab.js modification in its own changeset. ::: devtools/server/actors/tab.js:1218 (Diff revision 1) > > + // No need to do anything more if the actor is not attached anymore > + // e.g. the client has been closed and the actors destroyed in the meantime. > + if (!this.attached) { > + return; > + } Why not doing that at first line of _windowReady? Also, do you know what is the STR to get this? The code calling windowReady most probably shouldn't be executed if the actor isn't attached.

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. Manually re-new the r? flag on this patch, in comment 93 there are also some notes about the last changes applied on this patch when I pushed the previous round of updates. (for some reason mozreview and bugzilla have two different opinion of the r? flag on these patches, and the r? has not been auto-renewed on the last updates pushed on mozreview)

Comment on attachment 8857092 [details] Bug 1302702 - ExtensionParent DebugUtils xpcshell test unit. Manually re-new the r? flag on this patch, which contains the changes suggested by Kris in Comment 50 (and the brief discussion that Kris and I had over IRC). (the r? has not been auto-renewed on the last updates pushed on mozreview)

Comment on attachment 8866437 [details] Bug 1302702 - Add devtools webextension actor mochitest-chrome unit tests. https://reviewboard.mozilla.org/r/138054/#review141486 Yeah, my plan is to port some of these tests into the xpcshell test suite in a follow up, we already ported a lot of the webextensions tests into xpcshell tests, not everything that the webextensions support can be tested in a xpcshell test but for most of these tests it should be enough. I decided to create these mochitests first because I was absolutely sure that there wasn't anything that would prevent them to be able to recreate the needed testing environment, nevertheless I totally agree and I will prepare an xpcshell-based testing environment in a follow up of this issue. In the updated version of this patch: - the tab.js modification has been moved in its own changeset - added some changes in the webextensions-helpers.js test module, to automatically check for DebugUtils leaks when a test is completed (and removed the modification to the aboutdebugging tests that was doing the same kind of leak checks) > Why not doing that at first line of _windowReady? > Also, do you know what is the STR to get this? > The code calling windowReady most probably shouldn't be executed if the actor isn't attached. I didn't added this check to the first line of `_windowReady` because I was assuming that the the other lines could be necessary even when the threadActor doesn't exist anymore, but I think that you are right and I can move it before `_windowReady` has been called. About the STR, I've been able to reproduce this issue with the new mochitests (the issue doesn't make the test to fail, it just spam the logs with the exception), and it happens when the target is destroyed at the end of the tests (and if I recall correctly it was logged only when the tests were running in non-oop mode) . The `_windowReady` call is coming from `_changeTopLevelWindow` (http://searchfox.org/mozilla-central/rev/d66b9f27d5630a90b2fce4d70d4e9050f43df9b4/devtools/server/actors/tab.js#1167-1173) and before the call to `DevToolsUtils.executeSoon` from line 1167 the actor is still attached, but is not attached anymore when it reach line 1169. We can probably check `this.attached` at line 1169 and never call `_windowReady` if the actor is not attached anymore, how that sounds to you?

(In reply to Luca Greco [:rpl] from comment #108) > Comment on attachment 8866437 [details] > We can probably check `this.attached` at line 1169 and never call > `_windowReady` if the actor is not attached anymore, how that sounds to you? Yes, it may help track other unexpected calls to _windowReady by moving it there instead. Also it looks like your test should also be waiting for `navigate` event. So that you wait for the end of that call to changeTopLevelDocument before finishing the test. But I don't see you doing frame switching in your test, so you may have an unexpected call to changeTopLevelDocument!

(In reply to Alexandre Poirot [:ochameau] from comment #109) > (In reply to Luca Greco [:rpl] from comment #108) > > Comment on attachment 8866437 [details] > > We can probably check `this.attached` at line 1169 and never call > > `_windowReady` if the actor is not attached anymore, how that sounds to you? > > Yes, it may help track other unexpected calls to _windowReady by moving it > there instead. sounds good, I'll move the check there. > Also it looks like your test should also be waiting for `navigate` event. > So that you wait for the end of that call to changeTopLevelDocument before > finishing the test. > But I don't see you doing frame switching in your test, so you may have an > unexpected call to changeTopLevelDocument! I forgot to mention explicitly in the previous comment that the _changeTopLevelDocument is not due to a request to change of the current selected frame, it is related to the addon uninstall behavior: in one of the new mochitest (test_webextension-addon-debugging-reload.html), we are testing the scenario where the addon actor is supposed to be detached automatically when the addon is uninstalled, ans when the addon is uninstalling, the background page is destroyed while the actor is still attached (and so the actor will switch automatically to the fallback window), but in the meantime the actor is not attached anymore because the parent is exiting (because of the addon uninstall) and it detaches itself from the child process actor (and the addon target on the client side has been also automatically detached).

Comment on attachment 8866437 [details] Bug 1302702 - Add devtools webextension actor mochitest-chrome unit tests. https://reviewboard.mozilla.org/r/138054/#review141486 > I didn't added this check to the first line of `_windowReady` because I was assuming that the the other lines could be necessary even when the threadActor doesn't exist anymore, but I think that you are right and I can move it before `_windowReady` has been called. > > About the STR, I've been able to reproduce this issue with the new mochitests (the issue doesn't make the test to fail, it just spam the logs with the exception), and it happens when the target is destroyed at the end of the tests (and if I recall correctly it was logged only when the tests were running in non-oop mode) . > > The `_windowReady` call is coming from `_changeTopLevelWindow` (http://searchfox.org/mozilla-central/rev/d66b9f27d5630a90b2fce4d70d4e9050f43df9b4/devtools/server/actors/tab.js#1167-1173) and before the call to `DevToolsUtils.executeSoon` from line 1167 the actor is still attached, but is not attached anymore when it reach line 1169. > > We can probably check `this.attached` at line 1169 and never call `_windowReady` if the actor is not attached anymore, how that sounds to you? I'm marking this fixed in this patch, because this change has been moved in its own changeset.

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. https://reviewboard.mozilla.org/r/107256/#review142024 Looks good, thanks Luca! Do not hesitate to r? if you want me to look at it again or reply to some other questions. ::: devtools/client/framework/target.js:411 (Diff revision 13) > + if (this._form.isWebExtension && > + this.client.mainRoot.traits.webExtensionAddonConnect) { > + // This addonActor has not been connected to the webextension running in the > + // child process during the listAddons request, on the contrary the connection > + // is deferred until it is actually needed (e.g. when the addon debugger is > + // opened on it). It would help to mention what actors we are playing with here. Something like: _form is a WebExtensionParent instance. It isn't a tab actor. It is an actor living in the parent process with some addon metadata and can control the addon. We have to call its "connect" method to fetch a TabActor instance, i.e. WebExtensionChild. nit: I wouldn't put that in attachTab, but later in this method, around line 385 if (this.isLocalTab) { if (!DebuggerServer.initialized) { ... } else if (this._form.isWebExtension && ...) { // put your code and comment here } ::: devtools/server/actors/webextension-parent.js:43 (Diff revision 13) > + this._childActorProxy = null; > + > + AddonManager.addAddonListener(this); > + }, > + > + exit() { Wait. Exit is a convention from TabActor (from webbrowser.js), but I don't think it will ever be called here. Is it? Better rename this function "destroy", at least it will be called on connection shutdown. nit: to prevent leaks if this object happen to be leaks, it would help to nullify this.addon. ::: devtools/server/actors/webextension-parent.js:81 (Diff revision 13) > + if (!this._childActorProxy) { > + this._childActorProxy = new ProxyChildActor(this.conn, this); > + this._waitChildActorForm = this._childActorProxy.connect(); > + } > + > + return this._waitChildActorForm.then(form => { nit: I would save a reference to the proxy, by doing that: connect() { if (this._childForm) { return this._childForm; } let proxy = new ProxyChildActor(this.conn, this); this._childForm = proxy.connect().then(({ form, isOOP }) => { return Object.assign(form, { ..., isOOp }); }); return this._childForm; } ::: devtools/server/actors/webextension-parent.js:119 (Diff revision 13) > + onUninstalled(addon) { > + if (addon != this.addon) { > + return; > + } > + > + this.exit(); (do not forget to call destroy instead of exit here) ::: devtools/server/actors/webextension-parent.js:133 (Diff revision 13) > + this._parentActor = parentActor; > + this.addonId = parentActor.id; > + > + this._onChildExit = this._onChildExit.bind(this); > + > + this._form = null; nit: you are already nullifying _form on line 127. ::: devtools/server/actors/webextension.js:25 (Diff revision 13) > - * add-on. > + * add-on running in a child extension process. > * Most of the implementation is inherited from ChromeActor (which inherits most of its > * implementation from TabActor). > - * WebExtensionActor is a child of RootActor, it can be retrieved via > - * RootActor.listAddons request. > - * WebExtensionActor exposes all tab actors via its form() request, like TabActor. > + * WebExtensionChildActor is created by a WebExtensionParentActor counterpart, when its > + * parent actor's `connect` method has been called (on the listAddons RDP package), > + * and it runs in the child extension process. My comprehension is that WebExtensionChildActor is going to be used even for WebExt in parent process. So this comment may say "it runs in whatever process the extension is running into. It can be parent or extension one." ::: devtools/server/actors/webextension.js:192 (Diff revision 13) > + // the addon debugging toolbox is switched to the fallback window. > + if (this.attached && wasCurrentDocShellDestroyed) { > + // Create a fallback window to use during addon reloads. > + this._createFallbackWindow(); > + this._changeTopLevelDocument(this.fallbackWindow); > + } Oh this is really bad. If I follow this correctly, you are going to call changeTopLevelDocument multiple times in a row here, right? We should really not do that! I would suggest moving this piece: http://searchfox.org/mozilla-central/source/devtools/server/actors/tab.js#789-817 to TabActor._onDocShellDestroy. So that from here, we call: this._onDocShellDestroy(docShell) instead of ChromeActor.prototype._onDocShellDestroy.call(this, docShell); And prevent the change-toplevel-document dance. ::: devtools/server/actors/webextension.js:275 (Diff revision 13) > +}; > + > +WebExtensionChildActor.prototype.isExtensionWindowDescendent = function (window) { > + // Check if the source is coming from a descendant docShell of an extension window. > + let docShell = window.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDocShell); Can you use this.docShell? ::: devtools/server/actors/webextension.js:341 (Diff revision 13) > - return global.document.nodePrincipal.addonId == this.id; > + // Filter out any global which contains a XUL document > + if (global.document instanceof Ci.nsIDOMXULDocument) { > + return false; > + } > + > + const isExtensionWindow = this.isExtensionWindow(global); nit:looks like you do not necessarely need an intermediate variable here.

Comment on attachment 8849614 [details] Bug 1302702 - Check if the threadActor is attached before using it in tab actor's _windowReady. https://reviewboard.mozilla.org/r/122402/#review142048

The last pushed update on the attached patch is related to a rebase on a recent mozilla-central tip.

Comment on attachment 8830498 [details] Bug 1302702 - Make WebExtension Addon Debugging oop-compatible. https://reviewboard.mozilla.org/r/107256/#review142024 > It would help to mention what actors we are playing with here. > Something like: _form is a WebExtensionParent instance. It isn't a tab actor. It is an actor living in the parent process with some addon metadata and can control the addon. We have to call its "connect" method to fetch a TabActor instance, i.e. WebExtensionChild. > > nit: I wouldn't put that in attachTab, but later in this method, around line 385 > if (this.isLocalTab) { > if (!DebuggerServer.initialized) { > ... > } else if (this._form.isWebExtension && ...) { > // put your code and comment here > } In the last updated patch I've updated the comment and moved this code as suggested. > Wait. Exit is a convention from TabActor (from webbrowser.js), but I don't think it will ever be called here. Is it? > Better rename this function "destroy", at least it will be called on connection shutdown. > > nit: to prevent leaks if this object happen to be leaks, it would help to nullify this.addon. I've renamed this method `destroy` and added an explicit assertion for this leak check in the new mochitests (and this.addon is now nullified as suggested). > nit: I would save a reference to the proxy, by doing that: > connect() { > if (this._childForm) { > return this._childForm; > } > let proxy = new ProxyChildActor(this.conn, this); > this._childForm = proxy.connect().then(({ form, isOOP }) => { > return Object.assign(form, { > ..., > isOOp > }); > }); > return this._childForm; > } When the target addon is uninstalled, the parent actor (that is subscribed as a listener of the addon lifecycle events on the AddonManager) have to destroy the ProxyChildActor instance. In the updated patch I opted to use the suggested approach, with the addition on creating an arrow function called `this._destroyProxyChildActor` which will be called when the parent actor needs to destroy the "proxy child actor" instance. > Oh this is really bad. > If I follow this correctly, you are going to call changeTopLevelDocument multiple times in a row here, right? > We should really not do that! > I would suggest moving this piece: > http://searchfox.org/mozilla-central/source/devtools/server/actors/tab.js#789-817 > to TabActor._onDocShellDestroy. > So that from here, we call: > this._onDocShellDestroy(docShell) > instead of ChromeActor.prototype._onDocShellDestroy.call(this, docShell); > > And prevent the change-toplevel-document dance. Great call! I've refactored the `_onDocShellDestroy` and the `_notifyDocShellDestroy` methods in tab.js and the `WebExtensionChildActor._onDocShellDestroy` is cleaner (and the change-toplevel-document dance prevented). > Can you use this.docShell? No, unfortunately we can't: the window passed as a parameter to this function is a new global that we need to choose if it should be listed in the Addon Debugging frame list (it could become this.docShell only if the user explicitly decides to switch to this frame, which can only happen if we allow it to be listed in the "frames" selector). (side note: this actor will never choose an "extension window" subframe as a top level window automatically, only top-level extension window can be automatically selected if there is no extension window currently selected, while subframes have to be explicitly selected by the user). > nit:looks like you do not necessarely need an intermediate variable here. That's true, it is a leftover. Removed in the updated patch.

Comment on attachment 8849613 [details] Bug 1302702 - Provide a DebugUtils object from ExtensionParent.jsm. https://reviewboard.mozilla.org/r/107254/#review142512 ::: toolkit/mozapps/extensions/AddonManager.jsm:2234 (Diff revision 18) > > this.getAddonByInstanceID(aInstanceID).then(wrapper => { > if (!wrapper) { > throw Error("No addon matching instanceID:", aInstanceID.toString()); > } > - let addonId = wrapper.addonId(); > + let addonId = wrapper.id; Hi Kris, in the last update on attachment 8849615 [details] I've added this additional fixed line (it is one last user of the Addon private wrapper that this patch is going to remove from the XPIProvider.jsm, I caught it by investigating an xpishell test failure that was happening in the try pushes.)

Besides the changes briefly described in Comment 130 and Comment 131, the updated patches contains the following additional changes: - added "debug browsers" leak checks that are automatically executed when a test has been completed (as we already do similar checks for the test webextension, which is expected to be unloaded when the test has been completed) - tweaked the ProxyChildActor destroy method and the fallback window creation to prevent a failed assertion when the new mochitests run on a debug builds (spotted while running the new mochitests on try)

Push to try of the rebased and updated patches: - https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad0b1e5d2ec32bac04d7043fb2ed5821cebe14da

Comment on attachment 8860912 [details] Bug 1302702 - Shorter extension urls in addon debugger window title and frames list selector. Re-assigned the r+ flag on this patch (which has been already r+ from ochameau on mozreview, but for unknown reasons it is not visible from bugzilla).

Comment on attachment 8860913 [details] Bug 1302702 - Fix inspector panel deadwrapper exceptions on addon reloads. Re-assigned the r+ flag on this patch (which has been already r+ from ochameau on mozreview, but for unknown reasons it is not visible from bugzilla).

Just pushed an additional round of updates with the following change: - rewrote tests from "Task.jsm / generators" syntax to "async / await". (Attachment 8860912 [details] and attachment 8860913 [details] are both unchanged, but they have lost the r+ flag again on bugzilla while they are r+ as expected on mozreview)

Comment on attachment 8857092 [details] Bug 1302702 - ExtensionParent DebugUtils xpcshell test unit. https://reviewboard.mozilla.org/r/128986/#review143152

Comment on attachment 8849613 [details] Bug 1302702 - Provide a DebugUtils object from ExtensionParent.jsm. https://reviewboard.mozilla.org/r/107254/#review143552 ::: devtools/server/tests/mochitest/webextension-helpers.js:145 (Diff revisions 19 - 20) > SimpleTest.registerCleanupFunction(() => { > + flushJarCache(addonFile.path); > + Services.ppmm.broadcastAsyncMessage("Extension:FlushJarCache", > + {path: addonFile.path}); > + > if (addonFile.exists()) { > - addonFile.remove(false); > + OS.File.remove(addonFile.path); > } > }); As part of the last round of changes on these patches: this change in the new devtools mochitest test webextension-helpers.js has been introduced to fix a failure of the new devtools mochitests when running on window (due to not being able to remove the file at the end of the test)

Comment on attachment 8849613 [details] Bug 1302702 - Provide a DebugUtils object from ExtensionParent.jsm. https://reviewboard.mozilla.org/r/107254/#review143560 ::: toolkit/components/extensions/ExtensionParent.jsm:718 (Diff revisions 19 - 20) > } > > this.unloaded = true; > > + this.chromeShell = null; > + this.waitInitialized = null; This line introduces a fix for a leak in the first of the attached patches: this `this.waitInitialized` promise should be "nullified", so that the reference to the windowless browser can be garbage collected.

Besides the changes related to the fixes described in Comment 154 and Comment 155, all the patches have been rebased one last time on a recent mozilla-central tip (just to be sure that there were no conflicts with some recently landed changes on the internals that are changed by these patches). Push to try: - https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e38438e884571af0d9185ba8136dba0490d9e03

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5c8ee0487056 Provide a DebugUtils object from ExtensionParent.jsm. r=kmag https://hg.mozilla.org/integration/autoland/rev/d4ebca627094 ExtensionParent DebugUtils xpcshell test unit. r=kmag https://hg.mozilla.org/integration/autoland/rev/9b16857ca48d Make WebExtension Addon Debugging oop-compatible. r=ochameau https://hg.mozilla.org/integration/autoland/rev/9859873385bc Remove from ext-backgroundPage any code that uses the AddonManager object. r=kmag https://hg.mozilla.org/integration/autoland/rev/ef12ed2b4641 Add devtools webextension actor mochitest-chrome unit tests. r=ochameau https://hg.mozilla.org/integration/autoland/rev/b5e20d92d062 Shorter extension urls in addon debugger window title and frames list selector. r=ochameau https://hg.mozilla.org/integration/autoland/rev/3173e1090b3c Fix inspector panel deadwrapper exceptions on addon reloads. r=ochameau https://hg.mozilla.org/integration/autoland/rev/2255bb2a4215 Check if the threadActor is attached before using it in tab actor's _windowReady. r=ochameau

Backed out for failing xpcshell's toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js and more on Android: https://hg.mozilla.org/integration/autoland/rev/8c2e9b2ee8eb5dda30a5ca99de9b2809bf91599a https://hg.mozilla.org/integration/autoland/rev/d30987357f75027720173b6f84414faa0f1d7acc https://hg.mozilla.org/integration/autoland/rev/293882a12e97b55a3ec1b60cbf1a81d0e9fccc8e https://hg.mozilla.org/integration/autoland/rev/98571411cda063841870427562b32586861dcde1 https://hg.mozilla.org/integration/autoland/rev/a2b003445fe793cdf9ac5ef62552c4f2cd4e059e https://hg.mozilla.org/integration/autoland/rev/d728f32c4449de2b3d61ecae275d5c43087e3952 https://hg.mozilla.org/integration/autoland/rev/0d25af257e6a42a3fb6fb3c3fcc6ab7ccc271fc7 https://hg.mozilla.org/integration/autoland/rev/43bc4f4903cb3b52b6107bd3636b50aabad7d7b5 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2255bb2a42156fc84f1a0e56883582d19db174aa&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=99850871&repo=autoland [task 2017-05-17T17:40:22.898074Z] 17:40:22 INFO - TEST-START | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js [task 2017-05-17T17:40:34.206674Z] 17:40:34 WARNING - TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js | xpcshell return code: 0 [task 2017-05-17T17:40:34.214269Z] 17:40:34 INFO - TEST-INFO took 11308ms [task 2017-05-17T17:40:34.214712Z] 17:40:34 INFO - >>>>>>> [task 2017-05-17T17:40:34.215136Z] 17:40:34 INFO - xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js | xpcw: cd /storage/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell [task 2017-05-17T17:40:34.215700Z] 17:40:34 INFO - xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js | xpcw: xpcshell -r /storage/sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/target.apk -m -s -e const _HEAD_JS_PATH = "/storage/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/storage/sdcard/tests/xpc/p/mozinfo.json"; -e const _TESTING_MODULES_DIR = "/storage/sdcard/tests/xpc/m"; -f /storage/sdcard/tests/xpc/head.js -e const _SERVER_ADDR = "localhost" -e const _HEAD_FILES = ["/storage/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell/head.js"]; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_ext_shutdown_cleanup.js"]; -e const _TEST_NAME = "xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js" -e _execute_test(); quit(0); [task 2017-05-17T17:40:34.215761Z] 17:40:34 INFO - (xpcshell/head.js) | test MAIN run_test pending (1) [task 2017-05-17T17:40:34.215811Z] 17:40:34 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2) [task 2017-05-17T17:40:34.216144Z] 17:40:34 INFO - (xpcshell/head.js) | test MAIN run_test finished (2) [task 2017-05-17T17:40:34.216192Z] 17:40:34 INFO - running event loop [task 2017-05-17T17:40:34.216530Z] 17:40:34 INFO - xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js | Starting test_global_manager_shutdown_cleanup [task 2017-05-17T17:40:34.216590Z] 17:40:34 INFO - (xpcshell/head.js) | test test_global_manager_shutdown_cleanup pending (2) [task 2017-05-17T17:40:34.216697Z] 17:40:34 INFO - TEST-PASS | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js | test_global_manager_shutdown_cleanup - [test_global_manager_shutdown_cleanup : 9] GlobalManager start as not initialized - false == false [task 2017-05-17T17:40:34.216729Z] 17:40:34 INFO - "Extension attached" [task 2017-05-17T17:40:34.216770Z] 17:40:34 INFO - (xpcshell/head.js) | test run_next_test 0 finished (2) [task 2017-05-17T17:40:34.216848Z] 17:40:34 INFO - "CONSOLE_MESSAGE: (error) [JavaScript Error: "Failed to load module resource:///modules/E10SUtils.jsm." {file: "resource://gre/modules/XPCOMUtils.jsm" line: 279}] [task 2017-05-17T17:40:34.216893Z] 17:40:34 INFO - XPCU_moduleLambda@resource://gre/modules/XPCOMUtils.jsm:279:9 [task 2017-05-17T17:40:34.216933Z] 17:40:34 INFO - get@resource://gre/modules/XPCOMUtils.jsm:198:21 [task 2017-05-17T17:40:34.216979Z] 17:40:34 INFO - createBrowserElement@resource://gre/modules/ExtensionParent.jsm:872:7 [task 2017-05-17T17:40:34.217022Z] 17:40:34 INFO - build@chrome://extensions/content/ext-backgroundPage.js:35:11 [task 2017-05-17T17:40:34.217069Z] 17:40:34 INFO - onManifestEntry@chrome://extensions/content/ext-backgroundPage.js:64:12 [task 2017-05-17T17:40:34.217116Z] 17:40:34 INFO - asyncEmitManifestEntry@resource://gre/modules/ExtensionCommon.jsm:879:14 [task 2017-05-17T17:40:34.217156Z] 17:40:34 INFO - _do_main@/storage/sdcard/tests/xpc/head.js:211:5 [task 2017-05-17T17:40:34.217197Z] 17:40:34 INFO - _execute_test@/storage/sdcard/tests/xpc/head.js:542:5 [task 2017-05-17T17:40:34.217222Z] 17:40:34 INFO - @-e:1:1 [task 2017-05-17T17:40:34.217245Z] 17:40:34 INFO - " [task 2017-05-17T17:40:34.217375Z] 17:40:34 INFO - xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js | Extension error: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import] resource://gre/modules/XPCOMUtils.jsm:273 :: XPCU_moduleLambda@resource://gre/modules/XPCOMUtils.jsm:273:9 [task 2017-05-17T17:40:34.217445Z] 17:40:34 INFO - xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js | get@resource://gre/modules/XPCOMUtils.jsm:198:21 [task 2017-05-17T17:40:34.217535Z] 17:40:34 INFO - xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js | createBrowserElement@resource://gre/modules/ExtensionParent.jsm:872:7 [task 2017-05-17T17:40:34.217615Z] 17:40:34 INFO - xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js | build@chrome://extensions/content/ext-backgroundPage.js:35:11 [task 2017-05-17T17:40:34.217693Z] 17:40:34 INFO - xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js | onManifestEntry@chrome://extensions/content/ext-backgroundPage.js:64:12 [task 2017-05-17T17:40:34.217772Z] 17:40:34 INFO - xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js | asyncEmitManifestEntry@resource://gre/modules/ExtensionCommon.jsm:879:14 [task 2017-05-17T17:40:34.217842Z] 17:40:34 INFO - xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js | _do_main@/storage/sdcard/tests/xpc/head.js:211:5 [task 2017-05-17T17:40:34.217913Z] 17:40:34 INFO - xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js | _execute_test@/storage/sdcard/tests/xpc/head.js:542:5 [task 2017-05-17T17:40:34.217968Z] 17:40:34 INFO - xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_shutdown_cleanup.js | @-e:1:1 [task 2017-05-17T17:40:34.218043Z] 17:40:34 INFO - Unexpected exception NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import] [task 2017-05-17T17:40:34.218086Z] 17:40:34 INFO - XPCU_moduleLambda@resource://gre/modules/XPCOMUtils.jsm:273:9 [task 2017-05-17T17:40:34.218125Z] 17:40:34 INFO - get@resource://gre/modules/XPCOMUtils.jsm:198:21 [task 2017-05-17T17:40:34.218171Z] 17:40:34 INFO - createBrowserElement@resource://gre/modules/ExtensionParent.jsm:872:7 [task 2017-05-17T17:40:34.218214Z] 17:40:34 INFO - build@chrome://extensions/content/ext-backgroundPage.js:35:11 [task 2017-05-17T17:40:34.218270Z] 17:40:34 INFO - onManifestEntry@chrome://extensions/content/ext-backgroundPage.js:64:12 [task 2017-05-17T17:40:34.218322Z] 17:40:34 INFO - asyncEmitManifestEntry@resource://gre/modules/ExtensionCommon.jsm:879:14 [task 2017-05-17T17:40:34.218368Z] 17:40:34 INFO - _do_main@/storage/sdcard/tests/xpc/head.js:211:5 [task 2017-05-17T17:40:34.218423Z] 17:40:34 INFO - _execute_test@/storage/sdcard/tests/xpc/head.js:542:5 [task 2017-05-17T17:40:34.218589Z] 17:40:34 INFO - @-e:1:1 [task 2017-05-17T17:40:34.218778Z] 17:40:34 INFO - exiting test [task 2017-05-17T17:40:34.219347Z] 17:40:34 INFO - "CONSOLE_MESSAGE: (error) [JavaScript Error: "[Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: resource://gre/modules/XPCOMUtils.jsm :: XPCU_moduleLambda :: line 273" data: no]"] [task 2017-05-17T17:40:34.219751Z] 17:40:34 INFO - XPCU_moduleLambda@resource://gre/modules/XPCOMUtils.jsm:273:9 [task 2017-05-17T17:40:34.220152Z] 17:40:34 INFO - get@resource://gre/modules/XPCOMUtils.jsm:198:21 [task 2017-05-17T17:40:34.220447Z] 17:40:34 INFO - createBrowserElement@resource://gre/modules/ExtensionParent.jsm:872:7 [task 2017-05-17T17:40:34.220679Z] 17:40:34 INFO - build@chrome://extensions/content/ext-backgroundPage.js:35:11 [task 2017-05-17T17:40:34.220932Z] 17:40:34 INFO - onManifestEntry@chrome://extensions/content/ext-backgroundPage.js:64:12 [task 2017-05-17T17:40:34.221153Z] 17:40:34 INFO - asyncEmitManifestEntry@resource://gre/modules/ExtensionCommon.jsm:879:14 [task 2017-05-17T17:40:34.221391Z] 17:40:34 INFO - _do_main@/storage/sdcard/tests/xpc/head.js:211:5 [task 2017-05-17T17:40:34.221633Z] 17:40:34 INFO - _execute_test@/storage/sdcard/tests/xpc/head.js:542:5 [task 2017-05-17T17:40:34.221834Z] 17:40:34 INFO - @-e:1:1 [task 2017-05-17T17:40:34.222039Z] 17:40:34 INFO - " [task 2017-05-17T17:40:34.222270Z] 17:40:34 INFO - <<<<<<<

Also check these chrome failures: https://treeherder.mozilla.org/logviewer.html#?job_id=99850133&repo=autoland

Ouch, I should have definitely included Android in one of my last try pushes. I figured out the reason (well, it is pretty clearly logged in the failure logs, Fennec cannot load "E10SUtils.jsm" because it doesn't exist there), prepared a fix, I'm currently running the tests locally on an android artifact build. Fixed version is coming on mozreview pretty soon.

The pushed updated introduces the following fix (related to the failures on Android): - https://reviewboard.mozilla.org/r/122400/diff/7-8/ which means that ExtensionParent.jsm is going to access the E10SUtils lazy getter only if the extension is remote, because it is not needed otherwise, and so that it will never be accessed on Android until we explicitly introduce support for oop webextensions on Firefox for Android.