2006233 - browser.declarativeNetRequest.* calls throw sometimes, depending on whether a second extension is installed

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[Jacob Winters]

Steps to reproduce:

Minimal repro:

1. In Developer Edition 147.0b4,
2. Disable xpinstall.signatures.required in about:config
3. Download main-extension.zip and privacypass-extension.zip from https://github.com/jacobwinters/firefox-declarativeNetRequest-repro
4. Install them as extensions
5. Visit https://kagi.com
6. Restart the browser
7. Visit https://kagi.com again
8. In about:debugging, inspect the Kagi Search for Firefox extension (main-extension.zip)
9. In the extension's console, run browser.declarativeNetRequest.getDynamicRules()
10. "Uncaught (in promise) Error: An unexpected error occurred"
11. Most other functions on browser.declarativeNetRequest do that too

User-facing version from which it was derived: installing both the Kagi Search for Firefox and Kagi Privacy Pass extensions at the same time breaks the first one's auth functionality, which is implemented through declarativeNetRequest. When the full user-facing problem is happening, steps 8-11 apply unchanged.

Actual results:

Exception thrown from declarativeNetRequest.getDynamicRules.

Browser toolbox console shows the following:

Unexpected non-empty DNRStore data. DNR startupCache data load aborted. 24 ExtensionDNRStore.sys.mjs:1204:11
    #promiseStartupCacheLoaded resource://gre/modules/ExtensionDNRStore.sys.mjs:1204
    #readStoreDataFromStartupCache resource://gre/modules/ExtensionDNRStore.sys.mjs:1366
    _readData resource://gre/modules/ExtensionDNRStore.sys.mjs:1292
    #readData resource://gre/modules/ExtensionDNRStore.sys.mjs:1284
    #getDataPromise resource://gre/modules/ExtensionDNRStore.sys.mjs:841
    #initExtension resource://gre/modules/ExtensionDNRStore.sys.mjs:1155
    InterpretGeneratorResume self-hosted:1312
    AsyncFunctionNext self-hosted:780

I'm guessing that there's some sort of concurrency bug around that object's _data field that's triggered in the presence of multiple extensions. Not sure how else to explain the way the bug depends on the second extension, which in the minimal repro case is just a manifest file.

Expected results:

I don't think that declarativeNetRequest calls should fail under these conditions.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'WebExtensions::Untriaged' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Rob Wu [:robwu]]

I'll investigate.

Comment 3

[Rob Wu [:robwu]]

What happens here is as follows:

1. We load an extension that does not have any DNR rules. That causes the extension's StartupCache to be populated with the knowledge that the extension does not have any rules.
2. We load another extension with rules (whether static or dynamic).
3. After restarting Firefox, we see the first extension without rules, and as an optimization skip the more expensive full initialization, and partially initialize _data (link to #initExtension implementation).
4. However, when the second extension starts up, as yet another optimization we attempt to read the parsed rules (from another startup cache file) (entry point). This commences the full initialization from cached data, but the logic there assumes that we have not initialized any data yet. Because we did initialize (empty) data for an extension, that check fails and we raise an error (link to #promiseStartupCacheLoaded implementation).

We should accept this scenario and not raise an error in this case.

Comment 4

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

Set release status flags based on info from the regressing bug 1921353

Comment 5

[Rob Wu [:robwu]]

Attachment: Bug 2006233 - Accept partially initialized DNR store on browser startup

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/50ae7d88b8df
https://hg.mozilla.org/integration/autoland/rev/b07414fe6cac
Accept partially initialized DNR store on browser startup r=rpl

Comment 8

[Sandor Molnar[:smolnar]]

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

Comment 9

[Rob Wu [:robwu]]

Attachment: Bug 2006233 - Accept partially initialized DNR store on browser startup

Original Revision: https://phabricator.services.mozilla.com/D278132

Comment 10

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: The declarativeNetRequest extension API fails to initialize after a browser restart, when two or more extensions use this API. The exact conditions are described in https://bugzilla.mozilla.org/show_bug.cgi?id=2006233#c3
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Not needed, but provided in the bug report.
• Risk associated with taking this patch: low
• Explanation of risk level: Fix in DNR implementation covered by automated tests.
• String changes made/needed: None
• Is Android affected?: yes

Comment 11

[Rob Wu [:robwu]]

Attachment: Bug 2006233 - Accept partially initialized DNR store on browser startup

Original Revision: https://phabricator.services.mozilla.com/D278132

Comment 12

[Phabricator Automation]

firefox-esr115 Uplift Approval Request

• User impact if declined: The declarativeNetRequest extension API fails to initialize after a browser restart, when two or more extensions use this API. The exact conditions are described in https://bugzilla.mozilla.org/show_bug.cgi?id=2006233#c3
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Not needed, but provided in the bug report.
• Risk associated with taking this patch: low
• Explanation of risk level: Fix in DNR implementation covered by automated tests.
• String changes made/needed: None
• Is Android affected?: yes

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/a431cde535ab