Issue 394 - hit statistics tool data collection

lib/filterHits.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
- * Copyright (C) 2006-2015 Eyeo GmbH
+ * 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 {Utils} = require("utils");
-let {MILLIS_IN_DAY} = require("downloader");
+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_MINUTE, 
+  MILLIS_IN_HOUR, MILLIS_IN_DAY} = require("downloader");
+let {FilterNotifier} = require("filterNotifier");
+let {DownloadableSubscription} = require("subscriptionClasses");
+
+const CHECK_INTERVAL = MILLIS_IN_HOUR;
+const INITIAL_DELAY = MILLIS_IN_MINUTE * 2;
+const PUSH_INTERVAL = MILLIS_IN_DAY * 7;
 
 /**
- * This class reads filter hits statistics from SQLite database,
- * manages them in memory and writes them back.
+ * 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 =
 {
-  filters: {},
-  
-  _serviceURL: "",

[Thomas Greiner, 2015/02/23 18:43:05]

Where is the value for this supposed to be coming from?

-  _lastPush: 0,
-  _pushInterval: MILLIS_IN_DAY * 7,
-  _loading: false,
-  _saving: false,
-  _sending: false,

[saroyanm, 2015/02/28 15:24:30]

Done.

+  /**
+   * Used to prevent timers running in parallel
+   * @type Number
+   */
+  _prefToggleTimeout: 0,
+
+  get connection()
+  {
+    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))");
+    }
+    Object.defineProperty(this, "connection",
+    {
+      value: connection
+    });
+    return connection;
+  },
+
+  /**
+   * @return nsIFile SQLite database file with filter hits data
+   */
+  get storageFile()
+  {
+    let file = IO.resolveFilePath(Prefs.data_directory);
+    if (file)
+      file.append("adblockplus.sqlite");
+
+    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);
+    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 instance for the filter hits push.
+   */
+  _getDownloadables: function*()
+  {
+    let downloadable = new Downloadable(Prefs.sendstats_url);
+    if (Prefs.sendstats_status.lastError)
+      downloadable.lastError = Prefs.sendstats_status.lastError;
+    if (Prefs.sendstats_status.lastCheck)
+      downloadable.lastCheck = Prefs.sendstats_status.lastCheck;
+    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;
+    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 {Window} window  Window that the match originated in (required to get host)
-   */
-  increaseFilterHits: function(filter, wnd)
-  {
-    if (!filter.text || (filter.subscriptions[0] && filter.subscriptions[0].url.indexOf("~user~") == 0))

[Thomas Greiner, 2015/02/23 18:43:05]

This means that if the first subscription a filter is in, is a custom
subscription, then it shouldn't increase the filter hit count. That doesn't
match the behavior described in the issue "the data should include only filters
from filter subscriptions, not the user's own filters"

Instead you need to check whether there's a downloadable subscription in that
list.

+   * @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 (!(filter.text in this.filters))
-      this.filters[filter.text] = {};
-
-    var statFilter = this.filters[filter.text];

[Thomas Greiner, 2015/02/23 18:43:05]

Any reason why you're using `var` here?

-    let filterType = filter.thirdParty ? "thirdParty" : "firstParty";
-
-    if (!(filterType in statFilter))
-      statFilter[filterType] = {};
-
-    if (!("subscriptions" in statFilter))
-      statFilter.subscriptions = [];
-      
-    if (filter.subscriptions)
-    {
-      for (let i = 0; i < filter.subscriptions.length; i++)
-      {
-        if (statFilter.subscriptions.indexOf(filter.subscriptions[i]._title) == -1)
-          statFilter.subscriptions.push(filter.subscriptions[i]._title);
+    if (!this.connection)
+      return;
+
+    let statements = [];
+    let filterSubscriptions = JSON.stringify(downloadableSubscriptions);
+    let subscriptionStatement = this.connection.createStatement("INSERT OR " +
+      "IGNORE INTO filtersubscriptions (subscriptions) VALUES " +
+      "(:subscriptions)");
+    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 " +
+      "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;
+    statements.push(filterHitStatement);
+
+    this.connection.executeAsync(statements, statements.length,
+    {
+      handleError: (error) =>
+      {
+        Cu.reportError("Error updating filter hits: " + error.message);
+      },
+      handleCompletion: (reason) =>
+      {
+        if (reason != REASON_FINISHED)
+          Cu.reportError("Updating filter hits canceled or aborted: " + reason);
       }
-    }
-
-    let wndLocation = Utils.getOriginWindow(wnd).location.href;
-    let host = Utils.unwrapURL(wndLocation).host;
-    
-    if (!(host in statFilter[filterType]))
-      statFilter[filterType][host] = {hits: 1, latest: filter.lastHit};
-    else
-    {
-      statFilter[filterType][host].hits++;
-      statFilter[filterType][host].latest = filter.lastHit;
-    }
-  },
-  
+    });
+  },
+
+  /**
+   * Remove all local collected filter hits data
+   */
   resetFilterHits: function()
   {
-    this.filters = {};
-    this.saveFilterHitsToDatabase();
-  },
-  
-  sendFilterHitsToServer: function()
-  {
-    let prepareData = function()

[saroyanm, 2015/02/28 15:24:30]

Done.

-    {
-      let {addonName, addonVersion, application, applicationVersion, platform, platformVersion} = require("info");
-      return {
-        version: 1,
-        timeSincePush: this._lastPush,
-        addonName: addonName,
-        addonVersion: addonVersion,
-        application: application,
-        applicationVersion: applicationVersion,
-        platform: platform,
-        platformVersion: platformVersion,
-        filters: this.filters
+    if (!this.connection)
+      return;
+
+    this.connection.executeSimpleSQL("DELETE FROM filterhits");
+    this.connection.executeSimpleSQL("DELETE FROM filtersubscriptions");
+  },
+
+  _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(
+        {
+          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};
+            }
+            resolve({timeSincePush: roundToHours(Date.now() -
+              Prefs.sendstats_status.lastPush), "filters": filters});
+          },
+          handleError: (error) =>
+          {
+            Prefs.sendstats_status.lastError = Date.now();
+            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);
+            }
+          }
+        });
       }
-    }.bind(this);
-
-    let request = new XMLHttpRequest();
-    request.open("POST", this._serviceURL);
-    request.setRequestHeader("Content-Type", "application/json");
-    request.addEventListener("load", function(event)
-    {
-      let request = event.target;

[saroyanm, 2015/02/28 15:24:30]

Done.

-      FilterHits._sending = false;
-      if (request.status == 200)
-      {
-        FilterHits._lastPush = new Date().getTime();
-        FilterHits.resetFilterHits();
-      }
-      else
-        Cu.reportError("could not send filter hit statistics to AdBlock Plus server");

[Thomas Greiner, 2015/02/23 18:43:05]

Detail: In the other error messages you start with a capital letter and end with
a "."

[saroyanm, 2015/02/28 15:24:30]

Done.

[saroyanm, 2015/02/28 15:24:30]

Done.

-    }, false);
-    this._sending = true;
-    request.send(JSON.stringify(prepareData()));
-  },
-  
-  getStorageFile: function()
-  {
-    return FileUtils.getFile("ProfD", ["adblockplus.sqlite"]);
-  },
-  
-  checkCreateTable: function(connection)
-  {
-    if (!connection.tableExists("filterhits"))
-      connection.executeSimpleSQL("CREATE TABLE filterhits (id INTEGER PRIMARY KEY, filters TEXT, date INTEGER)");
-  },
-  
-  /**
-   * Load Filter hits from database
-   */
-  loadFilterHitsFromDatabase: function()
-  {
-    let storageFile = this.getStorageFile();
-    if (!storageFile)
-      return;
-
-    let connection = Services.storage.openDatabase(storageFile);
-    this.checkCreateTable(connection);
-    
-    let statement = connection.createStatement("SELECT * FROM filterhits");
-    if (!this._loading)
-    {
-      this._loading = true;
-      statement.executeAsync(
-      {
-        handleResult: function(aResultSet)
-        {
-          let row = aResultSet.getNextRow();
-          if (row)
-          {
-            let filters = row.getResultByName("filters");
-            let lastDate = row.getResultByName("date");
-            FilterHits.filters = JSON.parse(filters);
-            FilterHits._lastPush = lastDate;
-          }
-          FilterHits._loading = false;

[Thomas Greiner, 2015/02/23 18:43:05]

This seems redundant since `_loading` should already be set to `false` in the
`handleCompletion` callback.

-        },
-      
-        handleError: function(aError)
-        {
-          Cu.reportError(aError.message);

[Thomas Greiner, 2015/02/23 18:43:05]

Unless `handleCompletion` is also called when there's an error, the `_loading`
variable will not be set to `false` in case of an error.

[saroyanm, 2015/02/28 15:24:30]

Right, updated to always set `_loading` variable to false on handleCompletion.

-        },
-      
-        handleCompletion: function(aReason)
-        {
-          if (aReason != Components.interfaces.mozIStorageStatementCallback.REASON_FINISHED)
-          {
-            Cu.reportError("Loading of filter hits canceled or aborted.");
-            FilterHits._loading = false;
-          }
-        }
-      });
-    }
-    
-    connection.asyncClose();
-  },
-  
-  /**
-   * Save Filter hits to database
-   */
-  saveFilterHitsToDatabase: function()
-  {
-    if (!this._lastPush)
-      this._lastPush = new Date().getTime();

[Thomas Greiner, 2015/02/23 18:43:05]

You're using `new Date().getTime()` twice so best to create a new variable for
that (e.g. "now") to also ensure that it returns the same value.

[saroyanm, 2015/02/28 15:24:30]

Done.

-    
-    if (!this._sending && new Date().getTime() - this._lastPush > this._pushInterval)
-    {
-      this.sendFilterHitsToServer();
-      return;
-    }
-
-    let storageFile = this.getStorageFile();
-    if (!storageFile)
-      return;
-
-    let connection = Services.storage.openDatabase(storageFile);
-    this.checkCreateTable(connection);
-    
-    let statement = connection.createStatement("INSERT OR REPLACE INTO filterhits (id, filters, date) VALUES(0, :filters, :date)");
-    statement.params.filters = JSON.stringify(this.filters);
-    statement.params.date = this._lastPush;
-    if (!this._saving)
-    {
-      this._saving = true;
-      statement.executeAsync(
-      {
-        handleError: function(aError)
-        {
-          Cu.reportError(aError.message);

[saroyanm, 2015/02/28 15:24:30]

Handle completion should also be triggered during Error:
https://developer.mozilla.org/en-US/docs/MozIStorageStatementCallback#Constants 

So the `_saving` variable should be set to false even on error occurrence. 

-        },
-      
-        handleCompletion: function(aReason) 
-        {
-          if (aReason != Components.interfaces.mozIStorageStatementCallback.REASON_FINISHED)
-            Cu.reportError("Writing of filter hits canceled or aborted.");
-          FilterHits._saving = false;
-        }
-      });
-    }
-    
-    connection.asyncClose();
-  }
+    }
+  },
 };
+
+function roundToHours(timestamp)
+{
+  return Math.round(timestamp / MILLIS_IN_HOUR) *
+      (MILLIS_IN_HOUR / MILLIS_IN_SECOND);
+}
+
+FilterHits.init();