Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide way of handling unsupported DNR rules #449

Open
oliverdunk opened this issue Sep 13, 2023 · 8 comments
Open

Provide way of handling unsupported DNR rules #449

oliverdunk opened this issue Sep 13, 2023 · 8 comments
Labels
proposal Proposal for a change or new feature supportive: chrome Supportive from Chrome supportive: firefox Supportive from Firefox supportive: safari Supportive from Safari topic: dnr Related to declarativeNetRequest

Comments

@oliverdunk
Copy link
Member

In our TPAC meeting on Tuesday (#448), it was brought up that there is currently no easy way to support new types of DNR rules while being compatible with browsers that do not recognise them. This is primarily an issue for static rulesets.

It was mentioned that Chrome has a hard-error when any DNR rule in a ruleset fails to load, and Firefox has a soft failure where the rules are ignored. The behaviour in Safari needs to be determined.

There were suggestions to add API methods like getFailedRules() or isRuleSupported().

@oliverdunk oliverdunk added supportive: chrome Supportive from Chrome proposal Proposal for a change or new feature topic: dnr Related to declarativeNetRequest labels Sep 13, 2023
@oliverdunk
Copy link
Member Author

@sammacbeth, feel free to add anything you think I missed.

@Rob--W @xeenon Could you add respective labels here?

@sammacbeth
Copy link

sammacbeth commented Sep 14, 2023

There are two aspects to this:

  1. How agents should handle individual DNR rules they do not support.
  2. How the extension gets feedback about an invalid rule.

This, of course, can differ between static and dynamic/session rules. Here's a basic compat table from what I've been able to test and what I heard from the TPAC discussion:

What is an invalid rule?

  • A rule that the platform does not recognize nor support.
  • A rule/ruleset that cannot be loaded due to constraint failures (rule limit reached, duplicate ruleID)
Chrome Firefox Safari
Invalid rule in static ruleset: duplicate rule ID Extension will not load. Invalid rules are ignored, the rest are loaded. Warning is shown in about:debugging No errors or warnings thrown. Rules are loaded and active.
Invalid rule in static ruleset: unrecognized action type Unrecognized rules are ignored, rest are loaded. Warning shown on the extension errors page. Unrecognized rules are ignored, the rest are loaded. Warning is shown in about:debugging Unrecognized rules are ignored, the rest are loaded. Warning is shown on extension settings page.
Feedback for invalid static rule Modal error when loading extension or warning on the extension errors page Warning in about:debugging Error on extension settings page ('non-fatal issue')
Invalid rule in updateDynamicRules call Exception thrown. None of the rules are registered. Exception thrown. None of the rules are registered. Exception thrown. None of the rules are registered.
Feedback about which dynamic rule is not supported In text of exception. In text of exception. In text of exception.

Screenshots of feedback given for static rule errors:

Browser Error type
Chrome Duplicate rule ID Screenshot 2023-10-05 at 11 31 48
Chrome Unrecognized rule Screenshot 2023-10-05 at 11 31 08
Firefox Duplicate rule ID Screenshot 2023-10-05 at 12 19 38
Firefox Unrecognized rule Screenshot 2023-10-05 at 11 39 23
Safari Unrecognized rule Screenshot 2023-10-05 at 11 48 25

@xeenon xeenon added the supportive: safari Supportive from Safari label Sep 28, 2023
@sammacbeth
Copy link

I've updated my comment above to complete the current state of Chrome, Firefox and Safari w.r.t. handling of invalid static and dynamic rules. The method I used to test these can be seen from the tests I wrote: duckduckgo/mv3-compat-tests@c24af43.

Some conclusions:

  • updateDynamicRules seems to work consistently across platforms - i.e. it throws if there any any issues with the provided rules, and no rules from the batch get added.
  • Safari seems to ignore duplicate rule IDs in static rules.
  • For static rules, Chrome behavior varies depending on the kind of exception. Duplicate rule IDs prevent the extension loading entirely, while unsupported rules are just ignored with a warning.
  • Issues with static rules are reported on extension debugging pages, but are not available programmatically. I've attached screenshots of how these errors are reported on each platform.

@Rob--W Rob--W added the supportive: firefox Supportive from Firefox label Mar 20, 2024
@oliverdunk
Copy link
Member Author

oliverdunk commented Mar 20, 2024

We are supportive of changing the behavior in Chrome to avoid a hard error when loading an extension with duplicate rule IDs. Instead, we will ignore rules with duplicate IDs.

We are also interested in exploring the proposed getFailedRules() or isRuleSupported() APIs longer term.

Chromium bug: https://issues.chromium.org/issues/330579934

@danielhjacobs

This comment was marked as off-topic.

@danielhjacobs

This comment was marked as off-topic.

@danielhjacobs
Copy link
Contributor

@Rob--W
Copy link
Member

Rob--W commented Jun 20, 2024

The responseHeaders condition is a special case. Ordinarily Chrome does already throw for unsupported conditions. I explained what is going on in Chromium plus added a code snippet for feature detection at #638 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Proposal for a change or new feature supportive: chrome Supportive from Chrome supportive: firefox Supportive from Firefox supportive: safari Supportive from Safari topic: dnr Related to declarativeNetRequest
Projects
None yet
Development

No branches or pull requests

5 participants