1317844 - Allow a custom event target to be specified for each actor

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

These patches will allow clients to request that messages destined for a given actor should be dispatched to a particular event target (rather than the worker message loop). This is needed for the Quantum DOM work.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Convert IDMap so it can store nsCOMPtr

I want to use IDMap to store the event targets. It's nicer than XPCOM hashtables since they don't easily support 32-bit signed keys. We just need to fix the fact that it assumes the values are always raw pointers. This patch does that.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: Add IPC message constructor flag

This patch sets a special flag for constructor messages. Needed for the next patch.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: IPC event target work

This patch adds support for the custom event targets. There are some tricky parts.

1. We need a map that stores the event target. We need the event target on the link thread, so it's better to key the map on the actor's ID. That's something that's readily available on the message.

2. We need to set the event target before the actor can receive any messages. That's tricky because the actor doesn't have an ID before we send it to the other side. To make this work, you do the following:
  create the actor
  set the target, which gives it an ID
  send the constructor message, which re-uses an ID if one is already present

3. If we create an actor and its parent already has an event target, we want to inherit the parent's event target. This is also a bit tricky since the actor doesn't have a parent set at the time we set the target. So you need to call the SetEventTargetForActor method on the actor that will become the manager. This method sets itself as the manager. Later on, when we set the manager while sending the constructor message, we assert that it's correct.

4. If we receive a message from the other side, it may be managed by an actor that has a custom event target. In this case, it needs to inherit that event target. So we intercept constructor messages and pull out the ID of the new actor. We also give the top-level protocol a chance to assign a special event target. This is useful in some cases.

5. I found it confusing how the actor ID gets nulled out when an actor is destroyed. It happens in the generated code. So I moved this to the Unregister method.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: IPC event target work, v2

I fixed a few small bugs in this patch. The manager field now gets nulled out in Unregister at the same time as the ID. This means that the actor deallocation code can't use it anymore, so I had to change the codegen to use a temporary.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This page might help to understand what is going on: https://wiki.mozilla.org/Quantum/DOM

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8812006 [details] [diff] [review]
IPC event target work, v2

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

::: ipc/glue/ProtocolUtils.cpp
@@ +531,5 @@
> +void
> +IProtocol::SetEventTargetForActor(IProtocol* aActor, nsIEventTarget* aEventTarget)
> +{
> +  // Make sure we have a manager for the internal method to access.
> +  aActor->SetManager(this);

It looks like before this patch, Register() was supposed to be called only once, but now actors will implicitly register before being Sent if they call SetEventTargetForActor?

I just wanted to make sure this wouldn't have any side-effects like causing the actor to get an ActorDestroy even if it wasn't sent. But it looks okay, since it's just the id.

@@ +663,5 @@
> +
> +  MutexAutoLock lock(mEventTargetMutex);
> +  if (mEventTargetMap.Lookup(aId)) {
> +    mEventTargetMap.Remove(aId);
> +  }

r=me on adding a helper for this to IDMap, if you care

@@ +810,5 @@
> +
> +  // Register the actor early. When it's registered again, it will keep the same
> +  // ID.
> +  int32_t id = Register(aActor);
> +  aActor->SetId(id);

Is there a reason Register doesn't set the id for us? (Possibly another micro-removal for lower.py)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9141a7635a71
Convert IDMap so it can store nsCOMPtr (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/41fa5d1d170b
Add IPC message constructor flag (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/10452c86cfbd
Allow an event target to be specified for each IPC actor (r=dvander)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d78b098b868b
Revert "Bug 1317844 - Allow an event target to be specified for each IPC actor (r=dvander)"

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55a2c9549408
Allow an event target to be specified for each IPC actor (r=dvander)

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/55a2c9549408