Open Bug 1477137 Opened 5 years ago Updated 1 year ago

Removed extensions persist on about:debugging until refreshing the page

Categories

(DevTools :: about:debugging, enhancement, P5)

Product:
DevTools
Component:
about:debugging
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: hiro, Assigned: daisuke, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1477137 - Part 1: Add onUninstalling and onOperationCancelled listeners to detect removal/undo extension in aboutaddons.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1477137 - Part 2: Add a test for remove temporary and undo.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details 
When an extensions listed on about:addons is removed there, the extension on about:debugging page still persists as if the extension is still enabled.  But unfortunately, if 'Debug' button for the extension is pressed, below error happens.

Connecting to localhost:41861, ws: false
Start protocol client for connection
Get root form for toolbox
Create toolbox target: {
  "0": {
    "form": {
      "actor": "server2.conn0.webExtension13",
      "id": "{13b3aeaa-570d-4f28-aeff-b77e82f29b11}",
      "name": "Animation Toggle Icon",
      "url": "https://addons.mozilla.org/firefox/downloads/file/890416/animation_toggle_icon-1.0-an+fx.xpi?src=search",
      "iconURL": "https://addons.cdn.mozilla.net/user-media/addon_icons/950/950052-64.png?modified=0fecfd74",
      "debuggable": false,
      "temporarilyInstalled": false,
      "type": "extension",
      "isWebExtension": true,
      "manifestURL": null,
      "warnings": []
    },
    "chrome": true,
    "isBrowsingContext": true
  }
}
[object Object]
Setting P5 since I think this isn't a big problem for normal use case.  I just wanted to know how to observe extensions removal there, but it actually  didn't there. :)
Priority: -- → P5
I just realized that about:debugging observes 'onUninstalled' notification [1] but unfortunately it isn't called at all.  I don't know why.

Anyway this might be a problem in new about:debugging, since in the new about:debugging we have to stop listening remote device connections/disconnections when the new adbhelper extension is removed.

[1] https://hg.mozilla.org/mozilla-central/file/4aa8eb6e5ca7/devtools/client/aboutdebugging/components/addons/Panel.js#l51
Comment on attachment 8994748 [details]
Bug 1477137 - Part 1: Add onUninstalling and onOperationCancelled listeners to detect removal/undo extension in aboutaddons.

https://reviewboard.mozilla.org/r/259272/#review266586

Works for me, thanks!
Attachment #8994748 - Flags: review?(jdescottes) → review+
Comment on attachment 8994749 [details]
Bug 1477137 - Part 2: Add a test for remove temporary and undo.

https://reviewboard.mozilla.org/r/259274/#review266588

As discussed I would like a simpler test, that doesn't involve about:addons.

We already have a `uninstallAddon` test helper, which we need to modify to support sending a "allowUndo" parameter to addon.uninstall.
We also need a new API to cancel the uninstall (normally simply calling addon.cancelUninstall())
Attachment #8994749 - Flags: review?(jdescottes)
Comment on attachment 8994749 [details]
Bug 1477137 - Part 2: Add a test for remove temporary and undo.

https://reviewboard.mozilla.org/r/259274/#review266900

The test looks great Daisuke, thanks! I have a few suggestions to reformulate the comments and logs, but that's it.

Thanks for fixing this!

::: devtools/client/aboutdebugging/test/browser_addons_remove_temporary_and_undo.js:5
(Diff revision 3)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +"use strict";
> +
> +add_task(async () => {

Add a small comment explaining what the test is about.

::: devtools/client/aboutdebugging/test/browser_addons_remove_temporary_and_undo.js:21
(Diff revision 3)
> +    name: addonName,
> +    isWebExtension: true,
> +  });
> +  ok(getTargetEl(document, addonID), "Add-on is shown");
> +
> +  info("Uninstalling with allowing undo to remove");

reformulate: "Uninstall the addon but allow to undo the action (same as when uninstalling from about:addons)"

::: devtools/client/aboutdebugging/test/browser_addons_remove_temporary_and_undo.js:24
(Diff revision 3)
> +  ok(getTargetEl(document, addonID), "Add-on is shown");
> +
> +  info("Uninstalling with allowing undo to remove");
> +  await uninstallAddon({ document, id: addonID, name: addonName, allowUndo: true });
> +  await waitUntil(() => !getTargetEl(document, addonID), 100);
> +  ok(true, "Add-on is shown after removal");

Should say "hidden" rather than "shown"

::: devtools/client/aboutdebugging/test/browser_addons_remove_temporary_and_undo.js:26
(Diff revision 3)
> +  info("Uninstalling with allowing undo to remove");
> +  await uninstallAddon({ document, id: addonID, name: addonName, allowUndo: true });
> +  await waitUntil(() => !getTargetEl(document, addonID), 100);
> +  ok(true, "Add-on is shown after removal");
> +
> +  info("Canceling unninstalling to undo");

nit: unninstalling -> uninstalling

::: devtools/client/aboutdebugging/test/browser_addons_remove_temporary_and_undo.js:29
(Diff revision 3)
> +  ok(true, "Add-on is shown after removal");
> +
> +  info("Canceling unninstalling to undo");
> +  await cancelUninstallAddon({ document, id: addonID, name: addonName });
> +  await waitUntil(() => getTargetEl(document, addonID), 100);
> +  ok(true, "Add-on is re-appeared after undo");

reformulate: either
"add-on re-appeared after undo"
"add-on is displayed after undo"
Attachment #8994749 - Flags: review?(jdescottes) → review+
Comment on attachment 8994749 [details]
Bug 1477137 - Part 2: Add a test for remove temporary and undo.

https://reviewboard.mozilla.org/r/259274/#review266900

Thanks Julian!
I'll address those, then will make them to land!
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd5e331122a9
Part 1: Add onUninstalling and onOperationCancelled listeners to detect removal/undo extension in aboutaddons. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/c75f46c67e27
Part 2: Add a test for remove temporary and undo. r=jdescottes
Backed out 2 changesets (bug 1477137) for devtools failures at devtools/client/aboutdebugging/test/browser_addons_toggle_debug.js

Backout: https://hg.mozilla.org/integration/autoland/rev/6ddec20d1e35b91e52af72b963f92320c111536c

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c75f46c67e2733766dc9f2a3b6ac7979b6ef65f7

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=190471019&repo=autoland&lineNumber=5275

02:17:33     INFO - GECKO(767) |   file:////Users/cltbld/tasks/task_1532682364/build/tests/modules/
02:17:33     INFO - GECKO(767) |   chrome://browser/skin/tabbrowser/loading.svg
02:17:33     INFO - GECKO(767) |   chrome://browser/skin/stop.svg
02:17:33     INFO - GECKO(767) |   chrome://browser/skin/tabbrowser/loading-burst.svg
02:17:33     INFO - GECKO(767) |   chrome://browser/skin/reload-to-stop.svg
02:17:33     INFO - GECKO(767) |   chrome://browser/content/browser.xul
02:17:33     INFO - GECKO(767) |   chrome://global/content/editMenuOverlay.js
02:17:33     INFO - GECKO(767) |   chrome://global/content/bindings/menu.xml#menu-menubar
02:17:33     INFO - GECKO(767) |   chrome://browser/skin/identity-icon-notice.svg
02:17:33     INFO - GECKO(767) |   chrome://browser/skin/tabbrowser/tab-loading.png
02:17:33     INFO - GECKO(767) |   chrome://global/content/bindings/browser.xml#browser
02:17:33     INFO - GECKO(767) |   chrome://browser/content/search/search.xml#search-one-offs
02:17:33     INFO - GECKO(767) | Assertion failure: isEmpty() (failing this assertion means this LinkedList's creator is buggy: it should have removed all this list's elements before the list's destruction), at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/LinkedList.h:458
02:17:33     INFO - GECKO(767) | #01: libsystem_c.dylib + 0x5e72d
02:17:33     INFO - 
02:17:33     INFO - GECKO(767) | #02: libsystem_c.dylib + 0x5ea30
02:17:33     INFO - 
02:17:33     INFO - TEST-INFO | Main app process: killed by SIGSEGV
02:17:33    ERROR - TEST-UNEXPECTED-FAIL | devtools/client/aboutdebugging/test/browser_addons_toggle_debug.js | leaked 2 window(s) until shutdown [url = moz-extension://11270e66-8e55-9549-af0b-c20694991ef4/_generated_background_page.html]
02:17:33     INFO - TEST-INFO | devtools/client/aboutdebugging/test/browser_addons_toggle_debug.js | windows(s) leaked: [pid = 773] [serial = 38], [pid = 773] [serial = 40]
02:17:33    ERROR - TEST-UNEXPECTED-FAIL | devtools/client/aboutdebugging/test/browser_addons_toggle_debug.js | leaked 1 docShell(s) until shutdown
02:17:33     INFO - TEST-INFO | devtools/client/aboutdebugging/test/browser_addons_toggle_debug.js | docShell(s) leaked: [pid = 773] [id = {4fb562f2-0378-8647-8c52-67812ea5a585}]
02:17:33     INFO - Buffered messages finished
02:17:33    ERROR - TEST-UNEXPECTED-FAIL | Last test finished | application terminated with exit code -11
02:17:33     INFO - runtests.py | Application ran for: 0:03:01.839497
02:17:33     INFO - zombiecheck | Reading PID log: /var/folders/pk/6b10kgg50h302fsym38kdmtm00000w/T/tmpoKbBIRpidlog
02:17:33     INFO - ==> process 767 launched child process 771
02:17:33     INFO - ==> process 767 launched child process 773
02:17:33     INFO - ==> process 767 launched child process 774
02:17:33     INFO - ==> process 767 launched child process 775
02:17:33     INFO - ==> process 767 launched child process 778
02:17:33     INFO - ==> process 767 launched child process 788
02:17:33     INFO - ==> process 767 launched child process 789
02:17:33     INFO - ==> process 767 launched child process 790
02:17:33     INFO - ==> process 767 launched child process 791
02:17:33     INFO - ==> process 767 launched child process 792
02:17:33     INFO - ==> process 767 launched child process 793
02:17:33     INFO - ==> process 767 launched child process 794
02:17:33     INFO - ==> process 767 launched child process 795
02:17:33     INFO - ==> process 767 launched child process 796
02:17:33     INFO - zombiecheck | Checking for orphan process with PID: 771
02:17:33     INFO - zombiecheck | Checking for orphan process with PID: 773
02:17:33     INFO - zombiecheck | Checking for orphan process with PID: 774
02:17:33     INFO - zombiecheck | Checking for orphan process with PID: 775
02:17:33     INFO - zombiecheck | Checking for orphan process with PID: 778
02:17:33     INFO - zombiecheck | Checking for orphan process with PID: 788
02:17:33     INFO - zombiecheck | Checking for orphan process with PID: 789
02:17:33     INFO - zombiecheck | Checking for orphan process with PID: 790
02:17:33     INFO - zombiecheck | Checking for orphan process with PID: 791
02:17:33     INFO - zombiecheck | Checking for orphan process with PID: 792
02:17:33     INFO - zombiecheck | Checking for orphan process with PID: 793
02:17:33     INFO - zombiecheck | Checking for orphan process with PID: 794
02:17:33     INFO - zombiecheck | Checking for orphan process with PID: 795
02:17:33     INFO - zombiecheck | Checking for orphan process with PID: 796
02:17:33     INFO - Stopping web server
02:17:33     INFO - Stopping web socket server
02:17:33     INFO - Stopping ssltunnel
Flags: needinfo?(dakatsuka)

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Flags: needinfo?(daisuke)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.