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 {Downloader, Downloadable, MILLIS_IN_SECOND, MILLIS_IN_MINUTE, 
+  MILLIS_IN_HOUR, MILLIS_IN_DAY} = 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 CHECK_INTERVAL = MILLIS_IN_HOUR;
+const INITIAL_DELAY = MILLIS_IN_MINUTE * 2;
 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,
+  _prefToggleTimeout: 0,
 
   get connection()
   {
-    let connection = Services.storage.openDatabase(this.getStorageFile);
+    if (!this.storageFile)
+      return null;
+
+    let connection = Services.storage.openDatabase(this.storageFile);
     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.

+  get storageFile()
   {
     let file = IO.resolveFilePath(Prefs.data_directory);
     if (file)
       file.append("adblockplus.sqlite");
 
-    Object.defineProperty(this, "getStorageFile", 
+    Object.defineProperty(this, "storageFile", 
     {
       value: file
     });
     return file;
   },
 
   /**
    * Called on module startup.
    */
   init: function()
   {
+    if (!Prefs.sendstats_status.lastPush)
+    {
+      Prefs.sendstats_status.lastPush = Date.now();
+      this._saveFilterHitPrefs();
+    }
     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.

+      INITIAL_DELAY, CHECK_INTERVAL);
     downloader.onExpirationChange = this._onExpirationChange.bind(this);
+    downloader.generateRequestData = this._generateFilterHitsData.bind(this);
+    downloader.onDownloadSuccess = this._onDownloadSuccess.bind(this);
+    downloader.validResponses.push(204);
     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.

+   * Yields a Downloadable instance for the filter hits push.
    */
   _getDownloadables: function*()
   {
-    let downloadable = new Downloadable();
-    if (typeof Prefs.sendstats_status.lastError === "number")
+    let downloadable = new Downloadable(Prefs.sendstats_url);
+    if (Prefs.sendstats_status.lastError)
       downloadable.lastError = Prefs.sendstats_status.lastError;
-    if (typeof Prefs.sendstats_status.lastCheck === "number")
+    if (Prefs.sendstats_status.lastCheck)
       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;
+    if (Prefs.sendstats_status.lastPush)
+    {
+      downloadable.softExpiration = Prefs.sendstats_status.lastPush + PUSH_INTERVAL;

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

I'm not sure regarding this, should we specify softExpiration ?
BTW why do we have softExpiration ? What it suppose to do ?

+      downloadable.hardExpiration = Prefs.sendstats_status.lastPush + PUSH_INTERVAL;
+    }
     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.

+    this._saveFilterHitPrefs();
+  },
+
+  _saveFilterHitPrefs: function()
+  {
+    Prefs.sendstats_status = JSON.parse(JSON.stringify(Prefs.sendstats_status));
+  },
+
+  _onDownloadSuccess: function(downloadable, responseText, errorCallback, redirectCallback)
+  {
+    Prefs.sendstats_status.lastError = 0;
+    Prefs.sendstats_status.lastPush = Date.now();
+    this._saveFilterHitPrefs();
   },
 
   /**
    * 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.

+    if (!this.connection)
       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 statements = [];
+    let filterSubscriptions = JSON.stringify(downloadableSubscriptions);
     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.

+    subscriptionStatement.params.subscriptions = filterSubscriptions;
+    statements.push(subscriptionStatement);
+    let filterHitStatement = this.connection.createStatement("INSERT OR " +
       "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.

+      "subscriptions) VALUES (:filter, :host, :thirdParty, COALESCE ((SELECT " +
       "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.

+    filterHitStatement.params.filter = filter.text;
+    filterHitStatement.params.host = host;
+    filterHitStatement.params.lastHit = roundToHours(filter.lastHit);
+    filterHitStatement.params.thirdParty = thirdParty;
+    filterHitStatement.params.subscriptions = filterSubscriptions;
+    statements.push(filterHitStatement);
+
+    this.connection.executeAsync(statements, statements.length,
+    {
+      handleError: (error) =>
+      {
+        Cu.reportError("Error updating filter hits: " + error.message);
       },
-      handleCompletion: function(reason)
+      handleCompletion: (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.

+          Cu.reportError("Updating filter hits canceled or aborted: " + reason);
       }

[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.

+    if (!this.connection)
       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())
+  _generateFilterHitsData: function(downloadable)
+  {
+    return {
+      then: (resolve) =>
+      {
+        if (!this.connection)
+          return;
+
+        let statement = this.connection.createStatement("SELECT filterhits.*, " +
+        "filtersubscriptions.subscriptions FROM filterhits LEFT JOIN " +
+        "filtersubscriptions filtersubscriptions ON " +
+        "filterhits.subscriptions=filtersubscriptions.id");
+        statement.executeAsync(
         {
-          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)
+          handleResult: (result) =>
           {
-            Prefs.sendstats_status.lastError = 0;
-            Prefs.sendstats_last_push = Date.now();
-            FilterHits.resetFilterHits();
-          }
-          else
+            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};
+            }
+            resolve({timeSincePush: roundToHours(Date.now() -
+              Prefs.sendstats_status.lastPush), "filters": filters});
+          },
+          handleError: (error) =>
           {
             Prefs.sendstats_status.lastError = Date.now();
-            this._sending = false;
-            Cu.reportError("Couldn't send filter hit statistics to server. Status code: " + request.status);
+            this._saveFilterHitPrefs();
+            Cu.reportError("Error loading filter hits:" + error.message);
+          },
+          handleCompletion: (reason) =>
+          {
+            if (reason != REASON_FINISHED)
+            {
+              Prefs.sendstats_status.lastError = Date.now();
+              this._saveFilterHitPrefs();
+              Cu.reportError("Loading of filter hits canceled or aborted: " + 
+                reason);
+            }
           }
-        }, 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.

+  return Math.round(timestamp / MILLIS_IN_HOUR) *
+      (MILLIS_IN_HOUR / MILLIS_IN_SECOND);
 }
 
 FilterHits.init();