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)

55 Branch
defect

Tracking

(firefox57 verified, firefox58 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox57 --- verified
firefox58 --- verified

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
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
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: nobody → lgreco
Status: REOPENED → ASSIGNED
Priority: -- → P1
Attachment #8906650 - Flags: review?(aswan)
Attachment #8906651 - Flags: review?(aswan)
Attachment #8906675 - Flags: review?(aswan)
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 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 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 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 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).
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
Blocks: 1401293
Attached file Bug1374590.zip
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.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.