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

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

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ac86ec3
POC: on Firefox inject scriplets with browser.contentScripts
chrmod Nov 19, 2024
a3ec1b4
fix injection
chrmod Dec 11, 2024
bcdaf85
Enable with experimental filters
chrmod Dec 11, 2024
9ad938d
Addressing code review
chrmod Dec 12, 2024
9e251aa
Support pausing
chrmod Dec 12, 2024
b0198ba
Cleanup
chrmod Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 95 additions & 6 deletions src/background/adblocker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ export async function reloadMainEngine() {
await engines.create(engines.MAIN_ENGINE);
console.info('[adblocker] Main engine reloaded with no filters');
}
if (__PLATFORM__ === 'firefox') {
contentScripts.unregisterAll();
}
}

let updating = false;
Expand Down Expand Up @@ -123,6 +126,8 @@ async function updateEngines() {
}
}

let EXPERIMENTAL_SCRIPTLETS_INJECTION = false;

const HOUR_IN_MS = 60 * 60 * 1000;
export const setup = asyncSetup('adblocker', [
OptionsObserver.addListener(
Expand Down Expand Up @@ -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.

if (!value) {
contentScripts.unregisterAll();
}
}
engines.setEnv('env_experimental', value);

// Experimental filters changed to enabled
Expand All @@ -163,7 +174,51 @@ export const setup = asyncSetup('adblocker', [
),
]);

function injectScriptlets(filters, tabId, frameId) {
const contentScripts = (() => {
smalluban marked this conversation as resolved.
Show resolved Hide resolved
const map = new Map();
return {
async register(hostname, code) {
this.unregister(hostname);
try {
const contentScript = await browser.contentScripts.register({
js: [
{
code,
},
],
allFrames: true,
matches: [`https://*.${hostname}/*`, `http://*.${hostname}/*`],
matchAboutBlank: true,
matchOriginAsFallback: true,
runAt: 'document_start',
world: 'MAIN',
});
map.set(hostname, contentScript);
} catch (e) {
console.warn(e);
this.unregister(hostname);
}
},
isRegistered(hostname) {
return map.has(hostname);
},
unregister(hostname) {
const contentScript = map.get(hostname);
if (contentScript) {
contentScript.unregister();
map.delete(hostname);
}
},
unregisterAll() {
for (const hostname of map.keys()) {
this.unregister(hostname);
}
},
};
})();

function injectScriptlets(filters, tabId, frameId, hostname) {
let contentScript = '';
for (const filter of filters) {
const parsed = filter.parseScript();

Expand All @@ -177,12 +232,19 @@ function injectScriptlets(filters, tabId, frameId) {

const scriptletName = `${parsed.name}${parsed.name.endsWith('.js') ? '' : '.js'}`;
const scriptlet = scriptlets[scriptletName];
const func = scriptlet.func;
const args = parsed.args.map((arg) => decodeURIComponent(arg));

if (!scriptlet) {
console.warn('[adblocker] unknown scriptlet with name:', scriptletName);
continue;
}

if (__PLATFORM__ === 'firefox' && EXPERIMENTAL_SCRIPTLETS_INJECTION) {
contentScript += `(${func.toString()})(...${JSON.stringify(args)});\n`;
continue;
}

chrome.scripting.executeScript(
{
injectImmediately: true,
Expand All @@ -193,8 +255,8 @@ function injectScriptlets(filters, tabId, frameId) {
tabId,
frameIds: [frameId],
},
func: scriptlet.func,
args: parsed.args.map((arg) => decodeURIComponent(arg)),
func,
args,
},
() => {
if (chrome.runtime.lastError) {
Expand All @@ -203,6 +265,16 @@ function injectScriptlets(filters, tabId, frameId) {
},
);
}

if (__PLATFORM__ === 'firefox' && EXPERIMENTAL_SCRIPTLETS_INJECTION) {
if (filters.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.

} else if (!contentScripts.isRegistered(hostname)) {
contentScripts.register(hostname, contentScript);
} else {
// do nothing if already registered
}
}
}

function injectStyles(styles, tabId, frameId) {
Expand All @@ -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.


try {
setup.pending && (await setup.pending);
} catch (e) {
Expand All @@ -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


if (isPaused(options, hostname)) return;

const tabHostname = tabStats.get(tabId)?.hostname;
Expand All @@ -245,7 +322,6 @@ async function injectCosmetics(details, config) {
}

const engine = engines.get(engines.MAIN_ENGINE);
const isBootstrap = config.bootstrap;

{
const { matches } = engine.matchCosmeticFilters({
Expand Down Expand Up @@ -282,8 +358,12 @@ async function injectCosmetics(details, config) {
}
}

if (isBootstrap && scriptFilters.length > 0) {
injectScriptlets(scriptFilters, tabId, frameId);
if (isBootstrap) {
injectScriptlets(scriptFilters, tabId, frameId, hostname);
}

if (scriptletsOnly) {
return;
}

const { styles } = engine.injectCosmeticFilters(styleFilters, {
Expand Down Expand Up @@ -377,6 +457,15 @@ function isTrusted(request, type) {
}

if (__PLATFORM__ === 'firefox') {
chrome.webNavigation.onBeforeNavigate.addListener(
(details) => {
if (EXPERIMENTAL_SCRIPTLETS_INJECTION) {
injectCosmetics(details, { bootstrap: true, scriptletsOnly: true });
}
},
{ url: [{ urlPrefix: 'http://' }, { urlPrefix: 'https://' }] },
);

function isExtensionRequest(details) {
return (
(details.tabId === -1 && details.url.startsWith('moz-extension://')) ||
Expand Down
Loading