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.

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

[WIP] Using contentScripts API for conditional content script injection #1876

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

alexristich
Copy link
Contributor

See #1865.

@ghostwords
Copy link
Member

ghostwords commented Mar 21, 2018

To summarize our chat, let's fix the fallback injection (fallback for Chrome/pre-59 Firefox/any other browser that doesn't have browser.contentScripts) to happen once per frame only. This probably entails calling insertContentScripts in response to main_frame and sub_frame type requests only, and providing frame ID (allFrames should be false) as well as tab ID to chrome.tabs.executeScript.

It would be nice to understand why the localStorage tracking detection tests are failing in pre-59 Firefoxes. Actual problem or just tests that need updating? Might be related to #1522.

@ghostwords
Copy link
Member

Do we need to switch to using chrome.tabs.executeScript as fallback? Can we keep existing manifest-defined injection and just expand the check for whether the top-level document is whitelisted to also check whether this is a browser that uses browser.contentScripts or not? The benefit will be to minimize code (and behavior, see above comment regarding failing tests) changes. The downside is a bit more overhead since we'll be injecting scripts twice in browser.contentScripts browsers. Seems worth it for purposes of this PR.

@alexristich
Copy link
Contributor Author

Hmm, with your proposed approach we would have to maintain separate scripts, as there's no benefit in using browser.contentScripts in the case where would still need to check synchronously whether the domain is whitelisted. By maintaining two versions of the content scripts, we could inject ones with browser.contentScripts that don't rely on message passing to confirm whether they should do anything.

Is this what you mean? Would this be preferable?

@ghostwords
Copy link
Member

It was an idea meant to reduce the amount of stuff that needs to change. I didn't consider that the scripts need to work differently though: messaging to get go-ahead with manifest injection, no messaging with browser.contentScripts injection.

@alexristich
Copy link
Contributor Author

Made some changes to pass the frame_id to the tabs.executeScript function call. Upon making this change, the background page was showing a bunch of errors similar to this:

Unchecked runtime.lastError while running tabs.executeScript: 
Cannot access contents of url "https://pubads.g.doubleclick.net/gampad/ads?sz=850x250&gdfp_req=1&ad_rule=0&iu=/4061/com.ythome&impl=ifr&scp=ssl%3D1%26dc_yt%3D1%26kga%3D-1%26kgg%3D-1%26klg%3Den%26kmyd%3Dvideo-masthead%26tfcd%3D1%26ytdevice%3D1%26ytexp%3D9422596,23729478,9471239,23705778,23730995,9488572,9488991,23732788%26kbsg%3DHPCA180329&d_imp=1&ytdevice=1&pucrd=CgoIARAAGAI4AXABeAI&correlator=9130296973774806". 
Extension manifest must request permission to access this host.

I'm not yet sure why these are occurring, as we appear to have the required permissions with http://*/* and https://*/*. For now, I added a noop callback function, borrowing from @gorhill's approach: https://github.com/gorhill/uBlock/blob/master/platform/chromium/vapi-background.js#L631. Still, the noop error handler doesn't feel like an appropriate way to solve the problem.

In looking further at this, it is a significant change and I'm not sure we're benefitting much (if at all) from using browser.contentScripts given the complexity and slower injection of content scripts in other browser versions using tabs.executeScript.

I think if we are to proceed with this, maintaining two version of the scripts as discussed above is the most sensible approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants