mozilla / positron

a experimental, Electron-compatible runtime on top of Gecko
Other
562 stars 64 forks source link

page in <webview> that accesses process global triggers initialization #64

Open mykmelez opened 8 years ago

mykmelez commented 8 years ago

When the page https://github.com/ in a <webview> tries to access the process global, Positron tries to initialize it, even though the Process.webidl binding is marked [ChromeOnly], and the window's docshell is a content docshell. This eventually leads to an error when a renderer module tries to get a reference to the BrowserWindow for the page. We should figure out why this is happening and stop it, as no page loaded in a <webview> should trigger initialization of the process global.

mykmelez commented 8 years ago

Perhaps this is because chrome code can still access [ChromeOnly] globals in a content document. Still, if it's the content that is accessing the chrome-only global, I wouldn't expect Gecko to instantiate it.

In any case, I've landed https://github.com/mozilla/positron/pull/60/commits/62dc6fa7ee0658dc1afd1276bcb17860b2dfbe52 in #60 to work around the problem, but I'm leaving this open for the real fix.

mykmelez commented 8 years ago

The global also gets initialized for the hidden window, although that's less of a problem, since we can control what the hidden window references and ensure that it doesn't reference the global (whereas content windows are beyond our control, and at least https://github.com/ actually does reference it).

Currently, this isn't a problem, as we've given the BrowserWindow document the system principal, and we've moved the component to the JavaScript-global-privileged-property category, so it only gets initialized for chrome windows. But I'm looking into removing the hack that gives the BrowserWindow document the system principal, at which point we'll need to make the component a JavaScript-global-property again, and then this'll become an issue again.

Probably the correct fix is to restrict the WebIDL binding specifically to BrowserWindow documents using the Func extended attribute. We may have to reimplement the binding natively in that case, as I don't think Func is implementable in JS. But perhaps we can get away with implementing Func natively and the rest of the binding in JS.

mykmelez commented 7 years ago

This is a WIP hack to work around this issue. Unsure if it still works, as it's a few months old. (I had it lying around in my local clone and want to make sure I don't lose it.)

diff --git a/positron/components/Process.js b/positron/components/Process.js
index 14a5544..5d94b1d 100644
--- a/positron/components/Process.js
+++ b/positron/components/Process.js
@@ -35,16 +35,31 @@ Process.prototype = {
   /**
    * Initialize the global `process` property.  See the code comment
    * https://dxr.mozilla.org/mozilla-central/rev/21bf1af/toolkit/mozapps/extensions/amInstallTrigger.js#124-133
    * for an explanation of the behavior of this method.
    */
   init: function(window) {
     this._contentWindow = window;

+    // The binding appears to get initialized even for a document in a content
+    // docshell, even though it's marked [ChromeOnly].  Hack around that.
+    // TODO: https://github.com/mozilla/positron/issues/64
+    const docShellType = window.QueryInterface(Ci.nsIInterfaceRequestor).
+                                getInterface(Ci.nsIWebNavigation).
+                                QueryInterface(Ci.nsIDocShell).itemType;
+    if (docShellType !== Ci.nsIDocShellTreeItem.typeChrome) {
+      this._processGlobal = new Proxy({}, {
+        get: function(target, name) {
+          window.console.error("'process' not defined in hidden window");
+        }
+      });
+      return window.processImpl._create(window, this);
+    }
+
     // The WebIDL binding applies to the hidden window too, but we don't want
     // to initialize the Electron environment for that window, so return early
     // if we've been called to initialize `process` for that window.
     //
     // TODO: restrict the WebIDL binding to windows opened by the Electron app
     // using the Func extended attribute
     // <https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Func>.
     //