Skip to content
This repository has been archived by the owner on Jul 3, 2018. It is now read-only.

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

Open
mykmelez opened this issue Jun 1, 2016 · 3 comments
Open

Comments

@mykmelez
Copy link
Contributor

mykmelez commented Jun 1, 2016

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
Copy link
Contributor Author

mykmelez commented Jun 1, 2016

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 62dc6fa in #60 to work around the problem, but I'm leaving this open for the real fix.

@mykmelez
Copy link
Contributor Author

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
Copy link
Contributor Author

mykmelez commented Oct 4, 2016

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant