879475 - Allow nested oop <iframe mozbrowser> without nested content processes

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

+++ This bug was initially created as a clone of Bug #761935 +++

B2G is multi-process, but the hierarchy of processes is depth two: Only the root process can have child processes.

This creates a problem for us on B2G with the browser app.  We'd like the browser app to run outside the root process, but we'd also like the browser app's tabs to run outside the browser app process (so that when a tab crashes e.g. due to OOM, it doesn't take down the whole browser).

Right now we have to compromise.  We can either run the browser app in the root process and run the browser tabs in their own processes, or we can run the browser app outside the root process and run the browser tabs inside the same process as the browser app.  We chose the former.

Bug 761935 is about one way to fix this problem: We could allow child processes to have their own children.

But I think this would be a lot of work.  One problem is that because an arbitrary-depth hierarchy of processes doesn't exist right now, I expect that most of our APIs won't work properly if we have nested content processes.  Many of them explicitly don't even try to work correctly under these circumstances.  We'd have to fix up all of these APIs and then get the graphics stack to work with nested content processes (I'm not at all familiar with the graphics stack, so I don't know how easy or hard this would be).

I want to propose in this bug an alternative and perhaps simpler way of accomplishing our goal of moving the browser app out of the main process.  Hopefully you can tell me whether this is a bad idea or not.

It turns out that a child-of-a-child doesn't need to talk to its parent very often.  Most of what it needs (e.g. network access, permission checks, hardware access) comes from the main process.  So what if we made our browser tab inside the browser process a child of the main process (*)?  We'd then fix up the few cases when the process wants to talk to its now-sibling.

We'd have to fix up mozbrowser, but I don't think this would be particularly complicated.  We'd just funnel communication between the two sibling processes through the main process.

We'd also have to fix up some stuff in graphics so that the stack would know where to draw the inner mozbrowser.  I don't know how complicated this would be, but it doesn't strike me as out of the question.

What do you guys think of this plan?  Have I misunderestimated something here, or does this sound like a good approach to try?

dzbarsky has said he might be interested in doing this work, but we need to figure out if it's of appropriate scope.

(*) This isn't an original idea, but I don't remember where I heard it.  Sorry to whomever I'm plagiarizing!

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I like this idea :)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Seems reasonable to me, modulo the graphics concerns.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes! We should absolutely do this! Like you say, in many cases we don't have a need to go through the intermediary process but it's better to talk to the parent process directly.

There are some cases in the browser API where we need to talk between the child and the grandchild, but I suspect that that communication is low-bandwidth enough that going through the parent process is fine. We can always set up a direct IPC connection between the child and the grandchild later if needed.

Comment 4

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

I like.
Communication between siblings needs to be asynchronous, or there must be one master sibling
to which other siblings can send synchronous messages, but the master can send only async messages.
Or master send sync and async, and others only async.

Comment 5

[Jason Duell]

Sounds fine from a necko perspective. (I assume we'd still have TabParent/Child to do security checks, or something equivalent).  Only thing that is likely to need changes would be popup windows for auth.

One benefit of this would be that it would allow us to start enforcing IsInBrowserElement (right now we have to trust child process's description for that, which potentially allows rogue tabs access to browser's cookies/cache/appcache/etc).

Comment 6

[Jason Duell]

FWIW I believe it's also fine for all the {cache|cookie|etc|}jar code if the architecture here is to have a different appID for the tab process than the browser app.

Comment 7

[Justin Lebar (not reading bugmail)]

(In reply to Jason Duell (:jduell) from comment #6)
> FWIW I believe it's also fine for all the {cache|cookie|etc|}jar code if the
> architecture here is to have a different appID for the tab process than the
> browser app.

I think we'd have the same app-id + different is-browser flag for the browser processes, just like we have now.

Comment 8

[Jason Duell]

OK, good.  I realized one case where we do rely on app id being same for browser and tab: deletion notifications, where IIRC we do get a single appId then do two deletes: (appId, notInBrowser), (appId, inbrowser).

Comment 9

[Matt Woodrow (:mattwoodrow)]

So what we do currently, is the RenderFrameParent (in the main proces) builds a 'RefLayer' as a leaf layer in the layer tree.

This serves as an attachment point for the layer sub-tree from the child process, and also defines the positioning.

The child process builds its own layer (relative to itself), and both processes separately shadow these layer trees to the compositor.

The compositor then attaches the child process layer trees to the RefLayer attachment points in the main layer tree.

So I think this should still work. The browser process would still need a RenderFrameParent (or equivalent) to build RefLayers to attach the layers for the tab process. We'd need to modify the RefLayer attaching code to also walk through sub-trees looking for nested RefLayers. I don't think layer positioning should be a problem.

Comment 10

[Nicolas Silva [:nical]]

sorry for the delay,
I think it's a good idea and should not be too difficult from a layers point of view as Matt said. We already do something somewhat similar to attach the async-video stuff to transfer textures that are going through a different IPDL protocol than layer transactions.

Comment 11

[David Zbarsky (:dzbarsky)]

Attachment: Part 0: Move TabContext to its own ipdl header

Comment 12

[David Zbarsky (:dzbarsky)]

Attachment: Part 1: Rename the MsgStart messages to MsgStartChild for child protocols

I'm not really happy with this patch, but the idea is to fix 

            case PContentBridgeMsgStart:
                {
                    Transport* t;
                    if ((!(t = mozilla::ipc::OpenDescriptor(td, mozilla::ipc::Transport::MODE_SERVER)))) {
                        return MsgValueError;
                    }
                    if ((!(AllocPContentBridgeParent(t, pid)))) {
                        return MsgProcessingError;
                    }
                    break;
                }
            case PContentBridgeMsgStart:
                {
                    Transport* t;
                    if ((!(t = mozilla::ipc::OpenDescriptor(td, mozilla::ipc::Transport::MODE_CLIENT)))) {
                        return MsgValueError;
                    }
                    if ((!(AllocPContentBridgeChild(t, pid)))) {
                        return MsgProcessingError;
                    }
                    break;
                }

Comment 13

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Move some blob constructor params to ipdlh headers

Comment 14

[David Zbarsky (:dzbarsky)]

Attachment: Part 3: Make PBrowser manually keep track of its manager

We need this because PBrowser can be managed by either PContent or PContentBridge, so we can't rely on IPDL to generate the right thing for us.

Comment 15

[David Zbarsky (:dzbarsky)]

Attachment: Part 4: Make PBlob manually keep track of its manager

Comment 16

[David Zbarsky (:dzbarsky)]

Attachment: Part 5: Add an nsIContentParent api

This will be shared by ContentParent and ContentBridgeParent.

Comment 17

[David Zbarsky (:dzbarsky)]

Attachment: Part 6: Add nsIContentChild api

Comment 18

[David Zbarsky (:dzbarsky)]

Attachment: Part 7: Use nsIContentParent in indexeddb

Comment 19

[Thinker Li [:sinker]]

Don't notice this bug before!  I hope it would not be late.  I think we can make two content processes talk to each other directly without a bridge at the b2g process by passing a socketpair to parent and child content processes.

Comment 20

[David Zbarsky (:dzbarsky)]

(In reply to Thinker Li [:sinker] from comment #19)
> Don't notice this bug before!  I hope it would not be late.  I think we can
> make two content processes talk to each other directly without a bridge at
> the b2g process by passing a socketpair to parent and child content
> processes.

That's what I'm doing.  The main process sets up the bridge when it creates the child and then the content processes talk directly to each other.

Comment 21

[Thinker Li [:sinker]]

(In reply to David Zbarsky (:dzbarsky) from comment #20)
> That's what I'm doing.  The main process sets up the bridge when it creates
> the child and then the content processes talk directly to each other.
This is cool!

Comment 22

[Thinker Li [:sinker]]

Comment on attachment 766385 [details] [diff] [review]
Part 1: Rename the MsgStart messages to MsgStartChild for child protocols

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

Could you explain why we need this changes? As I know, Bridge() will send a channel opened message to both processes, and both them known each other.  They can tell parent or child actor that should be created.  You can also change ChannelOpened() to indicate which side of actors that message receiver should create.

Comment 23

[Thinker Li [:sinker]]

Comment on attachment 770016 [details] [diff] [review]
Part 4: Make PBlob manually keep track of its manager

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

::: dom/ipc/ContentChild.cpp
@@ +696,3 @@
>    NS_ENSURE_TRUE(actor, nullptr);
>  
>    if (!SendPBlobConstructor(actor, params)) {

I think this function will set the value of mManager for you.  So, you don't need to pass the pointer of mManager around.

Comment 24

[David Zbarsky (:dzbarsky)]

Attachment: Part n: Implement the ContentBridge

Comment 25

[David Zbarsky (:dzbarsky)]

(In reply to Thinker Li [:sinker] from comment #22)
> Comment on attachment 766385 [details] [diff] [review]
> Part 1: Rename the MsgStart messages to MsgStartChild for child protocols
> 
> Review of attachment 766385 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could you explain why we need this changes? As I know, Bridge() will send a
> channel opened message to both processes, and both them known each other. 
> They can tell parent or child actor that should be created.  You can also
> change ChannelOpened() to indicate which side of actors that message
> receiver should create.

Both sides of the bridge are opened by ContentChild, so it needs to be able to distinguish the case statements.  See comment 12.(In reply to Thinker Li [:sinker] from comment #23)

> Comment on attachment 770016 [details] [diff] [review]
> Part 4: Make PBlob manually keep track of its manager
> 
> Review of attachment 770016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/ContentChild.cpp
> @@ +696,3 @@
> >    NS_ENSURE_TRUE(actor, nullptr);
> >  
> >    if (!SendPBlobConstructor(actor, params)) {
> 
> I think this function will set the value of mManager for you.  So, you don't
> need to pass the pointer of mManager around.

It will, but I need to keep track of mManager so that Manager() will continue to work when Blobs can be managed by either PContent or PContentBridge.  Hopefully the WIP Part (n) makes things clearer.

Comment 26

[David Zbarsky (:dzbarsky)]

Attachment: Part 8: Fix IndexedDBObjectStore to not assume that PContent connects the same processes as PBrowser

Comment 27

[David Zbarsky (:dzbarsky)]

Attachment: Part 9: nsIContentChild::GetOrCreateActorForBlob should ensure the blob has the correct manager

We were already doing this on the parent side

Comment 28

[David Zbarsky (:dzbarsky)]

Attachment: Part 10: Allow nested remote mozbrowsers to push layer transactions to the compositor

Comment 29

[Thinker Li [:sinker]]

(In reply to David Zbarsky (:dzbarsky) from comment #25)
> Both sides of the bridge are opened by ContentChild, so it needs to be able
> to distinguish the case statements.
The channel opened message would be passed with the PID of the peer.  Is the PID enough for distinguish?

Comment 30

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 771132 [details] [diff] [review]
Part 10: Allow nested remote mozbrowsers to push layer transactions to the compositor

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +67,5 @@
>            }
>          } else {
>            ref->DetachReferentLayer(referent);
>            referent->SetAsyncPanZoomController(nullptr);
> +          WalkTheTree<OP>(referent, aReady, aTargetConfig);

Don't we need to do this for OP == Resolve too?

Comment 31

[David Zbarsky (:dzbarsky)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #30)
> Comment on attachment 771132 [details] [diff] [review]
> Part 10: Allow nested remote mozbrowsers to push layer transactions to the
> compositor
> 
> Review of attachment 771132 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/AsyncCompositionManager.cpp
> @@ +67,5 @@
> >            }
> >          } else {
> >            ref->DetachReferentLayer(referent);
> >            referent->SetAsyncPanZoomController(nullptr);
> > +          WalkTheTree<OP>(referent, aReady, aTargetConfig);
> 
> Don't we need to do this for OP == Resolve too?

When you call ConnectReferentLayer, you set the subtree as the child of the RefLayer.  So when you walk the children at the end of the function, you walk the subtree.  We only need to call into the subtree in the detach case because we remove it as a child.

Comment 32

[Justin Lebar (not reading bugmail)]

Comment on attachment 770015 [details] [diff] [review]
Part 3: Make PBrowser manually keep track of its manager

I was concerned that we're just hiding the existing PTab{Child,Parent}::Manager() functions, but dzbarsky said that we're going to get rid of these in a later patch.

This is nice; it cleans up the code quite a bit now that we don't have these static_cast's.

Comment 33

[David Zbarsky (:dzbarsky)]

Attachment: Rollup patch based off changeset 137652:bb4f5079e009

Comment 34

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/763319cfd1e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1c50de5dd95
https://hg.mozilla.org/integration/mozilla-inbound/rev/219590f024e8

Comment 35

[Justin Lebar (not reading bugmail)]

Comment on attachment 770016 [details] [diff] [review]
Part 4: Make PBlob manually keep track of its manager

r=me, with the same caveat as the first paragraph of comment 32.

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/763319cfd1e2
https://hg.mozilla.org/mozilla-central/rev/f1c50de5dd95
https://hg.mozilla.org/mozilla-central/rev/219590f024e8

Comment 37

[David Zbarsky (:dzbarsky)]

Attachment: Part 5: Add an nsIContentParent api

Comment 38

[David Zbarsky (:dzbarsky)]

Attachment: Part 6: Add nsIContentChild api

Comment 39

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 772868 [details] [diff] [review]
Rollup patch based off changeset 137652:bb4f5079e009

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

I tried this out on a device and it worked as expected. After thinking about it some and discussing with smaug I don't believe that this will make things any worse than they already are in terms of input events, so I have no objections to this landing. We'll still need to clean up how input events work at some point.

Comment 40

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e482a2e5d3b4

Comment 41

[David Zbarsky (:dzbarsky)]

Attachment: Part 11: Implement ContentBridge

Comment 42

[David Zbarsky (:dzbarsky)]

Attachment: Part 12: Fix the places where we cast nsIContentParent to ContentParent

Comment 43

[David Zbarsky (:dzbarsky)]

Attachment: Part 13: Allow creation of nested remote mozbrowsers

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e482a2e5d3b4

Comment 45

[Justin Lebar (not reading bugmail)]

Some general comments:

* It would be helpful if you added some documentation to the new classes you're
  adding indicating what they do.  It doesn't have to be a lot, but a few
  sentences in a header goes a long way.

* Maybe we should think a bit about the names we're using here.

  nsIContentParent probably shouldn't have the "ns" prefix, because it's in a
  C++ namespace.

  But is "IContentParent", "ContentParent", and "ContentBridgeParent" the best
  we can do?  It's not obvious from the name that IContentParent is a common
  interface for ContentParent and ContentBridgeParent, and it's not obvious
  from the names that ContentBridgeParent is for "sibling-to-sibling"
  communication.

  Supposing you could choose any names and not have to deal with the annoyance
  of renaming everything in the tree, what would you pick?

Comment 46

[Justin Lebar (not reading bugmail)]

Comment on attachment 776710 [details] [diff] [review]
Part 12: Fix the places where we cast nsIContentParent to ContentParent

>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
>index 8fb4235..aff1f4b 100644
>--- a/dom/ipc/TabParent.cpp
>+++ b/dom/ipc/TabParent.cpp
>@@ -273,26 +273,28 @@ TabParent::Destroy()
>     static_cast<IndexedDBParent*>(idbParents[i])->Disconnect();
>   }
> 
>   if (RenderFrameParent* frame = GetRenderFrame()) {
>     frame->Destroy();
>   }
>   mIsDestroyed = true;
> 
>-  // XXXdz what if manager is not ContentParent
>-  static_cast<ContentParent*>(Manager())->NotifyTabDestroying(this);
>+  if (XRE_GetProcessType() == GeckoProcessType_Default) {
>+    static_cast<ContentParent*>(Manager())->NotifyTabDestroying(this);
>+  }
>   mMarkedDestroying = true;
> }
> 
> bool
> TabParent::Recv__delete__()
> {
>-  // XXXdz what if manager is not ContentParent
>-  static_cast<ContentParent*>(Manager())->NotifyTabDestroyed(this, mMarkedDestroying);
>+  if (XRE_GetProcessType() == GeckoProcessType_Default) {
>+    static_cast<ContentParent*>(Manager())->NotifyTabDestroyed(this, mMarkedDestroying);
>+  }
>   return true;
> }
>
Would it be worthwhile to have a getter in TabParent that encapsulates this?

Or even better, what if we had a method on nsIContentParent to tell us whether
it's a ContentParent or a ContentParentBridge, and another one to cast down to
ContentParent* or ContentBridgeParent*, as appropriate?  You're not going to
screw up these static_cast's, but I bet someone else will.

Comment 47

[David Zbarsky (:dzbarsky)]

Attachment: Add an nsIContentParent api r=jlebar

Comment 48

[David Zbarsky (:dzbarsky)]

Attachment: Add an nsIContentChild api r=jlebar

Comment 49

[David Zbarsky (:dzbarsky)]

Attachment: Implement ContentBridge r=jlebar

Comment 50

[David Zbarsky (:dzbarsky)]

Comment on attachment 777469 [details] [diff] [review]
Add an nsIContentParent api r=jlebar

These are the patches that are in my branch, I just hadn't updated the attachments after merging to the CPOW stuff.  Hopefully this doesn't mess up reviewing too much for you.

Comment 51

[Justin Lebar (not reading bugmail)]

> Hopefully this doesn't mess up reviewing too much for you.

No, I'm just going off what's in your branch; that's easier for me, since I can search between patches and so on.

In case anyone else is interested, the branch in question is https://github.com/dzbarsky/mozilla-central/tree/nestedprocesses.

Comment 52

[David Zbarsky (:dzbarsky)]

(In reply to Justin Lebar [:jlebar] from comment #46)
> Comment on attachment 776710 [details] [diff] [review]
> Part 12: Fix the places where we cast nsIContentParent to ContentParent
> 
> 
> Or even better, what if we had a method on nsIContentParent to tell us
> whether
> it's a ContentParent or a ContentParentBridge, and another one to cast down
> to
> ContentParent* or ContentBridgeParent*, as appropriate?  You're not going to
> screw up these static_cast's, but I bet someone else will.

I can add AsContentParent().  That's the only one we need.  In general though, you shouldn't need to downcast nsIContentParent.(In reply to Justin Lebar [:jlebar] from comment #45)

> * It would be helpful if you added some documentation to the new classes
> you're
>   adding indicating what they do.  It doesn't have to be a lot, but a few
>   sentences in a header goes a long way.
> 

Sure, I'll add some explanations.

> * Maybe we should think a bit about the names we're using here.
> 

I haven't come up with anything great yet.  I don't think we want things like GrandchildProcessParent.

Comment 53

[David Zbarsky (:dzbarsky)]

(In reply to David Zbarsky (:dzbarsky) from comment #52)
> (In reply to Justin Lebar [:jlebar] from comment #46)

> 
> I haven't come up with anything great yet.  I don't think we want things
> like GrandchildProcessParent.


How about:
ContentParent -> ProcessParent
ContentBridgeParent -> SiblingBridgeParent
nsIContentParent -> ProcessOpenerParent or something

Comment 54

[Justin Lebar (not reading bugmail)]

Some of these names are already taken, but they're taken by unimportant things that we could rename.

I think maybe

  ContentParent -> ProcessParent
  ContentBridgeParent -> SiblingProcessParent
  nsIContentParent -> IProcessParent

might work, but the proof will be doing the replace and looking at it in context.

Comment 55

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

Drive-by nit: the "*Content*" names were chosen because there are other subprocess things in gecko.  The things here are the "content" (agree that's not a great name) subprocess things.  Something overly general like "ProcessParent" is a step backwards IMHO.

Comment 56

[Justin Lebar (not reading bugmail)]

Comment on attachment 777471 [details] [diff] [review]
Implement ContentBridge r=jlebar

I'm not done reviewing this, but I wanted to give you some feedback now, particularly since I've seen enough to give r-.

When you revise this patch, would you mind doing it as a new commit, so your
branch has an interdiff built in?  The easiest thing for me would be if the new
commit was placed immediately after this commit.  And then that way, when you
go to push, you can squash all of the fixup commits.

>diff --git a/dom/ipc/PContentBridge.ipdl b/dom/ipc/PContentBridge.ipdl
>new file mode 100644
>index 0000000..1ee3098
>--- /dev/null
>+++ b/dom/ipc/PContentBridge.ipdl

>+rpc protocol PContentBridge

Please add a (brief) comment explaining what this thing is.

>+{
>+    bridges PContent, PContent;
>+
>+    manages PBlob;
>+    manages PBrowser;
>+    manages PJavaScript;
>+
>+parent:
>+    sync SyncMessage(nsString aMessage, ClonedMessageData aData, CpowEntry[] aCpows)
>+      returns (nsString[] retval);
>+both:
>+    // Depending on exactly how the new browser is being created, it might be
>+    // created from either the child or parent process!
>+    //
>+    // The child creates the PBrowser as part of
>+    // TabChild::BrowserFrameProvideWindow (which happens when the child's
>+    // content calls window.open()), and the parent creates the PBrowser as part
>+    // of ContentParent::CreateTab.
>+    //
>+    // When the parent constructs a PBrowser, the child trusts the app token it
>+    // receives from the parent.  In that case, context can be any of the
>+    // IPCTabContext subtypes.
>+    //
>+    // When the child constructs a PBrowser, the parent doesn't trust the app
>+    // token it receives from the child.  In this case, context must have type
>+    // PopupIPCTabContext.  The browser created using a PopupIPCTabContext has
>+    // the opener PBrowser's app-id and containing-app-id.  The parent checks
>+    // that if the opener is a browser element, the context is also for a
>+    // browser element.
>+    //
>+    // This allows the parent to prevent a malicious child from escalating its
>+    // privileges by requesting a PBrowser corresponding to a highly-privileged
>+    // app; the child can only request privileges for an app which the child has
>+    // access to (in the form of a TabChild).
>+    async PBrowser(IPCTabContext context, uint32_t chromeFlags);

It would probably be better to put a pointer to the comment on
PContent::PBrowser(), as opposed to copy-pasting it.

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>index cca3f85..7bc4e1c 100644
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp

>   nsCOMPtr<nsIDOMElement> ownerElement = do_QueryInterface(mOwnerContent);
>-  mRemoteBrowser = ContentParent::CreateBrowserOrApp(context, ownerElement);
>+  if (XRE_GetProcessType() == GeckoProcessType_Default) {
>+    mRemoteBrowser = ContentParent::CreateBrowserOrApp(context, ownerElement);
>+  } else {
>+    mRemoteBrowser = ContentParent::CreateTab(context, ownerElement);
>+  }

We need to figure out the nomenclature here; it's not at all clear what you
mean by "CreateTab", especially since the comment in ContentParent is identical
to the comment for CreateBrowserOrApp.  :)

Maybe it would be better to move the GetProcessType() check into ContentParent,
so there's one less footgun in the public ContentParent API.

>diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h
>index 031bd15..871db2f 100644
>--- a/dom/ipc/ContentParent.h
>+++ b/dom/ipc/ContentParent.h

>     /**
>      * Get or create a content process for the given TabContext.  aFrameElement
>      * should be the frame/iframe element with which this process will
>      * associated.
>      */
>     static TabParent*
>     CreateBrowserOrApp(const TabContext& aContext,
>                        nsIDOMElement* aFrameElement);
> 
>+    /**
>+     * Get or create a content process for the given TabContext.  aFrameElement
>+     * should be the frame/iframe element with which this process will
>+     * associated.
>+     */
>+    static TabParent*
>+    CreateTab(const TabContext& aContext,
>+              nsIDOMElement* aFrameElement);

This either needs a different comment, or it needs to be merged with
CreateBrowserOrApp.  (The latter seems more sensible to me, offhand.)

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
>index 019d148..6b8bf03 100644
>--- a/dom/ipc/ContentParent.cpp
>+++ b/dom/ipc/ContentParent.cpp

>@@ -324,34 +326,37 @@ ContentParent::JoinAllSubprocesses()
> /*static*/ already_AddRefed<ContentParent>
>-ContentParent::GetNewOrUsed(bool aForBrowserElement)
>+ContentParent::GetNewOrUsed(bool aForBrowserElement, ContentParent* aOpener)
> {
>     if (!sNonAppContentParents)
>         sNonAppContentParents = new nsTArray<ContentParent*>();
> 
>     int32_t maxContentProcesses = Preferences::GetInt("dom.ipc.processCount", 1);
>     if (maxContentProcesses < 1)
>         maxContentProcesses = 1;
> 
>     if (sNonAppContentParents->Length() >= uint32_t(maxContentProcesses)) {
>         uint32_t idx = rand() % sNonAppContentParents->Length();
>         nsRefPtr<ContentParent> p = (*sNonAppContentParents)[idx];
>         NS_ASSERTION(p->IsAlive(), "Non-alive contentparent in sNonAppContentParents?");
>-        return p.forget();
>+        if (p->mOpener == aOpener) {
>+            return p.forget();
>+        }
>     }
> 
>     nsRefPtr<ContentParent> p =
>         new ContentParent(/* app = */ nullptr,
>+                          aOpener,
>                           aForBrowserElement,
>                           /* isForPreallocated = */ false,
>                           base::PRIVILEGES_DEFAULT,
>                           PROCESS_PRIORITY_FOREGROUND);
>     p->Init();
>     sNonAppContentParents->AppendElement(p);
>     return p.forget();
> }

This doesn't seem quite right.

The purpose of this code is to put a cap on the number of non-app content
processes.  If we already are at the limit of such processes and we try to
create a new non-app content process, we randomly pick one of the existing
processes.

I agree that we need some check like what you're added here, but as written
this lets us exceed the cap.

>@@ -484,16 +489,17 @@ ContentParent::CreateBrowserOrApp(const TabContext& aContext,
> 
>     if (!p) {
>         ChildPrivileges privs = PrivilegesForApp(ownApp);
>         p = MaybeTakePreallocatedAppProcess(manifestURL, privs,
>                                             initialPriority);
>         if (!p) {
>             NS_WARNING("Unable to use pre-allocated app process");
>             p = new ContentParent(ownApp,
>+                                  /* aOpener = */ nullptr,
>                                   /* isForBrowserElement = */ false,
>                                   /* isForPreallocated = */ false,
>                                   privs,
>                                   initialPriority);
>             p->Init();
>         }
>         sAppContentParents->Put(manifestURL, p);
>     }

>@@ -505,16 +511,91 @@ ContentParent::CreateBrowserOrApp(const TabContext& aContext,
>         aContext.AsIPCTabContext(),
>         /* chromeFlags */ 0);
> 
>     p->MaybeTakeCPUWakeLock(aFrameElement);
> 
>     return static_cast<TabParent*>(browser);
> }
> 
>+typedef std::map<ContentParent*, std::set<ContentParent*> > GrandchildMap;
>+static GrandchildMap sGrandchildProcessMap;

You need to remove |this| from the map (as both a key and a value!) in
MarkAsDead().

I have no idea what our rule is on using std::map in Gecko.  Can you ask
around, unless you're sure it's OK?  I don't have a problem with it on
principal.

>+bool
>+ContentParent::RecvCreateChildProcess(uint64_t* id)
>+{
>+    nsRefPtr<ContentParent> cp = GetNewOrUsed(/* isBrowserElement = */ true, this);
>+    *id = reinterpret_cast<uint64_t>(cp.get());

Using a pointer as an ID is bad, bad, bad, because malloc can reuse pointers.

We already have an ID on ContentParent; can we use that?

>+    GrandchildMap::iterator iter = sGrandchildProcessMap.find(this);
>+    if (iter == sGrandchildProcessMap.end()) {
>+        std::set<ContentParent*> children;
>+        children.insert(cp.get());
>+        sGrandchildProcessMap.insert(std::pair<ContentParent*, std::set<ContentParent*> >(this, children));

I think it'd be cleaner to write

          sGrandchildProcessMap[this] = children;

>+    } else {
>+        iter->second.insert(cp.get());
>+    }
>+    return true;
>+}

Do we need all of these |.get()|'s on cp?  I'd have thought we could rely on
the implicit conversion to ContentParent*.

>+bool
>+ContentParent::AnswerBridgeToChildProcess(const uint64_t& id)
>+{
>+    ContentParent* cp = reinterpret_cast<ContentParent*>(id);

What happens if a malicious child process sends us a bogus ID?  :)

>+    GrandchildMap::iterator iter = sGrandchildProcessMap.find(this);
>+    if (iter != sGrandchildProcessMap.end() &&
>+        iter->second.find(cp) != iter->second.end()) {
>+        return PContentBridge::Bridge(this, cp);
>+    } else {
>+        // You can't bridge to a process you didn't open!
>+        KillHard();
>+        return false;
>+    }
>+}

>@@ -1003,16 +1084,28 @@ ContentParent::ActorDestroy(ActorDestroyReason why)
>     // IPDL rules require actors to live on past ActorDestroy, but it
>     // may be that the kungFuDeathGrip above is the last reference to
>     // |this|.  If so, when we go out of scope here, we're deleted and
>     // all hell breaks loose.
>     //
>     // This runnable ensures that a reference to |this| lives on at
>     // least until after the current task finishes running.
>     NS_DispatchToCurrentThread(new DelayedDeleteContentParentTask(this));
>+
>+    // Destroy any processes created by this ContentParent
>+    GrandchildMap::iterator iter = sGrandchildProcessMap.find(this);
>+    if (iter != sGrandchildProcessMap.end()) {
>+        for(std::set<ContentParent*>::iterator child = iter->second.begin();
>+            child != iter->second.end();
>+            child++) {
>+            MessageLoop::current()->PostTask(
>+              FROM_HERE,
>+              NewRunnableMethod(*child, &ContentParent::ShutDownProcess, true));

Please comment what true is.  Also, it maybe should be false.  I think,
probably.  :)

>+        }
>+    }
> }

Comment 57

[David Zbarsky (:dzbarsky)]

(In reply to Justin Lebar [:jlebar] from comment #56)
> Comment on attachment 777471 [details] [diff] [review]
> Implement ContentBridge r=jlebar
> 

> 
> >     /**
> >      * Get or create a content process for the given TabContext.  aFrameElement
> >      * should be the frame/iframe element with which this process will
> >      * associated.
> >      */
> >     static TabParent*
> >     CreateBrowserOrApp(const TabContext& aContext,
> >                        nsIDOMElement* aFrameElement);
> > 
> >+    /**
> >+     * Get or create a content process for the given TabContext.  aFrameElement
> >+     * should be the frame/iframe element with which this process will
> >+     * associated.
> >+     */
> >+    static TabParent*
> >+    CreateTab(const TabContext& aContext,
> >+              nsIDOMElement* aFrameElement);
> 
> This either needs a different comment, or it needs to be merged with
> CreateBrowserOrApp.  (The latter seems more sensible to me, offhand.)
> 

I merged these together.  

> 
> This doesn't seem quite right.
> 
> The purpose of this code is to put a cap on the number of non-app content
> processes.  If we already are at the limit of such processes and we try to
> create a new non-app content process, we randomly pick one of the existing
> processes.
> 
> I agree that we need some check like what you're added here, but as written
> this lets us exceed the cap.
> 

We can pick a random process and if the opener doesn't match, iterate through the array in order, using that process as the starting point and wrapping around.  I think it's possible that none of the processes will match, in which case it may be ok to exceed the cap anyway. (For example, if there are two browser apps and one is using all the processes, we should open one process for the second one.  Any subsequently loaded tabs will share that process so we only exceed the limit by 1.)

> 
> I have no idea what our rule is on using std::map in Gecko.  Can you ask
> around, unless you're sure it's OK?  I don't have a problem with it on
> principal.
> 

We already use std::map in this file for CompositorParent::sIndirectLayerTrees.  I think the policy is it's ok to use STL containers that are exception-safe.

> >+bool
> >+ContentParent::RecvCreateChildProcess(uint64_t* id)
> >+{
> >+    nsRefPtr<ContentParent> cp = GetNewOrUsed(/* isBrowserElement = */ true, this);
> >+    *id = reinterpret_cast<uint64_t>(cp.get());
> 
> Using a pointer as an ID is bad, bad, bad, because malloc can reuse pointers.
> 
> We already have an ID on ContentParent; can we use that?

We could, but then we would need another map between ID and ContentParent.


> Do we need all of these |.get()|'s on cp?  I'd have thought we could rely on
> the implicit conversion to ContentParent*.
> 

I think that didn't work for some reason, but I'll try again.

> >+bool
> >+ContentParent::AnswerBridgeToChildProcess(const uint64_t& id)
> >+{
> >+    ContentParent* cp = reinterpret_cast<ContentParent*>(id);
> 
> What happens if a malicious child process sends us a bogus ID?  :)
> 

That should be fine, we won't find the ContentParent in the map and kill the process.  We check that before we dereference the pointer.

> >@@ -1003,16 +1084,28 @@ ContentParent::ActorDestroy(ActorDestroyReason why)
> >     // IPDL rules require actors to live on past ActorDestroy, but it
> >     // may be that the kungFuDeathGrip above is the last reference to
> >     // |this|.  If so, when we go out of scope here, we're deleted and
> >     // all hell breaks loose.
> >     //
> >     // This runnable ensures that a reference to |this| lives on at
> >     // least until after the current task finishes running.
> >     NS_DispatchToCurrentThread(new DelayedDeleteContentParentTask(this));
> >+
> >+    // Destroy any processes created by this ContentParent
> >+    GrandchildMap::iterator iter = sGrandchildProcessMap.find(this);
> >+    if (iter != sGrandchildProcessMap.end()) {
> >+        for(std::set<ContentParent*>::iterator child = iter->second.begin();
> >+            child != iter->second.end();
> >+            child++) {
> >+            MessageLoop::current()->PostTask(
> >+              FROM_HERE,
> >+              NewRunnableMethod(*child, &ContentParent::ShutDownProcess, true));
> 
> Please comment what true is.  Also, it maybe should be false.  I think,
> probably.  :)
> 
> >+        }
> >+    }
> > }

When we call ShutDownProcess right above, we pass true to ensure that we close our channel, so perhaps we must do the same here.  Although, I think you understand process shutdown the best at this point ;)

Comment 58

[Justin Lebar (not reading bugmail)]

> We can pick a random process and if the opener doesn't match, iterate through the array in order, 
> using that process as the starting point and wrapping around.  I think it's possible that none of 
> the processes will match, in which case it may be ok to exceed the cap anyway.

Yes, that sounds totally right.

> We could, but then we would need another map between ID and ContentParent.

Yes.

> That should be fine, we won't find the ContentParent in the map and kill the process.  We check 
> that before we dereference the pointer.

Mmm.  That is still very scary, but you are correct.

Comment 59

[Justin Lebar (not reading bugmail)]

Except this still suffers from the "ABA problem", where we allocate a ContentParent at address X, then free it, then allocate a /different/ ContentParent at address X.

Comment 60

[Justin Lebar (not reading bugmail)]

> When we call ShutDownProcess right above, we pass true to ensure that we close our channel, so 
> perhaps we must do the same here.  Although, I think you understand process shutdown the best at 
> this point ;)

It's different -- above, we're calling ShutDownProcess as a last-ditch attempt to shut ourselves down.  We pass |true| so that we call CloseWithError on the channel (which, like you say, ensures that the channel is closed).

But here, I think we want to try a soft shutdown of the grandchild process.  It's not time to shut it down as an error...yet.  I'm not sure when is that time.  Judging by the other bugs we're fighting against, we'll probably have to circle back around after you land this and figure out how to ensure that grandchildren are always killed.

Comment 61

[David Zbarsky (:dzbarsky)]

(In reply to Justin Lebar [:jlebar] from comment #59)
> Except this still suffers from the "ABA problem", where we allocate a
> ContentParent at address X, then free it, then allocate a /different/
> ContentParent at address X.

Really?  Keep in mind that the reinterpret_cast will go away because we'll be using the ContentParent's mId.  Those should be unique, and the rest of the function ensures we have permission to muck with the ContentParent identified by the id.

Comment 62

[Justin Lebar (not reading bugmail)]

If we use the ContentParent's mID, which is non-repeating, and map from that to ContentParent*, there's no problem.

Comment 63

[Justin Lebar (not reading bugmail)]

Comment on attachment 777471 [details] [diff] [review]
Implement ContentBridge r=jlebar

Hooray, finished with this one.

>diff --git a/dom/ipc/PContentBridge.ipdl b/dom/ipc/PContentBridge.ipdl
>new file mode 100644
>index 0000000..1ee3098
>--- /dev/null
>+++ b/dom/ipc/PContentBridge.ipdl

>+rpc protocol PContentBridge

I think |rpc protocol| is only necessary if this protocol has rpc messages.
Since they're all sync or async, can this be a |sync protocol|?  Or does it
inherit its rpc-ness from PContent?

>+{
>+    bridges PContent, PContent;
>+
>+    manages PBlob;
>+    manages PBrowser;
>+    manages PJavaScript;
>+
>+parent:
>+    sync SyncMessage(nsString aMessage, ClonedMessageData aData, CpowEntry[] aCpows)
>+      returns (nsString[] retval);
>+both:
>+    async PBrowser(IPCTabContext context, uint32_t chromeFlags);
>+
>+    async PBlob(BlobConstructorParams params);
>+
>+    async PJavaScript();
>+
>+    AsyncMessage(nsString aMessage, ClonedMessageData aData, CpowEntry[] aCpows);
>+};
>+
>+}
>+}

>diff --git a/dom/ipc/ContentBridgeChild.cpp b/dom/ipc/ContentBridgeChild.cpp
>new file mode 100644
>index 0000000..832f87d
>--- /dev/null
>+++ b/dom/ipc/ContentBridgeChild.cpp
>+namespace mozilla {
>+namespace dom {
>+
>+NS_IMPL_ISUPPORTS1(ContentBridgeChild, nsIContentChild)
>+
>+ContentBridgeChild::ContentBridgeChild(Transport* aTransport)
>+  : mTransport(aTransport)
>+{}
>+
>+ContentBridgeChild::~ContentBridgeChild()
>+{
>+  XRE_GetIOMessageLoop()->PostTask(FROM_HERE, new DeleteTask<Transport>(mTransport));
>+}

Can you add a comment in the header that mTransport is an owning reference?
Also, how do you know that's true?

>+/*static*/ PContentBridgeChild*
>+ContentBridgeChild::Create(Transport* aTransport, ProcessId aOtherProcess)

Why not have this return ContentBridgeChild*?

>+{
>+  nsRefPtr<ContentBridgeChild> bridge =
>+    new ContentBridgeChild(aTransport);
>+  ProcessHandle handle;
>+  if (!base::OpenProcessHandle(aOtherProcess, &handle)) {
>+    // XXX need to kill |aOtherProcess|, it's boned
>+    return nullptr;
>+  }

It's OK if you want to do this in another bug, but this is an important XXX to
fix.

>+  bridge->mSelfRef = bridge;

When do you drop this ref?

>+  DebugOnly<bool> ok = bridge->Open(aTransport, handle, XRE_GetIOMessageLoop());
>+  MOZ_ASSERT(ok);

>+  // The return value is just compared to null for success checking,
>+  // we're not sharing a ref.
>+  return bridge.get();

I don't understand this comment; if this method only needs to return a bool,
then can we make it return a bool?  If we can't, we clearly are using the
pointer value for something...

>+}

>+jsipc::JavaScriptChild *
>+ContentBridgeChild::GetCPOWManager()
>+{
>+  if (ManagedPJavaScriptChild().Length()) {
>+    return static_cast<JavaScriptChild*>(ManagedPJavaScriptChild()[0]);
>+  }
>+  JavaScriptChild* actor = static_cast<JavaScriptChild*>(SendPJavaScriptConstructor());
>+  return actor;
>+}

Can you add a comment explaining why this can't live on nsIContentChild?

>diff --git a/dom/ipc/ContentBridgeParent.cpp b/dom/ipc/ContentBridgeParent.cpp
>new file mode 100644
>index 0000000..36be212
>--- /dev/null
>+++ b/dom/ipc/ContentBridgeParent.cpp

>+ContentBridgeParent::~ContentBridgeParent()
>+{
>+  XRE_GetIOMessageLoop()->PostTask(FROM_HERE, new DeleteTask<Transport>(mTransport));

Similarly about this mTransport.

>+}

>+/*static*/ PContentBridgeParent*
>+ContentBridgeParent::Create(Transport* aTransport, ProcessId aOtherProcess)
>+{
>+  nsRefPtr<ContentBridgeParent> bridge =
>+    new ContentBridgeParent(aTransport);
>+  ProcessHandle handle;
>+  if (!base::OpenProcessHandle(aOtherProcess, &handle)) {
>+    // XXX need to kill |aOtherProcess|, it's boned

Similarly about this XXX.

>+    return nullptr;
>+  }
>+  bridge->mSelfRef = bridge;

Similarly about breaking this ref.

>+  DebugOnly<bool> ok = bridge->Open(aTransport, handle, XRE_GetIOMessageLoop());
>+  MOZ_ASSERT(ok);
>+  // The return value is just compared to null for success checking,
>+  // we're not sharing a ref.
>+  return bridge.get();

I similarly don't get this comment.

>+}

>+jsipc::JavaScriptParent*
>+ContentBridgeParent::GetCPOWManager()
>+{
>+  if (ManagedPJavaScriptParent().Length()) {
>+    return static_cast<JavaScriptParent*>(ManagedPJavaScriptParent()[0]);
>+  }
>+  JavaScriptParent* actor = static_cast<JavaScriptParent*>(SendPJavaScriptConstructor());
>+  return actor;
>+}

Can you add a comment explaining why this can't live on nsIContentParent?

>diff --git a/dom/ipc/ContentChild.h b/dom/ipc/ContentChild.h
>index a5d68a8..1d37333 100644
>--- a/dom/ipc/ContentChild.h
>+++ b/dom/ipc/ContentChild.h

>+    ContentBridgeParent* GetLastBridge() { return mLastBridge; }
>+    ContentBridgeParent* mLastBridge;

Initialize mLastBridge to nullptr in the ContentChild constructor?  Also it's
not clear why we have a public getter and also a public member.

Maybe the method should be called GetLastBridgeParent()?

>diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h
>index 031bd15..871db2f 100644
>--- a/dom/ipc/ContentParent.h
>+++ b/dom/ipc/ContentParent.h

>+    virtual bool RecvCreateChildProcess(uint64_t* id) MOZ_OVERRIDE;
>+    virtual bool AnswerBridgeToChildProcess(const uint64_t& id) MOZ_OVERRIDE;

>@@ -433,16 +446,17 @@ private:
>+    ContentParent* mOpener;

This is a bit subtle; I'd appreciate a comment explaining what this is.

Also, I don't think you have guaranteed that |this| is always destructed before
|mOpener|.  The easy thing to do would be to make this a weak ref.

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
>index 019d148..6b8bf03 100644
>--- a/dom/ipc/ContentParent.cpp
>+++ b/dom/ipc/ContentParent.cpp

>+/*static*/ TabParent*
>+ContentParent::CreateTab(const TabContext& aContext,
>+                         nsIDOMElement* aFrameElement)
>+{
>+    MOZ_ASSERT(aContext.IsBrowserElement());
>+    ContentChild* child = ContentChild::GetSingleton();
>+    uint64_t id;
>+    if (!child->SendCreateChildProcess(&id)) {
>+        return nullptr;
>+    }
>+    if (!child->CallBridgeToChildProcess(id)) {
>+        return nullptr;
>+    }

I'm nervous about the RPC usage here, but I haven't looked through those
patches yet, so I guess this gets a pass for now.  :)

>+    nsRefPtr<TabParent> tp(new TabParent(child->GetLastBridge(), aContext));

Using GetLastBridge() like this is a hack.  I'm not familiar enough with IPDL
to know what's the right way to do this, but there has got to be a better way.
Maybe bent can help.

>+    tp->SetOwnerElement(aFrameElement);
>+    uint32_t chromeFlags = 0;
>+
>+    // Propagate the private-browsing status of the element's parent
>+    // docshell to the remote docshell, via the chrome flags.
>+    nsCOMPtr<Element> frameElement = do_QueryInterface(aFrameElement);
>+    MOZ_ASSERT(frameElement);
>+    nsIDocShell* docShell =
>+        frameElement->OwnerDoc()->GetWindow()->GetDocShell();

I don't personally like MOZ_ASSERTs where, in the absence of the assert, you're
going to crash with a null-pointer exception on the next line.

Note also that we're going to crash if frameElement has no window.  I think a
busted child process could cause us to do that.

>+    MOZ_ASSERT(docShell);
>+    nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(docShell);

It's not clear why docShell must be non-null here, for the same reason as
window might be null.

Comment 64

[David Zbarsky (:dzbarsky)]

(In reply to Justin Lebar [:jlebar] from comment #63)
> Comment on attachment 777471 [details] [diff] [review]
> Implement ContentBridge r=jlebar
> 
> Hooray, finished with this one.

> 
> I think |rpc protocol| is only necessary if this protocol has rpc messages.
> Since they're all sync or async, can this be a |sync protocol|?  Or does it
> inherit its rpc-ness from PContent?
> 

It must be rpc because PBrowser is rpc.


> >+ContentBridgeChild::~ContentBridgeChild()
> >+{
> >+  XRE_GetIOMessageLoop()->PostTask(FROM_HERE, new DeleteTask<Transport>(mTransport));
> >+}
> 
> Can you add a comment in the header that mTransport is an owning reference?
> Also, how do you know that's true?
> 

This is what CompositorParent and ImageBridgeParent do.

> >+{
> >+  nsRefPtr<ContentBridgeChild> bridge =
> >+    new ContentBridgeChild(aTransport);
> >+  ProcessHandle handle;
> >+  if (!base::OpenProcessHandle(aOtherProcess, &handle)) {
> >+    // XXX need to kill |aOtherProcess|, it's boned
> >+    return nullptr;
> >+  }
> 
> It's OK if you want to do this in another bug, but this is an important XXX
> to
> fix.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#804 =(

> 
> >+  bridge->mSelfRef = bridge;
> 
> When do you drop this ref?
> 

Oops!

> >+  DebugOnly<bool> ok = bridge->Open(aTransport, handle, XRE_GetIOMessageLoop());
> >+  MOZ_ASSERT(ok);
> 
> >+  // The return value is just compared to null for success checking,
> >+  // we're not sharing a ref.
> >+  return bridge.get();
> 
> I don't understand this comment; if this method only needs to return a bool,
> then can we make it return a bool?  If we can't, we clearly are using the
> pointer value for something...
> 

Need to look into this.


> >@@ -433,16 +446,17 @@ private:
> >+    ContentParent* mOpener;
> 
> This is a bit subtle; I'd appreciate a comment explaining what this is.
> 
> Also, I don't think you have guaranteed that |this| is always destructed
> before
> |mOpener|.  The easy thing to do would be to make this a weak ref.
> 

Are you concerned about a race between when the app shuts down and tells its children to shut down and when the destructors actually run?


> >diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
> >index 019d148..6b8bf03 100644
> >--- a/dom/ipc/ContentParent.cpp
> >+++ b/dom/ipc/ContentParent.cpp
> 
> >+/*static*/ TabParent*
> >+ContentParent::CreateTab(const TabContext& aContext,
> >+                         nsIDOMElement* aFrameElement)
> >+{
> >+    MOZ_ASSERT(aContext.IsBrowserElement());
> >+    ContentChild* child = ContentChild::GetSingleton();
> >+    uint64_t id;
> >+    if (!child->SendCreateChildProcess(&id)) {
> >+        return nullptr;
> >+    }
> >+    if (!child->CallBridgeToChildProcess(id)) {
> >+        return nullptr;
> >+    }
> 
> I'm nervous about the RPC usage here, but I haven't looked through those
> patches yet, so I guess this gets a pass for now.  :)
> 
> >+    nsRefPtr<TabParent> tp(new TabParent(child->GetLastBridge(), aContext));
> 
> Using GetLastBridge() like this is a hack.  I'm not familiar enough with IPDL
> to know what's the right way to do this, but there has got to be a better
> way.
> Maybe bent can help.
> 

It's a hack, but I think it should be safe.

> >+    tp->SetOwnerElement(aFrameElement);
> >+    uint32_t chromeFlags = 0;
> >+
> >+    // Propagate the private-browsing status of the element's parent
> >+    // docshell to the remote docshell, via the chrome flags.
> >+    nsCOMPtr<Element> frameElement = do_QueryInterface(aFrameElement);
> >+    MOZ_ASSERT(frameElement);
> >+    nsIDocShell* docShell =
> >+        frameElement->OwnerDoc()->GetWindow()->GetDocShell();
> 
> I don't personally like MOZ_ASSERTs where, in the absence of the assert,
> you're
> going to crash with a null-pointer exception on the next line.
> 

This is preexisting code.  the assert is there mostly for sanity - it's impossible for the the owner of a process to not be an Element.  I can add a null-check for window.
 
> Note also that we're going to crash if frameElement has no window.  I think a
> busted child process could cause us to do that.
> 
> >+    MOZ_ASSERT(docShell);
> >+    nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(docShell);
> 
> It's not clear why docShell must be non-null here, for the same reason as
> window might be null.

Again, this is preexisting but I can add a check so we don't kill the parent process.

Comment 65

[David Zbarsky (:dzbarsky)]

(In reply to Justin Lebar [:jlebar] from comment #63)
> Comment on attachment 777471 [details] [diff] [review]
> Implement ContentBridge r=jlebar
> 


> 
> I don't understand this comment; if this method only needs to return a bool,
> then can we make it return a bool?  If we can't, we clearly are using the
> pointer value for something...
> 

We don't use the pointer value, so I can change this to return bool.  It needs an IPDL compiler patch though, so I'll do it as another patch.

Comment 66

[David Zbarsky (:dzbarsky)]

Attachment: Part 14: Allocing an actor for a bridged or opened protocol should return bool

Comment 67

[David Zbarsky (:dzbarsky)]

Attachment: TabChild/TabParent should just call Manager()->GetCPOWManager() instead of static_casting r=jlebar

Comment 68

[Justin Lebar (not reading bugmail)]

Comment on attachment 766385 [details] [diff] [review]
Part 1: Rename the MsgStart messages to MsgStartChild for child protocols

r=me, but I think it would be cleaner to have _messageStartName() take a
keyword arg (e.g., _messageStartName(p.decl.type, child=True)).

>diff --git a/ipc/ipdl/ipdl/lower.py b/ipc/ipdl/ipdl/lower.py
>--- a/ipc/ipdl/ipdl/lower.py
>+++ b/ipc/ipdl/ipdl/lower.py

>@@ -1532,17 +1532,17 @@ class _GenerateProtocolCode(ipdl.ast.Vis
>         # spit out message type enum and classes
>         msgenum = TypeEnum('MessageType')
>         msgstart = _messageStartName(self.protocol.decl.type) +' << 16'
>         msgenum.addId(self.protocol.name + 'Start', msgstart)
>-        msgenum.addId(self.protocol.name +'PreStart', '('+ msgstart +') - 1')
>+        #msgenum.addId(self.protocol.name +'PreStart', '('+ msgstart +') - 1')

I guess this is an unrelated change?  I'd prefer if we just deleted the line if
it's not necessary.

Comment 69

[Justin Lebar (not reading bugmail)]

Comment on attachment 779983 [details] [diff] [review]
Part 14: Allocing an actor for a bridged or opened protocol should return bool

Yay.

Comment 70

[David Zbarsky (:dzbarsky)]

(In reply to Justin Lebar [:jlebar] from comment #68)
> Comment on attachment 766385 [details] [diff] [review]
> Part 1: Rename the MsgStart messages to MsgStartChild for child protocols
> 

> 
> >@@ -1532,17 +1532,17 @@ class _GenerateProtocolCode(ipdl.ast.Vis
> >         # spit out message type enum and classes
> >         msgenum = TypeEnum('MessageType')
> >         msgstart = _messageStartName(self.protocol.decl.type) +' << 16'
> >         msgenum.addId(self.protocol.name + 'Start', msgstart)
> >-        msgenum.addId(self.protocol.name +'PreStart', '('+ msgstart +') - 1')
> >+        #msgenum.addId(self.protocol.name +'PreStart', '('+ msgstart +') - 1')
> 
> I guess this is an unrelated change?  I'd prefer if we just deleted the line
> if
> it's not necessary.

Yeah, we were generating an enum value that's never used.  Not a big deal.

Comment 71

[Justin Lebar (not reading bugmail)]

Comment on attachment 777470 [details] [diff] [review]
Add an nsIContentChild api r=jlebar

>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp
>index 0464ddf..4d5d356 100644
>--- a/dom/ipc/ContentChild.cpp
>+++ b/dom/ipc/ContentChild.cpp

>@@ -958,19 +850,19 @@ ContentChild::ActorDestroy(ActorDestroyReason why)
> 
>-    if (sFirstIdleTask) {
>+    /*if (sFirstIdleTask) {
>         sFirstIdleTask->Cancel();
>-    }
>+    }*/

What's going on here?

Comment 72

[Justin Lebar (not reading bugmail)]

Comment on attachment 777469 [details] [diff] [review]
Add an nsIContentParent api r=jlebar

>diff --git a/content/base/src/nsFrameLoader.h b/content/base/src/nsFrameLoader.h
>index 315a8b0..8178733 100644
>--- a/content/base/src/nsFrameLoader.h
>+++ b/content/base/src/nsFrameLoader.h

>@@ -431,17 +425,17 @@ private:
>   bool mObservingOwnerContent : 1;
> 
>   // Backs nsIFrameLoader::{Get,Set}Visible.  Visibility state here relates to
>   // whether this frameloader's <iframe mozbrowser> is setVisible(true)'ed, and
>   // doesn't necessarily correlate with docshell/document visibility.
>   bool mVisible : 1;
> 
>   // XXX leaking
>-  nsCOMPtr<nsIObserver> mChildHost;
>+  nsCOMPtr<mozilla::dom::nsIContentParent> mChildHost;

I've bitrotted you a bit here, sorry.

The reason for the bitrot is that it was helpful, when debugging ContentParent
leaks, for nsFrameLoader.h to maintain a pointer of type X, while our memory
reporter reported the addresses of ContentParent*'s also cast to type X.  It
would be helpful if you could maintain that invariant (perhaps by changing the
memory reporter in ContentParent.cpp to report nsIContentParent's, or
something).

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>index e23249f..cca3f85 100644
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp

>@@ -2062,17 +2071,17 @@ nsFrameLoader::TryRemoteBrowser()
>     nsCOMPtr<nsIDOMWindow> rootWin = do_GetInterface(rootItem);
>     nsCOMPtr<nsIDOMChromeWindow> rootChromeWin = do_QueryInterface(rootWin);
>     NS_ABORT_IF_FALSE(rootChromeWin, "How did we not get a chrome window here?");
> 
>     nsCOMPtr<nsIBrowserDOMWindow> browserDOMWin;
>     rootChromeWin->GetBrowserDOMWindow(getter_AddRefs(browserDOMWin));
>     mRemoteBrowser->SetBrowserDOMWindow(browserDOMWin);
> 
>-    mChildHost = mRemoteBrowser->Manager();
>+    mChildHost = do_QueryInterface(mRemoteBrowser->Manager());
>   }
>   return true;
> }

It doesn't look like nsIContent{Child,Parent} is the sort of interface you can
QI to.

Also I don't think you need to QI this, because TabParent::Manager() returns an
nsIContentParent*.

>diff --git a/dom/ipc/nsIContentParent.cpp b/dom/ipc/nsIContentParent.cpp
>new file mode 100644
>index 0000000..27d841e
>--- /dev/null
>+++ b/dom/ipc/nsIContentParent.cpp

We need to be careful with this one.  We're doing security checks here, but
those security checks must be done (or redone) in the root process.

It's not clear to me that we're meeting this burden here if the
nsIContentParent is a ContentBridgeParent.  Am I missing something?

Comment 73

[David Zbarsky (:dzbarsky)]

(In reply to Justin Lebar [:jlebar] from comment #71)
> Comment on attachment 777470 [details] [diff] [review]
> Add an nsIContentChild api r=jlebar
> 
> >diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp
> >index 0464ddf..4d5d356 100644
> >--- a/dom/ipc/ContentChild.cpp
> >+++ b/dom/ipc/ContentChild.cpp
> 
> >@@ -958,19 +850,19 @@ ContentChild::ActorDestroy(ActorDestroyReason why)
> > 
> >-    if (sFirstIdleTask) {
> >+    /*if (sFirstIdleTask) {
> >         sFirstIdleTask->Cancel();
> >-    }
> >+    }*/
> 
> What's going on here?

Ah, I left that because I wasn't sure what to do about it; good thing you caught it!  So the issue here is that we only create the sFirstIdleTask when the ContentChild allocates PBrowserChild.  The easiest solution here is probably to have ContentBridgeChild call ContentChild::GetSingleton()->RecvPBrowserConstructor.

Comment 74

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef5322627c86

Comment 75

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ef5322627c86

Comment 76

[David Zbarsky (:dzbarsky)]

(In reply to Justin Lebar [:jlebar] from comment #72)
> Comment on attachment 777469 [details] [diff] [review]
> Add an nsIContentParent api r=jlebar
> 

> I've bitrotted you a bit here, sorry.
> 
> The reason for the bitrot is that it was helpful, when debugging
> ContentParent
> leaks, for nsFrameLoader.h to maintain a pointer of type X, while our memory
> reporter reported the addresses of ContentParent*'s also cast to type X.  It
> would be helpful if you could maintain that invariant (perhaps by changing
> the
> memory reporter in ContentParent.cpp to report nsIContentParent's, or
> something).
> 

Hmm, are you talking about 
nsPrintfCString path("queued-ipc-messages/content-parent"               
                              "(%s, pid=%d, %s, 0x%p, refcnt=%d)",               
                              NS_ConvertUTF16toUTF8(friendlyName).get(),         
                             cp->Pid(), channelStr, cp, refcnt);

That doesn't seem to cast the pointer to nsIObserver.  nsIObserver isn't the first interface ContentParent inherits from, so aren't you getting the pointer to the PContentParent*? Am I missing something here?

> 
> It doesn't look like nsIContent{Child,Parent} is the sort of interface you
> can
> QI to.
> 

We can add an IID to it later which would allow QI.  But you're right, this doesn't need a QI, and I actually took it out at some point.

Comment 77

[Justin Lebar (not reading bugmail)]

> That doesn't seem to cast the pointer to nsIObserver.  nsIObserver isn't the first interface 
> ContentParent inherits from, so aren't you getting the pointer to the PContentParent*?

You're understanding this perfectly.  :)

It's a result of the fact that this prints a PContentParent* that I changed the pointer in nsFrameLoader.h from nsCOMPtr<nsIObserver> to nsRefPtr<ContentParent>.

Comment 78

[David Zbarsky (:dzbarsky)]

Ben, review ping?

Comment 79

[David Zbarsky (:dzbarsky)]

Attachment: Use nsIContentParent in indexeddb r=bent

Comment 80

[David Zbarsky (:dzbarsky)]

Attachment: Fix IndexedDBObjectStore to not assume that PContent connects the same processes as PBrowser r=bent

Comment 81

[David Zbarsky (:dzbarsky)]

Attachment: nsIContentChild::GetOrCreateActorForBlob should ensure the blob has the correct manager r=bent

Comment 82

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 806124 [details] [diff] [review]
Use nsIContentParent in indexeddb r=bent

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

r=me, sorry for the delay.

Comment 83

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 806125 [details] [diff] [review]
Fix IndexedDBObjectStore to not assume that PContent connects the same processes as PBrowser r=bent

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

r=me, just nits here really.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +13,5 @@
>  
>  #include "jsfriendapi.h"
>  #include "mozilla/dom/ContentChild.h"
>  #include "mozilla/dom/nsIContentParent.h"
> +#include "mozilla/dom/TabChild.h"

Nit: Please put in alphabetic order

@@ +2845,5 @@
> +  // Our "parent" process may be either the root process or another content
> +  // process if this indexedDB is managed by a PBrowser that is managed by a
> +  // PContentBridge.  We need to find which one it is so that we can create
> +  // PBlobs that are managed by the right nsIContentChild.
> +  IndexedDBChild* indexedDB =

Nit: Call this 'rootActor' maybe?

@@ +2846,5 @@
> +  // process if this indexedDB is managed by a PBrowser that is managed by a
> +  // PContentBridge.  We need to find which one it is so that we can create
> +  // PBlobs that are managed by the right nsIContentChild.
> +  IndexedDBChild* indexedDB =
> +    static_cast<IndexedDBChild*>(objectStoreActor->Manager()->Manager()->Manager());

Nit: wrap to 80 chars

@@ +3108,5 @@
>  }
>  
>  nsresult
> +AddHelper::PackArgumentsForParentProcess(ObjectStoreRequestParams& aParams,
> +                                         nsIContentChild* aBlobCreator)

In all of these please assert a non-null aBlobCreator once at the beginning of the function (after any others that may be there already)

::: dom/indexedDB/ipc/IndexedDBChild.cpp
@@ +162,5 @@
> +                               const nsCString& aASCIIOrigin)
> +: mFactory(nullptr)
> +, mManagerContent(aContentChild)
> +, mManagerTab(nullptr)
> +, mASCIIOrigin(aASCIIOrigin)

Nit: Please stick with existing style and fit as many items on a line as possible under 80 chars.

Comment 84

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 806126 [details] [diff] [review]
nsIContentChild::GetOrCreateActorForBlob should ensure the blob has the correct manager r=bent

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

r=me!

Comment 85

[David Zbarsky (:dzbarsky)]

These patches no longer pass try: https://tbpl.mozilla.org/?tree=Try&rev=43f712a3e13e

I'm not sure when I'll have time to look into this.

Comment 86

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f36185787040

Comment 87

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f36185787040

Comment 88

[Patrick McManus [:mcmanus]]

This patch breaks my build (linux clang x64). the parent cset builds.

0:10.33 nsMemoryInfoDumper.o
 0:11.79 In file included from ../../../http2/xpcom/base/nsMemoryInfoDumper.cpp:1:
 0:11.79 In file included from ../../../http2/xpcom/base/nsMemoryInfoDumper.cpp:13:
 0:11.79 ../../dist/include/mozilla/dom/ContentParent.h:254:5: error: virtual function 'AllocPCompositorParent' has a different return type ('bool') than the function it overrides (which has return type 'PCompositorParent *' (aka 'mozilla::layers::PCompositorParent *'))
 0:11.79     AllocPCompositorParent(mozilla::ipc::Transport* aTransport,
 0:11.79     ^
 0:11.80 ../../ipc/ipdl/_ipdlheaders/mozilla/dom/PContentParent.h:707:5: note: overridden virtual function is here
 0:11.80     AllocPCompositorParent(
 0:11.80     ^
 0:11.80 In file included from ../../../http2/xpcom/base/nsMemoryInfoDumper.cpp:1:
 0:11.80 In file included from ../../../http2/xpcom/base/nsMemoryInfoDumper.cpp:13:
 0:11.80 ../../dist/include/mozilla/dom/ContentParent.h:257:5: error: virtual function 'AllocPImageBridgeParent' has a different return type ('bool') than the function it overrides (which has return type 'PImageBridgeParent *' (aka 'mozilla::layers::PImageBridgeParent *'))
 0:11.80     AllocPImageBridgeParent(mozilla::ipc::Transport* aTransport,
 0:11.80     ^
 0:11.80 ../../ipc/ipdl/_ipdlheaders/mozilla/dom/PContentParent.h:711:5: note: overridden virtual function is here
 0:11.80     AllocPImageBridgeParent(
 0:11.80     ^
 0:11.80 In file included from ../../../http2/xpcom/base/nsMemoryInfoDumper.cpp:1:
 0:11.80 In file included from ../../../http2/xpcom/base/nsMemoryInfoDumper.cpp:14:
 0:11.80 ../../dist/include/mozilla/dom/ContentChild.h:81:5: error: virtual function 'AllocPCompositorChild' has a different return type ('bool') than the function it overrides (which has return type 'PCompositorChild *' (aka 'mozilla::layers::PCompositorChild *'))
 0:11.80     AllocPCompositorChild(mozilla::ipc::Transport* aTransport,
 0:11.80     ^
 0:11.80 ../../ipc/ipdl/_ipdlheaders/mozilla/dom/PContentChild.h:602:5: note: overridden virtual function is here
 0:11.80     AllocPCompositorChild(
 0:11.80     ^
 0:11.80 In file included from ../../../http2/xpcom/base/nsMemoryInfoDumper.cpp:1:
 0:11.80 In file included from ../../../http2/xpcom/base/nsMemoryInfoDumper.cpp:14:
 0:11.80 ../../dist/include/mozilla/dom/ContentChild.h:84:5: error: virtual function 'AllocPImageBridgeChild' has a different return type ('bool') than the function it overrides (which has return type 'PImageBridgeChild *' (aka 'mozilla::layers::PImageBridgeChild *'))
 0:11.80     AllocPImageBridgeChild(mozilla::ipc::Transport* aTransport,
 0:11.80     ^
 0:11.80 ../../ipc/ipdl/_ipdlheaders/mozilla/dom/PContentChild.h:606:5: note: overridden virtual function is here
 0:11.80     AllocPImageBridgeChild(
 0:11.80     ^
 0:11.80 4 errors generated.
 0:11.80 distcc[15228] ERROR: compile /home/mcmanus/src/mozilla2/wd/http2/xpcom/base/nsMemoryInfoDumper.cpp on localhost failed
 0:11.81 
 0:11.81 In the directory  /home/mcmanus/src/mozilla2/wd/obj-debug-http2/xpcom/base
 0:11.81 The following command failed to execute properly:
 0:11.81 distcc clang++ -o nsMemoryInfoDumper.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /home/mcmanus/src/mozilla2/wd/http2/config/gcc_hidden.h -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -DOS_POSIX=1 -DOS_LINUX=1 -I/home/mcmanus/src/mozilla2/wd/http2/ipc/chromium/src -I/home/mcmanus/src/mozilla2/wd/http2/ipc/glue -I../../ipc/ipdl/_ipdlheaders -I/home/mcmanus/src/mozilla2/wd/http2/xpcom/base/../build -I/home/mcmanus/src/mozilla2/wd/http2/xpcom/ds -I/home/mcmanus/src/mozilla2/wd/http2/xpcom/base -I. -I../../dist/include -I/home/mcmanus/src/mozilla2/wd/obj-debug-http2/dist/include/nspr -I/home/mcmanus/src/mozilla2/wd/obj-debug-http2/dist/include/nss -fPIC -Qunused-arguments -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/nsMemoryInfoDumper.o.pp -Qunused-arguments -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -I/home/mcmanus/src/mozilla2/wd/http2/build/unix/headers -pthread -pipe -DDEBUG -D_DEBUG -DTRACING -g -Os -freorder-blocks -fno-omit-frame-pointer -I/home/mcmanus/src/mozilla2/wd/http2/widget/gtk/compat -pthread -I/usr/include/gtk-2.0 -I/usr/lib/x86_64-linux-gnu/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/harfbuzz -I/usr/include/gtk-unix-print-2.0 /home/mcmanus/src/mozilla2/wd/http2/xpcom/base/nsMemoryInfoDumper.cpp
 0:11.81 make[5]: *** [nsMemoryInfoDumper.o] Error 1

Comment 89

[Patrick McManus [:mcmanus]]

(In reply to Patrick McManus [:mcmanus] from comment #88)
> This patch breaks my build (linux clang x64). the parent cset builds.
> 

I actually think it was just missing a CLOBBER update (or had a bad interaction with ehsahn's #include work). my build has gotten farther now.

Comment 90

[David Zbarsky (:dzbarsky)]

Attachment: fixup, child id for ContentBridgeParent

Comment 91

[Tom S [:evilpie]]

Sorry, I don't really understand this code, can you please ask somebody else for review?

Comment 92

[Kartikaya Gupta (email:[e-mail])]

As a meta-comment on this bug, there are UX designs for the future of B2G that basically do away with the "browser as a separate app" concept. The new UX design makes the browser more or less an integral part of the root process, and so splitting it out into a separate app is no longer necessary. Given that plan, I was wondering if this bug is still useful at all. Jonas says that although the built-in browser app may not use it, the work itself is still useful:

On 13-10-11 14:18 , Jonas Sicking wrote:
> We should definitely finish the work in bug 879475. It's something
> that will be useful for many other things than the built-in browser
> app. For example I'd like to force all certified (and eventually
> privileged) apps to always use out-of-process iframes for all
> not-from-app-package content. We're already seeing partners creating
> certified apps which use <iframe>s to render web content, and the
> problem is going to get much more prevalent over time. Fixing that
> will require bug 879475 to be finished.
> 
> So absolutely full steam ahead!
> 
> Only thing I would say have changed is that it's even more important
> that we land good tests given that we won't get automatic testing from
> the browser app.

Comment 93

[Kan-Ru Chen [:kanru] (UTC+9)]

I think this is still useful if we want to implement a replaceable homescreen in the future. The homescreen itself could be a separate process and has many widgets (like the metro UI).

Comment 94

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Agreed. I don't think we'll have a shortage of features that will need OOP <iframe>s.

Comment 95

[Milan Sreckovic [:milan] (needinfo for best results)]

Just to catch up - are we happy with the patches we have, is there more stuff to come?  Based on the last few comments, sounds like we do want this, regardless of the future of "browser as a separate app".  Knowing what we'll do here and how we'll do it will inform the dynamic toolbar work.  Not a fire, just checking to see where we are.

Comment 96

[David Zbarsky (:dzbarsky)]

(In reply to Milan Sreckovic [:milan] from comment #95)
> Just to catch up - are we happy with the patches we have, is there more
> stuff to come?  Based on the last few comments, sounds like we do want this,
> regardless of the future of "browser as a separate app".  Knowing what we'll
> do here and how we'll do it will inform the dynamic toolbar work.  Not a
> fire, just checking to see where we are.

I've fixed up and rebased these patches, they're in my git repo.  I do think we want this for e10s, someone just needs to fix it up a little and land it.  There are currently a few windows+linux xpcshell and mochitest failures.  Unfortunately I don't have access to a linux or windows machine and won't have time to work on this stuff for a while.

Comment 97

[Milan Sreckovic [:milan] (needinfo for best results)]

Botond, do you have a linux or windows machine where you can setup the build with the patches and see if we can reproduce the test failures?  Just to give us a head start.

Comment 98

[Milan Sreckovic [:milan] (needinfo for best results)]

Sorry, put the second comment in without the first one - David, thanks for the update, and thanks for getting this patch almost to the end.  I don't know how far we can take it without you, but we can at least see if we can reproduce the test failures locally and take it from there.  Thus the comment 97.  Thanks again.

Comment 99

[Botond Ballo [:botond] [away until Aug 12]]

(In reply to Milan Sreckovic [:milan] from comment #97)
> Botond, do you have a linux or windows machine where you can setup the build
> with the patches and see if we can reproduce the test failures?  Just to
> give us a head start.

I have a Linux machine and a Windows 8 machine, I can try to reproduce on them. Is there a try run that shows the failing tests?

Comment 100

[Botond Ballo [:botond] [away until Aug 12]]

(In reply to David Zbarsky (:dzbarsky) from comment #96)
> I've fixed up and rebased these patches, they're in my git repo.

Is it this one: https://github.com/dzbarsky/mozilla-central/tree/nestedprocesses?

Comment 101

[David Zbarsky (:dzbarsky)]

Yep, it's this one(In reply to Botond Ballo [:botond] from comment #100)
> (In reply to David Zbarsky (:dzbarsky) from comment #96)
> > I've fixed up and rebased these patches, they're in my git repo.
> 
> Is it this one:
> https://github.com/dzbarsky/mozilla-central/tree/nestedprocesses?

Yep, it's this one.  I can't find a try run that show the failing tests, but IIRC it was dom/browser-element/mochitest/priority/test_Audio.html and some xpcshell tests under netwerk/ipc.

Comment 102

[Kan-Ru Chen [:kanru] (UTC+9)]

Any news? I'd like to take this if nobody is working on it. It's a bit hard to find what had been landed and what not though..

Comment 103

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

David, can you update Kan-Ru on the status of this?

Comment 104

[David Zbarsky (:dzbarsky)]

Kan-Ru,

I've marked the patches as obsolete/checked in.  However, some of the patches attached to this bug have had changes applied to them to address review comments.  You can see the patches that remain to be landed and their ordering at https://github.com/dzbarsky/mozilla-central/commits/nestedprocesses

These patches failed a few tests on tryserver.  I can't find a try run that show the failing tests, but IIRC it was dom/browser-element/mochitest/priority/test_Audio.html on linux and some xpcshell tests under netwerk/ipc.

Thanks for volunteering to pick this up, and let me know if there's anything else I can do to help you!

Comment 105

[Kan-Ru Chen [:kanru] (UTC+9)]

Got distracted by other blocking issues. I'll back to this soon.

Comment 106

[Kan-Ru Chen [:kanru] (UTC+9)]

While un-bitrotten the patches, I found I wanted to try another approach. Instead of making these two very different class (ContentParent, ContentBridgeParent) share one common base (IContentParent), I created a new top level protocol PContentContent that bridges PContent and PContent. PContentBridge is then used to manage protocols that are required for communicating between Contents, no matter it's B2G<->Content or Content<->Content.

The patches are recorded here

https://github.com/kanru/gecko-dev/commits/nestedoop

Comment 107

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is work here still progressing?

Comment 108

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kan-Ru Chen [:kanru] from comment #106)
> While un-bitrotten the patches, I found I wanted to try another approach.
> Instead of making these two very different class (ContentParent,
> ContentBridgeParent) share one common base (IContentParent), I created a new
> top level protocol PContentContent that bridges PContent and PContent.
> PContentBridge is then used to manage protocols that are required for
> communicating between Contents, no matter it's B2G<->Content or
> Content<->Content.
> 
> The patches are recorded here
> 
> https://github.com/kanru/gecko-dev/commits/nestedoop

Hey Kan-Ru, do you have some time to try to land those patches as if by just fixing the test failures and land any changes as a followup ? I understand that you want to experiment and made more changes, but fixing this bug in its current state will unlock some deadlines for the Gaia team :)

Basically this prevent us to iterate over 'widgets', and to identify which messages needs to be bridge between the child and the grand-child, versus grandchild-parent.

Comment 109

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #108)
> (In reply to Kan-Ru Chen [:kanru] from comment #106)
> > While un-bitrotten the patches, I found I wanted to try another approach.
> > Instead of making these two very different class (ContentParent,
> > ContentBridgeParent) share one common base (IContentParent), I created a new
> > top level protocol PContentContent that bridges PContent and PContent.
> > PContentBridge is then used to manage protocols that are required for
> > communicating between Contents, no matter it's B2G<->Content or
> > Content<->Content.
> > 
> > The patches are recorded here
> > 
> > https://github.com/kanru/gecko-dev/commits/nestedoop
> 
> Hey Kan-Ru, do you have some time to try to land those patches as if by just
> fixing the test failures and land any changes as a followup ? I understand
> that you want to experiment and made more changes, but fixing this bug in
> its current state will unlock some deadlines for the Gaia team :)
> 
> Basically this prevent us to iterate over 'widgets', and to identify which
> messages needs to be bridge between the child and the grand-child, versus
> grandchild-parent.

I agree that land the original patches maybe the fastest way to unlock Gaia development. I'll put priority on this.

Comment 110

[Kan-Ru Chen [:kanru] (UTC+9)]

To use TabParent in a Content process there are some remaining issues:

* RecvSyncMessage
* RecvRpcMessage
* RecvAsyncMessage

  Call AssertAppPrincipal which needs ContentParent::KillHard

* AllocPContentPermissionRequestParent

  Uses nsIContentPermissionPrompt which is B2G process only

* AllocPOfflineCacheUpdateParent
* RecvPOfflineCacheUpdateConstructor
* DeallocPOfflineCacheUpdateParent

  Uses nsOfflineCacheUpdateService, SecurityManager and PermissionManager

* RecvSetOfflinePermission

  Uses nsOfflineCacheUpdateService

* RecvPIndexedDBConstructor

  if (!IndexedDatabaseManager::IsMainProcess()) {
    NS_RUNTIMEABORT("Not supported yet!");
  }

* RecvZoomToRect
* RecvContentReceivedTouch
* RecvUpdateZoomConstraints

  APZC assums ContentParent

Comment 111

[Kan-Ru Chen [:kanru] (UTC+9)]

I'm currently unbitrotting the original patchset. After that me and Gina will fix the remaining issues mentioned above (comment 110). I think if all goes well we will need two or three weeks to get a rather stable patchset that could be reviewed and tested. If more issues were spotted during the process, we will need more time to triage and fix bugs.

Comment 112

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Awesome, thanks Kan-Ru!

Comment 113

[David Zbarsky (:dzbarsky)]

Let me know if I can help answer any questions about the original patches!

Comment 114

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Refresh patch in one file

Refreshed patch.

Currently it couldn't pass the Necko IPC security check because ContentParent doesn't have any PBrowsers.

Nested remote layers doesn't render because the remote layer subtree is not registered to the compositor.

Comment 115

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It's great if the PContent for each "nested" child process only holds the PBrowsers that actually run in that child process. I.e. it'd be good if the PBrowser for the innermost iframe only lives in the PContent for the innermost PContent, and not for the "middle" PContent. That should give us the security properties that we want.

Comment 116

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Jonas Sicking (:sicking) from comment #115)
> It's great if the PContent for each "nested" child process only holds the
> PBrowsers that actually run in that child process. I.e. it'd be good if the
> PBrowser for the innermost iframe only lives in the PContent for the
> innermost PContent, and not for the "middle" PContent. That should give us
> the security properties that we want.

Right. Currently the PBrowsers are managed either by a PContent or a PContentBridge. In the future I'd like to move all PBrowsers back to PContent and use PContentBridge purely for communication.

Comment 117

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 001. Add an nsIContentParent api. r=jlebar

Comment 118

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 002. Add an nsIContentChild api r=jlebar

Comment 119

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 003. Use nsIContentParent in indexeddb. r=bent

Comment 120

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 004. Fix IndexedDBObjectStore to not assume that PContent connects the same processes as PBrowser r=bent

Comment 121

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 005. Implement ContentBridge r=jlebar

Comment 122

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 006. Bug 890570: Fix http auth prompts for nested content processes r=honzab

Comment 123

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 007. Allow nested remote mozbrowsers to push layer transactions to the compositor r=mattwoodrow

Comment 124

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=d96b39482e29

Comment 125

[Kan-Ru Chen [:kanru] (UTC+9)]

Hi Jonas,

I'd like to land these patches first and fix the remaining issues in followup bugs. These patches tend to bitrot quickly. Land them first then we could identify remaining issues and fix them in parallel. It also makes developers aware that they should consider nested case when writing IPC code. How do you think?

Hi Bent, Honza and Matt, you reviewed the original patches. Are you OK with landing these patches pref'ed off?

Comment 126

[Honza Bambas (:mayhemer)]

Can you please provide the patch(es) with a 7 line context as we all do?  I'd like to compare it with https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=890570&attachment=782832.  Thanks.

Comment 127

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Honza Bambas (:mayhemer) from comment #126)
> Can you please provide the patch(es) with a 7 line context as we all do? 
> I'd like to compare it with
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=890570&attachment=782832.  Thanks.

My bad. Here are the patches with 7 line context.

Comment 128

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 001. Add an nsIContentParent api. r=jlebar

Comment 129

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 002. Add an nsIContentChild api r=jlebar

Comment 130

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 003. Use nsIContentParent in indexeddb. r=bent

Comment 131

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 004. Fix IndexedDBObjectStore to not assume that PContent connects the same processes as PBrowser r=bent

Comment 132

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 005. Implement ContentBridge r=jlebar

Comment 133

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 006. Bug 890570: Fix http auth prompts for nested content processes r=honzab

Comment 134

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 007. Allow nested remote mozbrowsers to push layer transactions to the compositor r=mattwoodrow

Comment 135

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm totally happy to have patches landed but turned off. In fact, for big complex features like this I'd recommend it. As long as the patches get reviews first of course. I.e. you still need review before landing.

I'd even say it's ok to land turned on as long as it doesn't regress any existing functionality/performance/stability. But we should be careful not to enable 3rd party apps that could use <iframe mozbrowser remote> to cause exploitable crashes.

Comment 136

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=17b0f6381f37 Some xpcshell crashes

Comment 137

[Matt Woodrow (:mattwoodrow)]

Yes, fine with me too.

Comment 138

[Honza Bambas (:mayhemer)]

Comment on attachment 8430924 [details] [diff] [review]
Part 006. Bug 890570: Fix http auth prompts for nested content processes r=honzab

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

The patch now looks much much better!  Thanks.

Few comments you should consider fixing tho.

And then I'm not against landing this.

::: dom/ipc/TabChild.cpp
@@ +612,5 @@
>      }
>  };
>  
>  StaticRefPtr<TabChild> sPreallocatedTab;
> +std::map<uint64_t, nsRefPtr<TabChild> > TabChild::sTabChildMap;

according [1] this is a static initializer.  please do something like this:

// public static
std::map<uint64_t, nsRefPtr<TabChild> >& TabChild::TabChildMap()
{
  static std::map<uint64_t, nsRefPtr<TabChild> > sTabChildMap;
  return sTabChildMap;
}

And instead of a direct access to TabChild::sTabChildMap just use TabChild::TabChildMap().  Note that the initialization is not thread safe..


[1] http://www.cplusplus.com/reference/map/map/map/

::: dom/ipc/TabChild.h
@@ +258,5 @@
>      virtual ~TabChild();
>  
>      bool IsRootContentDocument();
>  
> +    uint64_t Id() {

should be const

@@ +272,5 @@
> +        static uint64_t sId = 0;
> +        sId++;
> +        aTabChild->mUniqueId = sId;
> +        sTabChildMap[sId] = aTabChild;
> +        return sId;

on what thread(s) is this called?  because this code is far from being thread-safe you should at least do MOZ_ASSERT(NS_IsMainThread()) here.

::: netwerk/ipc/NeckoParent.cpp
@@ +697,5 @@
>    }
>    return actor.forget();
>  }
>  
> +std::map<uint64_t, nsCOMPtr<nsIAuthPromptCallback> > sCallbackMap;

static init as well.

@@ +705,5 @@
> +NeckoParent::NestedFrameAuthPrompt::NestedFrameAuthPrompt(PNeckoParent* aParent,
> +                                                          uint64_t aNestedFrameId)
> +  : mNeckoParent(aParent)
> +  , mNestedFrameId(aNestedFrameId)
> +{}

{
}

@@ +731,5 @@
> +  if (mNeckoParent->SendAsyncAuthPromptForNestedFrame(mNestedFrameId,
> +                                                      spec,
> +                                                      realm,
> +                                                      callbackId)) {
> +    sCallbackMap[callbackId] = nsCOMPtr<nsIAuthPromptCallback>(callback);

Nit: this does addref/addref/release.  Try to static_cast |callback| to nsISupports* and assign directly, it should work (if not, leave like this).  Then the code will do just a single addref.

Comment 139

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=eeab2e0f8fae Looks good

Comment 140

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 001. Add an nsIContentParent api r=jlebar

Comment 141

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 002. Add an nsIContentChild api r=jlebar

Comment 142

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 003. Use nsIContentParent in indexeddb. r=bent

Comment 143

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 004. Fix IndexedDBObjectStore to not assume that PContent connects the same processes as PBrowser r=bent

Comment 144

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 005. Implement ContentBridge r=jlebar

Comment 145

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 006. Bug 890570: Fix http auth prompts for nested content processes r=honzab

Fix static initializer

Comment 146

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 007. Allow nested remote mozbrowsers to push layer transactions to the compositor r=mattwoodrow

Comment 147

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 001. Add an nsIContentParent api r=jlebar

Comment 148

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 002. Add an nsIContentChild api r=jlebar

Comment 149

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 003. Use nsIContentParent in indexeddb. r=bent

Comment 150

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 004. Fix IndexedDBObjectStore to not assume that PContent connects the same processes as PBrowser r=bent

Comment 151

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 005. Implement ContentBridge r=jlebar

Comment 152

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 006. Bug 890570: Fix http auth prompts for nested content processes r=honzab

Comment 153

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 007. Allow nested remote mozbrowsers to push layer transactions to the compositor r=mattwoodrow

Comment 154

[Kan-Ru Chen [:kanru] (UTC+9)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e0ffc1debd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f1406960c26a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3f1a5417af05
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5b65bf834d5f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a7872dbfa5ce
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/820e34a74647
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/00c2ba04f8d4

Remaining issues are tracked by bug 1020135

Comment 155

[Ed Morley [:emorley]]

Backed out for crashes running reftest on B2G:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=00c2ba04f8d4&jobname=b2g
(the try run in comment 139 didn't include B2G)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/da23ba53e4af
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1208d93b9428
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/716eb9adb1fc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2d457b6bae2d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ef5fa0fa46c3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/403e528b14dd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/43a2f570cea2

Comment 156

[Ed Morley [:emorley]]

There were also failures on the b2g emulator across multiple suites:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c5f9a189da8b&jobname=b2g_emulator
(jobs only just completed)

Btw part 6 had the wrong bug number - can you fix before landing?

Comment 157

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=29e748a0efd7 looks good

Comment 158

[Kan-Ru Chen [:kanru] (UTC+9)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8689a51c6520
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5155f3aaa479
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c32f7e7dbceb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0467d104b3f2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5132e8ddbbda
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/96ddd6ea6abd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a72228f459ec

Comment 159

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8689a51c6520
https://hg.mozilla.org/mozilla-central/rev/5155f3aaa479
https://hg.mozilla.org/mozilla-central/rev/c32f7e7dbceb
https://hg.mozilla.org/mozilla-central/rev/0467d104b3f2
https://hg.mozilla.org/mozilla-central/rev/5132e8ddbbda
https://hg.mozilla.org/mozilla-central/rev/96ddd6ea6abd
https://hg.mozilla.org/mozilla-central/rev/a72228f459ec

Comment 160

[Malini Das [:mdas] - Away, not checking bugmail]

This bug is a bit difficult to follow. Does the patch enable content process to content process communication?

Comment 161

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Malini Das [:mdas] from comment #160)
> This bug is a bit difficult to follow. Does the patch enable content process
> to content process communication?

It does. Content and Content are communicated through the PContentBridge protocol.

Comment 162

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

We'll probably want message manager communication for sibling processes.

Comment 163

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Olli Pettay [:smaug] from comment #162)
> We'll probably want message manager communication for sibling processes.

+1!

In the meantime, is there an example of using PContentBridge in js (if it is possible)?

Comment 164

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Malini Das [:mdas] from comment #163)
> (In reply to Olli Pettay [:smaug] from comment #162)
> > We'll probably want message manager communication for sibling processes.
> 
> +1!
> 
> In the meantime, is there an example of using PContentBridge in js (if it is
> possible)?

This is not possible yet. In the future if we want we could create a new type of message manager that connects sibling processes.

Currently PContentBridge only connects sibling processes that are logically parent/children. They could communicate using the familiar browser message managers.

Comment 165

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Kan-Ru Chen [:kanru] from comment #164)
> (In reply to Malini Das [:mdas] from comment #163)
> > (In reply to Olli Pettay [:smaug] from comment #162)
> > > We'll probably want message manager communication for sibling processes.
> > 
> > +1!
> > 
> > In the meantime, is there an example of using PContentBridge in js (if it is
> > possible)?
> 
> This is not possible yet. In the future if we want we could create a new
> type of message manager that connects sibling processes.
> 
> Currently PContentBridge only connects sibling processes that are logically
> parent/children. They could communicate using the familiar browser message
> managers.

Coo! Will you raise a bug about this? I'm not sure which Product/Compartment to put it in. Thanks!

It would really help b2g automation if we could have access to content-to-content message passing. Right now, we're hacking a way around basic information passing by storing data in the marionette server running in the shared b2g root process, and retrieving it from the correct child process, but this is indirect, makes us store extra data and send extra messages around, and if our needs get any more complicated, it will be difficult to figure out what the 'correct' child process will be without direct message passing.

Comment 166

[Malini Das [:mdas] - Away, not checking bugmail]

I've filed Bug 1026021 about content-to-content communication