1793365 - Preexisting site permission XPI with non-default OriginAttributes breaks new site permission addon provider UI

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Bobby Holley (:bholley)]

STR:

• Open a new window in private browsing
• Navigate to https://bholley.net/testcases/webmidi/test.html
• install the addon
• leave PB
• about:config, toggle dom.sitepermsaddon-provider.enabled
• go to about:addons
• click the site permissions tab
• open browser console

Results:
UI doesn't update, browser console shows the following:

Uncaught (in promise) TypeError: URL constructor: https://bholley.net^privateBrowsingId=1 is not a valid URL.
    isKnownPublicSuffix resource://gre/modules/addons/siteperms-addon-utils.sys.mjs:23
    handlePermissionChange resource://gre/modules/addons/SitePermsAddonProvider.sys.mjs:375
    _initPromise resource://gre/modules/addons/SitePermsAddonProvider.sys.mjs:423
    lazyInit resource://gre/modules/addons/SitePermsAddonProvider.sys.mjs:419
    getAddonByID resource://gre/modules/addons/SitePermsAddonProvider.sys.mjs:445
    promiseCallProvider resource://gre/modules/AddonManager.jsm:265
    promises resource://gre/modules/AddonManager.jsm:2831
    getAddonByID resource://gre/modules/AddonManager.jsm:2830
    getAddonByID resource://gre/modules/AddonManager.jsm:4270
    _checkScreenshotsPref resource:///modules/BrowserGlue.jsm:2039
    _monitorScreenshotsPref resource:///modules/BrowserGlue.jsm:2064
    BG__onWindowsRestored resource:///modules/BrowserGlue.jsm:2347
    BG_observe resource:///modules/BrowserGlue.jsm:950
    ssi_sendRestoreCompletedNotifications resource:///modules/sessionstore/SessionStore.jsm:5782
    ssi_restoreWindow resource:///modules/sessionstore/SessionStore.jsm:4465
    _restoreWindowsFeaturesAndTabs resource:///modules/sessionstore/SessionStore.jsm:4537
    _restoreWindowsInReversedZOrder resource:///modules/sessionstore/SessionStore.jsm:4561
    ssi_restoreWindows resource:///modules/sessionstore/SessionStore.jsm:4621
6 siteperms-addon-utils.sys.mjs:23:20

Comment 1

[Bobby Holley (:bholley)]

This situation is unusual both because there is a preexisting site permission XPI, and because the origin has non-default OriginAttributes (the PB ID), which get serialized as a suffix to the origin. It's not yet clear to me whether both of these are required to trigger the bug, but ideally we'd handle both (the former because some of our partners may have XPIs installed from the previous iteration of this setup, and the latter because it's a real user scenario).

Comment 2

[Bobby Holley (:bholley)]

OK, seems I can reach the same exception without any legacy XPI by enabling the new provider and then visiting https://ryoyakawai.github.io/smfplayer/ in either a PB window or a non-default container tab.

Interestingly, if I visit the same URL in a regular tab and proceed through the install flow, the container tab subsequently works (i.e. it acts as if the add-on is installed, even though it was unable to trigger the installation), whereas the PB tab does not. I could see an argument that we shouldn't offer WebMIDI in PB at all (because we can't guarantee non-persistence in the context of hardware peripherals), but it's worth understanding what's going on under the hood and then figuring out what kind of behavior makes sense.

Comment 3

[William Durand [:willdurand]]

I started to investigate a bit and it looks like we could possibly use siteOriginNoPrefix instead of siteOrigin on the principal when we attempt to make an URL from an origin. This is obviously what we see in the stack in comment 0 but I am wondering if that would allow us to have a single add-on for a given origin, no matter whether we are in a PB window or container tab.

Comment 4

[Bobby Holley (:bholley)]

(In reply to William Durand [:willdurand] from comment #3)

I started to investigate a bit and it looks like we could possibly use siteOriginNoPrefix

I assume you mean NoSuffix.

instead of siteOrigin on the principal when we attempt to make an URL from an origin. This is obviously what we see in the stack in comment 0 but I am wondering if that would allow us to have a single add-on for a given origin, no matter whether we are in a PB window or container tab.

I think this is reasonable, and matches how other addons work (if you install them in any context they're available in all contexts [1]). It's not how other permissions work (they are per-context), but given that we're using the add-on metaphor here I think we should align on that axis.

[1] Modulo the "allow in PB" option on add-ons, which appears to be force-set to true for Site Permission add-ons.

Comment 5

[William Durand [:willdurand]]

(In reply to Bobby Holley (:bholley) from comment #4)

I assume you mean NoSuffix.

Ugh yeah, sorry, it should have been siteOriginNoSuffix.

Comment 6

[William Durand [:willdurand]]

Attachment: Bug 1793365 - Use siteOriginNoSuffix in site permission addon provider. r?rpl!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc7f1f2b0e06
Use siteOriginNoSuffix in site permission addon provider. r=rpl

Comment 8

[Iulian Moraru]

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

Comment 9

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

I managed to reproduce the error in the browser console on a 2022-10-02 Nightly build but I don't fully understand what the "expected" results on the fixed builds should be. On latest Nightly I can't reproduce the error anymore. Is this the expected behavior of the fix or are there any other changes in the UI/console I should be looking for?
Thank you.

Comment 10

[Bobby Holley (:bholley)]

That should be sufficient, thanks.

Comment 11

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Re-tested Firefox 107.0b4(build ID: 20221023190001), Nightly 108.0a1(build ID: 20221024212806) on Windows 10, macOS 12, Ubuntu 22. Browser console error doesn't reproduce anymore. Verified as fixed on Firefox 107.0b4(build ID: 20221023190001), Nightly 108.0a1(build ID: 20221024212806) on Windows 10, macOS 12, Ubuntu 22.