1477137 - Removed extensions persist on about:debugging until refreshing the page

Status: NEW

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

Description

[Hiroyuki Ikezoe (:hiro)]

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]

Comment 1

[Hiroyuki Ikezoe (:hiro)]

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. :)

Comment 2

[Hiroyuki Ikezoe (:hiro)]

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 3

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1477137 - Part 1: Add onUninstalling and onOperationCancelled listeners to detect removal/undo extension in aboutaddons.

Review commit: https://reviewboard.mozilla.org/r/259272/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259272/

Comment 4

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1477137 - Part 2: Add a test for remove temporary and undo.

Review commit: https://reviewboard.mozilla.org/r/259274/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259274/

Comment 5

[Daisuke Akatsuka (:daisuke)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcaacc216e3ac02767fc81a255c74c197556c96a

Comment 6

[Daisuke Akatsuka (:daisuke)]

Modified the test patch since memory leak failure occurred.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a8cf530033ede0c9c944e75ced452ff521e8284

Comment 7

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994748 [details]
Bug 1477137 - Part 1: Add onUninstalling and onOperationCancelled listeners to detect removal/undo extension in aboutaddons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259272/diff/1-2/

Comment 8

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994749 [details]
Bug 1477137 - Part 2: Add a test for remove temporary and undo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259274/diff/1-2/

Comment 9

[Julian Descottes [:jdescottes]]

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!

Comment 10

[Julian Descottes [:jdescottes]]

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())

Comment 11

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994748 [details]
Bug 1477137 - Part 1: Add onUninstalling and onOperationCancelled listeners to detect removal/undo extension in aboutaddons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259272/diff/2-3/

Comment 12

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994749 [details]
Bug 1477137 - Part 2: Add a test for remove temporary and undo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259274/diff/2-3/

Comment 13

[Daisuke Akatsuka (:daisuke)]

try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6008e1dd8b1cfab6887d2063df7dbbbae20bab4

Comment 14

[Julian Descottes [: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"

Comment 15

[Daisuke Akatsuka (:daisuke)]

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!

Comment 16

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994749 [details]
Bug 1477137 - Part 2: Add a test for remove temporary and undo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259274/diff/3-4/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Cristina Coroiu [:ccoroiu]]

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

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit auto_nag documentation.

Comment 20

[Suhaib Mujahid [:suhaib]]

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