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 142 matching lines @@
       return errorCallback("synchronize_invalid_data");
     let minVersion = headerMatch[1];
 
     let params = {
       redirect: null,
       homepage: null,
       title: null,
       version: null,
       expires: null
     };
-    for (let i = 0; i < lines.length; i++)
+    for (let i = 1; i < lines.length; i++)
     {
-      let match = /^\s*!\s*(\w+)\s*:\s*(.*)/.exec(lines[i]);
-      if (match)
+      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).

+      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.)

       {
-        let keyword = match[1].toLowerCase();
-        let value = match[2];
-        if (keyword in params)
-        {
-          params[keyword] = value;
-          lines.splice(i--, 1);
-        }
+        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 (params.redirect)
       return redirectCallback(params.redirect);
 
     // Handle redirects
     let subscription = Subscription.fromURL(downloadable.redirectURL ||
                                             downloadable.url);
     if (downloadable.redirectURL &&
@@ skipping 148 matching lines @@
               subscription.filters.map(f => f.text).join("\n");
             redirectCallback("data:text/plain," + encodeURIComponent(data));
           }
         }, false);
         request.send(null);
       }
     }
   }
 };
 Synchronizer.init();