Last Comment Bug 745148 - Support content processes pushing layer transactions directly to omtc in parent process
: Support content processes pushing layer transactions directly to omtc in pare...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
Depends on: 930838 598868 761962 763234 763238 776217
Blocks: 774386 b2g-layers-work b2g-e10s-work 774388
  Show dependency treegraph
 
Reported: 2012-04-13 03:55 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2013-10-24 19:03 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
first version (109.03 KB, patch)
2012-06-26 03:49 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Core of the logic being added here (19.03 KB, patch)
2012-07-10 21:42 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
roc: feedback+
Details | Diff | Splinter Review
Rebased (105.26 KB, patch)
2012-07-11 14:11 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part 0: Log the PID with MOZ_IPC_MESSAGE_LOG logging (1.25 KB, patch)
2012-07-12 17:17 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part 1: SyncLaunch() content processes when we're using direct-to-omtc drawing (7.60 KB, patch)
2012-07-12 17:27 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part 1: Allow sending messages on an Opening channel, since they'll be queued if we're really actually still awaiting connect (2.42 KB, patch)
2012-07-12 23:34 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
2: GeckoChildProcessHost can't drop messages on the floor. Queue them and hand them off to the *Channel. (11.62 KB, patch)
2012-07-12 23:41 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part 3: SyncLaunch() content processes when we're using direct-to-omtc drawing (7.60 KB, patch)
2012-07-12 23:41 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part 4: Remove duplicated code in Basic*ContainerLayer (8.06 KB, patch)
2012-07-13 14:18 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
roc: review+
Details | Diff | Splinter Review
part 5: Pass the layer tree to ShadowLayersUpdate() (6.13 KB, patch)
2012-07-13 14:19 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
ajuma.bugzilla: review+
Details | Diff | Splinter Review
part 6: Allow layer trees to be given IDs so that the referent can be used in another context (28.76 KB, patch)
2012-07-13 14:20 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
cjones.bugs: review+
Details | Diff | Splinter Review
part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition (26.65 KB, patch)
2012-07-13 14:20 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
b56girard: review-
roc: superreview+
Details | Diff | Splinter Review
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor (31.61 KB, patch)
2012-07-13 14:22 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part 9: Hook up the pieces and enable direct compositor (10.52 KB, patch)
2012-07-13 14:22 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Rollup, based on m-c rev 1dbe06319739 (222.73 KB, patch)
2012-07-13 14:43 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part 9: Hook up the pieces and enable direct compositor, v2 (45.24 KB, patch)
2012-07-15 00:20 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
roc: review+
Details | Diff | Splinter Review
part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition, v2 (26.90 KB, patch)
2012-07-16 11:40 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
b56girard: review+
cjones.bugs: superreview+
Details | Diff | Splinter Review
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v2 (31.88 KB, patch)
2012-07-16 11:48 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
cjones.bugs: review+
Details | Diff | Splinter Review
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v3 (32.47 KB, patch)
2012-07-16 12:36 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
b56girard: review+
cjones.bugs: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-13 03:55:05 PDT
In b2g, we'll have a set of content processes that want to push their updates directly to the off-main-thread compositor in the top-most parent (server) process.

Basically, we need to hand the widget code an "opens" or "bridge" channel to the compositor's thread context.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-20 17:01:28 PDT
Should we extend CompositorFrameParent to support this? We don't want to duplicate that code.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-20 17:07:06 PDT
There's no such animal.  My WIP is http://hg.mozilla.org/users/cjones_mozilla.com/mcmq/file/6e11a9641634/745148-directcompositor if you want to get a sense of what things look like.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-26 03:49:51 PDT
Created attachment 636647 [details] [diff] [review]
first version
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-10 21:42:02 PDT
Created attachment 640912 [details] [diff] [review]
Core of the logic being added here

The real patch is of course much larger than this, but it's all boilerplate to get here.

I suspect adding a RefLayer type will be contentious, hence feedback request.  Alternate suggestions welcome.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-10 21:43:02 PDT
Please ignore the |sCurrent| stuff in CompositorParent, it's a temporary hack.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-10 22:04:23 PDT
Comment on attachment 640912 [details] [diff] [review]
Core of the logic being added here

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

This seems reasonable assuming we can later extend RefLayer to refer to layer subtrees in the same layer manager, perhaps by adding a way to attach referent IDs to individual layers.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-11 14:11:28 PDT
Created attachment 641193 [details] [diff] [review]
Rebased
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-12 17:17:49 PDT
Created attachment 641666 [details] [diff] [review]
part 0: Log the PID with MOZ_IPC_MESSAGE_LOG logging

Trivial helper.
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-12 17:27:19 PDT
Created attachment 641668 [details] [diff] [review]
part 1: SyncLaunch() content processes when we're using direct-to-omtc drawing

This really stinks, but blame win32 and its lack of fork().

The motivating problem is, when we're using PCompositor to send messages directly from the content main thread to the compositor thread in the parent process, we need to PCompositor::Open() *right* after launching the content process.  Before any PBrowsers are created.

But to open the new channel, we have to use ShareHandle() on windows to send the child end of the pipe, and this requires having the HANDLE for the new process.  And getting that HANDLE requires disk IO.  Sigh.

This is bad, but I don't see a way around it, unfortunately.  On the bright side though, we only do this when we're using off-main-thread compositing, which means we can hide any latency here with an async animation.
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-12 17:27:50 PDT
I had one more patch for you, but looking back over it I'm not sure it's needed.  Will clean up and verify asap.
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-12 23:34:50 PDT
Created attachment 641754 [details] [diff] [review]
part 1: Allow sending messages on an Opening channel, since they'll be queued if we're really actually still awaiting connect

Oh yeah, I remember what the problem was now.  Sigh.

So here's the story.  Chapter 1.  With content processes and OMTC, the main process does PCompositor::Open() to the content process, sending it a compositor Channel.  Previously, when opening these kinds of Channels, we blocked until we received the other side's pid.  That turns out to be unnecessary for "Opens" channels and child-process channels --- we already have the PID.

Who cares, you're probably asking yourself.  Well, we PCompositor::Open() asynchronously from the parent main thread.  The main thread goes back to its business, let's say painting using the sync PLayers protocol.  Meanwhile, we initialize the CompositorParent on the compositor thread, blocking in AsyncChannel::Open() on the other-side pid to be returned by the content-process main thread.  And when the content process starts up, the first thing it does is initialize the crash reporter, through sync SendPCrashReporterConstructor.  Hm.

 parent main thread --blocked on--> compositor thread
 compositor thread --blocked on--> content process
 content process --blocked on--> parent main thread

Whups.  The reason compositor thread blocks on content process is just to find out its pid basically, even though it already has it.  So we break the cycle there.

This patch doesn't wait for OnConnected, since we can send/recv IPC just fine before.
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-12 23:41:20 PDT
Created attachment 641756 [details] [diff] [review]
2: GeckoChildProcessHost can't drop messages on the floor.  Queue them and hand them off to the *Channel.

Chapter 2.  After that fix, I noticed the content process would hang on startup sometimes.  The content main thread was blocked on the sync PCrashReporter ctor again.

After some poking, I found that the ctor message was arriving at the parent process and being dispatch to the Channel listener.  We were just never sending a reply, so the content process stayed hung.

Then this hypothesis came to me while eating pizza, and I added a MOZ_NOT_REACHED() to GeckoChildProcessHost::OnMessageReceived(), and it blew up.  So what was happening is that the sync ctor message was being dropped on the floor.

This patch queues up those too-early messages and then hands them off for delivery when we switch channel listeners.

This bug has been latent forever.  I think the combination of SyncLaunch() (waiting for OnConnected) and the sync PCrashReporter() ctor asap after Channel::Open exarcerbated the problem.  But, even without those this bug could still pop up.
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-12 23:41:53 PDT
Created attachment 641757 [details] [diff] [review]
part 3: SyncLaunch() content processes when we're using direct-to-omtc drawing

Same patch as before.
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 03:49:34 PDT
I'm going to reuse some machinery Nico built in bug 598868 for finding the right CompositorParent to trigger recomposition for on async updates.
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 14:18:52 PDT
Created attachment 642046 [details] [diff] [review]
part 4: Remove duplicated code in Basic*ContainerLayer
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 14:19:17 PDT
Created attachment 642047 [details] [diff] [review]
part 5: Pass the layer tree to ShadowLayersUpdate()
Comment 17 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 14:20:05 PDT
Created attachment 642049 [details] [diff] [review]
part 6: Allow layer trees to be given IDs so that the referent can be used in another context

This won't compile everywhere but I'll mop up those trivial compilation fixes in parallel.
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 14:20:40 PDT
Created attachment 642050 [details] [diff] [review]
part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition
Comment 19 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 14:22:10 PDT
Created attachment 642053 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor

Not really a good way to split this up more, but it's mostly boilerplate.  The CompositorParent hunks are most interesting.  Please let me know if you don't comfortable reviewing the IPC bits.
Comment 20 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 14:22:36 PDT
Created attachment 642054 [details] [diff] [review]
part 9: Hook up the pieces and enable direct compositor
Comment 21 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 14:43:21 PDT
Created attachment 642059 [details] [diff] [review]
Rollup, based on m-c rev 1dbe06319739

Includes dependencies.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-13 16:02:01 PDT
Comment on attachment 642046 [details] [diff] [review]
part 4: Remove duplicated code in Basic*ContainerLayer

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

Nice.
Comment 23 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-07-13 19:43:19 PDT
Comment on attachment 641756 [details] [diff] [review]
2: GeckoChildProcessHost can't drop messages on the floor.  Queue them and hand them off to the *Channel.

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

::: ipc/glue/AsyncChannel.cpp
@@ +715,5 @@
> +
> +    // Dispatch whatever messages the previous listener had queued up.
> +    while (!pending.empty()) {
> +        OnMessageReceived(pending.front());
> +        pending.pop();

Should you pop before calling OnMessageReceived in case it reenters somehow? Something like:

  Message* msg = pending.front();
  pending.pop();
  OnMessageReceived(msg);

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +692,4 @@
>  void
>  GeckoChildProcessHost::OnChannelError()
>  {
>    // XXXbent Notify that the child process is gone?

Should probably file a followup since this can happen too, right?

::: ipc/glue/GeckoChildProcessHost.h
@@ +106,5 @@
>                                    base::ProcessArchitecture arch);
> +
> +  // There's a short window in between we launch the subprocess and we
> +  // hand the channel off that messages can be received by this, while
> +  // it's still the channel listener.  If so, the messages would

Nit: This comment is all sorts of grammatical nightmare. Must have been late ;) Let's clean it up a little.
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 20:18:54 PDT
(In reply to ben turner [:bent] from comment #23)
> Comment on attachment 641756 [details] [diff] [review]
> 2: GeckoChildProcessHost can't drop messages on the floor.  Queue them and
> hand them off to the *Channel.
> 
> Review of attachment 641756 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/glue/AsyncChannel.cpp
> @@ +715,5 @@
> > +
> > +    // Dispatch whatever messages the previous listener had queued up.
> > +    while (!pending.empty()) {
> > +        OnMessageReceived(pending.front());
> > +        pending.pop();
> 
> Should you pop before calling OnMessageReceived in case it reenters somehow?

There's no possibility of reentry, but also no reason to use a nonstandard idiom.  Will change.

> >  {
> >    // XXXbent Notify that the child process is gone?
> 
> Should probably file a followup since this can happen too, right?
> 

Sure.  Our wild youth is catching up with us ;).

> ::: ipc/glue/GeckoChildProcessHost.h
> @@ +106,5 @@
> >                                    base::ProcessArchitecture arch);
> > +
> > +  // There's a short window in between we launch the subprocess and we
> > +  // hand the channel off that messages can be received by this, while
> > +  // it's still the channel listener.  If so, the messages would
> 
> Nit: This comment is all sorts of grammatical nightmare. Must have been late
> ;) Let's clean it up a little.

It was both late and a nightmare.
Comment 25 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-07-13 20:46:51 PDT
Comment on attachment 641757 [details] [diff] [review]
part 3: SyncLaunch() content processes when we're using direct-to-omtc drawing

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

::: dom/ipc/ContentParent.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#if defined(ANDROID) || defined(XP_UNIX)
> +# include <sys/time.h>
> +# include <sys/resource.h>

Aha! You're the one who does this crazy indent thing. Ordinarily I'd balk, but you did alphabetize everything... Tough choice.
Comment 26 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 23:15:25 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #24)
> (In reply to ben turner [:bent] from comment #23)
> > Comment on attachment 641756 [details] [diff] [review]
> > 2: GeckoChildProcessHost can't drop messages on the floor.  Queue them and
> > hand them off to the *Channel.
> > 
> > Review of attachment 641756 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: ipc/glue/AsyncChannel.cpp
> > @@ +715,5 @@
> > > +
> > > +    // Dispatch whatever messages the previous listener had queued up.
> > > +    while (!pending.empty()) {
> > > +        OnMessageReceived(pending.front());
> > > +        pending.pop();
> > 
> > Should you pop before calling OnMessageReceived in case it reenters somehow?
> 
> There's no possibility of reentry, but also no reason to use a nonstandard
> idiom.  Will change.
> 

Oh, now I remember what this was doing.  It's queue<Message> (not Message*), so the impl in the patch avoids a copy when calling OnMessageReceived.  So I didn't change this.
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-14 03:27:20 PDT
Comment on attachment 642049 [details] [diff] [review]
part 6: Allow layer trees to be given IDs so that the referent can be used in another context

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

This could also be considered an r+ if that helps.
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-14 03:56:56 PDT
Comment on attachment 642050 [details] [diff] [review]
part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition

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

::: gfx/layers/Layers.h
@@ +1431,5 @@
> +  virtual void InsertAfter(Layer* aChild, Layer* aAfter)
> +  { MOZ_NOT_REACHED("no"); }
> +
> +  virtual void RemoveChild(Layer* aChild)
> +  { MOZ_NOT_REACHED("no"); }

It would be cleaner to not expose RefLayer's ContainerLayer-ness. Private inheritance might work but we don't often use that. This will be much easier to address once layers are split into content-side and compositor-side objects, so let's not worry about it for now.

@@ +1442,5 @@
> +   * Set the ID of the layer's referent.
> +   */
> +  void SetReferentId(int64_t aId)
> +  {
> +    mId = aId;

Assert that aId is >= 0?

::: gfx/layers/ipc/CompositorParent.cpp
@@ +28,5 @@
>  static Thread* sCompositorThread = nsnull;
>  
> +/**
> + */
> +static Layer* GetIndirectShadowTree(int64_t aId);

Remove non-comment
Comment 29 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-14 04:07:13 PDT
Comment on attachment 642054 [details] [diff] [review]
part 9: Hook up the pieces and enable direct compositor

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

I don't understand why we need mIndirectLayerManager. It seems a bit vestigial. Why can't we use some other means to ensure RenderFrameParent creates a RefLayer?

::: layout/ipc/RenderFrameParent.cpp
@@ +552,5 @@
> +
> +    nsRefPtr<RefLayer> layer = aManager->CreateRefLayer();
> +    if (!layer) {
> +      // Probably a temporary layer manager that doesn't know how to
> +      // use ref layers.

Do we actually hit this? We shouldn't, right? Shouldn't this be an NS_ERROR?
Comment 30 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-14 13:52:45 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> Comment on attachment 642049 [details] [diff] [review]
> part 6: Allow layer trees to be given IDs so that the referent can be used
> in another context
> 
> Review of attachment 642049 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This could also be considered an r+ if that helps.

Sure!  Thanks.
Comment 31 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-14 13:55:33 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> Comment on attachment 642054 [details] [diff] [review]
> part 9: Hook up the pieces and enable direct compositor
> 
> Review of attachment 642054 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand why we need mIndirectLayerManager. It seems a bit
> vestigial. Why can't we use some other means to ensure RenderFrameParent
> creates a RefLayer?
> 

We don't absolutely need it.  I'll reorganize things so that RenderFrameParent doesn't use that layer manager anymore.

> ::: layout/ipc/RenderFrameParent.cpp
> @@ +552,5 @@
> > +
> > +    nsRefPtr<RefLayer> layer = aManager->CreateRefLayer();
> > +    if (!layer) {
> > +      // Probably a temporary layer manager that doesn't know how to
> > +      // use ref layers.
> 
> Do we actually hit this? We shouldn't, right? Shouldn't this be an NS_ERROR?

We do.
Comment 32 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-14 14:22:44 PDT
Pushed 0-2 since those are more generic things we might step on

https://hg.mozilla.org/integration/mozilla-inbound/rev/d84d1d760483
https://hg.mozilla.org/integration/mozilla-inbound/rev/453481dbb828
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f19e4f90e7

Please leave this bug open.
Comment 33 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-14 17:34:54 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> > Comment on attachment 642054 [details] [diff] [review]
> > part 9: Hook up the pieces and enable direct compositor
> > 
> > Review of attachment 642054 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I don't understand why we need mIndirectLayerManager. It seems a bit
> > vestigial. Why can't we use some other means to ensure RenderFrameParent
> > creates a RefLayer?
> > 
> 
> We don't absolutely need it.  I'll reorganize things so that
> RenderFrameParent doesn't use that layer manager anymore.
> 

Hmm ... I started to change this but it's actually somewhat nontrivial.  How strongly do you feel about changing this for this bug?
Comment 35 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-14 20:06:06 PDT
See comment 32.  Thanks for merging!
Comment 36 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-14 21:37:47 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #33)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> > > Comment on attachment 642054 [details] [diff] [review]
> > > part 9: Hook up the pieces and enable direct compositor
> > > 
> > > Review of attachment 642054 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I don't understand why we need mIndirectLayerManager. It seems a bit
> > > vestigial. Why can't we use some other means to ensure RenderFrameParent
> > > creates a RefLayer?
> > > 
> > 
> > We don't absolutely need it.  I'll reorganize things so that
> > RenderFrameParent doesn't use that layer manager anymore.
> > 
> 
> Hmm ... I started to change this but it's actually somewhat nontrivial.  How
> strongly do you feel about changing this for this bug?

Never mind again, I have to do pretty much the same thing for bug 750977, so might as well knock it out here.
Comment 37 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-15 00:20:13 PDT
Created attachment 642346 [details] [diff] [review]
part 9: Hook up the pieces and enable direct compositor, v2
Comment 38 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-15 03:54:52 PDT
With latest inbound and gaia, I'm seeing a ColorLayer being set as the root of the layer tree a content process.  That breaks a lot of things.
Comment 39 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-15 13:51:50 PDT
This doesn't happen on a desktop-b2g build.  Very disturbing.
Comment 40 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-15 14:00:01 PDT
We're hitting

#1  0x406d52c2 in PresShell::Paint (this=0x2187370, aViewToPaint=0x216e5f0, aWidgetToPaint=0x1f1d168, aDirtyRegion=..., aIntDirtyRegion=..., aWillSendDidPaint=true) at /home/cjones/mozilla/inbound/layout/base/nsPresShell.cpp:5293

  nsRefPtr<ColorLayer> root = layerManager->CreateColorLayer();
  if (root) {
    nsPresContext* pc = GetPresContext();
    nsIntRect bounds =
      pc->GetVisibleArea().ToOutsidePixels(pc->AppUnitsPerDevPixel());
    bgcolor = NS_ComposeColors(bgcolor, mCanvasBackgroundColor);
    root->SetColor(bgcolor);
    root->SetVisibleRegion(bounds);
    layerManager->SetRoot(root);

I was under the impression that the root layer must be a container layer, but that's not mentioned anywhere in Layers.h.  OK.
Comment 41 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-15 14:13:01 PDT
Folded this patch

   void TemporarilyCompensateForContentScrollOffset(Layer* aContainer,
                                                    Layer* aShadowContent)
   {
     ContainerLayer* c = aShadowContent->AsContainerLayer();
+    if (!c) {
+      return;
+    }

into part 8.
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-15 15:47:21 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #40)
> I was under the impression that the root layer must be a container layer,
> but that's not mentioned anywhere in Layers.h.  OK.

Right, sometimes we paint before we've constructed a root frame.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> > ::: layout/ipc/RenderFrameParent.cpp
> > @@ +552,5 @@
> > > +
> > > +    nsRefPtr<RefLayer> layer = aManager->CreateRefLayer();
> > > +    if (!layer) {
> > > +      // Probably a temporary layer manager that doesn't know how to
> > > +      // use ref layers.
> > 
> > Do we actually hit this? We shouldn't, right? Shouldn't this be an NS_ERROR?
> 
> We do.

Under what conditions?
Comment 43 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-15 16:03:17 PDT
At least for -moz-element.
Comment 44 Benoit Girard (:BenWa) 2012-07-16 10:25:55 PDT
Comment on attachment 642050 [details] [diff] [review]
part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition

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

r- since I'd like to see the clarifications below before it goes in even if they are minor.

::: gfx/layers/Layers.h
@@ +1421,5 @@
> + * Composition:
> + *   look up subtree for GetReferentId()
> + *   ConnectReferentLayer(subtree)
> + *   compose
> + *   ClearReferentLayer()

It's not obvious to me why you want to connect/clear the referent layer for every composite. I think it's worth documenting that here.

@@ +1442,5 @@
> +   * Set the ID of the layer's referent.
> +   */
> +  void SetReferentId(int64_t aId)
> +  {
> +    mId = aId;

Instead of asserting that mId is >= 0 shouldn't we just use uint?

@@ +1462,5 @@
> +  /**
> +   * DRAWING PHASE ONLY
> +   * |aLayer| is the same as the argument to ConnectReferentLayer().
> +   */
> +  void ClearReferentLayer(Layer* aLayer)

Maybe we should rename this to disconnect/detach to better reflect that we're not clearing the referent layer but rather just detaching.

@@ +1491,5 @@
> +
> +  virtual nsACString& PrintInfo(nsACString& aTo, const char* aPrefix);
> +
> +  Layer* mTempReferent;
> +  int64_t mId;

Should we document that '0' is a special value? If for nothing else it's useful for asserting that mId has been set without introducing a debug only flag.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +294,5 @@
> +/**
> + * DRAWING PHASE ONLY
> + *
> + * For reach RefLayer in |aRoot|, look up its referent and connect it
> + * to the layer tree, if found.  On exiting scope, clears all resolved

clear=disconnect?

@@ +313,5 @@
> +
> +private:
> +  enum Op { Resolve, Clear };
> +  template<Op OP>
> +  void WalkTheTree(Layer* aLayer, Layer* aParent)

Why is this a template instead of making Op a parameter to WalkTheTree? As I understand it this will create two instance of this function.

@@ +332,5 @@
> +  }
> +
> +  Layer* mRoot;
> +
> +  AutoResolveRefLayers(const AutoResolveRefLayers&);

MOZ_DELETE

::: gfx/layers/ipc/ShadowLayersParent.cpp
@@ +265,5 @@
>            specific.get_CanvasLayerAttributes().filter());
>          break;
>  
> +      case Specific::TRefLayerAttributes:
> +        MOZ_LAYERS_LOG(("[ParentSide]   canvas layer"));

s/canvas/reference
Comment 45 Ali Juma [:ajuma] 2012-07-16 10:32:40 PDT
Comment on attachment 642053 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor

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

r+ with followups filed for the three "FIXME/bug XXXXX" comments.
Comment 46 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 11:20:32 PDT
(In reply to Benoit Girard (:BenWa) from comment #44)

> ::: gfx/layers/Layers.h
> @@ +1421,5 @@
> > + * Composition:
> > + *   look up subtree for GetReferentId()
> > + *   ConnectReferentLayer(subtree)
> > + *   compose
> > + *   ClearReferentLayer()
> 
> It's not obvious to me why you want to connect/clear the referent layer for
> every composite. I think it's worth documenting that here.
> 

 * Clients will usually want to Connect/Clear() on each transaction to
 * avoid difficulties managing memory across multiple layer subtrees.

> @@ +1442,5 @@
> > +   * Set the ID of the layer's referent.
> > +   */
> > +  void SetReferentId(int64_t aId)
> > +  {
> > +    mId = aId;
> 
> Instead of asserting that mId is >= 0 shouldn't we just use uint?
> 

It was supposed to be uint64_t, this is a typo.  But we want to assert that it's != 0, i.e. a valid ID.  Having a RefLayer with the "null ID" is meaningless.

> @@ +1462,5 @@
> > +  /**
> > +   * DRAWING PHASE ONLY
> > +   * |aLayer| is the same as the argument to ConnectReferentLayer().
> > +   */
> > +  void ClearReferentLayer(Layer* aLayer)
> 
> Maybe we should rename this to disconnect/detach to better reflect that
> we're not clearing the referent layer but rather just detaching.
> 

Disconnect() is quite overloaded in gfx/layers.  Switched to Detach*() isn't.  

> @@ +1491,5 @@
> > +
> > +  virtual nsACString& PrintInfo(nsACString& aTo, const char* aPrefix);
> > +
> > +  Layer* mTempReferent;
> > +  int64_t mId;
> 
> Should we document that '0' is a special value? If for nothing else it's
> useful for asserting that mId has been set without introducing a debug only
> flag.
> 

  // 0 is a special value that means "no ID".

> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +294,5 @@
> > +/**
> > + * DRAWING PHASE ONLY
> > + *
> > + * For reach RefLayer in |aRoot|, look up its referent and connect it
> > + * to the layer tree, if found.  On exiting scope, clears all resolved
> 
> clear=disconnect?
> 

s/clear/detach/

> @@ +313,5 @@
> > +
> > +private:
> > +  enum Op { Resolve, Clear };
> > +  template<Op OP>
> > +  void WalkTheTree(Layer* aLayer, Layer* aParent)
> 
> Why is this a template instead of making Op a parameter to WalkTheTree? As I
> understand it this will create two instance of this function.
> 

That's what we want.  Compilers are highly optimized to boil away constant parameters of templates.

> @@ +332,5 @@
> > +  }
> > +
> > +  Layer* mRoot;
> > +
> > +  AutoResolveRefLayers(const AutoResolveRefLayers&);
> 
> MOZ_DELETE
> 

Fixed.

> ::: gfx/layers/ipc/ShadowLayersParent.cpp
> @@ +265,5 @@
> >            specific.get_CanvasLayerAttributes().filter());
> >          break;
> >  
> > +      case Specific::TRefLayerAttributes:
> > +        MOZ_LAYERS_LOG(("[ParentSide]   canvas layer"));
> 
> s/canvas/reference

Fixed.
Comment 47 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 11:40:01 PDT
Created attachment 642662 [details] [diff] [review]
part 7: Create a RefLayer type to temporarily contain a foreign layer subtree during composition, v2

Addressed comments, carrying over sr=roc.
Comment 48 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 11:47:18 PDT
(In reply to Ali Juma [:ajuma] from comment #45)
> Comment on attachment 642053 [details] [diff] [review]
> part 8: Implement the little boilerplate-y bits and pieces needed for
> cross-process compositor
> 
> Review of attachment 642053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with followups filed for the three "FIXME/bug XXXXX" comments.

Thanks for the catch!  This slipped off my to-do list.
Comment 49 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 11:48:05 PDT
Created attachment 642666 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v2

Carrying over r=ajuma.
Comment 50 Benoit Girard (:BenWa) 2012-07-16 11:54:52 PDT
Comment on attachment 642053 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor

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

Posting my review draft I wrote before the patch was updated.

::: gfx/layers/ipc/CompositorChild.cpp
@@ +44,5 @@
> +{
> +  // There's only one compositor per child process.
> +  MOZ_ASSERT(!sCompositor);
> +
> +  nsRefPtr<CompositorChild> cc(new CompositorChild(nsnull));

Not a fan of variable shorthand.

@@ +57,5 @@
> +    NS_RUNTIMEABORT("Couldn't Open() Compositor channel.");
> +    return nsnull;
> +  }
> +  // We release this ref in ActorDestroy().
> +  return sCompositor = cc.forget().get();

Why not make sCompositor a RefPtr?

@@ +78,5 @@
> +CompositorChild::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  MOZ_ASSERT(sCompositor == this);
> +  sCompositor = NULL;
> +  MessageLoop::current()->PostTask(

Comment to explain why we're posting this as a task would be nice. Is this accurate/
// Release doesn't need to happen synchronously, postpone it.

::: gfx/layers/ipc/CompositorChild.h
@@ +43,4 @@
>  private:
>    nsRefPtr<LayerManager> mLayerManager;
>  
> +  // When we're in a child process, this is the process-global

What about when we're not in a child process? Maybe we can add a getter and assert if this is only valid in the child process?

::: gfx/layers/ipc/CompositorParent.cpp
@@ +28,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> +// FIXME/bug XXXXXX: There can only be one.  No wait, can be *more*
> +// than one.  Fix this.

If you plan on landing this as-is you should clarify the comment.
Comment 51 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 12:09:23 PDT
(In reply to Benoit Girard (:BenWa) from comment #50)
> Comment on attachment 642053 [details] [diff] [review]
> part 8: Implement the little boilerplate-y bits and pieces needed for
> cross-process compositor
> 
> ::: gfx/layers/ipc/CompositorChild.cpp
> @@ +44,5 @@
> > +{
> > +  // There's only one compositor per child process.
> > +  MOZ_ASSERT(!sCompositor);
> > +
> > +  nsRefPtr<CompositorChild> cc(new CompositorChild(nsnull));
> 
> Not a fan of variable shorthand.
> 

s/cc/child/

> @@ +57,5 @@
> > +    NS_RUNTIMEABORT("Couldn't Open() Compositor channel.");
> > +    return nsnull;
> > +  }
> > +  // We release this ref in ActorDestroy().
> > +  return sCompositor = cc.forget().get();
> 
> Why not make sCompositor a RefPtr?
> 

It creates static ctors which slow down startup.  We need jlebar's StaticRefPtr to fix this properly.

> @@ +78,5 @@
> > +CompositorChild::ActorDestroy(ActorDestroyReason aWhy)
> > +{
> > +  MOZ_ASSERT(sCompositor == this);
> > +  sCompositor = NULL;
> > +  MessageLoop::current()->PostTask(
> 
> Comment to explain why we're posting this as a task would be nice. Is this
> accurate/
> // Release doesn't need to happen synchronously, postpone it.
> 

  // We don't want to release the ref to sCompositor here, during
  // cleanup, because that will cause it to be deleted while it's
  // still being used.  So defer the deletion to after it's not in
  // use.

> ::: gfx/layers/ipc/CompositorChild.h
> @@ +43,4 @@
> >  private:
> >    nsRefPtr<LayerManager> mLayerManager;
> >  
> > +  // When we're in a child process, this is the process-global
> 
> What about when we're not in a child process? Maybe we can add a getter and
> assert if this is only valid in the child process?
> 

I don't feel very strongly about this, but went ahead and moved the getter into the .cpp and added an assert.

> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +28,5 @@
> >  namespace mozilla {
> >  namespace layers {
> >  
> > +// FIXME/bug XXXXXX: There can only be one.  No wait, can be *more*
> > +// than one.  Fix this.
> 
> If you plan on landing this as-is you should clarify the comment.

Yeah, it was early in the morning

// FIXME/bug 774386: we're assuming that there's only one
// CompositorParent, but that's not always true.  This assumption only
// affects CrossProcessCompositorParent below.
Comment 52 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 12:36:18 PDT
Created attachment 642682 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v3
Comment 53 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 12:36:49 PDT
Comment on attachment 642682 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v3

Carrying over r=ajuma.
Comment 54 Benoit Girard (:BenWa) 2012-07-16 13:57:47 PDT
Comment on attachment 642682 [details] [diff] [review]
part 8: Implement the little boilerplate-y bits and pieces needed for cross-process compositor, v3

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +736,5 @@
> +  static uint64_t ids;
> +  return ++ids;
> +}
> +
> +/** */

Missing a comment here. Maybe how this class differs from PCompositorParent.

@@ +814,2 @@
>  {
> +  LayerTreeMap::const_iterator cit = sIndirectLayerTrees.find(aId);

Quite a few variable acronym in this page. I have no idea what the 'c' stands for here, let's use something more obvious.
Comment 55 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 14:54:09 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> Comment on attachment 642050 [details] [diff] [review]
> part 7: Create a RefLayer type to temporarily contain a foreign layer
> subtree during composition
> 
> @@ +1442,5 @@
> > +   * Set the ID of the layer's referent.
> > +   */
> > +  void SetReferentId(int64_t aId)
> > +  {
> > +    mId = aId;
> 
> Assert that aId is >= 0?
> 

Done, --> uint64_t and asserted != 0.

> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +28,5 @@
> >  static Thread* sCompositorThread = nsnull;
> >  
> > +/**
> > + */
> > +static Layer* GetIndirectShadowTree(int64_t aId);
> 
> Remove non-comment

I meant to document this ... added

/**
 * Lookup the indirect shadow tree for |aId| and return it if it
 * exists.  Otherwise null is returned.  This must only be called on
 * the compositor thread.
 */
Comment 56 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-16 15:08:32 PDT
(In reply to Benoit Girard (:BenWa) from comment #54)
> Comment on attachment 642682 [details] [diff] [review]
> part 8: Implement the little boilerplate-y bits and pieces needed for
> cross-process compositor, v3
> 
> Review of attachment 642682 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +736,5 @@
> > +  static uint64_t ids;
> > +  return ++ids;
> > +}
> > +
> > +/** */
> 
> Missing a comment here. Maybe how this class differs from PCompositorParent.
> 

/**
 * This class handles layer updates pushed directly from child
 * processes to the compositor thread.  It's associated with a
 * CompositorParent on the compositor thread.  While it uses the
 * PCompositor protocol to manage these updates, it doesn't actually
 * drive compositing itself.  For that it hands off work to the
 * CompositorParent it's associated with.
 */

> @@ +814,2 @@
> >  {
> > +  LayerTreeMap::const_iterator cit = sIndirectLayerTrees.find(aId);
> 
> Quite a few variable acronym in this page. I have no idea what the 'c'
> stands for here, let's use something more obvious.

In this case here, "cit" is an STL idiom that's shorthand for "Const ITerator".
Comment 57 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-17 01:29:38 PDT
Kinda cute: every single try build failed on https://tbpl.mozilla.org/?tree=Try&rev=83b060905e12, but all of my local {mac,android,b2g,linux} builds succeed.  (Even with clobbers.) :|

Note You need to log in before you can comment on or make changes to this bug.