Closed Bug 1456923 Opened 2 years ago Closed 2 years ago

can't switch to dev-tools add-on that is loaded temporarily

Categories

(DevTools :: General, defect, P1)

57 Branch
defect

Tracking

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox59 unaffected, firefox60 unaffected, firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 --- fixed

People

(Reporter: mjf, Assigned: miker)

References

Details

(Keywords: regression)

Attachments

(1 file)

Nightly 61.0a1 (2018-04-25) (64-bit) seems to break switching to dev-tools panels that are loaded temporarily.  Mozregression leads me to Bug 1441070 from the following results:
26:07.02 INFO: Last good revision: be5ba8b6dea2d025e0e1f7268336e1d1f0c32399
26:07.02 INFO: First bad revision: 3577a6b9b1a13a0dfd93073f4ef9209c08764eb4

Steps to reproduce:
- Download https://github.com/karolien/media-devtools-panel-react
- Navigate to about:debugging
- Click load temporary add-on, navigate to {location-of-download}/media-devtools-panel-react/extension/ and select any file to load the add-on.
- Open the Developer Tools, and attempt to show the "Media" tab.

Expected results:
- See the media panel

Actual results:
- Does not switch away from current panel

This worked as of 4/24, but stopped working this morning, 4/25.
After trying the steps above in Nightly 4/25 (build 20180425100122), I can confirm that it doesn't switch to away from the current panel.
Blocks: 1441070
I can also confirm that after loading the media-tools extension temporarily on todays Nightly (61.0a1 (2018-04-25)) only results in the tab being shown, but I can't switch to the tab.

But when I removed the temporary loaded extension and instead re-enable the last released version of the "Media Panel" add-on it still works.
In the browser console I get this error when trying to switch to the temporary loaded add-on:

Error: The property "next_panel" was added to a telemetry event with the signature devtools.main,exit,webconsole,null but it's value "webext-devtools-panel-45545ec3cbb7c85b7fa0b74e33259502e2dbdbff_temporary-addon-6119-0" is longer than the maximum allowed length of 80 characters
Mike, are we already cleaning up the panel names for extensions; or is this one not matching the expression?
Flags: needinfo?(mratcliffe)
Moving to M1 for splitting off M2 work.
Hi Mike,
I've briefly looked into this (especially about why none of the existent tests have been failed because of this issue).

The code that prepares these telemetry probes is cleaning up the panel names and the extension devtools panel ids get matched as expected and the cleaned panel name returned is definitely "other" (I'm wondering if it would make sense to return something like "none" if the id is undefined, but that is totally unrelated to this issue).

The reason why this issue is being triggered is that the telemetry probes currently contain the original panel id in other fields, e.g. in the "panel selection" telemetry probe I have found the following two:

- `"panel_name": id` (which I guess it could/should use the cleaned up `panelName` instead), from https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/devtools/client/framework/toolbox.js#1890

- `"next_panel": id` (which seems that it could/should also be set to `panelName`), from https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/devtools/client/framework/toolbox.js#1901

Besides fixing the above two, there is also another one related to when the toolbox is being destroyed:

- `"panel_name": this.currentToolId` (which it seems to be that it could/should be set to `prevPanelName`, from https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/devtools/client/framework/toolbox.js#2839

About the reasons why it doesn't fails on the existent tests and with the extensions installed from AMO but it does fail when the extension is installed temporarily by a user:

- the existent tests are installing the test extension temporarily (similarly to how the issue is being triggered in the comment 0 from this issue), but the extension id is specified in the test extension's manifest.json (to make it easier to retrieve the panel from the opened toolbox for testing it), on the contrary when an extension is installed temporarily and it doesn't have an extension id in its manifest.json (like for the media panel extension mentioned in this issue), one is assigned automatically by the XPIProvider:

- https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/toolkit/mozapps/extensions/internal/XPIInstall.jsm#884

and that makes the resulting devtools panel id longer than 80 chars.

To be fair, an extension (even one installed from AMO) may specify its preferred extension id in the manifest (usually because the extension has been migrated from an existent Firefox legacy extension and it needed to be able to keep its previous id to update automatically its own users to the WebExtensions version), and so technically this issue can be triggered not only by temporarily installed extension, but from any extension that has an extension id long enough to make its generated panel id longer then 80 chars.

We could tweak the existent devtools panel tests a bit so that we can be sure that we cover this scenario from at least one of the existent test cases, e.g. I've applied the following change locally to make it fail and check that changing the telemetry probes mentioned above makes it pass again:

```
diff --git a/browser/components/extensions/test/browser/browser_ext_devtools_panel.js b/browser/components/extensions/test/browser/browser_ext_devtools_panel.js
--- a/browser/components/extensions/test/browser/browser_ext_devtools_panel.js
+++ b/browser/components/extensions/test/browser/browser_ext_devtools_panel.js
@@ -237,17 +237,18 @@ add_task(async function test_devtools_pa
     // and accessible from the devtools_page when the panel.onShown
     // event has been received.
     window.TEST_PANEL_GLOBAL = "test_panel_global";
 
     browser.test.sendMessage("devtools_panel_inspectedWindow_tabId",
                              browser.devtools.inspectedWindow.tabId);
   }
 
-  const EXTENSION_ID = "@create-devtools-panel.test";
+  const longPrefix = (new Array(80)).fill('x').join('');
+  const EXTENSION_ID = `${longPrefix}@create-devtools-panel.test`;
 
   let extension = ExtensionTestUtils.loadExtension({
     useAddonManager: "temporary",
     manifest: {
       devtools_page: "devtools_page.html",
       applications: {
         gecko: {id: EXTENSION_ID},
       },
```

I'm about to refactor the above test file in Bug 1442604, but if we want to apply this small change to the test in the meantime (so that we are sure that we land the fix for this issue with a test that would fail if the issue is still there),
I'm 100% ok with it, I'll rebase my patches on top of it and solve the conflicting changes myself, otherwise I will make sure to apply this change to Bug 1442604 (so that we can prevent it from regressing without being noticed).
Priority: -- → P1
@Luca I have added your test change to create a long extension ID.

Apart from that we now log only for the main panels and "other", which was the original idea behind this telemetry event.
Flags: needinfo?(mratcliffe)
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Comment on attachment 8971999 [details]
Bug 1456923 - can't switch to dev-tools add-on that is loaded temporarily

https://reviewboard.mozilla.org/r/240738/#review246888

Changes look fine to me. Can you please also ask for a review from Luca on browser/components/extensions/test/browser/browser_ext_devtools_panel.js. This is outside of the DevTools module and might require a rubber stamp from another peer than myself.
Attachment #8971999 - Flags: review?(pbrosset) → review+
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e6f674c99538
can't switch to dev-tools add-on that is loaded temporarily r=pbro
https://hg.mozilla.org/mozilla-central/rev/e6f674c99538
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.