Don't store domains on disk that don't track by cowlicks · Pull Request #1462 · EFForg/privacybadger

cowlicks · 2017-06-29T07:31:28Z

With this PR we no longer store "empty" action map objects on disk, meaning the action map has no user action, no dnt, and no tracking (but the action map would have a domain and timestamp).

A lot of our DNT checking depended on these "empty" action map objects. Before, we would use the timestamp to know when to recheck.

Now we keep up to the last 1000 "empty" domains we've seen in memory. We don't recheck a domain if we have it in this data structure.

The first commit 91364da makes it so we don't create an action map object for a domain when get a negative check for DNT

Then e410490 cleans up actionMap objects that become "empty". Like when a domain has some user action, but nothing else, but then the user action gets reverted.

Then from 9cd4bae to 01ff998 I change the way we recheck for DNT.

Closes #1446.

cowlicks · 2017-06-29T21:15:34Z

TODO:

fix unittests that broke from DNT changes
add unittests
add migrations to remove "empty" action map objects

cowlicks · 2017-06-29T21:20:01Z

  return false;
 }

+function ifRelevant(details, callback) {


I need to document these functions.

cowlicks · 2017-06-29T21:20:54Z

+ * Wraps _recordPrevalence for use from webRequest listeners.
+ * Also saves tab (page) origins. TODO Should be handled by tabData instead.
+ * Also sets a timeout for checking DNT policy for third-party FQDNs.
+ * TODO Does too much, should be broken up ...


update this docstring

https://developers.google.com/speed/articles/optimizing-javascript#avoiding-pitfalls-with-closures

cowlicks · 2017-06-29T21:39:45Z

+
+function onRelevantBeforeSendHeaders(details, fqdn, origin, tabOrigin) {
+  badger.checkForDNTPolicy(fqdn, badger.storage.getNextUpdateForDomain(fqdn));
+  return heuristicBlockingAccounting(details, fqdn, origin, tabOrigin);


I broke this out into a separate function because I was speculating about performance. I should measure this before it gets committed.

cowlicks · 2017-07-06T01:06:23Z

On master, after visiting 100 sites, the action_map has 1199 entries, and 212kB
With this patch, after visiting 100 sites, the action_map has 534 entries, and 93kB

cowlicks · 2017-07-06T01:18:03Z

Ready for review

cowlicks · 2017-07-06T21:02:04Z

Closes #1446

cowlicks · 2017-07-06T21:12:35Z

Does anyone have a set of exported storage data I could use to test this migration? cc @alexristich @terrorist96

EDIT: Or if you'd like to keep the data to yourself, you are welcome to test out these changes yourself.

alexristich · 2017-07-06T23:33:08Z

Sure, let me know where I can email you the file.

terrorist96 · 2017-07-07T04:22:08Z

Sure, but as I noted in #1318, I can't export my data.

cowlicks · 2017-07-07T19:07:59Z

@alexristich I emailed you.

@terrorist96 If you want to try running this branch, it should dramatically reduce the size of your storage. However this is a testing branch, so I can't guarantee that it won't mess up your PB installation.

terrorist96 · 2017-07-07T19:10:38Z

I'll wait for the next stable version. 😃

cowlicks · 2017-07-07T19:21:18Z

I crawled 300 sites (data). Then ran this migration. It worked fine and removed 1132 uneccessary entries from my action map. Here's the exported data from after the migration.

alexristich · 2017-07-08T06:03:48Z

-    badger.storage.touchDNTRecheckTime(domain, recheckTime);
-
-    this._checkPrivacyBadgerPolicy(domain, function (success) {
+    self._checkPrivacyBadgerPolicy(domain, function (success) {


Looking at it, maybe it's a good time to rename _checkPrivacyBadgerPolicy to _checkDNTPolicy to make it more consistent.

Great idea!

alexristich · 2017-07-08T06:12:28Z

+  return _processWebRequest(details, heuristicBlockingAccounting, true);
+}
+
+function onBeforeResponseStarted(details) {


onBeforeResponseStarted -> onResponseStarted so as to match the webRequest API. Might be removed in a future PR, though in case it doesn't we should fix it here.

alexristich · 2017-07-08T06:31:44Z

    this._setupDomainAction(domain, action, "heuristicAction");
+    if (this.isActionMapEmptyForDomain(domain)) {
+      this.getBadgerStorageObject('action_map').deleteItem(domain);
+    }


Is this if condition ever true? A quick check in the code doesn't reveal any cases where setupHeuristicAction is called with either "" or constants.NO_TRACKING.

You are correct we do not do that anywhere. It is totally preventative.

I want this layer of the API (things wrapping _setupDomainAction) to manage garbage collecting action_map objects. Without this, there is a leak in that abstraction.

Ah, good thinking. Best not have to end up with another migration to fix the same issue in the future :)

alexristich · 2017-07-08T06:37:13Z

+CheckedDNTBuffer.prototype = {
+  size: 1000,
+  // if we've seen item, return true. if we haven't store it and return false
+  hasOrSet: function(item) {


I find the name hasOrSet to be a tad confusing.

Could it instead be split into two functions? One being has(), which checks if a domain has already been checked. Another which is add(), which is called in the case that _checkPrivacyBadgerPolicy() returns false to the callback.

I believe this would also eliminate the need for an explicit delete() function, as we wouldn't be adding anything that might need to be removed.

I agree hasOrSet is a bad name, I'm open to suggestions.

But I don't think breaking it into two functions is the correct thing to do, I actually considered mimicking the Set api first (like you suggest). However if add gets put into the callback, then the domain won't get added until the dnt check finishes, which could take a while, possibly after we ask for another DNT check for the same domain.

I also think having a seperate add and has could lead to misuse of the API. For example add might be called on an item that is already in the buffer. I could guard against this inside add to make it it idempotent, but then I'd be checking the set for containment of the domain twice for my main use case which is what happenes in the hasOrSet function.

I'm not sure misuse of the API ought to be a huge concern; making it idempotent is fairly light (considering it's a couple lines of code and a constant time operation). I guess my only thought then is whether an adequate name can be found that makes it clear what this is expected to do.

How about something like containsOrInsert or containsOrAdd?

How about isContaintedAndAddIfNot?

I do think API misuse is a pretty big concern. I don't trust myself to come back to this code in 2 years and remember that I'm only supposed to call add after has.

Haha, fair enough. Maybe hasElseAdd to keep it a bit more concise?

alexristich · 2017-07-08T06:39:34Z

Huge improvements, looking nice! I left a few short comments; hopefully some of it is of use.

@ghostwords give this PR a look when you get a chance.

cowlicks · 2017-07-13T21:31:23Z

@ghostwords Here are the changes to heuristicblocking.js from a high level. Previously we
called heuristicBlockingAccounting in onBeforeSendHeaders,
onResponseStarted (and once in webrequests.js). In
heuristicBlockingAccounting we took the details object that webRequests
gives us and did the following:

process the request by checking validity, and storing associated tab for
synchronous access later Check the origin for DNT check for cookie tracking
store tracking info

The problem here is that DNT checking didn't need to happen for the
onResponseStarted accounting, and the heuristicBlockingAccounting call in
webrequests.js wasn't even needed at all.

So I moved the proccessing and DNT check into a _processWebRequest function.
The _processWebRequest functions optionally checks for DNT, so we don't check
it for responses. So now heuristicBlockingAccounting just checks for cooking
tracking, and stores tracking info if needed.

I also avoided putting anonymous functions in the webrequest listeners,
because they have a higher constant time overhead than a named function that is
created at "compile time". The anonymous function gets redefined every time the
the listener is called. I haven't tested the performance improvement, js engine
optimization is pretty good so this could be pointless, but it is a simple
change, and having separate named functions makes things more readable (for
me). Performance sources (I read others but can't find them):

https://developers.google.com/speed/articles/optimizing-javascript#avoiding-pitfalls-with-closures
https://stackoverflow.com/questions/17234923/javascript-heavily-executed-functions-inline-or-declared

cowlicks · 2017-07-14T17:12:29Z

@ghostwords I fixed that test. It was failing from the changes I made to how DNT is rechecked. Instead of getting ratelimited we were discarding extra calls since they had already been checked. So now I test that the internal _doDNTCheck function is rate limited.

This is ready for review.

ghostwords · 2017-07-27T14:17:28Z

Just a quick note, the "closes"/"fixes" references to the original issue only work in the description of the ticket, not the subject or the comments. I updated the description to include the reference.

ghostwords · 2017-08-22T00:41:56Z

Could you please rebase and resolve conflicts?

This codepath was only being used by the special "third-strike" code path which was removed. So it is unneeded.

cowlicks · 2017-09-11T23:06:37Z

Blocked by #1642

ghostwords · 2017-12-07T14:53:29Z

Closing in favor of #1795.

andresbase assigned cowlicks Jun 29, 2017

cowlicks commented Jun 29, 2017

View reviewed changes

andresbase added the in progress label Jul 3, 2017

cowlicks changed the title ~~WIP: Don't store domains on disk that don't track~~ Don't store domains on disk that don't track Jul 6, 2017

cowlicks removed the in progress label Jul 6, 2017

alexristich reviewed Jul 8, 2017

View reviewed changes

cowlicks requested a review from ghostwords July 11, 2017 19:00

cowlicks force-pushed the stop-tracking-no-tracking-domains-lol branch 2 times, most recently from 5698509 to c1241ad Compare July 14, 2017 08:26

andresbase added this to the W30-31 milestone Jul 25, 2017

ghostwords mentioned this pull request Jul 26, 2017

Fix export data #1532

Closed

cowlicks force-pushed the stop-tracking-no-tracking-domains-lol branch from b18cdb6 to e4edcfa Compare July 28, 2017 03:25

cowlicks added 26 commits August 21, 2017 17:47

Add heuristicblocking onBeforeResponseStarted

65d4288

Remove checks from heuristicBlockingAccounting

463a921

remove extra heuristic accounting in background

3b8c064

Use new listeners

1420fc9

Add checkedDNT tracking

a176a2c

Move heuristicBlockingAccounting into function.

0d47878

Refactoring and moving functions around.

be903b4

Add CheckedDNTBuffer.

b5f7949

Allow good DNT's to be rechecked in same session.

fee832c

Update DNT checking tests.

686d702

Add tests for utils.CheckedDNTBuffer

ce7f0a1

All these are DNT tests so rename to tests/dnt.js

276e9f3

Update docstrings, refactor listeners.

13db9c5

rm timestamp from signature of checkForDNTPolicy

c1e01e9

refactoring, fix bug getting next update time

5ceb9cd

Add removeEmptyActionMap migration

507a349

reflect checkForDNTPolicy change in selenium tests

b502989

Remove unused codeblock.

5183f36

This codepath was only being used by the special "third-strike" code path which was removed. So it is unneeded.

_checkPBPolicy -> _doDNTCheck

bcd6eac

onBeforeResponseStarted -> onResponseStarted

ceb32fe

hasOrSet -> hasElseAdd

a7a5bb4

Put old test back.

18bb633

Use _doDNTCheck for ratelimit check.

20542c6

Move xhrSpy into base of module.

f21bf1c

remove debugging code

02b514b

Add changelog note.

e689a57

cowlicks force-pushed the stop-tracking-no-tracking-domains-lol branch from e4edcfa to e689a57 Compare August 22, 2017 01:33

ghostwords closed this Dec 7, 2017

Uh oh!

Uh oh!

Conversation

cowlicks commented Jun 29, 2017 • edited by ghostwords Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cowlicks commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cowlicks commented Jul 6, 2017

Uh oh!

cowlicks commented Jul 6, 2017

Uh oh!

cowlicks commented Jul 6, 2017

Uh oh!

cowlicks commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexristich commented Jul 6, 2017

Uh oh!

terrorist96 commented Jul 7, 2017

Uh oh!

cowlicks commented Jul 7, 2017

Uh oh!

terrorist96 commented Jul 7, 2017

Uh oh!

cowlicks commented Jul 7, 2017

Uh oh!

alexristich Jul 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cowlicks Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexristich commented Jul 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cowlicks commented Jul 13, 2017

Uh oh!

cowlicks commented Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghostwords commented Jul 27, 2017

Uh oh!

ghostwords commented Aug 22, 2017

Uh oh!

cowlicks commented Sep 11, 2017

Uh oh!

ghostwords commented Dec 7, 2017

Uh oh!

Reviewers

Assignees

cowlicks commented Jun 29, 2017 •

edited by ghostwords

Loading

cowlicks commented Jun 29, 2017 •

edited

Loading

cowlicks commented Jul 6, 2017 •

edited

Loading

alexristich Jul 8, 2017 •

edited

Loading

cowlicks Jul 11, 2017 •

edited

Loading

alexristich commented Jul 8, 2017 •

edited

Loading

cowlicks commented Jul 14, 2017 •

edited

Loading