Issue 394 - hit statistics tool data collection

lib/filterHits.js

+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2016 Eyeo GmbH
+ *
+ * Adblock Plus is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Adblock Plus is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+let {Services} = Cu.import("resource://gre/modules/Services.jsm", null);
+let {FileUtils} = Cu.import("resource://gre/modules/FileUtils.jsm", null);
+let {setTimeout, clearTimeout} = Cu.import("resource://gre/modules/Timer.jsm", null);
+
+let {IO} = require("io");
+let {Prefs} = require("prefs");
+let {Downloader, Downloadable, MILLIS_IN_SECOND, MILLIS_IN_DAY, MILLIS_IN_HOUR} = require("downloader");
+let {FilterNotifier} = require("filterNotifier");
+let {DownloadableSubscription} = require("subscriptionClasses");
+
+const CHECK_INTERVAL = MILLIS_IN_DAY;

[Wladimir Palant, 2016/03/22 21:32:32]

IMHO this should be one hour, like for subscription downloads. Otherwise many
people will only push when opening their browser (meaning: some time in the
morning), simply because their sessions never last a whole day.

[saroyanm, 2016/04/06 15:15:32]

Done.

+const INITIAL_DELAY = MILLIS_IN_SECOND * 5;

[Wladimir Palant, 2016/03/22 21:32:34]

IMHO this is much too short. Given that this isn't something we need to send out
at any cost, 2 minutes should do as well (not 1 minute because we run
subscription updates there). This will make sure that all the startup tasks are
already done.

[saroyanm, 2016/04/06 15:15:38]

Done.

+const PUSH_INTERVAL = MILLIS_IN_DAY * 7;
+
+/**
+ * The value of storage statement normal execution constant 
+ * @type Number
+ */
+const REASON_FINISHED = Components.interfaces.mozIStorageStatementCallback.REASON_FINISHED;
+
+/**
+ * This class collects filter hits statistics in a SQLite database
+ * and sends them to the server when user opt-in for that
+ * @class
+ */
+let FilterHits = exports.FilterHits =
+{
+  /**
+   * Used to prevent timers running in parallel
+   * @type Number
+   */
+  _prefToggleTimeoutId: 0,
+
+  /**
+   * Indicates whether the data is being sent to the server 
+   * @type Boolean
+   */
+  _sending: false,
+
+  get connection()
+  {
+    let connection = Services.storage.openDatabase(this.getStorageFile);
+    if (!connection.tableExists("filtersubscriptions"))
+    {
+      connection.executeSimpleSQL("CREATE TABLE filtersubscriptions " +
+        "(id INTEGER, subscriptions TEXT, PRIMARY KEY" +
+        "(id), UNIQUE(subscriptions))");
+    }
+    if (!connection.tableExists("filterhits"))
+    {
+      connection.executeSimpleSQL("CREATE TABLE filterhits (filter TEXT, " +
+        "host TEXT, thirdParty INTEGER, hitCount INTEGER, lastHit INTEGER, " +
+        "subscriptions INTEGER NOT NULL, PRIMARY KEY(filter, host, " +
+        "thirdParty), FOREIGN KEY(subscriptions) REFERENCES "+
+        "filtersubscriptions(id))");
+      Prefs.sendstats_last_push = Date.now();
+    }
+    Object.defineProperty(this, "connection",
+    {
+      value: connection
+    });
+    return connection;
+  },
+
+  /**
+   * @return nsIFile SQLite database file with filter hits data
+   */
+  get getStorageFile()

[Wladimir Palant, 2016/03/22 21:32:27]

This is a property, its name shouldn't start with "get" - storageFile?

[saroyanm, 2016/04/06 15:15:26]

Done.

+  {
+    let file = IO.resolveFilePath(Prefs.data_directory);
+    if (file)
+      file.append("adblockplus.sqlite");
+
+    Object.defineProperty(this, "getStorageFile", 
+    {
+      value: file
+    });
+    return file;
+  },
+
+  /**
+   * Called on module startup.
+   */
+  init: function()
+  {
+    Prefs.addListener(name =>
+    {
+      if (name == "sendstats")
+      {
+        if (this._prefToggleTimeout)
+          clearTimeout(this._prefToggleTimeout);
+        this._prefToggleTimeout = setTimeout(() =>
+        {
+          if (!Prefs.sendstats)
+            this.resetFilterHits();
+        }, 1000);
+      }
+    });
+
+    let downloader = new Downloader(this._getDownloadables.bind(this), 
+      INITIAL_DELAY, CHECK_INTERVAL, this.sendFilterHitsToServer);

[Wladimir Palant, 2016/03/22 21:32:28]

Why isn't sendFilterHitsToServer bound? How does it even work with a bogus
`this` value?

[saroyanm, 2016/04/06 15:15:31]

My bad, changed the implementation already.

+    downloader.onExpirationChange = this._onExpirationChange.bind(this);
+    onShutdown.add(() => downloader.cancel());
+  },
+
+  /**
+   * Yields a Downloadable instances for the notifications download.

[Wladimir Palant, 2016/03/22 21:32:25]

instances => instance
notifications download => filter hits push

[saroyanm, 2016/04/06 15:15:23]

Done.

+   */
+  _getDownloadables: function*()
+  {
+    let downloadable = new Downloadable();
+    if (typeof Prefs.sendstats_status.lastError === "number")
+      downloadable.lastError = Prefs.sendstats_status.lastError;
+    if (typeof Prefs.sendstats_status.lastCheck === "number")
+      downloadable.lastCheck = Prefs.sendstats_status.lastCheck;
+    if (typeof Prefs.sendstats_status.softExpiration === "number")
+      downloadable.softExpiration = Prefs.sendstats_status.softExpiration;
+    if (typeof Prefs.sendstats_status.hardExpiration === "number")
+      downloadable.hardExpiration = Prefs.sendstats_status.hardExpiration;
+    yield downloadable;
+  },
+
+  _onExpirationChange: function(downloadable)
+  {
+    Prefs.sendstats_status.lastCheck = downloadable.lastCheck;
+    Prefs.sendstats_status.softExpiration = downloadable.softExpiration;
+    Prefs.sendstats_status.hardExpiration = downloadable.hardExpiration;

[Wladimir Palant, 2016/03/22 21:32:26]

There is a limitation of Prefs module when it comes to objects - changes will
only be saved if you assign a different value. So you need to add the following:

  Prefs.sendstats_status = JSON.parse(JSON.stringify(Prefs.sendstats_status));

[saroyanm, 2016/04/06 15:15:42]

Done.

+  },
+
+  /**
+   * Increases the filter hit count
+   * @param {Filter} filter
+   * @param {String} host 
+   */
+  increaseFilterHits: function(filter, host, thirdParty)
+  {
+    let subscriptions = filter.subscriptions;
+    let downloadableSubscriptions = [];
+    for (let i = 0; i < subscriptions.length; i++)
+    {
+      if (subscriptions[i] instanceof DownloadableSubscription)
+        downloadableSubscriptions.push(subscriptions[i].url);
+    }
+
+    if (downloadableSubscriptions.length == 0)
+      return;
+
+    if (!this.getStorageFile)

[Wladimir Palant, 2016/03/22 21:32:35]

How is the storage file related to the code here? I assume that you really want
to check this.connection - and make sure that this.connection is null if storage
file cannot be resolved.

[saroyanm, 2016/04/06 15:15:41]

Done.

+      return;
+
+    let statementsArray = [];

[Wladimir Palant, 2016/03/22 21:32:20]

I don't think there is much point into adding data type to the variable name, at
least not in this case. Why not just `statements`?

[saroyanm, 2016/04/06 15:15:30]

Done.

+    let filtersubscriptions = JSON.stringify(downloadableSubscriptions);

[Wladimir Palant, 2016/03/22 21:32:31]

Nit: this should be camel case, filterSubscriptions?

[saroyanm, 2016/04/06 15:15:24]

Done.

+    let subscriptionStatement = this.connection.createStatement("INSERT OR " +
+      "IGNORE INTO filtersubscriptions (subscriptions) VALUES " +
+      "(:subscriptions)");
+    subscriptionStatement.params.subscriptions = filtersubscriptions;
+    statementsArray.push(subscriptionStatement);
+    let filterHitstatement = this.connection.createStatement("INSERT OR " +

[Wladimir Palant, 2016/03/22 21:32:30]

Nit: proper camel case would be filterHitStatement.

[saroyanm, 2016/04/06 15:15:39]

Done.

+      "REPLACE INTO filterhits (filter, host, thirdParty, hitCount, lastHit, " +
+      "subscriptions) VALUES (:filter, :host, :thirdParty, coalesce ((SELECT " +

[Wladimir Palant, 2016/03/22 21:32:18]

Nit: COALESCE should be capitalized.

[saroyanm, 2016/04/06 15:15:40]

Done.

+      "hitCount  FROM filterhits WHERE filter=:filter AND host=:host AND " +
+      "thirdParty=:thirdParty), 0) + 1, :lastHit, (SELECT id " +
+      "FROM filtersubscriptions WHERE subscriptions=:subscriptions))");
+    filterHitstatement.params.filter = filter.text;
+    filterHitstatement.params.host = host;
+    filterHitstatement.params.lastHit = roundToHours(filter.lastHit);
+    filterHitstatement.params.thirdParty = thirdParty;
+    filterHitstatement.params.subscriptions = filtersubscriptions;
+    statementsArray.push(filterHitstatement);
+
+    this.connection.executeAsync(statementsArray, statementsArray.length,
+    {
+      handleError: function(error)
+      {
+        Cu.reportError("Error during writing of filter hits: " + reason);

[Wladimir Palant, 2016/03/22 21:32:28]

Variable reason is not defined.

Also a nit: "Error updating filter hits"?

[saroyanm, 2016/04/06 15:15:28]

Done.

+      },
+      handleCompletion: function(reason)
+      {
+        if (reason != REASON_FINISHED)
+          Cu.reportError("Writing of filter hits canceled or aborted: " + reason);

[Wladimir Palant, 2016/03/22 21:32:32]

Nit: "Updating filter hits canceled or aborted"?

[saroyanm, 2016/04/06 15:15:27]

Done.

+      }

[Wladimir Palant, 2016/03/22 21:32:27]

I actually meant getting rid of bind(this) by means of arrow functions - even
though we don't use `this` in these two callbacks, with arrow functions we
wouldn't need to think about that.

[saroyanm, 2016/04/06 15:15:33]

Done.

+    });
+  },
+
+  /**
+   * Remove all local collected filter hits data
+   */
+  resetFilterHits: function()
+  {
+    if (!this.getStorageFile)

[Wladimir Palant, 2016/03/22 21:32:34]

As above, this should be checking this.connection rather than some unrelated
property.

[saroyanm, 2016/04/06 15:15:22]

Done.

+      return;
+
+    this.connection.executeSimpleSQL("DELETE FROM filterhits");
+    this.connection.executeSimpleSQL("DELETE FROM filtersubscriptions");
+  },
+
+  /**
+   * Send local stored filter hit data to the 
+   * service specified in Prefs.sendstats_url
+   */
+  sendFilterHitsToServer: function()
+  {
+    let now = Date.now();
+    if (Prefs.sendstats_last_push > now)
+      Prefs.sendstats_last_push = now;

[Wladimir Palant, 2016/03/22 21:32:25]

I don't think you want to set sendstats_last_push before actually sending the
data.

[saroyanm, 2016/04/06 15:15:30]

I tried to fix the local time change issue here, but doesn't matter anymore
while this is being handled by donwloader with new implemantation.

+
+    if ((now - Prefs.sendstats_last_push < PUSH_INTERVAL) || !Prefs.sendstats 

[Wladimir Palant, 2016/03/22 21:32:24]

Why are you double-checking the push interval? Supposedly, Downloader has done
all this already.

[saroyanm, 2016/04/06 15:15:28]

My bad, re-implemented this part.

+        || !this.getStorageFile)
+      return;
+
+    let statement = this.connection.createStatement("SELECT filterhits.*, " +

[Wladimir Palant, 2016/03/22 21:32:33]

I guess you want to check this.connection here as well?

[saroyanm, 2016/04/06 15:15:25]

Done.

+      "filtersubscriptions.subscriptions FROM filterhits LEFT JOIN " +
+      "filtersubscriptions filtersubscriptions ON " +
+      "filterhits.subscriptions=filtersubscriptions.id");
+    statement.executeAsync(
+    {
+      handleResult: (result) =>
+      {
+        let filters = Object.create(null);
+        for (let row = result.getNextRow(); row;
+             row = result.getNextRow())
+        {
+          let filterText = row.getResultByName("filter");
+          let host = row.getResultByName("host");
+          let matchType = (row.getResultByName("thirdParty") ? 
+                            "thirdParty" : "firstParty");
+          let hitCount = row.getResultByName("hitCount");
+          let lastHit = row.getResultByName("lastHit");
+          let subscriptions = row.getResultByName("subscriptions");
+          
+          if (!(filterText in filters))
+           filters[filterText] = Object.create(null);
+          
+          let filter = filters[filterText];
+          filter.subscriptions = subscriptions;
+          filter[matchType] = Object.create(null);
+          filter[matchType][host] = {"hits": hitCount, latest: lastHit};
+        }
+
+        let request = new XMLHttpRequest();
+        request.open("POST", Prefs.sendstats_url);
+        request.setRequestHeader("Content-Type", "application/json");
+        request.addEventListener("load", () =>
+        {
+          this._sending = false;
+          if (request.status >= 200 && request.status < 300)
+          {
+            Prefs.sendstats_status.lastError = 0;
+            Prefs.sendstats_last_push = Date.now();
+            FilterHits.resetFilterHits();
+          }
+          else
+          {
+            Prefs.sendstats_status.lastError = Date.now();
+            this._sending = false;
+            Cu.reportError("Couldn't send filter hit statistics to server. Status code: " + request.status);
+          }
+        }, false);
+        request.addEventListener("error", () =>
+        {
+          this._sending = false;

[Wladimir Palant, 2016/03/22 21:32:33]

Set lastError here as well?

[saroyanm, 2016/04/06 15:15:41]

Changed implementation, this should be done by Downloader already.

+          Cu.reportError("Error occured while sending filter hit statistics to server.");
+        }, false);
+
+        let {addonName, addonVersion, application, 
+            applicationVersion, platform, platformVersion} = require("info");
+        let data = {
+          version: 1,
+          timeSincePush: roundToHours(Date.now() - Prefs.sendstats_last_push),
+          addonName: addonName,
+          addonVersion: addonVersion,
+          application: application,
+          applicationVersion: applicationVersion,
+          platform: platform,
+          platformVersion: platformVersion,
+          filters: filters
+        };
+        if (!this._sending)

[Wladimir Palant, 2016/03/22 21:32:29]

Shouldn't you check this *before* going through all the effort compiling the
data? Like at the beginning of the function?

[saroyanm, 2016/04/06 15:15:21]

Changed implementation, this should be done by Downloader already.

+        {
+          this._sending = true;
+          request.send(JSON.stringify(data));
+        }
+      },
+      handleError: function(error)
+      {
+        Prefs.sendstats_status.lastError = Date.now();
+        Cu.reportError(error.message);

[Wladimir Palant, 2016/03/22 21:32:19]

More informative message here as well? "Error loading filter hits: " +
error.message

[saroyanm, 2016/04/06 15:15:33]

Done.

+      },
+      handleCompletion: function(reason)
+      {
+        if (reason != REASON_FINISHED)
+        {
+          Prefs.sendstats_status.lastError = Date.now();
+          Cu.reportError("Loading of filter hits canceled or aborted: " + reason);
+        }
+      }
+    });
+  },
+};
+
+function roundToHours(timestamp)
+{
+  return Math.round(timestamp / MILLIS_IN_HOUR) * 3600;

[Wladimir Palant, 2016/03/22 21:32:30]

3600 is actually (MILLIS_IN_HOUR / MILLIS_IN_SECOND)

[saroyanm, 2016/04/06 15:15:24]

Done.

+}
+
+FilterHits.init();