Issue 6923 - Only parse metadata from special comments at the top of the file

lib/synchronizer.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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
@@ skipping 18 matching lines @@
 const {filterNotifier} = require("./filterNotifier");
 const {Prefs} = require("prefs");
 const {Subscription,
        DownloadableSubscription} = require("./subscriptionClasses");
 
 const INITIAL_DELAY = 1 * MILLIS_IN_MINUTE;
 const CHECK_INTERVAL = 1 * MILLIS_IN_HOUR;
 const DEFAULT_EXPIRATION_INTERVAL = 5 * MILLIS_IN_DAY;
 
 /**
- * The object providing actual downloading functionality.
- * @type {Downloader}
+ * Downloads filter subscriptions whenever necessary.
  */
-let downloader = null;
-
-/**
- * This object is responsible for downloading filter subscriptions whenever
- * necessary.
- * @class
- */
-let Synchronizer = exports.Synchronizer =
+class Synchronizer
 {
   /**
-   * Called on module startup.
-   */
-  init()
-  {
-    downloader = new Downloader(this._getDownloadables.bind(this),
-                                INITIAL_DELAY, CHECK_INTERVAL);
+   * @hideconstructor
+   */
+  constructor()
+  {
+    /**
+     * The object providing actual downloading functionality.
+     * @type {Downloader}
+     */
+    this._downloader = new Downloader(this._getDownloadables.bind(this),
+                                      INITIAL_DELAY, CHECK_INTERVAL);
     onShutdown.add(() =>
     {
-      downloader.cancel();
+      this._downloader.cancel();
     });
 
-    downloader.onExpirationChange = this._onExpirationChange.bind(this);
-    downloader.onDownloadStarted = this._onDownloadStarted.bind(this);
-    downloader.onDownloadSuccess = this._onDownloadSuccess.bind(this);
-    downloader.onDownloadError = this._onDownloadError.bind(this);
-  },
+    this._downloader.onExpirationChange = this._onExpirationChange.bind(this);
+    this._downloader.onDownloadStarted = this._onDownloadStarted.bind(this);
+    this._downloader.onDownloadSuccess = this._onDownloadSuccess.bind(this);
+    this._downloader.onDownloadError = this._onDownloadError.bind(this);
+  }
 
   /**
    * Checks whether a subscription is currently being downloaded.
    * @param {string} url  URL of the subscription
-   * @return {boolean}
+   * @returns {boolean}
    */
   isExecuting(url)
   {
-    return downloader.isDownloading(url);
-  },
+    return this._downloader.isDownloading(url);
+  }
 
   /**
    * Starts the download of a subscription.
    * @param {DownloadableSubscription} subscription
    *   Subscription to be downloaded
    * @param {boolean} manual
-   *   true for a manually started download (should not trigger fallback
-   *   requests)
+   *   <code>true</code> for a manually started download (should not trigger
+   *   fallback requests)
    */
   execute(subscription, manual)
   {
-    downloader.download(this._getDownloadable(subscription, manual));
-  },
-
-  /**
-   * Yields Downloadable instances for all subscriptions that can be downloaded.
+    this._downloader.download(this._getDownloadable(subscription, manual));
+  }
+
+  /**
+   * Yields {@link Downloadable} instances for all subscriptions that can be
+   * downloaded.
+   * @yields {Downloadable}
    */
   *_getDownloadables()
   {
     if (!Prefs.subscriptions_autoupdate)
       return;
 
     for (let subscription of FilterStorage.subscriptions)
     {
       if (subscription instanceof DownloadableSubscription)
         yield this._getDownloadable(subscription, false);
     }
-  },
-
-  /**
-   * Creates a Downloadable instance for a subscription.
+  }
+
+  /**
+   * Creates a {@link Downloadable} instance for a subscription.
    * @param {Subscription} subscription
    * @param {boolean} manual
-   * @return {Downloadable}
+   * @returns {Downloadable}
    */
   _getDownloadable(subscription, manual)
   {
     let result = new Downloadable(subscription.url);
     if (subscription.lastDownload != subscription.lastSuccess)
       result.lastError = subscription.lastDownload * MILLIS_IN_SECOND;
     result.lastCheck = subscription.lastCheck * MILLIS_IN_SECOND;
     result.lastVersion = subscription.version;
     result.softExpiration = subscription.softExpiration * MILLIS_IN_SECOND;
     result.hardExpiration = subscription.expires * MILLIS_IN_SECOND;
     result.manual = manual;
     result.downloadCount = subscription.downloadCount;
     return result;
-  },
+  }
 
   _onExpirationChange(downloadable)
   {
     let subscription = Subscription.fromURL(downloadable.url);
     subscription.lastCheck = Math.round(
       downloadable.lastCheck / MILLIS_IN_SECOND
     );
     subscription.softExpiration = Math.round(
       downloadable.softExpiration / MILLIS_IN_SECOND
     );
     subscription.expires = Math.round(
       downloadable.hardExpiration / MILLIS_IN_SECOND
     );
-  },
+  }
 
   _onDownloadStarted(downloadable)
   {
     let subscription = Subscription.fromURL(downloadable.url);
     filterNotifier.emit("subscription.downloading", subscription);
-  },
+  }
 
   _onDownloadSuccess(downloadable, responseText, errorCallback,
                      redirectCallback)
   {
     let lines = responseText.split(/[\r\n]+/);
     let headerMatch = /\[Adblock(?:\s*Plus\s*([\d.]+)?)?\]/i.exec(lines[0]);
     if (!headerMatch)
       return errorCallback("synchronize_invalid_data");
     let minVersion = headerMatch[1];
 
     let params = {
       redirect: null,
       homepage: null,
       title: null,
       version: null,
       expires: null
     };
     for (let i = 1; i < lines.length; i++)
     {
-      let match = /^\s*!\s*(\S.*?)\s*:\s*(.*)/.exec(lines[i]);

[Sebastian Noack, 2018/09/03 18:47:13]

You might wonder why I changed this regular expression. This is necessary
because many filter lists contain special/metadata comments with a malformed key
(most notably "Last modified"). While such a key has no effect, without having
this regular expression match them, we'd ignore any metadata that might follow.

[Manish Jethani, 2018/09/04 07:14:41]

Acknowledged.

[Sebastian Noack, 2018/09/04 15:32:38]

You have a point there. But in this regard, IMO we should rather use a Map
object here. But I leave that up to another change (by someone else).

+      let match = /^\s*!\s*(?:(\w+)\s*:\s*(.*)|\S)/.exec(lines[i]);
       if (!match)
         break;
 
-      let keyword = match[1].toLowerCase();
-      if (keyword in params)

[Manish Jethani, 2018/09/04 07:14:41]

BTW I find in my tests that `params.hasOwnProperty(keyword)` is much faster than
`keyword in params`. Like about 30% faster here.

[Sebastian Noack, 2018/09/04 14:22:06]

IMO the in-operator reads much nicer, and this isn't a hotspot anymore (now this
code is only hit a few times for each filter list parsed). Also if we want to
further optimize this code, we might want to benchmark it in context, rather
than relying on micro-benchmarks. It's possible that hasOwnProperty() isn't
faster after all in this context, since the prototype and properties of params
might be known to the JIT. Either way, this seems to be an unrelated change,
since the only thing I changed in this line is the indentation.

[Manish Jethani, 2018/09/04 15:01:45]

Acknowledged.

(Aside from performance, it's also about correctness. `'toString' in params`
would also be `true`, which is wrong here. Also I'm pretty sure `hasOwnProperty`
performs better when the keys being looked up are not statically embedded in the
source code, which is the case here. But I agree this would be a separate issue,
unrelated to what we're doing here.)

-      {
-        params[keyword] = match[2];
-        lines.splice(i--, 1);

[Manish Jethani, 2018/09/04 07:14:41]

Since we're considering malformed keywords as well, shouldn't we splice out
those lines too?

So we could start with `paramIndex = 1` and then after the loop just do
`lines.splice(1, paramIndex - 1)`.

[Sebastian Noack, 2018/09/04 14:22:06]

Historically, if it's not recognized as supported metadata, we treat it as
CommentFilter. Moreover, splice() is expensive, so if we want to optimize here,
we might rather not remove any items at all here.

[Manish Jethani, 2018/09/04 15:01:45]

OK, fair enough. I'd like to consider not using `splice` here but let's make
that a separate change.

+      if (typeof match[1] != "undefined")
+      {
+        let keyword = match[1].toLowerCase();
+        if (params.hasOwnProperty(keyword))
+        {
+          params[keyword] = match[2];
+          lines.splice(i--, 1);
+        }
       }
     }
 
     if (params.redirect)
       return redirectCallback(params.redirect);
 
     // Handle redirects
     let subscription = Subscription.fromURL(downloadable.redirectURL ||
                                             downloadable.url);
     if (downloadable.redirectURL &&
@@ skipping 59 matching lines @@
         if (match[2])
           expirationInterval = interval * MILLIS_IN_HOUR;
         else
           expirationInterval = interval * MILLIS_IN_DAY;
       }
     }
 
     let [
       softExpiration,
       hardExpiration
-    ] = downloader.processExpirationInterval(expirationInterval);
+    ] = this._downloader.processExpirationInterval(expirationInterval);
     subscription.softExpiration = Math.round(softExpiration / MILLIS_IN_SECOND);
     subscription.expires = Math.round(hardExpiration / MILLIS_IN_SECOND);
 
     if (minVersion)
       subscription.requiredVersion = minVersion;
     else
       delete subscription.requiredVersion;
 
     // Process filters
     lines.shift();
     let filters = [];
     for (let line of lines)
     {
       line = Filter.normalize(line);
       if (line)
         filters.push(Filter.fromText(line));
     }
 
     FilterStorage.updateSubscriptionFilters(subscription, filters);
-  },
+  }
 
   _onDownloadError(downloadable, downloadURL, error, channelStatus,
                    responseStatus, redirectCallback)
   {
     let subscription = Subscription.fromURL(downloadable.url);
     subscription.lastDownload = Math.round(Date.now() / MILLIS_IN_SECOND);
     subscription.downloadStatus = error;
 
     // Request fallback URL if necessary - for automatic updates only
     if (!downloadable.manual)
@@ skipping 46 matching lines @@
           {
             let data = "[Adblock]\n" +
               subscription.filters.map(f => f.text).join("\n");
             redirectCallback("data:text/plain," + encodeURIComponent(data));
           }
         }, false);
         request.send(null);
       }
     }
   }
-};
-Synchronizer.init();
+}
+
+/**
+ * This object is responsible for downloading filter subscriptions whenever
+ * necessary.
+ * @type {Synchronizer}
+ */
+let synchronizer = new Synchronizer();
+
+exports.Synchronizer = synchronizer;