Open oliverdunk opened 9 months ago
@sammacbeth, feel free to add anything you think I missed.
@Rob--W @xeenon Could you add respective labels here?
There are two aspects to this:
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?
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 | |
Chrome | Unrecognized rule | |
Firefox | Duplicate rule ID | |
Firefox | Unrecognized rule | |
Safari | Unrecognized rule |
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: https://github.com/duckduckgo/mv3-compat-tests/commit/c24af43690a9c7ead3adb8c173d76038cdd0ca86.
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.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
From my testing, in Chrome, invoking declarativeNetRequest.updateDynamicRules
with an unsupported condition in one of the rules causes that single condition to be silently ignored while the rest of the conditions are applied to the rule and the rule is registered. Is that desired behavior?
If it is desired behavior, would the proposed APIs be able to check if a given rule's RuleConditions are all supported, or would these count as supported rules that are not failing (not returned by getFailedRules()
and supported according to isRuleSupported()
)?
I can confirm, in Firefox, this results in an error:
Error: Type error for parameter options (Error processing addRules.0.condition: Unexpected property "responseHeaders") for declarativeNetRequest.updateDynamicRules.
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 https://github.com/w3c/webextensions/issues/638#issuecomment-2181124486
In our TPAC meeting on Tuesday (https://github.com/w3c/webextensions/pull/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()
orisRuleSupported()
.