656115 - Re-enable CPOWs on desktop

Status: RESOLVED WONTFIX

(Core :: IPC, defect)

Description

[Drew Willcoxon :adw]

Brendan suggests that add-ons and the front-end be able to access content DOM synchronously, as a crutch, while they are (or in lieu of) being rewritten for e10s.  CPOWs are such a crutch but they're disabled on desktop.  This bug experiments with re-enabling them.

Comment 1

[Drew Willcoxon :adw]

Attachment: WIP patch

This WIP patch reverts bug 564382 as best I could with the simple goal of getting it to compile, but it segfaults when I run the following in test-ipc.xul's "Eval script in chrome context" textbox:

document.getElementById("page").QueryInterface(Components.interfaces.nsIFrameLoaderOwner).crossProcessObjectWrapper

The segfault happens in nsFrameLoader::GetCrossProcessObjectWrapper when it dereferences mRemoteBrowser, which is null.  mRemoteBrowser is null because nsFrameLoader::DestroyChild is called when test-ipc.xul is starting up.  nsFrameLoader::DestroyChild is called because there's some IPC error.  (I can see AsyncChannel::OnNotifyMaybeChannelError and PContentParent::OnChannelError on the stack.)  I presume these errors that are printed to the terminal just before nsFrameLoader::DestroyChild is called are related:

###!!! ASSERTION: IPDL error:: 'Error', file PNeckoParent.cpp, line 640
###!!! ASSERTION: error deserializing (better message TODO): 'Error', file PNeckoParent.cpp, line 641
###!!! ASSERTION: [PNeckoParent] killing child side as a result: 'Error', file PNeckoParent.cpp, line 643
###!!! [Parent][AsyncChannel] Error: Value error: message was deserialized, but contained an illegal value
###!!! [Parent][AsyncChannel] Error: Route error: message sent to unknown actor ID
###!!! [Parent][AsyncChannel] Error: Channel error: cannot send/recv

Slightly different errors are printed without my patch:

###!!! [Parent][RPCChannel] Error: Channel error: cannot send/recv
###!!! [Parent][AsyncChannel] Error: Channel error: cannot send/recv

Comment 2

[Josh Matthews [:jdm]]

test-ipc.xul is known broken at the moment due to bug 644325. The patch there should be enough to get it in a non-crashing state.

Comment 3

[Drew Willcoxon :adw]

D'oh, the IPC errors I saw were due to my not rebuilding netwerk.  Necko was receiving messages it couldn't read.

So now I'm able to start test-ipc.xul and access the xul:browser's CPOW successfully.  Now what?  Modify the contentWindow getter of XUL elements, particularly xul:browser, to return the CPOW?

(In reply to comment #2)
> test-ipc.xul is known broken at the moment due to bug 644325. The patch
> there should be enough to get it in a non-crashing state.

Great, thanks Josh.  This fixed the compartment errors I was seeing.

Comment 4

[Drew Willcoxon :adw]

Attachment: WIP patch 2

This patch cleans up my earlier patch.  It also updates xul:browser's contentWindow getter to return its CPOW.

It seems to work... I'm not sure what to do next.

I had to rewrite most of TabParent::ReceiveMessage because js_NewArrayObjectWithCapacity apparently doesn't exist anymore.  I found JS_NewArrayObject and looked around mxr for how to use it.  I'm not sure whether the js::AutoArrayRooter is needed.  But a breakpoint on TabParent::ReceiveMessage isn't tripped in my testing (starting test-ipc.xul and accessing the xul:browser's contentWindow using the chrome script textbox), so it might be totally wrong.

Comment 5

[Drew Willcoxon :adw]

Attachment: WIP patch 3

This is re-based on mozilla-central.  (The previous patches were based on the e10s branch.)

Comment 6

[Josh Matthews [:jdm]]

The way to test that ReceiveMessage still works would be to click the various buttons at the top of test-ipc.xul.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #4)
> Created attachment 532101 [details] [diff] [review] [review]
> WIP patch 2
> 
> This patch cleans up my earlier patch.  It also updates xul:browser's
> contentWindow getter to return its CPOW.
> 
> It seems to work... I'm not sure what to do next.

I would talk with Felipe about bug 583976, get that bug and this playing well together, and run mochichrome or something with content processes forced on to get an initial idea of what's broken (assuming FF can start up ;).

Comment 8

[Drew Willcoxon :adw]

I ran into a potential problem.  The front-end uses content windows in basically two ways: 1) to access content, 2) as an XPCOM component through which docshells, nsIWebNavigation, nsIDOMWindowUtils, etc. are accessed.  Obviously CPOWs address the first use but not the second, so simply changing get_contentWindow to return a CPOW isn't right.

If the goal is potentially no add-on or front-end rewrites at all, it seems like content window objects will need to have two halves: one that delegates to the CPOW for DOM calls and properties, and one that does everything else as usual.  If that's not the goal, then either front-end stuff that needs the second use will have to be rewritten to not need it if possible, or CPOWS will need to be made available alongside something like the current docshell-whatever objects.  Right now no docshells are created for remote content, so they or something like them would need to be.

(I started to prototype a content window object using JS proxies that delegates to the CPOW for DOM and does the usual thing otherwise.  I modified nsFrameLoader to create a docshell even if remote content is turned on, but that screwed up CPOW creation: ContextWrapperParent::GetGlobalJSObject, which is called by nsFrameLoader::GetCrossProcessObjectWrapper, returns false unexpectedly, and I haven't been able to figure out why the PObjectWrapperParent ptr passed to RecvPObjectWrapperConstructor is null yet.)

Comment 9

[Benjamin Smedberg]

You can't have a "real" docshell in the chrome process: all of the interesting state is in the content process, and remoting that state securely is all-but-impossible. CPOWs are purely a JS solution.

Comment 10

[Drew Willcoxon :adw]

After discussions with Benjamin, it's not clear how useful re-enabling CPOWs would be, and in any case I'm no longer working on this and am not planning on continuing, so wontfixing.  If I or someone else needs to pick it up, it can be re-opened then.