Closed Bug 897839 Opened 7 years ago Closed 7 years ago

CrossProcessCompositorParent::AllocPLayerTransactionParent is broken

Categories

(Core :: Graphics: Layers, defect)

21 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: nrc, Assigned: nrc)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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?
(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.
Attached patch fixes (obsolete) — Splinter Review
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.
Attachment #781428 - Flags: review?(matt.woodrow)
Attached patch more fixesSplinter Review
Assignee: nobody → ncameron
Attachment #782785 - Flags: review?(matt.woodrow)
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 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 :)
Attachment #782785 - Flags: review?(matt.woodrow) → review+
(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.
(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 :-)
Attached patch fixesSplinter Review
Added some minor refactoring of isSmallPopup.

Can we explicitly test for remote content in ShouldUseOffMainThreadCompositing? That would be better, I think.
Attachment #781428 - Attachment is obsolete: true
Attachment #781428 - Flags: review?(matt.woodrow)
Attachment #783353 - Flags: review?(matt.woodrow)
Blocks: 899785
Blocks: 899787
Attachment #782785 - Flags: checkin+
Attachment #783353 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/2809a0f567ca
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.