-
Notifications
You must be signed in to change notification settings - Fork 142
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.
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Philipp Claßen <[email protected]>
Builds for commit 05fd0a5: |
Builds for commit 05fd0a5: |
Builds for commit ded4f50: |
ded4f50
to
d589cb3
Compare
Builds for commit d589cb3: |
Builds for commit 4ad49a1: |
Builds for commit 4ad49a1: |
@@ -196,6 +241,21 @@ async function injectScriptlets(scripts, tabId, frameId) { | |||
script.remove(); | |||
} | |||
|
|||
if (__PLATFORM__ === 'firefox') { | |||
if (scripts.length === 0) { | |||
contentScripts.unregister(hostname); |
There was a problem hiding this comment.
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.
Builds for commit b63b226: |
Tested version: #2074 (comment) Checked on:
Websites:
LGTM |
browser.contentScripts
on Firefox
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:
allFrames
safe? for example, can scriptlets from different hostnames be injected due toallFrames
?webRequest.onResponseStarted
?TODO: