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

lib/content/snippets.js

Index: lib/content/snippets.js
===================================================================
--- a/lib/content/snippets.js
+++ b/lib/content/snippets.js
@@ -10,18 +10,65 @@
  * 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";
 
+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);
+}
+
+function stringifyFunctionCall(func, ...params)
+{
+  return `"use strict";(${func})(${params.map(JSON.stringify).join(",")});`;
+}
+
+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);
+}
+
 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.

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

+{
+  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);