1801337 - Removed site permission is added back after refresh

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[vcarciu]

Attachment: MIDI_sample.mid

Prerequisites:
Set the “midi.testing” pref to “true” in about:config.
Check that the “dom.sitepermsaddon-provider.enabled” pref is set to “true” in about:config. Otherwise, set it to “true”.

Steps to reproduce:
1.Install both addons from https://ryoyakawai.github.io/smfplayer/ and https://webmidi-examples.glitch.me/
2. Go to https://ryoyakawai.github.io/smfplayer/ , select an Output , then upload the attached MIDI file and click on Play button
3.Go to about:addons, and click on Site Permissions from sidebar
4.Remove the permission for ryoyakawai.github.io
5.Refresh the about:addons page

Expected results:
After refresh, the permission is not displayed and if https://ryoyakawai.github.io/smfplayer/ is opened again, the install flow panel will be triggered

Actual results:
After refresh the permission for ryoyakawai.github.io is displayed even if it was removed and going to https://ryoyakawai.github.io/smfplayer/ will not trigger a new install

NOTES:
1.If you are not able to reproduce first time , please repeat all the steps one more time and the bug will be reproduced
2.Bug is NOT reproducible if we have a single permission in Site Permissions(so first step is mandatory exactly as described)
3.Reproducible on WIn, Linux, Mac, using letest FF 109 and FF 108
4. I will attach a video of the issue

Comment 1

[vcarciu]

Attachment: Video of the issue

Comment 2

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

Reduced steps to reproduce:

1. Install addon from https://webmidi-examples.glitch.me/
2. Go to about:addons, and select the "Site Permissions" section
3. Open a new https://ryoyakawai.github.io/smfplayer/ tab and install the addon
4. Remove the permission for ryoyakawai.github.io
5. Refresh the about:addons page

Result: ryoyakawai.github.io entry is still displayed

This happens because the addon that is used to uninstall the addon on step 4 (in this case, a SitePermsAddonInstalling) has an empty #permissions array. This property is initialized in the SitePermsAddonWrapper constructor (SitePermsAddonProvider.sys.mjs), but we don't pass a second argument from SitePermsAddonInstalling (https://searchfox.org/mozilla-central/rev/aa329cf7506ddd966542e642ec00223fd7461599/toolkit/mozapps/extensions/internal/SitePermsAddonProvider.sys.mjs#260)
So in the end, we don't remove the midi permission for the origin when uninstalling (SitePermsAddonProvider.sys.mjs)

Then on step 5, the list of addons displayed in the page is fetched from SitePermsAddonProvider#wrappersMapByOrigin, in which we do have a "proper" SitePermsAddonWrapper that was created as a result of the install SitePermsAddonProvider.sys.mjs

install() {
  if (this.state === lazy.AddonManager.STATE_PROMPTS_DONE) {
    lazy.AddonManagerPrivate.callAddonListeners("onInstalling", this.addon);
    Services.perms.addFromPrincipal(
      this.principal,
      this.newSitePerm,
      Services.perms.ALLOW_ACTION
    );

And since now the addon does have a permission in #permissions, it is properly revoked, and the addon is successfully "uninstalled

---

Since we need to uplift this, let's do the smallest change that will fix this bug: pass the permission when creating the SitePermsAddonWrapper from SitePermsAddonInstalling.

We might then revisit our approach in a follow up bug to make things easier to understand.

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1801337 - Fix uninstall on sitepermission AddonCard created on the AOM onInstalled event. r=rpl.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hey Nicolas,
I have been thinking about other corner cases and related STRs that may result in the same issue and not be covered with just the approach that we have discussed earlier today and here is another STR:

• Install addon from https://webmidi-examples.glitch.me/
• Go to about:addons, and select the "Site Permissions" section
• NOTE: The first two steps are important to be able to trigger the bug, the list view for the sitepermission addons has to be selected to make sure aboutaddons.js would be listing on the onInstalled AOM event and creating addon cards from the SitePermsAddonInstalling instance as a side effect of that
• Open a tab to https://developers.mozilla.org/
• Open the weconsole and execute await navigator.requestMIDIAccess(); (which will create the AddonCard and initially only the "midi" gated permission is granted to the mozilla.org siteOrigin.
• switch to about:addons tab and confirm the AddonCard has been created
• switch back to the https://developers.mozilla.org/ tab and execute in the webconsole await navigator.requestMIDIAccess({ sysex: true }) (which will add the "midi-sysex" gated permission, there is already an AddonCard created from the previous SitePermsAddonInstalling and so aboutaddons.js will not be recreating it)
• now uninstall the sitepermission addon for mozilla.org from the about:addons tab and reload the about:addons tab

Expected behavior:

• the entry for the mozilla.org sitepermission addon should have been gone, all associated gated permission should have been removed

Actual behavior:

• the entry for the mozilla.org sitepermission addon is back after reloading the tab (due to the midi-sysex gated permission still being granted)

The patch attached in comment 3 contains a still minimal set of changes that would handle both the STR from comment 2 and the one from this comment.

I didn't had time to look into the test case and so there is no test yet in the patch, it would be good to add a couple based on the STR we have identified.

Feel free to commandeer the patch to finalize it.

I'm needinfo-ing mainly to highlight to you the other STR described in this comment and that the patch attached to comment 3 contains a possible approach to handle both.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e27a40027b3
Fix uninstall on sitepermission AddonCard created on the AOM onInstalled event. r=rpl.

Comment 6

[Noemi Erli[:noemi_erli]]

Backed out changeset 5e27a40027b3 (Bug 1801337) for causing failures in browser_installtrigger_install.js CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=Vx0QCbOsSwy5BEzVMBmjWg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=5e27a40027b376525b6f52e085d4fe36427ccbb7

Log: https://treeherder.mozilla.org/logviewer?job_id=397431201&repo=autoland&lineNumber=3739

Backout: https://hg.mozilla.org/integration/autoland/rev/a3fa731b2b6eec6f7c9c9ba7b7e25fb85ed131d7

Comment 7

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

running only toolkit/mozapps/extensions/test/browser/browser_installtrigger_install.js is fine, but ./mach test toolkit/mozapps/extensions/test/browser/browser_html_sitepermission_addons.js toolkit/mozapps/extensions/test/browser/browser_installtrigger_install.js does indeed fail.
I'll check how the test added in my patch is interfering with browser_installtrigger_install.js

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aee8df762c94
Fix uninstall on sitepermission AddonCard created on the AOM onInstalled event. r=rpl.

Comment 9

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/aee8df762c94

Comment 10

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

Comment on attachment 9304172 [details]
Bug 1801337 - Fix uninstall on sitepermission AddonCard created on the AOM onInstalled event. r=rpl.

Beta/Release Uplift Approval Request

• User impact if declined: In some specific condition removing addons from about:addons won't work for Site Permission addons (a new type of addon that is added in 108 to allow shipping the WebMIDI API)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: There are 2 different STRs that both need to be tested.

Pre-requisite:

1. Go to about:config
2. Search for midi.testing
3. Set the entry to true
4. Restart the browser

First case:

1. Install addon from https://webmidi-examples.glitch.me/
2. Go to about:addons, and select the "Site Permissions" section
3. Open a new https://ryoyakawai.github.io/smfplayer/ tab and install the addon
4. Remove the permission for ryoyakawai.github.io
5. Refresh the about:addons page

Expected result: ryoyakawai.github.io entry is still removed

Second case:

1. Remove all existing "Site Permissions" addon first
2. Install addon from https://webmidi-examples.glitch.me/
3. Go to about:addons, and select the "Site Permissions" section
4. Open a tab to https://developer.mozilla.org
5. Open the weconsole, execute await navigator.requestMIDIAccess(), and install the addon
6. Switch to about:addons tab and confirm that there's a developers.mozilla.org entry
7. Switch back to the https://developers.mozilla.org tab, execute in the webconsole await navigator.requestMIDIAccess({ sysex: true }) and install the addon
8. Switch to about:addons tab and remove the mozilla.org entry
9. Reload the about:addons tab

Expected result: mozilla.org entry is still removed

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Changes are covered by automated test and impact a constrained part of the code
• String changes made/needed:
• Is Android affected?: No

Comment 11

[Alex Cornestean]

Verified as Fixed on the latest Nightly (109.0a1/20221123213526). Tested on Windows 10 x64 and Ubuntu 16.04 LTS.

Verified both scenarios listed in Comment 10 and the removed add-ons do not return/are not listed anymore after reloading about:addons, confirming the fix.

Comment 12

[Donal Meehan [:dmeehan]]

Comment on attachment 9304172 [details]
Bug 1801337 - Fix uninstall on sitepermission AddonCard created on the AOM onInstalled event. r=rpl.

Approved for 108.0b6.

Comment 13

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8108d442f165

Comment 14

[Alex Cornestean]

Verified as Fixed on the latest Beta (108.0b6/20221124185931). Tested on Windows 10 x64 and Ubuntu 16.04 LTS.

Verified both scenarios listed in Comment 10 and the removed add-ons do not return/are not listed anymore after reloading about:addons, confirming the fix.