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

Don't create Tab objects for initial tabs until config is loaded. #1056

Merged

Conversation

sammacbeth
Copy link
Collaborator

Reviewer:

Description:

Fixes an issue where we incorrectly showed the message "temporarily disabled Privacy Protection" for all tabs which are open on extension start.

Steps to test this PR:

Automated tests:

  • Unit tests
  • Integration tests
Reviewer Checklist:
  • Ensure the PR solves the problem
  • Review every line of code
  • Ensure the PR does no harm by testing the changes thoroughly
  • Get help if you're uncomfortable with any of the above!
  • Determine if there are any quick wins that improve the implementation
PR Author Checklist:
  • Get advice or leverage existing code
  • Agree on technical approach with reviewer (if the changes are nuanced)
  • Ensure that there is a testing strategy (and documented non-automated tests)
  • Ensure there is a documented monitoring strategy (if necessary)
  • Consider systems implications

@kdzwinel
Copy link
Member

Thanks for jumping on it Sam! Two things:

  1. Can you explain why moving this code fixes the message?
  2. I can see that the message is gone and I can see that stuff gets blocked on restored tabs, but popup doesn't seem to update with blocked content:
  • load interia.pl
  • don't approve the consent popup
  • reload the extension
  • approve the consent popup

what happens is that a bunch of trackers try to load and are blocked, but popup is showing 0
blocked

I think we can merge the message thing first and deal with that later if it will turn out to be a non-obvious to fix.

There is also one nit: extension icon doesn't update it shows Dax for me until I reload / navigate away. This might be related to 2, but might be separate. This is least important.

@sammacbeth
Copy link
Collaborator Author

Thanks for the review Konrad.

1/ When we create a new Site object we get the enabled features for it based on the config:

export function getEnabledFeatures (url) {
if (!tdsStorage.config.features) return
const enabledFeatures = []
for (const feature in tdsStorage.config.features) {
if (isFeatureEnabled(feature) && brokenListIndex(url, tdsStorage.config.features[feature].exceptions || []) === -1) {
enabledFeatures.push(feature)
}
}
return enabledFeatures
}

This requires that we've already loaded the tds config, otherwise the feature list is always empty. If the list is empty then we consider the site 'broken' in the UI and show the warning:

this.isBroken = this.tab.site.isBroken || !this.tab.site.enabledFeatures.includes('contentBlocking')

This change moves the tab initialization code after we've done await tdsStorage.getLists() which will load the config, so we are guaranteed to have features loaded when we build the enabled feature list.

2/ This looks like another bug with tabs created on browser start. I just pushed another commit to fix that.

Usually, a Tab object is created from a main_frame request. However, when we create Tabs on startup, we don't have that, so use a chrome.tab object instead. In our blocking code we were only adding blocking info when the tab has statusCode = 200 though, which would never be the case for Tabs created from a chrome.tab object (because that object does not include a status code), so we would never add tracker info for tabs created in this way.

@sammacbeth sammacbeth force-pushed the sammacbeth/fix-temp-unprotected branch from 803b93b to 092d38f Compare February 21, 2022 10:20
Copy link
Member

@kdzwinel kdzwinel left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much Sam!

@@ -163,10 +163,10 @@ function handleRequest (requestData) {
// just default to true.
const sameDomain = isSameDomainRequest(thisTab, requestData)

// only count trackers on pages with 200 response. Trackers on these sites are still
// Trackers on these sites are still
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we had this filtering, but as far as I can see this decision is >4y old, so we may never know.

@kdzwinel kdzwinel merged commit 8790d51 into duckduckgo:develop Feb 21, 2022
@sammacbeth sammacbeth deleted the sammacbeth/fix-temp-unprotected branch February 21, 2022 14:20
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