Issue 6538, 6781 - Add code injection wrapper to snippets library

lib/content/snippets.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
  * 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/>.
  */
 
 /* eslint-env webextensions */
 /* eslint no-console: "off" */
 
 "use strict";
 
+/**
+ * Injects JavaScript code into the document using a temporary
+ * <code>script</code> element.
+ *
+ * @param {string} code The code to inject.
+ * @param {Array.<function|string>} [dependencies] A list of dependencies
+ *   to inject along with the code. A dependency may be either a function or a
+ *   string containing some executable code.
+ */
 function injectCode(code, dependencies = [])
 {
   for (let dependency of dependencies)
     code += dependency;
 
   let script = document.createElement("script");
 
   script.type = "application/javascript";
   script.async = false;
 
   // Firefox 58 only bypasses site CSPs when assigning to 'src',
   // while Chrome 67 only bypasses site CSPs when using 'textContent'.
   if (browser.runtime.getURL("").startsWith("chrome-extension://"))
   {
     script.textContent = code;
     document.documentElement.appendChild(script);
   }
   else
   {
     let url = URL.createObjectURL(new Blob([code]));
     script.src = url;
     document.documentElement.appendChild(script);
     URL.revokeObjectURL(url);
   }
 
   document.documentElement.removeChild(script);
 }
 
+/**
+ * Converts a function and an optional list of arguments into a string of code
+ * containing a function call. The function is converted to its string
+ * representation using the <code>Function.prototype.toString</code> method.
+ * Each argument is stringified using <code>JSON.stringify</code>. The
+ * generated code begins with the <code>"use strict"</code> directive.
+ *
+ * @param {function} func The function to convert.
+ * @param {...*} [params] The arguments to convert.
+ *
+ * @returns {string} The generated code containing the function call.
+ */
 function stringifyFunctionCall(func, ...params)
 {
+  // Call JSON.stringify on the arguments to avoid any arbitrary code
+  // execution.
   return `"use strict";(${func})(${params.map(JSON.stringify).join(",")});`;
 }
 
+/**
+ * Wraps a function and its dependencies into an injector. The injector, when
+ * called with zero or more arguments, generates code that calls the function,
+ * with the given arguments, if any, and injects the code, along with any
+ * dependencies, into the document using a temporary <code>script</code>
+ * element.
+ *
+ * @param {function} injectable The function to wrap into an injector.
+ * @param {...(function|string)} [dependencies] Any dependencies of the
+ *   function. A dependency may be either a function or a string containing
+ *   some executable code.
+ *
+ * @returns {function} The generated injector.
+ */
 function makeInjector(injectable, ...dependencies)

[kzar, 2018/07/16 14:13:56]

Please could you add a JSDoc comment for this function too?

[Manish Jethani, 2018/07/17 10:44:42]

Will do.

[Manish Jethani, 2018/07/17 15:42:36]

Done.

 {
   return (...args) => injectCode(stringifyFunctionCall(injectable, ...args),

[kzar, 2018/07/16 14:13:56]

I'd prefer we did the work of `stringifyFunctionCall` here, and made the code a
bit easier to grok whilst we're at it. Something like this (untested):

return (...args) =>
{
  injectCode(`"use strict";
              ($(injectable)(...${JSON.stringify(args)}));`,
             dependencies);
};

Also, perhaps injecting the "use strict" without a surrounding function is a bad
idea. Unless I'm mistaken that will enable strict mode for the entire document.

[Manish Jethani, 2018/07/17 10:44:41]

I would really prefer if stringifyFunctionCall is a separate function. It
performs a well defined task, which I can document. We are going to have to
reuse the function elsewhere, mostly likely, and in any case if it's well
defined and generic it's better to have it as a separate function.

[...]
No, it will only enable strict mode for the script element, and that's what we
want.

[kzar, 2018/07/17 11:11:12]

Alright, but what about my comment about making it easier to read? 

return `"use strict";
        ($(injectable)(...${JSON.stringify(args)}));`;

VS

return `"use strict";(${func})(${params.map(JSON.stringify).join(",")});`;

Well, up to you anyway.

[Manish Jethani, 2018/07/17 15:42:36]

That would inject code like:

  foo(...["x", "y", "z"])

Rather than:

  foo("x", "y", "z")

I think I would prefer that the injected code is cleaner so it's easier to debug
and find problems, even if the code that injects it is a little bit more
complex.

[kzar, 2018/07/17 16:08:21]

Acknowledged.

                                  dependencies);
 }
 
+/**
+ * Logs its arguments to the console. This may be used for testing and
+ * debugging.
+ *
+ * @param {...*} [args] The arguments to log.
+ */
 function log(...args)
 {
   console.log(...args);
 }
 
 exports.log = log;

[kzar, 2018/07/16 14:13:56]

Why not just export `console.log`? I don't think it even needs to be bound.

[Manish Jethani, 2018/07/17 10:44:41]

Because console.log.toString() is "function log() { [native code] }" which is
not executable. The snippet's string form should be executable JS code.

[kzar, 2018/07/17 11:11:12]

Good point.

 
+/**
+ * Similar to {@link log}, but does the logging in the context of the document
+ * rather than the content script. This may be used for testing and debugging,
+ * especially to verify that the injection of snippets into the document is
+ * working without any errors.
+ *
+ * @param {...*} [args] The arguments to log.
+ */
 function trace(...args)

[Manish Jethani, 2018/07/13 14:17:33]

I had to do this, both because it helps to have an example but mainly also
because otherwise ESLint complains that makeInjector is unused.

[kzar, 2018/07/16 14:13:57]

Please could you add a JSDoc comment explaining that this is an example and what
it does?

[Manish Jethani, 2018/07/17 10:44:41]

Will do.

[Manish Jethani, 2018/07/17 15:42:36]

Done.

 {
+  // We could simply use console.log here, but the goal is to demonstrate the
+  // usage of snippet dependencies.
   log(...args);

[kzar, 2018/07/16 14:13:57]

Why not call `console.log(...args);` here directly?

[Manish Jethani, 2018/07/17 10:44:42]

Because the goal is to demonstrate the use of the dependencies argument to
makeInjector. Here log is a dependency of trace. It is deliberately contrived,
to be used as an example.

I will add some documentation to this patch.

[kzar, 2018/07/17 11:11:12]

Acknowledged.

 }
 
 exports.trace = makeInjector(trace, log);