Closed Bug 1051017 Opened 7 years ago Closed 7 years ago

Make it harder to get CPOWs by accident


(Core :: DOM: Content Processes, defect)

Not set





(Reporter: billm, Assigned: billm)



(Keywords: addon-compat)


(3 files)

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.
Here are the JS and add-on shim changes needed for this bug.
Attachment #8470263 - Flags: review?(mconley)
Attached patch cpp-changesSplinter Review
This patch removes the special e10s support for window.content.
Attachment #8470264 - Flags: review?(mrbkap)
Comment on attachment 8470264 [details] [diff] [review]

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.
Attachment #8470264 - Flags: review?(mrbkap) → review+
Comment on attachment 8470263 [details] [diff] [review]

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.
Attachment #8470263 - Flags: review?(mconley) → review+
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.
Attachment #8474016 - Flags: review?(past)
Comment on attachment 8474016 [details] [diff] [review]

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

Looks good to me.
Attachment #8474016 - Flags: review?(past) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Keywords: addon-compat
Depends on: 1057481
Depends on: 1059032
Depends on: 1067042
Depends on: 1067527
You need to log in before you can comment on or make changes to this bug.