1791370 - MV3 options_ui not opened in tab when user change optional host_permissions

Status: VERIFIED FIXED

(WebExtensions :: General, defect)

Description

[aRIEL]

Attachment: webext-test.zip

Steps to reproduce:

1. Open the Firefox nightly 106.0a1 (2022-09-18) with Manifest-V3 developer preview turned on (extensions.manifestV3.enabled, xpinstall.signatures.required).
2. Unzip attached file and load the temporary add-on webext-test.
3. Click the ‘Reload’ button (looks redundant but is vital).
4. From the webext-test sidebar click the button to open the preferences page.
5. Close the Preferences tab.
6. Go to the browser’s Add-ons Manager and open the webext-test Permissions tab.
7. Allow the ‘Access your data for all websites’.
8. From the webext-test sidebar click again on the button to open the preferences page.

Actual results:

The preferences page will not open. The following errors are logged:

13:32:57.702 [Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]" nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)" location: "JS frame :: chrome://browser/content/browser.js :: switchToTabHavingURI :: line 8587" data: no] browser.js:8587:24

switchToTabHavingURI chrome://browser/content/browser.js:8587
openOptionsPage chrome://browser/content/parent/ext-browser.js:87
openOptionsPage chrome://extensions/content/parent/ext-runtime.js:201
...

13:32:57.702 Uncaught (in promise) Error: An unexpected error occurred undefined

apply self-hosted:2810
applySafeWithoutClone resource://gre/modules/ExtensionCommon.jsm:622
wrapPromise resource://gre/modules/ExtensionCommon.jsm:870

From this point the preferences page will not be opened from the sidebar even if the user disallow the ‘Access your data for all websites’ or reload the sidebar.
Only reloading the add-on will temporarily remedy the problem. But any additional change to ‘Access your data for all websites’ will raised it again.

Comment 1

[Alex Cornestean]

Hello,

I reproduced the issue on the latest Nightly (107.0a1/20220920092542), Beta (106.0b2/20220920185943) and Release (105.0/20220915150737) under Windows 10 x64 and Ubuntu 16.04 LTS.

The preferences page will no longer open until the extension is reloaded, after performing the mentioned STR and the mentioned errors are logged to the browser console, confirming the issue.

Comment 2

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

The severity field is not set for this bug.
:willdurand, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[Rob Wu [:robwu]]

This is not MV3-specific. This also happens if you use the permissions API from MV2/MV3.

Technical implementation details below, for Firefox engineers.

The immediate trigger for the error is that extension.manifest.options_ui.page is preferences.html. That's an invalid value. It is expected to be moz-extension://UUID/preferences.html, because the schema specifies its type as an ExtensionURL, which should resolve to the absolute URL after parsing.

So, why is this not the case? extension.manifest looks like the raw manifest instead of the parsed manifest. This is surprising, so I put breakpoints for .manifest = assignments in Extension.jsm. Directly related to the reproduction steps here, I found that the following happens:

1. When an optional permission is added or removed, the cachePermissions() method is called.
2. The cachePermissions method is supposed to read the cached value, update the permissions and cache it again.
3. The cached value is supposedly read by a call to .parseManifest(), which in turn reads from StartupCache.
4. HOWEVER, the StartupCache appears to be empty. So the underlying ExtensionData's parseManifest method is called, which assigns the raw manifest to this.extension (among many other properties; it re-initializes the internal state of Extension instance!).

• NOTE: The implementation is not prepared to handle this, because parseManifest is not stateless. It modifies members of the Extension instance, and may log manifest validation warnings.

The minimal fix to avoid this bug is to avoid the this.manifest = manifest = this.rawManifest assignment if possible. This results in type confusion, and is the immediate cause of the observed behavior in this bug.

Then there is the question of why the cache is empty. There are three ways for the cache to be emptied:

• Extension constructor - calls clearAddonData for the ID: https://searchfox.org/mozilla-central/rev/b4150d1c6fae0c51c522df2d2c939cf5ad331d4c/toolkit/components/extensions/Extension.jsm#2416
• Extension shutdown - calls clearAddonData for the ID: https://searchfox.org/mozilla-central/rev/b4150d1c6fae0c51c522df2d2c939cf5ad331d4c/toolkit/components/extensions/Extension.jsm#3265
• startupcache-invalidate notification - clears all of StartupCache: https://searchfox.org/mozilla-central/rev/b4150d1c6fae0c51c522df2d2c939cf5ad331d4c/toolkit/components/extensions/ExtensionParent.jsm#2175-2176

The specific issue here is triggered by the StartupCache.clearAddonData call from the constructor. The method is async, and starts happening.
The extension startup immediately calls startup() after the Extension constructor, and the loadManifest method is immediately called at the start (in the same micro-task). Consequently, the clearAddonData and loadManifest / parseManifest methods run in parallel. As a result, when loadManifest finishes (and writes to StartupCache), clearAddonData wiped the cache immediately after that.

To avoid this issue, it may make sense to wait with populating the StartupCache until clearAddonData has completed. Whether in the implementation of StartupCache itself, or in the consumers of it in the Extension class. Or somehow make clearAddonData synchronous (completing before it returns).

In short, this comment identifies two actionable tasks:

• Don't assign this.manifest = manifest to avoid type confusion.
• Synchronize the order of StartupCache deletion (clearAddonData) and the usage of it.

Comment 4

[Tomislav Jovanovic :zombie]

Attachment: Bug 1791370 - Ensure proper order of startup cache clearing and saving, r=robwu

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50a57a750043
Ensure proper order of startup cache clearing and saving, r=robwu

Comment 6

[Cristian Tuns]

Backed out for private browsing failures on browser_html_detail_view.js

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_html_detail_view.js | The PB badge is now shown -

Comment 7

[Rob Wu [:robwu]]

The explicit (long) wait before add-on initialization may increase the likelihood of triggering intermittent failures.

Instead of waiting at the consumer, perhaps we should introduce a wait for pending deletion tasks before proceeding with get / set?: https://searchfox.org/mozilla-central/rev/670e2e0999f04dc7734c8c12b2c3d420a1e31f12/toolkit/components/extensions/ExtensionParent.jsm#2229-2230,2243-2244
That will not only fix the specific issue here, but more broadly all uses of StartupCache.

Comment 8

[Iulian Moraru]

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

Comment 9

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

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit auto_nag documentation.

Comment 10

[Tomislav Jovanovic :zombie]

(In reply to Rob Wu [:robwu] from comment #7)

Instead of waiting at the consumer, perhaps we should introduce a wait for pending deletion tasks before proceeding with get / set?: https://searchfox.org/mozilla-central/rev/670e2e0999f04dc7734c8c12b2c3d420a1e31f12/toolkit/components/extensions/ExtensionParent.jsm#2229-2230,2243-2244
That will not only fix the specific issue here, but more broadly all uses of StartupCache.

How would this be different? If we're waiting for deletion tasks to complete, what difference does it make where we're waiting?

The code in startup cache would be much more complex because we would only want to serialize operations by extension, not globally.

Also this is only happening during addon updates, which we only use in a few places, and can't affect either startup or 95% of tests.

Comment 11

[Alex Cornestean]

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

After performing the STR from Comment 0, the preferences page will now properly open. The mentioned errors are also no longer logged to the browser console, confirming the fix.

Comment 12

[Rob Wu [:robwu]]

(In reply to Tomislav Jovanovic :zombie from comment #10)

(In reply to Rob Wu [:robwu] from comment #7)

Instead of waiting at the consumer, perhaps we should introduce a wait for pending deletion tasks before proceeding with get / set?: https://searchfox.org/mozilla-central/rev/670e2e0999f04dc7734c8c12b2c3d420a1e31f12/toolkit/components/extensions/ExtensionParent.jsm#2229-2230,2243-2244
That will not only fix the specific issue here, but more broadly all uses of StartupCache.

How would this be different? If we're waiting for deletion tasks to complete, what difference does it make where we're waiting?

There is nothing in the StartupCache API or its implementation that ensures that a parallel run of StartupCache mutations/accesses are synchronized. StartupCache.clearAddonData calls CacheStore's delete. While awaiting clearAddonData addresses the immediate issue reported here, it doesn't fix the fact that a similar issue can happen again elsewhere.

The code in startup cache would be much more complex because we would only want to serialize operations by extension, not globally.

An alternative to synchronization is to make the operation atomic (completing within the microtask). There is only an await there because startup awaits StartupCache._dataPromise at https://searchfox.org/mozilla-central/rev/404408660a4d976e2ac25881cb1e1f2712f2d430/toolkit/components/extensions/ExtensionParent.jsm#2204. Once resolved, this promise will be resolved forever. So a way to almost always have an atomic operation in the .delete method is to make getStore sync, and optionally await at the start of delete, set, etc. - if (!StartupCache._data) { await StartupCache._dataPromise; }.

Even without synchronization, we could at least make debugging easier by marking pending delete/set/get calls (for a given path), and logging a warning that there is a concurrent access.

This all is not needed right now since we've fixed the bug in another way. I wouldn't be surprised for a similar issue to resurface in the future though.