Closed Bug 540126 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mozilla+ben, Assigned: mozilla+ben)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attachment #421979 - Flags: superreview?(benjamin)
Attachment #421979 - Flags: review?(avarma)
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?
Depends on: 540886
Blocks: 541003
These modifications depend on bug 540886.
Depends on: 542341
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.
Attachment #421979 - Attachment is obsolete: true
Attachment #423426 - Attachment is obsolete: true
Attachment #421979 - Flags: superreview?(benjamin)
Attachment #421979 - Flags: review?(avarma)
Summary: CPOW: Add BeginRequest, EndRequest methods to PContextWrapper → CPOW: Let PObjectWrapper actors use ContentProcessParent::RequestRunToCompletion()
(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?
(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.
Attached patch Latest version of the patch. (obsolete) — Splinter Review
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.
Attachment #424722 - Attachment is obsolete: true
Attachment #430391 - Flags: review?(mrbkap)
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.
Attachment #430391 - Flags: review?(mrbkap) → review+
(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.
Attachment #430391 - Attachment is obsolete: true
Attachment #433444 - Flags: superreview?(jst)
Attachment #433444 - Flags: review?(mrbkap)
Attachment #433444 - Flags: superreview?(jst) → superreview+
Attachment #433444 - Flags: review?(mrbkap) → review+
Pushed to projects/electrolysis:
http://hg.mozilla.org/projects/electrolysis/rev/55b6530567dd

Will mark resolved when tests cycle.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.