MV3 options_ui not opened in tab when user change optional host_permissions
Categories
(WebExtensions :: General, defect)
Tracking
(firefox105 wontfix, firefox106 wontfix, firefox107 wontfix, firefox108 wontfix, firefox109 verified)
People
(Reporter: ariel.hazard, Assigned: zombie)
References
Details
(Whiteboard: [addons-jira])
Attachments
(2 files)
Steps to reproduce:
- Open the Firefox nightly 106.0a1 (2022-09-18) with Manifest-V3 developer preview turned on (extensions.manifestV3.enabled, xpinstall.signatures.required).
- Unzip attached file and load the temporary add-on webext-test.
- Click the ‘Reload’ button (looks redundant but is vital).
- From the webext-test sidebar click the button to open the preferences page.
- Close the Preferences tab.
- Go to the browser’s Add-ons Manager and open the webext-test Permissions tab.
- Allow the ‘Access your data for all websites’.
- 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•2 years ago
|
||
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•2 years ago
|
||
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•2 years ago
•
|
||
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:
- When an optional permission is added or removed, the
cachePermissions()
method is called. - The
cachePermissions
method is supposed to read the cached value, update the permissions and cache it again. - The cached value is supposedly read by a call to
.parseManifest()
, which in turn reads fromStartupCache
. - HOWEVER, the StartupCache appears to be empty. So the underlying
ExtensionData
'sparseManifest
method is called, which assigns the raw manifest tothis.extension
(among many other properties; it re-initializes the internal state ofExtension
instance!).
- NOTE: The implementation is not prepared to handle this, because
parseManifest
is not stateless. It modifies members of theExtension
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#2416Extension
shutdown - calls clearAddonData for the ID: https://searchfox.org/mozilla-central/rev/b4150d1c6fae0c51c522df2d2c939cf5ad331d4c/toolkit/components/extensions/Extension.jsm#3265startupcache-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.
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Comment 4•1 year ago
|
||
Updated•1 year ago
|
Pushed by tjovanovic@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/50a57a750043 Ensure proper order of startup cache clearing and saving, r=robwu
Comment 6•1 year ago
|
||
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•1 year ago
|
||
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•1 year ago
|
||
bugherder |
Comment 9•1 year ago
|
||
Since nightly and release are affected, beta will likely be affected too.
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 10•1 year ago
|
||
(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.
Updated•1 year ago
|
Updated•1 year ago
|
Comment 11•1 year ago
|
||
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•1 year ago
|
||
(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.
Description
•