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.

Sign up for GitHub

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

Jump to bottom

fix(adblocker): inject scriplets with browser.contentScripts on Firefox #2074

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix(adblocker): inject scriplets with browser.contentScripts on Firefox #2074

wants to merge 6 commits into from

Conversation

chrmod
Copy link
Member

@chrmod chrmod commented Nov 19, 2024 •
edited
Loading

WARNING: This has to be tested very carefully.

On Firefox, scriptlet injection run into timing issues which makes it impossible to inject certain scriptlets before page scripts.

Examples we try to fix are:

The only way to ensure the scriptlet is always executed before the page script is to use browser.contentScripts API which available on Firefox only.

Open questions:

  • is injecting to allFrames safe? for example, can scriptlets from different hostnames be injected due to allFrames?
  • do we need more triggering methods like webRequest.onResponseStarted?
  • when content scripts should be cleaned?
  • do we handle redirects correctly? for example redirect from http://example.com to https://www.example.com ?
  • can we speed up by not matching cosmetic filters for scriptlet injection once content scripts are registered?

TODO:

  • support selective blocking
  • support pause

@chrmod chrmod marked this pull request as draft November 19, 2024 13:10
smalluban
smalluban requested changes Nov 19, 2024
View reviewed changes
src/background/adblocker.js Outdated Show resolved Hide resolved
src/background/adblocker.js Outdated Show resolved Hide resolved
src/background/adblocker.js Outdated Show resolved Hide resolved
src/background/adblocker.js Outdated Show resolved Hide resolved
src/background/adblocker.js Outdated Show resolved Hide resolved
src/background/adblocker.js Show resolved Hide resolved
@chrmod chrmod added Ad-Blocking Issues releated to Ad Blocking package CI: create extension packages labels Nov 19, 2024
@github-actions GitHub Actions
Copy link

Builds for commit 05fd0a5:

@github-actions GitHub Actions
Copy link

Builds for commit 05fd0a5:

@github-actions GitHub Actions
Copy link

Builds for commit ded4f50:

@chrmod chrmod force-pushed the content-script-injection-poc branch from ded4f50 to d589cb3 Compare November 19, 2024 14:52
@github-actions GitHub Actions
Copy link

Builds for commit d589cb3:

@github-actions GitHub Actions
Copy link

Builds for commit 4ad49a1:

@github-actions GitHub Actions
Copy link

Builds for commit 4ad49a1:

philipp-classen
philipp-classen reviewed Nov 19, 2024
View reviewed changes
src/background/adblocker.js
@@ -196,6 +241,21 @@ async function injectScriptlets(scripts, tabId, frameId) {
script.remove();
}

if (__PLATFORM__ === 'firefox') {
if (scripts.length === 0) {
contentScripts.unregister(hostname);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this part of the code is necessary or how it should be best handled.
When the engine updates, it should automatically clear the content scripts (contentScripts.unregisterAll). There might be potential for race conditions, but in principle, it should eventually take care of unregistering.

Is the code here intended especially for pause (which won't trigger an engine reload)? If so, would be good to leave a comment explaining this.

@github-actions GitHub Actions
Copy link

Builds for commit b63b226:

@GRadziejewski
Copy link
Contributor

GRadziejewski commented Nov 23, 2024 •
edited
Loading

Tested version: #2074 (comment)

Checked on:

Windows 11, Firefox 132.0.2, GBE 10.4.15 and 10.4.10
Windows 10, Firefox 132.0.2, GBE 10.4.15 and 10.4.10

Websites:

https://example.com/ - OK
https://stre4mplay.one/ - OK
https://www.officedepot.com/ - OK
https://picrew.me/ja/ - OK
https://www.foxbusiness.com/ - OK
https://edition.cnn.com/ - OK
https://www.br.de/index.html - OK
https://indeed.com/ - OK
https://pasteboard.co/ - OK
https://los40.com/ - OK
https://www.telegraph.co.uk/ - OK
https://www.verizon.com/ - On 10.4.10 version it's not working, on 10.4.15 it's working!
https://humanbenchmark.com/ - OK
https://www.politico.com/ - OK
https://kusonime.com/ - OK
https://modapk.link/ - OK
https://xxxymovies.com/ - OK
https://www.jetpunk.com/ - OK
https://imgur.com/ - OK
https://www.hentai-party.net/ - OK
https://ccurl.net/ - OK
https://mysexgames.com/ - OK
https://www.ancient-origins.net/ - OK
https://hideandseek.world/ - OK
https://pewgame.com/ - OK
https://haonguyen.top/ - OK
https://crazyblog.in/ - OK
https://cutearn.net/ - OK
rshrt.com - > https://reshort.com/ - OK
https://gplinks.org/ - OK
https://easysky.in/ - OK
https://veganab.co/ - OK
https://www.gyanitheme.com/ - OK
https://fitdynamos.com/ - OK
https://postazap.com/ - OK
https://blog24.me/ - OK
https://atglinks.com/ - OK
https://streamelements.com/ - OK
https://www.facebook.com - OK
https://infinityscans.xyz/ - OK
https://www.youtube.com/ - OK

LGTM

@smalluban smalluban changed the title POC: on Firefox inject scriplets with browser.contentScripts fix(adblocker): inject scriplets with browser.contentScripts on Firefox Nov 28, 2024
@chrmod chrmod force-pushed the content-script-injection-poc branch from b63b226 to 0eafaf4 Compare December 11, 2024 17:11
@chrmod chrmod force-pushed the content-script-injection-poc branch from 0eafaf4 to ac86ec3 Compare December 11, 2024 17:33
@github-actions GitHub Actions
Copy link

Builds for commit ac86ec3:

@github-actions GitHub Actions
Copy link

Builds for commit a3ec1b4:

@github-actions GitHub Actions
Copy link

Builds for commit bcdaf85:

smalluban
smalluban reviewed Dec 12, 2024
View reviewed changes
src/background/adblocker.js Outdated
@@ -224,6 +296,9 @@ function injectStyles(styles, tabId, frameId) {
}

async function injectCosmetics(details, config) {
const isBootstrap = config.bootstrap;
const scriptletsOnly = Boolean(config.scriptletsOnly);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are in control over the config, and undefined is just fine, you don't need to convert it to Boolean.

smalluban
smalluban requested changes Dec 12, 2024
View reviewed changes
src/background/adblocker.js Outdated
@@ -237,6 +312,8 @@ async function injectCosmetics(details, config) {
const domain = parsed.domain || '';
const hostname = parsed.hostname || '';

if (scriptletsOnly && contentScripts.isRegistered(hostname)) return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This scripletsOnly is not what the name suggests - it is a flag for EXPERIMENTAL_SCRIPTLETS_INJECTION - you can't use scripletsOnly in Chrome if you would wish for example.

There is an unrelated bug in Firefox requiring the separation of injecting styles and scriptlets (styles must be injected not in onCommited, as then we have permission for an issue in iframes). So as this function becomes harder to reason about, this is a moment to do so...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not quite right, this block is just an optimization that prevent a matching if content scripts are already registered. scriptletsOnly main role is to prevent style injection a bit below.

But yes, the condition should be improved

src/background/adblocker.js Outdated
@@ -153,6 +158,12 @@ export const setup = asyncSetup('adblocker', [
OptionsObserver.addListener(
'experimentalFilters',
async (value, lastValue) => {
if (__PLATFORM__ === 'firefox') {
EXPERIMENTAL_SCRIPTLETS_INJECTION = value;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create another observer to separate logic. You are using the same option, but for another purpose.

@chrmod chrmod requested a review from smalluban December 12, 2024 16:15
@chrmod chrmod marked this pull request as ready for review December 12, 2024 16:15
@github-actions GitHub Actions
Copy link

Builds for commit b0198ba:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipp-classen philipp-classen philipp-classen left review comments

@smalluban smalluban Awaiting requested review from smalluban

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Ad-Blocking Issues releated to Ad Blocking package CI: create extension packages
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chrmod @GRadziejewski @philipp-classen @smalluban