From ab50ad735b72049eee6fea6dec0ab9604f46d2ca Mon Sep 17 00:00:00 2001
From: Jimmy <jimmy@spalge.com>
Date: Sat, 31 Mar 2018 20:51:30 +1300
Subject: [PATCH 01/10] Greasemonkey: isolates scripts' writes to window

Since the global namespace of javascript in the browser is accessible
there where issues with scripts clobbering things that the page expected
to be attributes of window pages clobbering things that userscripts
saved to `window`. The latter was occuring with OneeChan. OneeChan was
setting `window.$` to a function that took a css selector and the 4chan
catalog script was setting `window.$` to some object. Since OneeChan was
set to run at document-start this broke OneeChan, switching it to
document-end broke scripts on 4chan.

I used OneeChan and 4chan-X on 4chan as the test case for this and
TamperMonkey as a guide for what is the correct way to handle scoping. I
didn't manage to pick apart just how TamperMonkey does what it does (I
think it might just be the environment that Chrome provides extensions
actually) but I got close to the same behaviour.

TamperMonkey provides a `window` object that appears to be what the
global window looked like before the webpage modified it. The global
scope though does have the pages modifications accessible. If the script
assigns something to an attribute `window` it can see that attribute in
the global scope. This implementation differs from that one in that, to
the scipt, `window` and the global scope always look the same, and that
is the same as the global scope looks in the environment provided by
TamperMonkey.

I am using the ES6 `Proxy` feature to allow the `window` object to look
like the actual (unsafe) one while allowing writing to it that doesn't
clobber the unsafe one. I am then using the ES4 `with` function to make
attributes of that window (proxy) object visible in the scope chain.
There may be other ways to do this without using `with` by using nested
functions and setting `this` creatively. There are notes around
alleging `with` to be various states of uncool[1].

I also ran into an issue where a userscript calling
`window.addEventListener(...)` would fail with `TypeError: Illegal
Execution` which is apparently due to `this` not being set correctly. I
looked at the functions which threw that error and those that didn't and
am using whether they have a `prototype` attribute or not to tell
whether I need to bind them with `window` as `this`. I am not sure how
correct that is but it worked for all the cases I ran into.

[1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/with
---
 .../javascript/greasemonkey_wrapper.js        | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/qutebrowser/javascript/greasemonkey_wrapper.js b/qutebrowser/javascript/greasemonkey_wrapper.js
index 0731e93ac..e9cdc9f1a 100644
--- a/qutebrowser/javascript/greasemonkey_wrapper.js
+++ b/qutebrowser/javascript/greasemonkey_wrapper.js
@@ -145,9 +145,53 @@
         }
     };
 
+    /* TamperMonkey allows assinging to `window` to change the visible global
+     * scope, without breaking other scripts. Eg if the page has set
+     * window.$ for an object for some reason (hello 4chan)
+     * - typeof $ === 'object'
+     * - typeof window.$ == 'undefined'
+     * - window.$ = function() {}
+     * - typeof $ === 'function'
+     * - typeof window.$ == 'function'
+     * Just shadowing `window` won't work because if you try to use '$'
+     * from the global scope you will still get the pages one.
+     * Additionally the userscript expects `window` to actually look
+     * like a fully featured browser window object.
+     *
+     * So let's try to use a Proxy on window and the possibly deprecated
+     * `with` function to make that proxy shadow the global scope.
+     * unsafeWindow should still be the actual global page window.
+     */
     const unsafeWindow = window;
+    let myWindow = {};
+    var windowProxyHandler = {
+      get: function(obj, prop) {
+        if (prop in myWindow)
+          return myWindow[prop];
+        if (prop in obj) {
+          if (typeof obj[prop] === 'function' && typeof obj[prop].prototype == 'undefined')
+            // Getting TypeError: Illegal Execution when callers try to execute
+            // eg addEventListener from here because they were returned
+            // unbound
+            return obj[prop].bind(obj);
+          return obj[prop];
+        }
+      },
+      set: function(target, prop, val) {
+        return myWindow[prop] = val;
+      }
+    };
+    var myProxy = new Proxy(unsafeWindow, windowProxyHandler);
 
     // ====== The actual user script source ====== //
+    with (myProxy) {
+      // can't assign window directly in with() scope because proxy doesn't
+      // allow assinging to things that a readonly on the target.
+      function blarg() {  // why can't this be anonymous?
+      var window = myProxy;
 {{ scriptSource }}
+      };
+      blarg();
+    };
     // ====== End User Script ====== //
 })();

From 23bfe6daa2bfb960397a1e0d6252382c0c1730eb Mon Sep 17 00:00:00 2001
From: Jimmy <jimmy@spalge.com>
Date: Sun, 1 Apr 2018 15:09:56 +1200
Subject: [PATCH 02/10] Greasemonkey: fix window proxy for new attributes

Previously scripts were failing to find attributes that they assigned to
window and then tried to use from the global scope. Eg

    window.newthing = function() {...};
    newthing(...);  // newthing is not defined error

This wasn't the case for things that already existed in the global scope
and were just being overwritten.

This change just overrides the `Proxy.has()` function which seems to fix
it. Probably the `while` implementation was failing to pick up new
attributes because of the lack.

I also tweaked some comments and variable names and const-ness to be a
little more production ready.
---
 .../javascript/greasemonkey_wrapper.js        | 39 ++++++++++++-------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/qutebrowser/javascript/greasemonkey_wrapper.js b/qutebrowser/javascript/greasemonkey_wrapper.js
index e9cdc9f1a..88024ff02 100644
--- a/qutebrowser/javascript/greasemonkey_wrapper.js
+++ b/qutebrowser/javascript/greasemonkey_wrapper.js
@@ -161,13 +161,16 @@
      * So let's try to use a Proxy on window and the possibly deprecated
      * `with` function to make that proxy shadow the global scope.
      * unsafeWindow should still be the actual global page window.
+     *
+     * There are other Proxy functions that we may need to override.
+     * set, get and has are definitely required.
      */
     const unsafeWindow = window;
-    let myWindow = {};
-    var windowProxyHandler = {
+    const qute_gm_window_shadow = {};  // stores local changes to window
+    const qute_gm_windowProxyHandler = {
       get: function(obj, prop) {
-        if (prop in myWindow)
-          return myWindow[prop];
+        if (prop in qute_gm_window_shadow)
+          return qute_gm_window_shadow[prop];
         if (prop in obj) {
           if (typeof obj[prop] === 'function' && typeof obj[prop].prototype == 'undefined')
             // Getting TypeError: Illegal Execution when callers try to execute
@@ -178,20 +181,28 @@
         }
       },
       set: function(target, prop, val) {
-        return myWindow[prop] = val;
+        return qute_gm_window_shadow[prop] = val;
+      },
+      has: function(target, key) {
+        return key in qute_gm_window_shadow || key in target;
       }
     };
-    var myProxy = new Proxy(unsafeWindow, windowProxyHandler);
+    const qute_gm_window_proxy = new Proxy(unsafeWindow, qute_gm_windowProxyHandler);
 
-    // ====== The actual user script source ====== //
-    with (myProxy) {
-      // can't assign window directly in with() scope because proxy doesn't
-      // allow assinging to things that a readonly on the target.
-      function blarg() {  // why can't this be anonymous?
-      var window = myProxy;
+    with (qute_gm_window_proxy) {
+      // We can't return `this` or `qute_gm_window_proxy` from
+      // `qute_gm_window_proxy.get('window')` because the Proxy implementation
+      // does typechecking on read-only things. So we have to shadow `window`
+      // more conventionally here. Except we can't do it directly within
+      // with `with` scope because then it would get assigned to the
+      // proxy and we would get the same problem, so we have to make yet
+      // another nested scope.
+      function qute_gm_window_scope() {  // why can't this be anonymous?
+        let window = qute_gm_window_proxy;
+        // ====== The actual user script source ====== //
 {{ scriptSource }}
+        // ====== End User Script ====== //
       };
-      blarg();
+      qute_gm_window_scope();
     };
-    // ====== End User Script ====== //
 })();

From c7a9792b67883fd560386868f7762714110d9f89 Mon Sep 17 00:00:00 2001
From: Jimmy <jimmy@spalge.com>
Date: Sun, 22 Apr 2018 17:51:50 +1200
Subject: [PATCH 03/10] Greasemonkey: Add test for window scoping refinements.

Adds a test to codify what I think greasemonkey scripts expect from
their scope chains. Particularly that they can:

1. access the global `window` object
2. access all of the attributes of the global window object as global
   objects themselves
3. see any changes the page made to the global scope
4. write to attributes of `window` and have those attributes, and changes
   to existing attributes, accessable via global scope
5. do number 4 without breaking the pages expectations, that is what
   `unsafeWindow` is for

There are some other points about greasemonkey scripts' environment that
I believe to be true but am not testing in this change:

* changes a page makes to `window` _after_ a greasemonkey script is
  injected will still be visible to the script if it cares to check and
  it hasn't already shadowed them
* said changes will not overwrite changes that the greasemonkey script
  has made.
---
 tests/unit/javascript/test_greasemonkey.py | 59 ++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/tests/unit/javascript/test_greasemonkey.py b/tests/unit/javascript/test_greasemonkey.py
index 8701e8170..7494c5c13 100644
--- a/tests/unit/javascript/test_greasemonkey.py
+++ b/tests/unit/javascript/test_greasemonkey.py
@@ -158,3 +158,62 @@ def test_required_scripts_are_included(download_stub, tmpdir):
     # Additionally check that the base script is still being parsed correctly
     assert "Script is running." in scripts[0].code()
     assert scripts[0].excludes
+
+
+class TestWindowIsolation:
+    """Check that greasemonkey scripts get a shadowed global scope."""
+
+    @classmethod
+    def setup_class(cls):
+        # Change something in the global scope
+        cls.setup_script = "window.$ = 'global'"
+
+        # Greasemonkey script to report back on its scope.
+        test_script = greasemonkey.GreasemonkeyScript.parse(
+            textwrap.dedent("""
+                // ==UserScript==
+                // @name scopetest
+                // ==/UserScript==
+                // Check the thing the page set is set to the expected type
+                result.push(window.$);
+                result.push($);
+                // Now overwrite it
+                window.$ = 'shadowed';
+                // And check everything is how the script would expect it to be
+                // after just writing to the "global" scope
+                result.push(window.$);
+                result.push($);
+            """)
+        )
+
+        # The compiled source of that scripts with some additional setup
+        # bookending it.
+        cls.test_script = "\n".join([
+            "var result = [];",
+            test_script.code(),
+            """
+            // Now check that the actual global scope has
+            // not been overwritten
+            result.push(window.$);
+            result.push($);
+            // And return our findings
+            result;"""
+        ])
+
+        # What we expect the script to report back.
+        cls.expected = [
+            "global", "global",
+            "shadowed", "shadowed",
+            "global", "global"]
+
+    def test_webengine(self, callback_checker, webengineview):
+        page = webengineview.page()
+        page.runJavaScript(self.setup_script)
+        page.runJavaScript(self.test_script, callback_checker.callback)
+        callback_checker.check(self.expected)
+
+    def test_webkit(self, webview):
+        elem = webview.page().mainFrame().documentElement()
+        elem.evaluateJavaScript(self.setup_script)
+        result = elem.evaluateJavaScript(self.test_script)
+        assert result == self.expected

From 19242b6cb2848dffa5eda979fc285f8b3a3af89e Mon Sep 17 00:00:00 2001
From: Jimmy <jimmy@spalge.com>
Date: Wed, 25 Apr 2018 16:51:35 +1200
Subject: [PATCH 04/10] Greasemonkey: fix scoping function decleration.

Apparently making a function like `function foo() {};` in block scope is
illegal. It should be named via assignment.

Switched to an IIFE anyway because it doesn't need a name.
---
 qutebrowser/javascript/greasemonkey_wrapper.js | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/qutebrowser/javascript/greasemonkey_wrapper.js b/qutebrowser/javascript/greasemonkey_wrapper.js
index 88024ff02..7f348dbcc 100644
--- a/qutebrowser/javascript/greasemonkey_wrapper.js
+++ b/qutebrowser/javascript/greasemonkey_wrapper.js
@@ -197,12 +197,11 @@
       // with `with` scope because then it would get assigned to the
       // proxy and we would get the same problem, so we have to make yet
       // another nested scope.
-      function qute_gm_window_scope() {  // why can't this be anonymous?
+      (function() {
         let window = qute_gm_window_proxy;
         // ====== The actual user script source ====== //
 {{ scriptSource }}
         // ====== End User Script ====== //
-      };
-      qute_gm_window_scope();
+      })();
     };
 })();

From 13249329f70830b25a4ee736f78f7b304deaf010 Mon Sep 17 00:00:00 2001
From: Jimmy <jimmy@spalge.com>
Date: Wed, 25 Apr 2018 16:26:43 +1200
Subject: [PATCH 05/10] Greasemonkey: skip window scoping test on webkit

The implementation of Proxy in JSCore used by current QtWebkit (webkit
602.1) doesn't support the `set()` handler for whatever reason. So
instead of testing for a specific behaviour that we can't ensure on that
version let's just skip the tests and handle user complaints with
sympathy.
---
 pytest.ini                                    |  1 +
 .../javascript/greasemonkey_wrapper.js        | 29 ++++++++-----------
 tests/conftest.py                             |  6 +++-
 tests/unit/javascript/test_greasemonkey.py    |  2 ++
 4 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/pytest.ini b/pytest.ini
index 1a3f625e9..c897f0be7 100644
--- a/pytest.ini
+++ b/pytest.ini
@@ -30,6 +30,7 @@ markers =
     qtbug60673: Tests which are broken if the conversion from orange selection to real selection  is flaky
     fake_os: Fake utils.is_* to a fake operating system
     unicode_locale: Tests which need an unicode locale to work
+    qtwebkit6021_skip: Tests which would fail on WebKit version 602.1
 qt_log_level_fail = WARNING
 qt_log_ignore =
     ^SpellCheck: .*
diff --git a/qutebrowser/javascript/greasemonkey_wrapper.js b/qutebrowser/javascript/greasemonkey_wrapper.js
index 7f348dbcc..b38aaeec9 100644
--- a/qutebrowser/javascript/greasemonkey_wrapper.js
+++ b/qutebrowser/javascript/greasemonkey_wrapper.js
@@ -145,22 +145,16 @@
         }
     };
 
-    /* TamperMonkey allows assinging to `window` to change the visible global
-     * scope, without breaking other scripts. Eg if the page has set
-     * window.$ for an object for some reason (hello 4chan)
-     * - typeof $ === 'object'
-     * - typeof window.$ == 'undefined'
-     * - window.$ = function() {}
-     * - typeof $ === 'function'
-     * - typeof window.$ == 'function'
-     * Just shadowing `window` won't work because if you try to use '$'
-     * from the global scope you will still get the pages one.
-     * Additionally the userscript expects `window` to actually look
-     * like a fully featured browser window object.
-     *
-     * So let's try to use a Proxy on window and the possibly deprecated
-     * `with` function to make that proxy shadow the global scope.
-     * unsafeWindow should still be the actual global page window.
+    /* 
+     * Try to give userscripts an environment that they expect. Which
+     * seems to be that the global window object should look the same as
+     * the page's one and that if a script writes to an attribute of
+     * window it should be able to access that variable in the global
+     * scope.
+     * Use a Proxy to stop scripts from actually changing the global
+     * window (that's what unsafeWindow is for).
+     * Use the "with" statement to make the proxy provide what looks
+     * like global scope.
      *
      * There are other Proxy functions that we may need to override.
      * set, get and has are definitely required.
@@ -187,7 +181,8 @@
         return key in qute_gm_window_shadow || key in target;
       }
     };
-    const qute_gm_window_proxy = new Proxy(unsafeWindow, qute_gm_windowProxyHandler);
+    const qute_gm_window_proxy = new Proxy(
+      unsafeWindow, qute_gm_windowProxyHandler);
 
     with (qute_gm_window_proxy) {
       // We can't return `this` or `qute_gm_window_proxy` from
diff --git a/tests/conftest.py b/tests/conftest.py
index 0b82bc7f6..97d562337 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -36,7 +36,7 @@ from helpers import logfail
 from helpers.logfail import fail_on_logging
 from helpers.messagemock import message_mock
 from helpers.fixtures import *  # noqa: F403
-from qutebrowser.utils import qtutils, standarddir, usertypes, utils
+from qutebrowser.utils import qtutils, standarddir, usertypes, utils, version
 from qutebrowser.misc import objects
 
 import qutebrowser.app  # To register commands
@@ -77,6 +77,10 @@ def _apply_platform_markers(config, item):
          "https://bugreports.qt.io/browse/QTBUG-60673"),
         ('unicode_locale', sys.getfilesystemencoding() == 'ascii',
          "Skipped because of ASCII locale"),
+        ('qtwebkit6021_skip',
+         version.qWebKitVersion and
+         version.qWebKitVersion() == '602.1',
+         "Broken on WebKit 602.1")
     ]
 
     for searched_marker, condition, default_reason in markers:
diff --git a/tests/unit/javascript/test_greasemonkey.py b/tests/unit/javascript/test_greasemonkey.py
index 7494c5c13..8f780c432 100644
--- a/tests/unit/javascript/test_greasemonkey.py
+++ b/tests/unit/javascript/test_greasemonkey.py
@@ -212,6 +212,8 @@ class TestWindowIsolation:
         page.runJavaScript(self.test_script, callback_checker.callback)
         callback_checker.check(self.expected)
 
+    # The JSCore in 602.1 doesn't fully support Proxy.
+    @pytest.mark.qtwebkit6021_skip
     def test_webkit(self, webview):
         elem = webview.page().mainFrame().documentElement()
         elem.evaluateJavaScript(self.setup_script)

From b0d1a137dae4099a79a5779a02bbb61875298b2f Mon Sep 17 00:00:00 2001
From: Jimmy <jimmy@spalge.com>
Date: Wed, 25 Apr 2018 16:54:54 +1200
Subject: [PATCH 06/10] Greasemonkey: Don't attempt scope isolation on webkit

Since the JSCore used by WebKit 602.1 doesn't fully support Proxy and I
can't think of a way to provide isolation otherwise just revert to the
old behaviour in that case. I am checking for the specific WebKit
version because I'm pretty sure that version just happened to be
released when Proxy support was only partially done, any later release
will presumably have a newer JSCore where it works.

There I changed the indentation of a block in the jinja template which
will have inflated the diff.

I added mocking of `objects.backend` to the `webview` and
`webenginewebview` fixtures, I am pretty sure they are mutually
exclusive so don't expect any issues from that.

Because of the feature detection being at template compile time I had to
tweak the test setup to be done via a fixture instead of the setupClass
functionality that I was using before.
---
 qutebrowser/browser/greasemonkey.py           |  10 +-
 .../javascript/greasemonkey_wrapper.js        | 108 +++++++++---------
 tests/helpers/fixtures.py                     |  10 +-
 tests/unit/javascript/test_greasemonkey.py    |  31 ++---
 tests/unit/javascript/test_js_execution.py    |   4 +-
 5 files changed, 91 insertions(+), 72 deletions(-)

diff --git a/qutebrowser/browser/greasemonkey.py b/qutebrowser/browser/greasemonkey.py
index 578830793..db8246bab 100644
--- a/qutebrowser/browser/greasemonkey.py
+++ b/qutebrowser/browser/greasemonkey.py
@@ -31,9 +31,10 @@ import attr
 from PyQt5.QtCore import pyqtSignal, QObject, QUrl
 
 from qutebrowser.utils import (log, standarddir, jinja, objreg, utils,
-                               javascript, urlmatch)
+                               javascript, urlmatch, version, usertypes)
 from qutebrowser.commands import cmdutils
 from qutebrowser.browser import downloads
+from qutebrowser.misc import objects
 
 
 def _scripts_dir():
@@ -108,13 +109,18 @@ class GreasemonkeyScript:
         browser's debugger/inspector will not match up to the line
         numbers in the source script directly.
         """
+        # Don't use Proxy on this webkit version, the support isn't there.
+        use_proxy = not (
+            objects.backend == usertypes.Backend.QtWebKit and
+            version.qWebKitVersion() == '602.1')
         template = jinja.js_environment.get_template('greasemonkey_wrapper.js')
         return template.render(
             scriptName=javascript.string_escape(
                 "/".join([self.namespace or '', self.name])),
             scriptInfo=self._meta_json(),
             scriptMeta=javascript.string_escape(self.script_meta),
-            scriptSource=self._code)
+            scriptSource=self._code,
+            use_proxy=use_proxy)
 
     def _meta_json(self):
         return json.dumps({
diff --git a/qutebrowser/javascript/greasemonkey_wrapper.js b/qutebrowser/javascript/greasemonkey_wrapper.js
index b38aaeec9..546de3c8a 100644
--- a/qutebrowser/javascript/greasemonkey_wrapper.js
+++ b/qutebrowser/javascript/greasemonkey_wrapper.js
@@ -145,58 +145,64 @@
         }
     };
 
-    /* 
-     * Try to give userscripts an environment that they expect. Which
-     * seems to be that the global window object should look the same as
-     * the page's one and that if a script writes to an attribute of
-     * window it should be able to access that variable in the global
-     * scope.
-     * Use a Proxy to stop scripts from actually changing the global
-     * window (that's what unsafeWindow is for).
-     * Use the "with" statement to make the proxy provide what looks
-     * like global scope.
-     *
-     * There are other Proxy functions that we may need to override.
-     * set, get and has are definitely required.
-     */
-    const unsafeWindow = window;
-    const qute_gm_window_shadow = {};  // stores local changes to window
-    const qute_gm_windowProxyHandler = {
-      get: function(obj, prop) {
-        if (prop in qute_gm_window_shadow)
-          return qute_gm_window_shadow[prop];
-        if (prop in obj) {
-          if (typeof obj[prop] === 'function' && typeof obj[prop].prototype == 'undefined')
-            // Getting TypeError: Illegal Execution when callers try to execute
-            // eg addEventListener from here because they were returned
-            // unbound
-            return obj[prop].bind(obj);
-          return obj[prop];
+    {% if use_proxy %}
+      /*
+       * Try to give userscripts an environment that they expect. Which
+       * seems to be that the global window object should look the same as
+       * the page's one and that if a script writes to an attribute of
+       * window it should be able to access that variable in the global
+       * scope.
+       * Use a Proxy to stop scripts from actually changing the global
+       * window (that's what unsafeWindow is for).
+       * Use the "with" statement to make the proxy provide what looks
+       * like global scope.
+       *
+       * There are other Proxy functions that we may need to override.
+       * set, get and has are definitely required.
+       */
+      const unsafeWindow = window;
+      const qute_gm_window_shadow = {};  // stores local changes to window
+      const qute_gm_windowProxyHandler = {
+        get: function(obj, prop) {
+          if (prop in qute_gm_window_shadow)
+            return qute_gm_window_shadow[prop];
+          if (prop in obj) {
+            if (typeof obj[prop] === 'function' && typeof obj[prop].prototype == 'undefined')
+              // Getting TypeError: Illegal Execution when callers try to execute
+              // eg addEventListener from here because they were returned
+              // unbound
+              return obj[prop].bind(obj);
+            return obj[prop];
+          }
+        },
+        set: function(target, prop, val) {
+          return qute_gm_window_shadow[prop] = val;
+        },
+        has: function(target, key) {
+          return key in qute_gm_window_shadow || key in target;
         }
-      },
-      set: function(target, prop, val) {
-        return qute_gm_window_shadow[prop] = val;
-      },
-      has: function(target, key) {
-        return key in qute_gm_window_shadow || key in target;
-      }
-    };
-    const qute_gm_window_proxy = new Proxy(
-      unsafeWindow, qute_gm_windowProxyHandler);
+      };
+      const qute_gm_window_proxy = new Proxy(
+        unsafeWindow, qute_gm_windowProxyHandler);
 
-    with (qute_gm_window_proxy) {
-      // We can't return `this` or `qute_gm_window_proxy` from
-      // `qute_gm_window_proxy.get('window')` because the Proxy implementation
-      // does typechecking on read-only things. So we have to shadow `window`
-      // more conventionally here. Except we can't do it directly within
-      // with `with` scope because then it would get assigned to the
-      // proxy and we would get the same problem, so we have to make yet
-      // another nested scope.
-      (function() {
-        let window = qute_gm_window_proxy;
-        // ====== The actual user script source ====== //
+      with (qute_gm_window_proxy) {
+        // We can't return `this` or `qute_gm_window_proxy` from
+        // `qute_gm_window_proxy.get('window')` because the Proxy implementation
+        // does typechecking on read-only things. So we have to shadow `window`
+        // more conventionally here. Except we can't do it directly within
+        // with `with` scope because then it would get assigned to the
+        // proxy and we would get the same problem, so we have to make yet
+        // another nested scope.
+        (function() {
+          let window = qute_gm_window_proxy;
+          // ====== The actual user script source ====== //
 {{ scriptSource }}
-        // ====== End User Script ====== //
-      })();
-    };
+          // ====== End User Script ====== //
+        })();
+      };
+    {% else %}
+      // ====== The actual user script source ====== //
+{{ scriptSource }}
+      // ====== End User Script ====== //
+    {% endif %}
 })();
diff --git a/tests/helpers/fixtures.py b/tests/helpers/fixtures.py
index c4756e1f5..cfaba7a55 100644
--- a/tests/helpers/fixtures.py
+++ b/tests/helpers/fixtures.py
@@ -43,10 +43,10 @@ import helpers.stubs as stubsmod
 import helpers.utils
 from qutebrowser.config import (config, configdata, configtypes, configexc,
                                 configfiles)
-from qutebrowser.utils import objreg, standarddir, utils
+from qutebrowser.utils import objreg, standarddir, utils, usertypes
 from qutebrowser.browser import greasemonkey
 from qutebrowser.browser.webkit import cookies
-from qutebrowser.misc import savemanager, sql
+from qutebrowser.misc import savemanager, sql, objects
 from qutebrowser.keyinput import modeman
 
 
@@ -360,9 +360,10 @@ def qnam(qapp):
 
 
 @pytest.fixture
-def webengineview(qtbot):
+def webengineview(qtbot, monkeypatch):
     """Get a QWebEngineView if QtWebEngine is available."""
     QtWebEngineWidgets = pytest.importorskip('PyQt5.QtWebEngineWidgets')
+    monkeypatch.setattr(objects, 'backend', usertypes.Backend.QtWebEngine)
     view = QtWebEngineWidgets.QWebEngineView()
     qtbot.add_widget(view)
     return view
@@ -379,9 +380,10 @@ def webpage(qnam):
 
 
 @pytest.fixture
-def webview(qtbot, webpage):
+def webview(qtbot, webpage, monkeypatch):
     """Get a new QWebView object."""
     QtWebKitWidgets = pytest.importorskip('PyQt5.QtWebKitWidgets')
+    monkeypatch.setattr(objects, 'backend', usertypes.Backend.QtWebKit)
 
     view = QtWebKitWidgets.QWebView()
     qtbot.add_widget(view)
diff --git a/tests/unit/javascript/test_greasemonkey.py b/tests/unit/javascript/test_greasemonkey.py
index 8f780c432..80285ffca 100644
--- a/tests/unit/javascript/test_greasemonkey.py
+++ b/tests/unit/javascript/test_greasemonkey.py
@@ -163,10 +163,14 @@ def test_required_scripts_are_included(download_stub, tmpdir):
 class TestWindowIsolation:
     """Check that greasemonkey scripts get a shadowed global scope."""
 
-    @classmethod
-    def setup_class(cls):
+    @pytest.fixture
+    def setup(self):
+        class SetupData:
+            pass
+        ret = SetupData()
+
         # Change something in the global scope
-        cls.setup_script = "window.$ = 'global'"
+        ret.setup_script = "window.$ = 'global'"
 
         # Greasemonkey script to report back on its scope.
         test_script = greasemonkey.GreasemonkeyScript.parse(
@@ -188,7 +192,7 @@ class TestWindowIsolation:
 
         # The compiled source of that scripts with some additional setup
         # bookending it.
-        cls.test_script = "\n".join([
+        ret.test_script = "\n".join([
             "var result = [];",
             test_script.code(),
             """
@@ -201,21 +205,22 @@ class TestWindowIsolation:
         ])
 
         # What we expect the script to report back.
-        cls.expected = [
+        ret.expected = [
             "global", "global",
             "shadowed", "shadowed",
             "global", "global"]
+        return ret
 
-    def test_webengine(self, callback_checker, webengineview):
+    def test_webengine(self, callback_checker, webengineview, setup):
         page = webengineview.page()
-        page.runJavaScript(self.setup_script)
-        page.runJavaScript(self.test_script, callback_checker.callback)
-        callback_checker.check(self.expected)
+        page.runJavaScript(setup.setup_script)
+        page.runJavaScript(setup.test_script, callback_checker.callback)
+        callback_checker.check(setup.expected)
 
     # The JSCore in 602.1 doesn't fully support Proxy.
     @pytest.mark.qtwebkit6021_skip
-    def test_webkit(self, webview):
+    def test_webkit(self, webview, setup):
         elem = webview.page().mainFrame().documentElement()
-        elem.evaluateJavaScript(self.setup_script)
-        result = elem.evaluateJavaScript(self.test_script)
-        assert result == self.expected
+        elem.evaluateJavaScript(setup.setup_script)
+        result = elem.evaluateJavaScript(setup.test_script)
+        assert result == setup.expected
diff --git a/tests/unit/javascript/test_js_execution.py b/tests/unit/javascript/test_js_execution.py
index c5b5018d2..81f999962 100644
--- a/tests/unit/javascript/test_js_execution.py
+++ b/tests/unit/javascript/test_js_execution.py
@@ -26,7 +26,7 @@ import pytest
 @pytest.mark.parametrize('js_enabled, expected', [(True, 2.0), (False, None)])
 def test_simple_js_webkit(webview, js_enabled, expected):
     """With QtWebKit, evaluateJavaScript works when JS is on."""
-    # If we get there (because of the webengineview fixture) we can be certain
+    # If we get there (because of the webview fixture) we can be certain
     # QtWebKit is available
     from PyQt5.QtWebKit import QWebSettings
     webview.settings().setAttribute(QWebSettings.JavascriptEnabled, js_enabled)
@@ -37,7 +37,7 @@ def test_simple_js_webkit(webview, js_enabled, expected):
 @pytest.mark.parametrize('js_enabled, expected', [(True, 2.0), (False, 2.0)])
 def test_element_js_webkit(webview, js_enabled, expected):
     """With QtWebKit, evaluateJavaScript on an element works with JS off."""
-    # If we get there (because of the webengineview fixture) we can be certain
+    # If we get there (because of the webview fixture) we can be certain
     # QtWebKit is available
     from PyQt5.QtWebKit import QWebSettings
     webview.settings().setAttribute(QWebSettings.JavascriptEnabled, js_enabled)

From 6573a4d61685a13a3906289d64f795d26038b212 Mon Sep 17 00:00:00 2001
From: Jimmy <jimmy@spalge.com>
Date: Sun, 6 May 2018 19:25:18 +1200
Subject: [PATCH 07/10] Tell pylint to shut its fat mouth.

I just want to return something I can refer to the attributes of via dot
syntax without having to pointlessly write the names both when I declare
the data class and when I assign the variables.

Such a stupid warning.
---
 tests/unit/javascript/test_greasemonkey.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/unit/javascript/test_greasemonkey.py b/tests/unit/javascript/test_greasemonkey.py
index 80285ffca..f3d29608c 100644
--- a/tests/unit/javascript/test_greasemonkey.py
+++ b/tests/unit/javascript/test_greasemonkey.py
@@ -165,6 +165,7 @@ class TestWindowIsolation:
 
     @pytest.fixture
     def setup(self):
+        # pylint: disable=attribute-defined-outside-init
         class SetupData:
             pass
         ret = SetupData()

From d162e01422393b5a5825ec1219efefcb7862877f Mon Sep 17 00:00:00 2001
From: Jimmy <jimmy@spalge.com>
Date: Sun, 20 May 2018 18:13:40 +1200
Subject: [PATCH 08/10] Greasemonkey: remove extra window scope IIFE

Also change the block scoped `window` declaration to be const because it
seemed like a good idea. The real window seems to just silently ignore
attempts to overwrite it.
---
 qutebrowser/javascript/greasemonkey_wrapper.js | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/qutebrowser/javascript/greasemonkey_wrapper.js b/qutebrowser/javascript/greasemonkey_wrapper.js
index 546de3c8a..aa8e5d166 100644
--- a/qutebrowser/javascript/greasemonkey_wrapper.js
+++ b/qutebrowser/javascript/greasemonkey_wrapper.js
@@ -189,16 +189,11 @@
         // We can't return `this` or `qute_gm_window_proxy` from
         // `qute_gm_window_proxy.get('window')` because the Proxy implementation
         // does typechecking on read-only things. So we have to shadow `window`
-        // more conventionally here. Except we can't do it directly within
-        // with `with` scope because then it would get assigned to the
-        // proxy and we would get the same problem, so we have to make yet
-        // another nested scope.
-        (function() {
-          let window = qute_gm_window_proxy;
-          // ====== The actual user script source ====== //
+        // more conventionally here.
+        const window = qute_gm_window_proxy;
+        // ====== The actual user script source ====== //
 {{ scriptSource }}
-          // ====== End User Script ====== //
-        })();
+        // ====== End User Script ====== //
       };
     {% else %}
       // ====== The actual user script source ====== //

From 749b29e599afd7385bc3db13939c9b383c2c965d Mon Sep 17 00:00:00 2001
From: Jimmy <jimmy@spalge.com>
Date: Sun, 20 May 2018 18:16:14 +1200
Subject: [PATCH 09/10] tests: Don't pretend to be using webkit if unavailable

Since `objects.backend` was being set to usertypes.Backend.QtWebKit by
default some feature detection code was calling
`version.qWebKitVersion()` which was failing because the import of
`PyQt5.QtWebKit` was failing in version.

This should not change behavior where both backends are available on if
any tests fail because they are expecting their environment to say they
are on webkit when they either aren't actually using any webkit features
or all mocked webkit features then they should probably be changed to
patch `objects.backend` or not depend on it.
---
 tests/conftest.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/conftest.py b/tests/conftest.py
index 97d562337..5043abb4d 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -223,8 +223,10 @@ def check_display(request):
 @pytest.fixture(autouse=True)
 def set_backend(monkeypatch, request):
     """Make sure the backend global is set."""
-    backend = (usertypes.Backend.QtWebEngine if request.config.webengine
-               else usertypes.Backend.QtWebKit)
+    if not request.config.webengine and version.qWebKitVersion:
+        backend = usertypes.Backend.QtWebKit
+    else:
+        backend = usertypes.Backend.QtWebEngine
     monkeypatch.setattr(objects, 'backend', backend)
 
 

From 47446baf294598d8e3a2faa93a627ae0ac4fb66c Mon Sep 17 00:00:00 2001
From: Jimmy <jimmy@spalge.com>
Date: Mon, 21 May 2018 21:23:31 +1200
Subject: [PATCH 10/10] s/obj/target/

Would a bit of consistency in variable names be too much to ask?
---
 qutebrowser/javascript/greasemonkey_wrapper.js | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/qutebrowser/javascript/greasemonkey_wrapper.js b/qutebrowser/javascript/greasemonkey_wrapper.js
index aa8e5d166..457118696 100644
--- a/qutebrowser/javascript/greasemonkey_wrapper.js
+++ b/qutebrowser/javascript/greasemonkey_wrapper.js
@@ -163,16 +163,16 @@
       const unsafeWindow = window;
       const qute_gm_window_shadow = {};  // stores local changes to window
       const qute_gm_windowProxyHandler = {
-        get: function(obj, prop) {
+        get: function(target, prop) {
           if (prop in qute_gm_window_shadow)
             return qute_gm_window_shadow[prop];
-          if (prop in obj) {
-            if (typeof obj[prop] === 'function' && typeof obj[prop].prototype == 'undefined')
+          if (prop in target) {
+            if (typeof target[prop] === 'function' && typeof target[prop].prototype == 'undefined')
               // Getting TypeError: Illegal Execution when callers try to execute
               // eg addEventListener from here because they were returned
               // unbound
-              return obj[prop].bind(obj);
-            return obj[prop];
+              return target[prop].bind(target);
+            return target[prop];
           }
         },
         set: function(target, prop, val) {