1597107 - listener not removed when removing permission with permissions.remove

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P5)

Description

[kernp25]

If you do:

browser.permissions.request({
  permissions: ["notifications"]
})

then

browser.notifications.onClicked.addListener(onClicked);

and finally

browser.permissions.remove({
  permissions: ["notifications"]
})

the listener will not be removed.

Because, after you do

browser.permissions.remove({
  permissions: ["notifications"]
})

browser.notifications is undefined and the listener added by the add-on should also be removed. Because the add-on can no longer use browser.notifications. So it doesn't make sense, to keep the listener alive.

Comment 1

[kernp25]

Attachment: background.zip

Comment 2

[kernp25]

Should

browser.permissions.remove({
  permissions: ["notifications"]
})

also remove the listener?

Comment 3

[kernp25]

I have a test add-on attached that shows the problem:
It should not log the message "already has listener" to the console.

Comment 4

[Andrew Swan [:aswan]]

First, please don't needinfo multiple people without asking specific questions and waiting a reasonable time for a reply.

(In reply to kernp25 from comment #0)

browser.notifications is undefined and the listener added by the add-on should also be removed. Because the add-on can no longer use browser.notifications. So it doesn't make sense, to keep the listener alive.

I'm not sure "it doesn't make sense" is a strong enough reason to write and maintain more code to clean up a listener in this case -- it appears to me that the listener won't ever actually be invoked so does it do any actual harm to keep it around? If you want to propose a change to the current behavior, please include a more detailed explanation of why the change would be valuable and direct a question to a current member of the addons engineering team (I think :robwu has been the most active developer of the notifications api, he would be a good place to start)

Comment 5

[Rob Wu [:robwu]]

This is not specific to the notifications API, but a bindings issue.

The console shows the following error:

map.keys is not a function ExtensionChild.jsm:1033

https://searchfox.org/mozilla-central/rev/524bed6dfbc5ae21c62632d83b7573448b29e0ac/toolkit/components/extensions/ExtensionChild.jsm#1033

To fix this, we should iterate over map.listeners.keys() instead of map.keys(), and add a regression test. Use .hasListener to check whether a listener has been removed and/or trigger the event to check whether the event has been dispatched.

Comment 6

[Shane Caraveo (:mixedpuppy)]

This is actually fixed in the patch for bug 1630419, and the test in that patch would fail without the fix.

Comment 7

[Rob Wu [:robwu]]

in bug 1630419