Adjust scriptlet injection timing #3162
Conversation
|
I'm not related to this repo in any way, but it might be better to have a descriptive commit message + PR description |
thank you, I wrote it. |
There was a problem hiding this comment.
Thanks a lot for your contribution @DrRoot-github. I left two comments for simplification. Overall it looks great! This has been a long standing issue.
remusao
changed the title
|
Thanks a lot @DrRoot-github, this change will be released shortly. |
| return ipcRenderer.sendSync('get-cosmetic-filters-first', window.location.href); | ||
| } | ||
| function getCosmeticsFiltersUpdate(data: Omit<IBackgroundCallback, 'lifecycle'>) { | ||
| setImmediate(() => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
oh shit I really don't know that. I'll fix it as soon as possible:sob:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
#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.
There was a problem hiding this comment.
@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)
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Found, it is this one: packages/adblocker/assets/ublock-origin/filters.txt:
Release Notes
Resolves #2757
Changed the timing of scriptlet injection to before the DOMContentLoaded event fires. Also changed the injection method from using Electron.WebContents.executeJavaScript (which is effectively executed when the tab finishes loading) to dynamically adding a script element, similar to how μBlock Origin does it.