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

Improve tab data bookkeeping with webNavigation #2454

Merged
merged 2 commits into from
Sep 19, 2019
Merged

Conversation

ghostwords
Copy link
Member

@ghostwords ghostwords commented Sep 4, 2019

Fixes #2423, fixes #2335, fixes #1144.

Use cases:

  • Navigate to a special browser or extension page (New Tab page, Privacy Badger's options, ...) from a website. Before, the popup on the special page would show you tracker findings for the site you were previously on. You should now see the "Nothing to do on this page" popup.

  • Navigate to a website that uses Service Workers (to initialize the SW). Navigate somewhere else within the same tab. Navigate back to the website with Service Workers. (For example: slate.comexample.comslate.com.) Before, the popup would show you all resources for the SW site as if they loaded on the site you went to previously. This means all first-party resources would show up as third-party if your previous site's domain is different from the SW site's domain.

  • Navigate to a SW site to get first party cookies set. Then navigate to the same SW site while on a different site. Privacy Badger attributes tracking by the SW site domain to the previous site you were on. This is due to separate, not-yet-corrected tab bookkeeping (tabOrigins) in heuristicblocking.js: the tabOrigins entry for the tab never gets updated for active SW sites.

  • Set a SW site domain to be blocked. Initialize the SW. Visit the SW site from a different site. The initial request for the site's Service Worker comes in before the webNavigation.onCommitted() callback gets to update tabData, and so the SW gets blocked and the user sees some version of the "you are offline" message on the site.

    Update: Listening to webNavigation.onBeforeNavigate() events instead of onCommitted() fixes this case as onBeforeNavigate callbacks seem to come before the initial Service Worker.

Does not do anything about #1997.

@ghostwords ghostwords marked this pull request as ready for review September 10, 2019 22:11
@ablanathtanalba
Copy link
Contributor

This rules. I just manual tested on current master to verify each error type, then again on this branch to verify that they behave as we want -- all LGTM! Great work 🎉

Going to take a closer look now at the code to see if there's any room for polishing.

Copy link
Contributor

@ablanathtanalba ablanathtanalba left a comment

Choose a reason for hiding this comment

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

Gave these lines (much appreciated that there are few lines of code with significant impact) another fresh look and I don't see anything that requires improvement. LGTM 👍

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