897839 - CrossProcessCompositorParent::AllocPLayerTransactionParent is broken

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nick Cameron [:nrc]]

Getting the layer manager from sIndirectLayerTrees rather than sCurrentCompositor doesn't work.

We get assertions trying to AddRef the layer manager (btw, I don't see why we need to, we can just use a raw pointer, and in fact a LayerManagerComposite* saving the later case) because we are AddRef'ing in a different thread. Using a raw pointer and skipping that assert leads to the PluginContainer crashing often because it can't find a valid shadow layer manager.

This totally breaks OMTC on windows, not sure about other platforms.

Comment 1

[Matt Woodrow (:mattwoodrow)]

This working fine for e10s on mac and linux.

What are in the two threads in play here?

I'd expect the LayerManagerComposite to be owned by the compositor thread, and all calls that touch it via sIndirectLayerTrees to also be there.

Also, why do you have multiple processes on windows?

Comment 2

[Nick Cameron [:nrc]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> This working fine for e10s on mac and linux.
> 
> What are in the two threads in play here?
> 

Not sure, will investigate tomorrow.

> I'd expect the LayerManagerComposite to be owned by the compositor thread,
> and all calls that touch it via sIndirectLayerTrees to also be there.
> 

I would have thought so. Maybe the layer manager is being created on a different thread?

> Also, why do you have multiple processes on windows?

I don't really know. Plugins? Although I don't think I am running any, it's a pretty clean profile. I'm pretty sure I haven't accidentally enabled e10sor something.

Comment 3

[Nick Cameron [:nrc]]

Attachment: fixes

It turns out that we weren't in the wrong thread, it was just un-initialised data. Once I fixed that, we still had problems because the RenderFrameParent constructor was getting a main-thread d3d9 compositor rather than a client layer manager. That was because invisible windows (this is the pages thumbnailer, running in plugin-container) didn't get OMTC. That seems wrong.

Comment 4

[Nick Cameron [:nrc]]

Attachment: more fixes

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 781428 [details] [diff] [review]
fixes

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

::: widget/xpwidgets/nsBaseWidget.cpp
@@ +979,5 @@
>  
>  bool nsBaseWidget::ShouldUseOffMainThreadCompositing()
>  {
>    bool isSmallPopup = ((mWindowType == eWindowType_popup) &&
> +                       (mPopupType != ePopupTypePanel)); // || (mWindowType == eWindowType_invisible);

Looks like this was adding during the layers refactoring. Do we know why that was, and why it isn't needed any more?

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 782785 [details] [diff] [review]
more fixes

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +683,5 @@
>  {
>    if (!sCompositorMap) {
>      return;
>    }
> +  //while (true) {}

Not the most helpful comment :)

Comment 7

[Nicolas Silva [:nical]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Comment on attachment 781428 [details] [diff] [review]
> fixes
> 
> Review of attachment 781428 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/xpwidgets/nsBaseWidget.cpp
> @@ +979,5 @@
> >  
> >  bool nsBaseWidget::ShouldUseOffMainThreadCompositing()
> >  {
> >    bool isSmallPopup = ((mWindowType == eWindowType_popup) &&
> > +                       (mPopupType != ePopupTypePanel)); // || (mWindowType == eWindowType_invisible);
> 
> Looks like this was adding during the layers refactoring. Do we know why
> that was, and why it isn't needed any more?

This existed before (the logic probably just moved into it's on ShouldUseOffMainThreadCompositing function during the refactoring though, i don't know).

The idea behind this is that it is way overkill to use layers acceleration and OMTC for tooltips and other very small popup widgets.

Comment 8

[Nick Cameron [:nrc]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Comment on attachment 781428 [details] [diff] [review]
> fixes
> 
> Review of attachment 781428 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/xpwidgets/nsBaseWidget.cpp
> @@ +979,5 @@
> >  
> >  bool nsBaseWidget::ShouldUseOffMainThreadCompositing()
> >  {
> >    bool isSmallPopup = ((mWindowType == eWindowType_popup) &&
> > +                       (mPopupType != ePopupTypePanel)); // || (mWindowType == eWindowType_invisible);
> 
> Looks like this was adding during the layers refactoring. Do we know why
> that was, and why it isn't needed any more?

I do not know why it was added, as you say, hg blame was not useful. But I believe simply as an optimisation similar to the popup/small window stuff. Since the window is invisible its hard to tell if there is a rendering error :-)

Comment 9

[Nick Cameron [:nrc]]

Attachment: fixes

Added some minor refactoring of isSmallPopup.

Can we explicitly test for remote content in ShouldUseOffMainThreadCompositing? That would be better, I think.

Comment 10

[Nick Cameron [:nrc]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2809a0f567ca

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2809a0f567ca