Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Description • 10 years ago

Right now the frontend transparently returns CPOWs on a few frequently used paths: window.content, browser.contentWindow, and browser.contentDocument. The intention here was to improve add-on compatibility. However, we're now ending up with Firefox frontend code that accidentally uses these paths in e10s. It's fine to use CPOWs in the frontend judiciously, but we definitely don't want to do so by accident.

This came up in bug 1050243. The original patches that made the developer tools work in e10s didn't use CPOWs at all. However, as the code has been refactored, CPOWs have crept in and now we can't open the developer tools without using CPOWs.

Now that we have add-on shims, there's no reason to expose CPOWs in this way anymore. We can go back to the old behavior where window.content and such return null in e10s. If frontend code specifically wants to use CPOWs, it can opt in by using a special name:

browser.contentWindow -> browser.contentWindowAsCPOW
browser.contentDocument -> browser.contentDocumentAsCPOW
window.content -> window.gBrowser.selectedBrowser.contentWindowAsCPOW

These patches will probably break a few things that work now, but it should be easy to fix this stuff by changing names in the short term. I'd like to fix the developer tools properly though.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 1 • 10 years ago

Here are the JS and add-on shim changes needed for this bug.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 2 • 10 years ago

This patch removes the special e10s support for window.content.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 3 • 10 years ago

Comment on attachment 8470264 [details] [diff] [review]
cpp-changes

Review of attachment 8470264 [details] [diff] [review]:
-----------------------------------------------------------------

I, for one, am a huge fan of this change.

::: docshell/base/nsIDocShellTreeOwner.idl
@@ -64,5 @@
>  	*/
>  	readonly attribute nsIDocShellTreeItem primaryContentShell;
>  
> -	[implicit_jscontext]
> -	readonly attribute jsval contentWindow;

Need to bump the IID here.

	Mike Conley (:mconley) (:⚙️)
	Comment 4 • 10 years ago

Comment on attachment 8470263 [details] [diff] [review]
disable-default-cpows

Review of attachment 8470263 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming a green try run, r=me with the following things fixed.

::: browser/base/content/browser.js
@@ +1100,5 @@
>      var isLoadingBlank = isBlankPageURL(uriToLoad);
>  
>      // This pageshow listener needs to be registered before we may call
>      // swapBrowsersAndCloseOther() to receive pageshow events fired by that.
> +    if (!gMultiProcessBrowser) {

We should update the documentation above this block to explain why gMultiProcessBrowser doesn't need this.

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm
@@ +625,5 @@
>    return remoteChromeGlobal.docShell;
>  };
>  
> +RemoteBrowserElementInterposition.getters.contentWindow = function(addon, target) {
> +  dump("content window CPOW\n");

We probably want to strip these dumps out.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 5 • 10 years ago

This patch fixes devtools so that they start up and appear to work without using CPOWs. The main issue is that contentWindow and contentDocument will now always return null for remote browsers, so I worked around this as best I could.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 6 • 10 years ago

Comment on attachment 8474016 [details] [diff] [review]
devtools-no-cpows

Review of attachment 8474016 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 7 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6bdd3272ccb
https://hg.mozilla.org/integration/mozilla-inbound/rev/252253bdd080

	Ed Morley [:emorley]
	Comment 8 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/d6bdd3272ccb
https://hg.mozilla.org/mozilla-central/rev/252253bdd080