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

Adjust scriptlet injection timing #3162

Merged
merged 2 commits into from
Apr 2, 2023
Merged

Adjust scriptlet injection timing #3162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4be6ac6
adjust injection timing
DrRoot-github Mar 30, 2023
2f432f9
:shirt: fix error handling style
DrRoot-github Mar 31, 2023
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
31 changes: 25 additions & 6 deletions packages/adblocker-electron-preload/preload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,13 @@ import { ipcRenderer } from 'electron';

import { DOMMonitor, IBackgroundCallback, IMessageFromBackground } from '@cliqz/adblocker-content';

function getCosmeticsFilters(data: IBackgroundCallback) {
setTimeout(() => {
function getCosmeticsFiltersFirst(): string[] | null {
return ipcRenderer.sendSync('get-cosmetic-filters-first', window.location.href);
}
function getCosmeticsFiltersUpdate(data: Omit<IBackgroundCallback, 'lifecycle'>) {
setImmediate(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is no longer present in modern browsers, and as such breaks the latest release on my system.
https://developer.mozilla.org/en-US/docs/Web/API/Window/setImmediate#browser_compatibility
@remusao

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh shit I really don't know that. I'll fix it as soon as possible:sob:

Copy link
Contributor

Choose a reason for hiding this comment

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

If I replace it with setTimeout I no longer get errors, but I still see ads on youtube, more than before... So maybe that's not the correct replacement. I had to inject the blocker in a custom way though, so I'll look into that first and get back when I know more.

Copy link

Choose a reason for hiding this comment

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

If I replace it with setTimeout I no longer get errors, but I still see ads on youtube, more than before

Weird... Isn't the difference between setImmediate and setTimeout just about 4ms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a thought, but do we still need this setImmediate?
We separated the method called in the first call, and since this method is called at DOMContentLoaded, I feel like the processing won't change whether we execute it immediately or insert a wait.

@Jelmerro
By the way, I'm curious about the issue of more ads showing up than before. Could you show me the Console and Network tabs in Devtools if you don't mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#3166
I submitted a PR for the fix. I'll also test it in the Vieb environment, so there might be some parts that still need to be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jelmerro
I briefly looked at the source code, but it seems like the block list is pulled from /app/blocklists/list.json, which contains two files: easylist.txt and easyprivacy.txt. However, the scriptlet to block video ads at the beginning is not included in these two files, so if there are no other lists being read, I don't think it can block the initial video ad. Specifically, the following entries are required:

youtube.com, youtubekids.com, youtube-nocookie.com##+js(json-prune, [].playerResponse.adPlacements [].playerResponse.playerAds playerResponse.adPlacements playerResponse.playerAds adPlacements playerAds)
youtube.com, youtubekids.com, youtube-nocookie.com##+js(set, ytInitialPlayerResponse.adPlacements, undefined)
youtube.com, youtubekids.com, youtube-nocookie.com##+js(set, playerResponse.adPlacements, undefined)

Copy link
Contributor

@Jelmerro Jelmerro Apr 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for looking into this, I will see if this makes it work. Do you happen to know which list used by ghostery includes these by default?

Update: these lines do make it work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/ghostery/adblocker/blob/master/packages/adblocker/src/fetch.ts
The URLs that are eventually loaded by fromPrebuiltAdsAndTracking and other functions are hard-coded here.

Copy link
Contributor

@Jelmerro Jelmerro Apr 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Found, it is this one: packages/adblocker/assets/ublock-origin/filters.txt:
image

ipcRenderer.send('get-cosmetic-filters', window.location.href, data);
}, 1);
});
}

if (window === window.top && window.location.href.startsWith('devtools://') === false) {
Expand Down Expand Up @@ -47,7 +50,24 @@ if (window === window.top && window.location.href.startsWith('devtools://') ===
},
);

getCosmeticsFilters({ lifecycle: 'start', ids: [], classes: [], hrefs: [] });
const scripts = getCosmeticsFiltersFirst();
if (scripts) {
const elems: HTMLScriptElement[] = [];
try {
scripts.forEach((script) => {
const e = document.createElement('script');
e.appendChild(document.createTextNode(script));
(document.head || document.documentElement || document).appendChild(e);
elems.push(e);
});
} catch {
// continue regardless of error
}
elems.forEach((removeIt) => {
removeIt.remove();
removeIt.textContent = '';
});
}

// On DOMContentLoaded, start monitoring the DOM. This means that we will
// first check which ids and classes exist in the DOM as a one-off operation;
Expand All @@ -59,9 +79,8 @@ if (window === window.top && window.location.href.startsWith('devtools://') ===
() => {
DOM_MONITOR = new DOMMonitor((update) => {
if (update.type === 'features') {
getCosmeticsFilters({
getCosmeticsFiltersUpdate({
...update,
lifecycle: 'dom-update',
});
}
});
Expand Down
72 changes: 49 additions & 23 deletions packages/adblocker-electron/adblocker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,14 @@ export class BlockingContext {
callback: (a: Electron.CallbackResponse) => void,
) => void;

private readonly onGetCosmeticFilters: (
private readonly onGetCosmeticFiltersUpdated: (
event: Electron.IpcMainEvent,
url: string,
msg: IBackgroundCallback,
) => void;

private readonly onGetCosmeticFiltersFirst: (event: Electron.IpcMainEvent, url: string) => void;

private readonly onHeadersReceived: (
details: Electron.OnHeadersReceivedListenerDetails,
callback: (a: Electron.HeadersReceivedResponse) => void,
Expand All @@ -75,15 +77,19 @@ export class BlockingContext {
private readonly blocker: ElectronBlocker,
) {
this.onBeforeRequest = (details, callback) => blocker.onBeforeRequest(details, callback);
this.onGetCosmeticFilters = (event, url, msg) => blocker.onGetCosmeticFilters(event, url, msg);

this.onGetCosmeticFiltersFirst = (event, url) => blocker.onGetCosmeticFiltersFirst(event, url);
this.onGetCosmeticFiltersUpdated = (event, url, msg) =>
blocker.onGetCosmeticFiltersUpdated(event, url, msg);
this.onHeadersReceived = (details, callback) => blocker.onHeadersReceived(details, callback);
this.onIsMutationObserverEnabled = (event) => blocker.onIsMutationObserverEnabled(event);
}

public enable(): void {
if (this.blocker.config.loadCosmeticFilters === true) {
this.session.setPreloads(this.session.getPreloads().concat([PRELOAD_PATH]));
ipcMain.on('get-cosmetic-filters', this.onGetCosmeticFilters);
ipcMain.on('get-cosmetic-filters-first', this.onGetCosmeticFiltersFirst);
ipcMain.on('get-cosmetic-filters', this.onGetCosmeticFiltersUpdated);
ipcMain.on('is-mutation-observer-enabled', this.onIsMutationObserverEnabled);
}

Expand All @@ -109,7 +115,7 @@ export class BlockingContext {

if (this.blocker.config.loadCosmeticFilters === true) {
this.session.setPreloads(this.session.getPreloads().filter((p) => p !== PRELOAD_PATH));
ipcMain.removeListener('get-cosmetic-filters', this.onGetCosmeticFilters);
ipcMain.removeListener('get-cosmetic-filters', this.onGetCosmeticFiltersUpdated);
}
}
}
Expand Down Expand Up @@ -161,7 +167,43 @@ export class ElectronBlocker extends FiltersEngine {
event.returnValue = this.config.enableMutationObserver;
};

public onGetCosmeticFilters = (
public onGetCosmeticFiltersFirst = (event: Electron.IpcMainEvent, url: string) => {
// Extract hostname from sender's URL
const parsed = parse(url);
const hostname = parsed.hostname || '';
const domain = parsed.domain || '';

const { active, styles, scripts, extended } = this.getCosmeticsFilters({
domain,
hostname,
url,

// This needs to be done only once per frame
getBaseRules: true,
getInjectionRules: true,
getExtendedRules: true,
getRulesFromHostname: true,
});

if (active === false) {
event.returnValue = null;
return;
}

// Inject custom stylesheets
this.injectStyles(event.sender, styles);

event.sender.send('get-cosmetic-filters-response', {
active,
extended,
styles: '',
} as IMessageFromBackground);

// to execute Inject scripts synchronously, simply return scripts to renderer.
event.returnValue = scripts;
};

public onGetCosmeticFiltersUpdated = (
event: Electron.IpcMainEvent,
url: string,
msg: IBackgroundCallback,
Expand All @@ -171,7 +213,7 @@ export class ElectronBlocker extends FiltersEngine {
const hostname = parsed.hostname || '';
const domain = parsed.domain || '';

const { active, styles, scripts, extended } = this.getCosmeticsFilters({
const { active, styles, extended } = this.getCosmeticsFilters({
domain,
hostname,
url,
Expand All @@ -180,14 +222,8 @@ export class ElectronBlocker extends FiltersEngine {
hrefs: msg.hrefs,
ids: msg.ids,

// This needs to be done only once per frame
getBaseRules: msg.lifecycle === 'start',
getInjectionRules: msg.lifecycle === 'start',
getExtendedRules: msg.lifecycle === 'start',
getRulesFromHostname: msg.lifecycle === 'start',

// This will be done every time we get information about DOM mutation
getRulesFromDOM: msg.lifecycle === 'dom-update',
getRulesFromDOM: true,
});

if (active === false) {
Expand All @@ -197,16 +233,10 @@ export class ElectronBlocker extends FiltersEngine {
// Inject custom stylesheets
this.injectStyles(event.sender, styles);

// Inject scriptlets
for (const script of scripts) {
this.injectScripts(event.sender, script);
}

// Inject scripts from content script
event.sender.send('get-cosmetic-filters-response', {
active,
extended,
scripts: [],
styles: '',
} as IMessageFromBackground);
};
Expand Down Expand Up @@ -267,10 +297,6 @@ export class ElectronBlocker extends FiltersEngine {
}
};

private injectScripts(sender: Electron.WebContents, script: string): void {
sender.executeJavaScript(script);
}

private injectStyles(sender: Electron.WebContents, styles: string): void {
if (styles.length > 0) {
sender.insertCSS(styles, {
Expand Down