540126 - CPOW: Let PObjectWrapper actors use ContentProcessParent::RequestRunToCompletion()

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Ben Newman (:bnewman) (:benjamn)]

Attachment: Patch to implement the new request model.

The original CPOW patch used JSAutoRequest to lock the child runtime for the duration of each individual CPOW operation, but true run-to-completion requires support for locking across multiple CPOW operations.

Note that BeginRequest and EndRequest only result in IPC calls when the request depth transitions from 0 to 1 or 1 to 0, so calling these methods in a nested fashion is perfectly fine.

The benefits of the new approach are already on display in CPOW_NewEnumerate, where the request is begun with NewEnumerateInit and ended when NewEnumerateNext sets statep to JSVAL_NULL (or when NewEnumerateDestroy is called, but that's less common).

I've removed all the JSAutoRequests from ObjectWrapperChild.cpp, so the JS engine should fail lots of CHECK_REQUEST(cx)s if I've missed any AutoRequests in ObjectWrapperParent.cpp.

JetPack will probably want to call BeginRequest and EndRequest around each fired event handler.

Comment 1

[Ben Newman (:bnewman) (:benjamn)]

After a conversation with jst, it seems to me that, although this patch is certainly necessary to prevent GC from interrupting a sequence of CPOW operations that should run to completion, other events may still interrupt, such as the firing of a timer.  JS_{Begin,End}Request do nothing to prevent non-CPOW IPC messages from being sent during or between CPOW operations.

cjones, is there a stronger mechanism for locking child processes?  But not so strong as to cause deadlocks in conjunction with RPC calls?

Comment 2

[Ben Newman (:bnewman) (:benjamn)]

Attachment: More powerful blocking semantics using BlockChild/UnblockChild.

These modifications depend on bug 540886.

Comment 3

[Ben Newman (:bnewman) (:benjamn)]

Attachment: Simpler patch using ContentProcessParent::RequestRunToCompletion()

Instead of forcing CPOW clients to call ContextWrapperParent::BeginRequest() and ContextWrapperParent::EndRequest(), we can just make use of the recently-landed method ContentProcessParent::RequestRunToCompletion().

The big win here is that now merely using a CPOW ensures run-to-completion semantics with respect to the child process, which means that a whole chunk of integration work (JetPack, event listeners) becomes trivial.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #3)
> The big win here is that now merely using a CPOW ensures run-to-completion
> semantics with respect to the child process, which means that a whole chunk of
> integration work (JetPack, event listeners) becomes trivial.
This sounds promising, but what does this mean in practice?
If I have an XPCNativeWrapper in content process, how to send that as an
CPOW to chrome process? Are CPOW objects somehow serializable, so that I
could use them as parameters to IPDL messages?

Comment 5

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #4)
> (In reply to comment #3)
> > The big win here is that now merely using a CPOW ensures run-to-completion
> > semantics with respect to the child process, which means that a whole chunk of
> > integration work (JetPack, event listeners) becomes trivial.
> This sounds promising, but what does this mean in practice?
> If I have an XPCNativeWrapper in content process, how to send that as an
> CPOW to chrome process?  Are CPOW objects somehow serializable, so that I
> could use them as parameters to IPDL messages?

The current interface is oriented towards the chrome-reaching-into-content model, so there's a method

  bool TabParent::GetGlobalJSObject(JSContext* cx, JSObject** globalp);

that hands back a CPOW JSObject* corresponding to the child's global object.

This is implemented as follows.  TabChild holds a ContextWrapperChild* member, from which it may request a PObjectWrapperChild* using

  PObjectWrapperChild*
  ContextWrapperChild::GetOrCreateWrapper(JSObject* obj, bool makeGlobal);

(see http://github.com/benjamn/e10s/blob/cf9adcb67c0163b59e56e6e530342a72c9a8d253/cpow.diff#L1066-1076).  The resulting PObjectWrapperChild* can be passed from child to parent in any IPC message that has a PObjectWrapper parameter.  On the parent side, you can static_cast the PObjectWrapperParent* to a ObjectWrapperParent* and then call

  JSObject* ObjectWrapperParent::GetJSObject(JSContext* cx) const;

to get the CPOW JSObject*.

As an aside, the makeGlobal parameter tells the ContextWrapperParent to treat the newly constructed PObjectWrapper as the global object, so that's how the global object ends up making its way to the parent process.  Right now this interface is a little awkward because the makeGlobal parameter is ignored if GetOrCreateWrapper returns an existing wrapper instead of creating a new one.  I'll look into that today.

Comment 6

[Ben Newman (:bnewman) (:benjamn)]

Attachment: Latest version of the patch.

This patch is functionally the same as the previous patch; i.e.,

1. It introduces the AutoContextPusher for ObjectWrapperChild methods, which creates a JSAutoRequest for the current context and pushes the context onto the context stack.

2. It adds a call to self->Manager()->RequestRunToCompletion() in the ObjectWrapperParent::CPOW_* hooks to block the child until the parent returns to its event loop.

The patch differs from the previous patch only because I've had to catch up with some bit-rot since I first posted it.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 430391 [details] [diff] [review]
Latest version of the patch.

>diff --git a/js/src/ipc/ObjectWrapperChild.cpp b/js/src/ipc/ObjectWrapperChild.cpp
>+    class AutoContextPusher {

It seems like you might be able to contain an nsContentUtils::nsCxPusher and let it do the stack manipulations for you (the reason you can't use that by itself is because it doesn't start a request, right?

Other than that, this looks fine to me.

Comment 8

[Ben Newman (:bnewman) (:benjamn)]

Attachment: Patch using nsCxPusher.

(In reply to comment #7)
> It seems like you might be able to contain an nsContentUtils::nsCxPusher and
> let it do the stack manipulations for you (the reason you can't use that by
> itself is because it doesn't start a request, right?

That's right.  Now using nsCxPusher for the stack manipulations.

This patch should be ready to land.

Comment 9

[Ben Newman (:bnewman) (:benjamn)]

Pushed to projects/electrolysis:
http://hg.mozilla.org/projects/electrolysis/rev/55b6530567dd

Will mark resolved when tests cycle.