1921353 - declarativeNetRequest API used by extension triggers a bug on next launch of browser

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[onkia.tova77]

Attachment: sample_extension.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:131.0) Gecko/20100101 Firefox/131.0

Steps to reproduce:

1. I am developing manifest v3 browser extension, that utilizes declarativeNetRequest API for dynamic redirection from some domains (in this specific case: adult websites) to static html pages embedded by extension.
2. On installation of extension everything works fine. Extension works as expected: no errors in the console and redirects from blocked "pornhub.com" to static html are happening.

Actual results:

However, if you turn off and then turn on the browser (equals any restart) -- extension is not working anymore. In console there are errors (attempt to define declarativeNetRequest rules failed with no explanation). Logs tells us:

00:34:57.754 Blocked domains loaded:
Array [ "pornhub.com" ]
background.js:27:13

00:34:57.754 Custom blocked domains loaded:
Array []
background.js:28:13

00:34:57.759
Error resetting rules: Error: An unexpected error occurred background.js:87:17

00:34:57.759
Error updating blocking rules: Error: An unexpected error occurred background.js:69:17

(!) The most ridiculous thing about all of that: if we now (just after getting those errors in console log) go to "Manage extensions" Firefox menu, open the extension settings and tweak "Run in Private Windows" to any opposing (from selected currently) position -- everything starts imminently work as expected and didn't even break after restart. Now extension will work as it should have since the very beginning.

Also Interesting: this is an extremely similar issue, actually, that Brave users discovered (and fixed) not so long ago here: https://github.com/brave/brave-browser/issues/30854

NOTE:

• Reproducible in stable, beta, for developers latest versions of browser.
• NOT reproducible in ESR 128 somewhy (here everything works like charm).

Expected results:

Extension should work (declarativeNetRequest rules should be able to setup) without black magic described above. I also consider current unexpected behavior as a security problem.

I stripped my extension to as little as possible DEMO for you to be able to easily reproduce. Attaching a .zip file with it.

Comment 1

[Rob Wu [:robwu]]

This does not look like a security bug - why was it marked as such? If you'd like to keep it confidential, we can do so without labeling it as a security bug. Do you have any objections to lifting the visibility restrictions of this bug?

However, if you turn off and then turn on the browser (equals any restart) -- extension is not working anymore. In console there are errors (attempt to define declarativeNetRequest rules failed with no explanation). Logs tells us:

"An unexpected error occurred" is a generic redacted error message. Could you open Browser Console and share the output from there?

Note: while we load DNR rules at startup, we do not pause network requests until all rules have loaded and applied (bug 1821676). This should not cause "An unexpected error occurred" errors though.

(!) The most ridiculous thing about all of that: if we now (just after getting those errors in console log) go to "Manage extensions" Firefox menu, open the extension settings and tweak "Run in Private Windows" to any opposing (from selected currently) position -- everything starts imminently work as expected and didn't even break after restart. Now extension will work as it should have since the very beginning.

When you do that, the extension is reloaded, as if the extension is installed. By doing so, any previously stored dynamic rules are deleted, by design.

Also Interesting: this is an extremely similar issue, actually, that Brave users discovered (and fixed) not so long ago here: https://github.com/brave/brave-browser/issues/30854

This is completely unrelated. There was an implementation bug in Brave's fork of Chromium. Side note, Chromium continues to have an implementation flaw around deletion of dynamic DNR rules (https://issues.chromium.org/issues/40825583)..

• Reproducible in stable, beta, for developers latest versions of browser.
• NOT reproducible in ESR 128 somewhy (here everything works like charm).

We haven't made any significant changes to the DNR implementation since 128 that could affect this bug. I'm surprised to see differences between ESR 128 vs others. Did you reproduce the issue with a new profile? If you are able to consistently reproduce the issue, could you use mozregression to identify the cause of the regression?

Comment 2

[Rob Wu [:robwu]]

I can reproduce the issue though, in Firefox 130. I can also reproduce in Firefox 128, and even Firefox 115. To reproduce, the extension has to be installed, so we cannot use about:debugging. We have to pack the extension as a xpi file and install it. This ordinarily requires the add-on to be signed (alternative is to disable signing with xpinstall.signatures.required, but that is only supported on Nightly).

The error message in the Browser Console is this._data.get(...) is undefined, triggered from https://searchfox.org/mozilla-central/rev/9fe73403523732f57cd82d30590ecc272fb0b165/toolkit/components/extensions/ExtensionDNRStore.sys.mjs#1696

We initialize _data for a given extension in #readData, which in turn is called by #getDataPromise. That in its turn is called in four places:

• getDisabledRuleIds (via declarativeNetRequest.getDisabledRuleIds)
• getDynamicRules (via declarativeNetRequest.getDynamicRules)
• getEnabledStaticRulesets (via declarativeNetRequest.updateEnabledStaticRulesets and declarativeNetRequest.getAvailableStaticRuleCount)
• #initExtension, but only if dynamic or static rules were stored by a previous session.

Interestingly, we have an optimization that avoids initialization when there haven't been any stored static or dynamic rules. But that optimization depends on what we store in StartupCache (source, and it looks like we never update the hasDynamicRules nor hasEnabledStaticRules entries after updating the dynamic or enabled/disabled static rules. Because of this, the implementation never loads previously stored DNR rules for regular browser restarts, for extensions that only have dynamic DNR rules (and no static rules).

Comment 3

[Rob Wu [:robwu]]

I confirmed this bug - reporter, please let me know if you have objections to opening this bug. We can redact the initial bug report if you'd like to omit some details.

I have also created a minimal test case to reproduce your issue.

Comment 4

[onkia.tova77]

I don't have objections to make this public. As for the rest -- give me some time and I'll answer everything.

Comment 5

[Rob Wu [:robwu]]

I have already got the other information by reproducing myself. The only open question is why you would not be able to reproduce in ESR 128.

Comment 6

[onkia.tova77]

"An unexpected error occurred" is a generic redacted error message. Could you open Browser Console and share the output from there?
Missing this, since you already did it for me (thanks).
This is completely unrelated. There was an implementation bug in Brave's fork of Chromium. Side note, Chromium continues to have an implementation flaw around deletion of dynamic DNR rules (https://issues.chromium.org/issues/40825583)..
Alright, it was just a guess of similar symptoms.
We haven't made any significant changes to the DNR implementation since 128 that could affect this bug. I'm surprised to see differences between ESR 128 vs others. Did you reproduce the issue with a new profile?
I can confirm that in Firefox 128.2.0esr this bug is not reproducible. Just double checked, within portableapps version and standalone installation from .msi with a clear profile in both cases. Maybe there are some default ESR-related about:config settings that affect reproducibility of this?
If you are able to consistently reproduce the issue, could you use mozregression to identify the cause of the regression?
I will try, but not promising anything cause I am completely unaware of this tool.

Comment 7

[Rob Wu [:robwu]]

(In reply to onkia.tova77 from comment #6)

I can confirm that in Firefox 128.2.0esr this bug is not reproducible. Just double checked, within portableapps version and standalone installation from .msi with a clear profile in both cases. Maybe there are some default ESR-related about:config settings that affect reproducibility of this?

It is possible that your special setup clears the startup cache of your browser. This bug is caused by skipped initialization logic when the cache states that the extension does not have any dynamic nor static rules, and only happens after the first startup (because the cache is only used after the first startup). This cache is cleared whenever an extension is updated, reloaded, re-installed, or the browser version or application location changes.

A work-around for extensions that encounter this problem is to call declarativeNetRequest.getDynamicRules() before updateDynamicRules.

If you are able to consistently reproduce the issue, could you use mozregression to identify the cause of the regression?
I will try, but not promising anything cause I am completely unaware of this tool.

Thanks for offering, but the issue is well understood given my analysis in comment 2. There is no need to run mozregression this time.

Comment 8

[onkia.tova77]

and only happens after the first startup

Just want to clarify, that for me it happens on every startup of browser, not just the first (after installing extension) one. The only thing that fixes it is to manually adjust private windows usage setting to any opposing state.

A work-around for extensions that encounter this problem is to call declarativeNetRequest.getDynamicRules() before updateDynamicRules.

Can't confirm this. Provided in the initial post sample extension already pings getDynamicRules() before updateDynamicRules().

Comment 9

[Rob Wu [:robwu]]

(In reply to onkia.tova77 from comment #8)

and only happens after the first startup

Just want to clarify, that for me it happens on every startup of browser, not just the first (after installing extension) one. The only thing that fixes it is to manually adjust private windows usage setting to any opposing state.

Yes, this is what I meant. Not after the installation, not only after the first startup, but every time after the first startup following install/reinstall/reload.

A work-around for extensions that encounter this problem is to call declarativeNetRequest.getDynamicRules() before updateDynamicRules.

Can't confirm this. Provided in the initial post sample extension already pings getDynamicRules() before updateDynamicRules().

Oops, this is indeed not working because the implementation looks at in-memory data structures only, without forcing initialization from what's on disk. This usually makes sense, except when the earlier initialization does not work as expected, as seen in this bug.

The only work-around is to declare a static ruleset in the manifest file. The referenced ruleset file can be an empty array.

Comment 10

[onkia.tova77]

The only work-around is to declare a static ruleset in the manifest file. The referenced ruleset file can be an empty array.

Nice trick, actually! Thank you!

Comment 11

[Rob Wu [:robwu]]

Attachment: Bug 1921353 - Ensure up-to-date flags in DNR cache

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4a6ecb74ec4
Ensure up-to-date flags in DNR cache r=rpl

Comment 13

[Cristian Tuns]

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

Comment 14

[Rob Wu [:robwu]]

Attachment: Bug 1921353 - Ensure up-to-date flags in DNR cache

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

Comment 15

[Rob Wu [:robwu]]

Attachment: Bug 1921353 - Ensure up-to-date flags in DNR cache

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

Comment 16

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: The declarativeNetRequest extension API does sometimes not work after a browser restart
• 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 bug report.
• Risk associated with taking this patch: Low
• Explanation of risk level: Precise change in extension code only. All relevant cases covered by automated tests.
• String changes made/needed: None
• Is Android affected?: yes

Comment 17

[Phabricator Automation]

esr115 Uplift Approval Request

• User impact if declined: The declarativeNetRequest extension API does sometimes not work after a browser restart
• 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 bug report.
• Risk associated with taking this patch: Low
• Explanation of risk level: Precise change in extension code only. All relevant cases covered by automated tests.
• String changes made/needed: None
• Is Android affected?: yes

Comment 18

[Rob Wu [:robwu]]

Attachment: Bug 1921353 - Ensure up-to-date flags in DNR cache

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

Comment 19

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: The declarativeNetRequest extension API does sometimes not work after a browser restart
• 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 bug report.
• Risk associated with taking this patch: Low
• Explanation of risk level: Precise change in extension code only. All relevant cases covered by automated tests.
• String changes made/needed: None
• Is Android affected?: yes

Comment 20

[Rob Wu [:robwu]]

dev-doc-needed:

Version 133 under the assumption that the beta uplift is approved; otherwise 134.

Add note to https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/declarativeNetRequest/updateDynamicRules

Prior to Firefox 133, dynamic rules were sometimes not applied after a browser restart, and the declarativeNetRequest.updateDynamicRules API would reject with an error (bug 1921353). As a work-around to avoid this bug, specify an enabled static ruleset in declarative_net_request.rule_resources. The ruleset file can be an empty list.

Add note to https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/declarativeNetRequest/updateEnabledRulesets

Prior to Firefox 133, static rulesets would not load after a browser restart when there were no registered static or dynamic rules at install time (bug 1921353). As a work-around to avoid this bug, make sure that declarative_net_request.rule_resources contains at least one enabled ruleset.

Add note to the Firefox for developers 133 release notes that mention the bugs with these two APIs.

Fixed a bug in the declarativeNetRequest API that prevented rule registration after a browser restart (bug 1921353). This affects extensions that rely on declarativeNetRequest.updateDynamicRules or declarativeNetRequest.updateEnabledRulesets.

(linkify the method names and manifest key where applicable)

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/372e3fd9f8f9

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/34ea820483b7

Comment 23

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/59da358232aa

Comment 24

[Donal Meehan [:dmeehan]]

:robwu backed out of esr115 for causing test failures

Test Failures:https://treeherder.mozilla.org/jobs?repo=mozilla-esr115&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=59da358232aa8b00811b56b99f482c19a701fda7&test_paths=toolkit%2Fcomponents%2Fextensions%2Ftest%2Fxpcshell&selectedTaskRun=ZFikqbIHQzKlXZmmtZbOeg.0

Backout: https://hg.mozilla.org/releases/mozilla-esr115/rev/d1848607d6ed5d8b79bdb148592b1274158edb59

Comment 25

[Rob Wu [:robwu]]

The failure happened because the test called a method that was introduced in 128 (bug 1810762). I fixed the test by deleting the lines that relied on that new method. The rest of the unit tests are still largely untouched, and they are sufficient to serve as regression tests.

Comment 26

[Richard Bloor]

Documentation in declarativeNetRequest API bug fix #36668

Comment 27

[Donal Meehan [:dmeehan]]

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

The failure happened because the test called a method that was introduced in 128 (bug 1810762). I fixed the test by deleting the lines that relied on that new method. The rest of the unit tests are still largely untouched, and they are sufficient to serve as regression tests.

Thanks, I did a try to double check
https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=GBJlXPsKRoGAOY-NxD-crg.0&revision=152ea059744fa246aac6ba7dd59633fdf3e7b161

Comment 28

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/6db69f2843f3