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*(?:(\w+)\s*:\s*(.*)|\S)/.exec(lines[i]);

[Manish Jethani, 2018/09/05 18:10:08]

How about this one: `/^\s*!\s*([^\s:]*)\s*(:)?\s*(.*)/`

It seems simpler to me but maybe it performs worse, I'd have to check. Anyway,
so with this we can check for `match[1]` being truthy.

[Sebastian Noack, 2018/09/06 00:56:03]

Done. Not sure if it's any easier to read through.
Nope, we have to check for match[2] instead.

[Manish Jethani, 2018/09/06 05:53:53]

I stand corrected, I meant `match[2]`

A couple of things to note with this regular expression:

 1.  Empty keys are valid, but of course they would get ignored in the code that
follows
 2.  Keys can now contain non-ASCII characters; if we stick with this, perhaps
we should document it; values could already contain non-ASCII characters, there
should be no reason to disallow them from the keys
 3.  This will continue even after empty comments; if you disagree, we need to
update it (I think it's fine to continue parsing after empty comments but you're
aware of the full context of this issue)

[Sebastian Noack, 2018/09/06 14:31:36]

This is just a stop gap, what is eventually considered a valid key is defined by
the contents of the params object. So whether the regexp allows non-ascii or not
doesn't matter, as long as we keep looking for more metadata after an invalid
key (regardless of whether it contained non-ascii or whitespaces, or simply
isn't given in the params object).
Opps, I missed that. Went back to my regular expression now.

As I said, look at EasyList for example, the metadata follow some non-metadata
comments separated by an empty comment. That is why I want to stop looking for
more metadata after an empty comment.

[Manish Jethani, 2018/09/10 07:24:49]

Acknowledged.

+      if (!match)
+        break;
+
+      if (match[1] != undefined)

[Manish Jethani, 2018/09/05 18:10:08]

We never compare with `undefined` in core. `undefined` is a global variable
(unlike `null`, which is a language keyword) and must be looked up in each
scope. It is slow. Normally we do `typeof foo != "undefined"` all over the
place.

[Sebastian Noack, 2018/09/06 00:56:03]

Actually, I cannot redefine undefined, neither through assignment nor through
Object.defineProperty(). But still it's different from null (trying to assign to
it is a syntax error), while undefined seems to be a read-only property on the
global object. Technically both could be optimized similarly, but not idea if
they are.

Just in case, also note that null == undefined, which might come handy, if this
should be a concern, and you don't want to treat actual null (if the value can
be) differently.

[Manish Jethani, 2018/09/06 05:53:53]

You can just shadow `undefined` in the local scope, which you cannot do with
`null`.
Acknowledged.

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