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

@@ -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({

qutebrowser/javascript/greasemonkey_wrapper.js

@@ -145,58 +145,64 @@
         }
     };
 
-    /* 
+    {% 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
+       * Try to give userscripts an environment that they expect. Which
-     * the page's one and that if a script writes to an attribute of
+       * seems to be that the global window object should look the same as
-     * window it should be able to access that variable in the global
+       * the page's one and that if a script writes to an attribute of
-     * scope.
+       * window it should be able to access that variable in the global
-     * Use a Proxy to stop scripts from actually changing the global
+       * scope.
-     * window (that's what unsafeWindow is for).
+       * Use a Proxy to stop scripts from actually changing the global
-     * Use the "with" statement to make the proxy provide what looks
+       * window (that's what unsafeWindow is for).
-     * like global scope.
+       * 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.
+       * 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 unsafeWindow = window;
-    const qute_gm_windowProxyHandler = {
+      const qute_gm_window_shadow = {};  // stores local changes to window
-      get: function(obj, prop) {
+      const qute_gm_windowProxyHandler = {
-        if (prop in qute_gm_window_shadow)
+        get: function(obj, prop) {
-          return qute_gm_window_shadow[prop];
+          if (prop in qute_gm_window_shadow)
-        if (prop in obj) {
+            return qute_gm_window_shadow[prop];
-          if (typeof obj[prop] === 'function' && typeof obj[prop].prototype == 'undefined')
+          if (prop in obj) {
-            // Getting TypeError: Illegal Execution when callers try to execute
+            if (typeof obj[prop] === 'function' && typeof obj[prop].prototype == 'undefined')
-            // eg addEventListener from here because they were returned
+              // Getting TypeError: Illegal Execution when callers try to execute
-            // unbound
+              // eg addEventListener from here because they were returned
-            return obj[prop].bind(obj);
+              // unbound
-          return obj[prop];
+              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) {
+      const qute_gm_window_proxy = new Proxy(
-        return qute_gm_window_shadow[prop] = val;
+        unsafeWindow, qute_gm_windowProxyHandler);
-      },
-      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);
 
-    with (qute_gm_window_proxy) {
+      with (qute_gm_window_proxy) {
-      // We can't return `this` or `qute_gm_window_proxy` from
+        // We can't return `this` or `qute_gm_window_proxy` from
-      // `qute_gm_window_proxy.get('window')` because the Proxy implementation
+        // `qute_gm_window_proxy.get('window')` because the Proxy implementation
-      // does typechecking on read-only things. So we have to shadow `window`
+        // does typechecking on read-only things. So we have to shadow `window`
-      // more conventionally here. Except we can't do it directly within
+        // more conventionally here. Except we can't do it directly within
-      // with `with` scope because then it would get assigned to the
+        // 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
+        // proxy and we would get the same problem, so we have to make yet
-      // another nested scope.
+        // another nested scope.
-      (function() {
+        (function() {
-        let window = qute_gm_window_proxy;
+          let window = qute_gm_window_proxy;
-        // ====== The actual user script source ====== //
+          // ====== The actual user script source ====== //
 {{ scriptSource }}
-        // ====== End User Script ====== //
+          // ====== End User Script ====== //
-      })();
+        })();
-    };
+      };
+    {% else %}
+      // ====== The actual user script source ====== //
+{{ scriptSource }}
+      // ====== End User Script ====== //
+    {% endif %}
 })();

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)

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
+    @pytest.fixture
-    def setup_class(cls):
+    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(setup.setup_script)
-        page.runJavaScript(self.test_script, callback_checker.callback)
+        page.runJavaScript(setup.test_script, callback_checker.callback)
-        callback_checker.check(self.expected)
+        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)
+        elem.evaluateJavaScript(setup.setup_script)
-        result = elem.evaluateJavaScript(self.test_script)
+        result = elem.evaluateJavaScript(setup.test_script)
-        assert result == self.expected
+        assert result == setup.expected

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)