Closed
Bug 1374590
Opened 7 years ago
Closed 7 years ago
Devtools break when trying to change dock location while using WebExtension devtools panel
Categories
(WebExtensions :: Developer Tools, defect, P1)
Tracking
(firefox57 verified, firefox58 verified)
VERIFIED
FIXED
mozilla57
People
(Reporter: adrian17, Assigned: rpl)
References
Details
Attachments
(4 files)
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:55.0) Gecko/20100101 Firefox/55.0 Build ID: 20170615070049 Steps to reproduce: - create a barebones WebExtension that creates a DevTools panel: chrome.devtools.panels.create("My Extension", "icon.png", "panel.html", function(panel){}); - load it into FF developer edition - open devtools - switch to the "My Extension" tab - try clicking "Dock to side of browser window" or "Show in separate window" buttons in devtools top bar Actual results: Completely white pane/window appears. The old devtools pane does not disappear. Reproduced on FF dev edition on Windows and Xubuntu. Expected results: The pane should dock/switch to window just like it does normally. The exact same extension behaves correctly in Chrome.
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Reporter | ||
Comment 1•7 years ago
|
||
This appears to have been fixed in Nightly.
per comment 1.
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Component: WebExtensions: Untriaged → WebExtensions: Developer Tools
Resolution: --- → WORKSFORME
Assignee | ||
Comment 3•7 years ago
|
||
This bug is still valid, and it is related to Bug 1075490: basically if the developer tools panels contains any `type="content"` browser elements, when the toolbox switches host (e.g. by switching between docked and undocked), `swapFrameLoader` raises an exception which breaks the toolbox rendering. The reason why it is not reproducible on Nightly (and Beta) is that we special cased it on RELEASE_OR_BETA as a temporary compromise, during the review the workaround has been locked on Nightly and Beta because it makes the browser element of `type="chrome"`. Unfortunately it doesn't seem that the underlying issue has been fixed (and it sounds like it is not an easy fix: https://bugzilla.mozilla.org/show_bug.cgi?id=1075490#c7) but in my opinion the current workaround is even worst (because at a first glance the issue seems fixed on Nightly and Beta even when it will still be broken when the version it the Release). I'm going to reopen this issue and to attach a proposed patch with a workaround that can be used on the released version as well (instead of being locked to Nightly and Beta as the current one): When the toolbox is switching host, it sends two messages: "host-will-change" and "host-changed", we can remove the browser element added by the WebExtensions add-on before the toolbox is switching the host, and so the swapFrameLoader call does not raise the exception and then we can than create the browser element again once the "host-changed" message has been received.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WORKSFORME → ---
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → lgreco
Status: REOPENED → ASSIGNED
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8906650 -
Flags: review?(aswan)
Attachment #8906651 -
Flags: review?(aswan)
Attachment #8906675 -
Flags: review?(aswan)
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8906650 [details] Bug 1374590 - Refactoring ParentDevToolsPanel class to provide create/destroyBrowserElement helper methods. https://reviewboard.mozilla.org/r/178364/#review183922 ::: browser/components/extensions/ext-devtools-panels.js:109 (Diff revision 1) > + toolbox.on("select", this.onToolboxPanelSelect); > + > + // Return a cleanup method that is when the panel is destroyed, e.g. > + // - when addon devtool panel has been disabled by the user from the toolbox preferences, > + // its ParentDevToolsPanel instance is still valid, but the built devtools panel is removed from > + // the toolbox (and re-built again if the user re-enable it from the toolbox preferences panel) nit: re-enable -> re-enables ::: browser/components/extensions/ext-devtools-panels.js:111 (Diff revision 1) > + // Return a cleanup method that is when the panel is destroyed, e.g. > + // - when addon devtool panel has been disabled by the user from the toolbox preferences, > + // its ParentDevToolsPanel instance is still valid, but the built devtools panel is removed from > + // the toolbox (and re-built again if the user re-enable it from the toolbox preferences panel) > + // - when the creator context has been destroyed, the ParentDevToolsPanel close method is called, > + // it remove the tool definition from the toolbox, which will call this destroy method. nit: remove -> removes ::: browser/components/extensions/ext-devtools-panels.js:230 (Diff revision 1) > + if (browser) { > browser.remove(); > - toolbox.off("select", this.onToolboxPanelSelect); > + this.browser = null; > + } > > - // If the panel has been disabled from the toolbox preferences, > + // If the panel has been remove or disabled (e.g. from the toolbox preferences nit: remove -> removed
Attachment #8906650 -
Flags: review?(aswan) → review+
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8906651 [details] Bug 1374590 - Fix changing devtools toolbox dock location while using WebExtension devtools panel. https://reviewboard.mozilla.org/r/178366/#review183924
Attachment #8906651 -
Flags: review?(aswan) → review+
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8906675 [details] Bug 1374590 - Add a new test case for toolbox dock mode switch with a devtools panel addon. https://reviewboard.mozilla.org/r/178388/#review183926 This looks reasonable to me but I don't know the devtools apis very well, might want to get a sanity check from somebody who knows them.
Attachment #8906675 -
Flags: review?(aswan) → review+
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8906650 [details] Bug 1374590 - Refactoring ParentDevToolsPanel class to provide create/destroyBrowserElement helper methods. https://reviewboard.mozilla.org/r/178364/#review184366 ::: browser/components/extensions/ext-devtools-panels.js:101 (Diff revision 1) > }); > > this.panelAdded = true; > } > > buildPanel(window, toolbox) { Why buildPanel receives `toolbox` argument whereas it is available via `this.toolbox`? ::: browser/components/extensions/ext-devtools-panels.js:113 (Diff revision 1) > + // its ParentDevToolsPanel instance is still valid, but the built devtools panel is removed from > + // the toolbox (and re-built again if the user re-enable it from the toolbox preferences panel) > + // - when the creator context has been destroyed, the ParentDevToolsPanel close method is called, > + // it remove the tool definition from the toolbox, which will call this destroy method. > + return () => { > + this.destroyBrowserElement(); Shouldn't destroyBrowserElement() be called from close(), and here call close() instead? ::: browser/components/extensions/ext-devtools-panels.js:135 (Diff revision 1) > + } else if (this.visible && id !== this.id) { > + this.visible = false; > + this.context.parentMessageManager.sendAsyncMessage("Extension:DevToolsPanelHidden", { > + toolboxPanelId: this.id, > + }); > + } It would be more readable by using async function: if (!this.visible && id === this.id) { await this.waitTopLevelContext; this.visible = true; } else if (this.visible && id !== this.id) { this.visible = false; } this.context.parentMessageManager.sendAsyncMessage("Extension:DevToolsPanel" + (this.visible ? "Shown" : "Hidden"), { toolboxPanelId: this.id, }); ::: browser/components/extensions/ext-devtools-panels.js:147 (Diff revision 1) > + throw new Error("Unable to destroy a closed devtools panel"); > + } > + > + // Explicitly remove the panel if it is registered and the toolbox is not > + // closing itself. > + if (this.panelAdded && toolbox.isToolRegistered(this.id) && !toolbox._destroyer) { You shouldn't use toolbox._destroyer from here. If that's really an issue, please add this toolbox._destroyer check within removeAdditonalTool. ::: browser/components/extensions/ext-devtools-panels.js:158 (Diff revision 1) > + this.waitTopLevelContext = null; > + this._resolveTopLevelContext = null; > + } > + > + createBrowserElement(containerEl) { > + const window = containerEl.ownerGlobal; What about doing: this.createBrowserElement(window); and remove: const window = containerEl.ownerGlobal;
Attachment #8906650 -
Flags: review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8906650 [details] Bug 1374590 - Refactoring ParentDevToolsPanel class to provide create/destroyBrowserElement helper methods. https://reviewboard.mozilla.org/r/178364/#review184366 > Why buildPanel receives `toolbox` argument whereas it is available via `this.toolbox`? yeah, there is no need to pass it to `buildPanel` (we already ensure that the toolbox is the expected one in the `build` property of the tool definition). Change applied in the updated version. > Shouldn't destroyBrowserElement() be called from close(), and here call close() instead? `close` is called when `this.context` is unloaded, which means that the extension context that has created the panel is being destroyed (e.g. when the extension is uninstalled the devtools page will be destroyed and when close is called, it unregister the panel from the toolbox (which will call `destroy` and the panel browser XUL element will be destroyed as well). In this patch we are refactoring out part of the code that is called when the registered panel is destroyed from the toolbox into the `destroyBrowserElement` helper method so that, when the toolbox is going to change its dock mode, we can temporarily destroy the browser XUL element where the extension page is loaded without unregistering the tool from the toolbox (e.g. because by unregistering the tool the panel position in the toolbox will also change as a side-effect and the panel will not be the selected one when the toolbox has been switched into the new dock mode) > You shouldn't use toolbox._destroyer from here. > If that's really an issue, please add this > toolbox._destroyer check within removeAdditonalTool. Yeah, you are definitely right, I moved that check into `toolbox.remoteAdditionalTool`. > What about doing: > this.createBrowserElement(window); > and remove: > const window = containerEl.ownerGlobal; yeah, that's better, I've changed it to get window as its parameter (and to store the window to be able to remove and re-create the browser XUL element while the toolbox is switching between dock modes).
Updated•7 years ago
|
status-firefox57:
--- → affected
Comment 18•7 years ago
|
||
Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/d1dc7082f9e3 Refactoring ParentDevToolsPanel class to provide create/destroyBrowserElement helper methods. r=aswan,ochameau https://hg.mozilla.org/integration/autoland/rev/97b428deeefa Fix changing devtools toolbox dock location while using WebExtension devtools panel. r=aswan https://hg.mozilla.org/integration/autoland/rev/a83df3d96eaf Add a new test case for toolbox dock mode switch with a devtools panel addon. r=aswan
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d1dc7082f9e3 https://hg.mozilla.org/mozilla-central/rev/97b428deeefa https://hg.mozilla.org/mozilla-central/rev/a83df3d96eaf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 20•6 years ago
|
||
I can reproduce this issue on Firefox 56.0.2 (20171024165158) under Wind 7 64-bit. Trying to dock on the side of the browser window or show in a separate page it will open a blank panel. This issue is verified as fixed on Firefox 58.0a1 (2017-11-08) and Firefox 57.0b14 (20171102181127) under Wind 7 64-bit and Mac OS X 10.13. Please see the attached video.
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•