728284 - Main container layer has siblings that are drawn in the background

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Joe Drew (not getting mail)]

The left and right edges of the viewport (when you pan all the way to the edge of the viewport, or zoom far far out) are either black or have the texture repeated inside them (or are otherwise corrupt).

This was on Ehsan's Galaxy Nexus.

Comment 1

[Joe Drew (not getting mail)]

If this is a general issue, it'll presumably need to be fixed before the mwc demo

Comment 2

[Patrick Walton (:pcwalton)]

Do you mean the "void beneath the web" -- i.e. not content itself, but the decorations?

Comment 3

[Joe Drew (not getting mail)]

I *think* so. It certainly looked as though all the content that was meant to be there was, indeed, there.

Comment 4

[Patrick Walton (:pcwalton)]

Ok. This is an issue with the Java compositor integration then.

Comment 5

[Erin Lancaster [:elan]]

who should be assigned to this bug? As far as I understand, this is a mwc blocker.

Comment 6

[Joe Drew (not getting mail)]

Patrick/Chris Lord/any of the graphics team who are free (which currently is the empty set).

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Can't see this anymore.

Comment 8

[Joe Drew (not getting mail)]

Ehsan still sees this on his Galaxy Nexus.

Comment 9

[Benoit Girard (:BenWa)]

Still an issue here as well. Picking this up.

Comment 10

[Benoit Girard (:BenWa)]

Partial fix:
https://hg.mozilla.org/projects/maple/rev/40b477c54da3

This is a small improvement but there are several bugs at play here.

Comment 11

[George Wright (:gw280) (needinfo me!)]

My findings so far:

- Putting an early return in ThebesLayerOGL::RenderLayer, ImageLayerOGL::RenderLayer and ColorLayerOGL::RenderLayer seems to make the rogue layer rendering go away, so it's one of these functions that's doing it.

- Best way to reproduce I find is to early return in ThebesLayerOGL and ImageLayerOGL, and watch for the rogue ColorLayerOGL to be rendered. Use http://www.cnn.com as a testcase, pinch zoom in to somewhere around 70% of max zoom, and scroll to the far right of the page. You will see a ColorLayer that represents the page's background in the overscroll region to the right of the page

- From the graphics side, it looks to me like we're doing the right thing and rendering the layer at the correct size and place, so I'm suspecting an issue with layouting. Joe has advised I ask Mats if he can take a look at this, so adjusting CC accordingly.

- The rogue layer isn't visible when zoomed in to the max or zoomed out to the max. I believe this is due to the x-position we're drawing the layer at; at min zoom the layer draws straight on top of the content and at max zoom it renders off to the right of the viewport. At zoom levels in between you can quite clearly see the layer moving to the left as the zoom level decreases, which suggests to me that we're applying the wrong transform to the page coordinates for the layer to compensate for zoom level.

Comment 12

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 728284 - Hide any sibling layers to the main container layer, as these are created by the XUL UI which is not applicable on Fennec. r=BenWa?

Comment 13

[Timothy Nikkel (:tnikkel)]

Comment on attachment 604549 [details] [diff] [review]
Bug 728284 - Hide any sibling layers to the main container layer, as these are created by the XUL UI which is not applicable on Fennec. r=BenWa?

Can we just not create the layers?

Comment 14

[George Wright (:gw280) (needinfo me!)]

(In reply to Timothy Nikkel (:tn) from comment #13)
> Comment on attachment 604549 [details] [diff] [review]
> Bug 728284 - Hide any sibling layers to the main container layer, as these
> are created by the XUL UI which is not applicable on Fennec. r=BenWa?
> 
> Can we just not create the layers?

I believe there's a separate bug for that, but I'm not sure off hand. We've looked at that and have no idea where to do it, and would love that patch and/or any pointers...

Comment 15

[Benoit Girard (:BenWa)]

Comment on attachment 604549 [details] [diff] [review]
Bug 728284 - Hide any sibling layers to the main container layer, as these are created by the XUL UI which is not applicable on Fennec. r=BenWa?

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

I agree that removing these layers is the ideal fix, but I'm not sure where that is. I'd like to this patch meanwhile.

Comment 16

[Matt Woodrow (:mattwoodrow)]

If you desperately need this for a deadline, then I'd be fine landing this, otherwise I think we need to look into stopping them being created in the first place.

Comment 17

[Joe Drew (not getting mail)]

We need someone from layout to figure out how we can not create these layers. -> mats

Comment 21

[George Wright (:gw280) (needinfo me!)]

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

Comment 22

[George Wright (:gw280) (needinfo me!)]

Workaround landed for now (with giant FIXME comment) until a real fix is applied.

Comment 23

[Johnathan Nightingale [:johnath]]

Do we feel good enough about the workaround that the remaining work needn't block fennec beta? If so, any chance of resolving this and moving the real fix to a follow up, if only for the glee of cleaner bookkeeping?

Comment 24

[Jet Villegas (inactive)]

Mats/tn: please review the FIXME code and comment on its viability. We're burning cycles creating these bogus layers but it's unclear how bad that is.

Comment 25

[Ed Morley [:emorley]]

(Leaving open for comment 23, comment 24)

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

Comment 26

[Mats Palmgren (inactive)]

I don't know this code well enough to fix this, sorry.

Comment 27

[Joe Drew (not getting mail)]

This patch is obviously wrong. We just want to find out what's generating these layers, so we know how to make them go away.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

A display list dump (with layer dump) (gfxUtils::sDumpPaintList set to true in a debug build) should shed light on why the rogue layers are being reated.

Comment 29

[George Wright (:gw280) (needinfo me!)]

Attachment: paintlist

Dump as requested by roc

Comment 30

[George Wright (:gw280) (needinfo me!)]

Assigning to tn as we've just worked through this and he has a plan of attack.

Comment 31

[Timothy Nikkel (:tnikkel)]

George and I debugged this and figured out what was going on.

We have code that makes sure we draw something opaque to the whole window when we are drawing to an opaque widget. If the content in that window (specified by XUL/HTML/CSS) fails to draw something opaque over the whole window then we use the default background color as defined by the prescontext to draw to the window. The problem is that the layer manager draws the checkerboarding by calling nsWindow::DrawWindowUnderlay just before drawing the layer tree. So it needs there to be nothing drawn by the content in the layer tree on top of the checkerboarding that nsWindow::DrawWindowUnderlay draws.

Comment 32

[Jeff Muizelaar [:jrmuizel]]

(In reply to Timothy Nikkel (:tn) from comment #31)
> George and I debugged this and figured out what was going on.
> 
> We have code that makes sure we draw something opaque to the whole window
> when we are drawing to an opaque widget. If the content in that window
> (specified by XUL/HTML/CSS) fails to draw something opaque over the whole
> window then we use the default background color as defined by the
> prescontext to draw to the window.

Why does the code that makes sure we draw something opaque need to use a separate layer? Why can't it just make sure the root ThebesLayer is filled with this solid color?

Comment 33

[Timothy Nikkel (:tnikkel)]

The code I'm talking about uses the display list infrastructure, so it goes through the same process as all display items. We have code that uses a color layer if the created thebes layer would be a single uniform color. If we disable this code we would just get a single thebes layer that had this color. But that wouldn't fix this problem.

Comment 34

[Timothy Nikkel (:tnikkel)]

Attachment: patch?

I tested this with a backout of the hack but I couldn't see what the problems were even without this patch (but with the backout).

Comment 35

[George Wright (:gw280) (needinfo me!)]

(In reply to Timothy Nikkel (:tn) from comment #34)
> Created attachment 613847 [details] [diff] [review]
> patch?
> 
> I tested this with a backout of the hack but I couldn't see what the
> problems were even without this patch (but with the backout).

I'll give it a whirl tomorrow on my test device

Comment 36

[JP Rosevear [:jpr]]

I wonder if this is impacted by Joe's change to a solid color background?

Comment 37

[Jeff Muizelaar [:jrmuizel]]

(In reply to JP Rosevear [:jpr] from comment #36)
> I wonder if this is impacted by Joe's change to a solid color background?

Nope. That's done by the java code which doesn't know anything about what's going on in layout.

Comment 38

[George Wright (:gw280) (needinfo me!)]

\o/ the patch seems to work (tested on an asus transformer running ics)

Comment 39

[Jeff Muizelaar [:jrmuizel]]

(In reply to Timothy Nikkel (:tn) from comment #33)
> The code I'm talking about uses the display list infrastructure, so it goes
> through the same process as all display items. We have code that uses a
> color layer if the created thebes layer would be a single uniform color. If
> we disable this code we would just get a single thebes layer that had this
> color. But that wouldn't fix this problem.

Here lies my confusion:

Assumptions:
1. The content in a browser element should always be opaque.
2. We don't create layers for content that is completely obscured by other content.

Shouldn't this imply that we should not be creating this layer in the first place? Or, under what circumstances would we normally see this color layer and with your patch won't checkerboard or what ever is under the content show through?

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 613847 [details] [diff] [review]
patch?

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

My understanding of this bug is that we're currently painting a background color over the displayport *of the chrome document*. Is that correct?

Can we not fix this just by telling the mobile browser's chrome document to make its widget transparent?

Comment 41

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40)
> Can we not fix this just by telling the mobile browser's chrome document to
> make its widget transparent?

We could do that, but the widget isn't actually transparent, and it would have other consequences, but I'm not sure if they are important consequences or not.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 613847 [details] [diff] [review]
patch?

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

Alright, let's do this, but call the method LayerManagerPaintsBackground instead of NeedOpaqueContent.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Although I suppose it would make more sense for LayerManagerPaintsBackground to actually be a method on LayerManager. What do you think?

Comment 44

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43)
> Although I suppose it would make more sense for LayerManagerPaintsBackground
> to actually be a method on LayerManager. What do you think?

Yeah that makes the most sense, but the widget is the one that has the DrawWindowUnderlay function that actually paints the background, so it is the one that knows if it paints the background or not, not the layer manager.

Jeff thinks we should investigate why the problematic display item isn't optimized away as not visible. His argument is that from layout's point of view it should actually be completely covered by the content document (even though this isn't true in reality, layout should think it's true based on the information that is given to layout).

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42)
> Alright, let's do this, but call the method LayerManagerPaintsBackground
> instead of NeedOpaqueContent.

Alright, let's call it WidgetPaintsBackground.

Comment 46

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 615526 [details] [diff] [review]
patch

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

::: widget/nsIWidget.h
@@ +1596,5 @@
>      virtual bool HasGLContext() { return false; }
>  
> +    /**
> +     * Returns true to indicate that this widget paints an opaque background
> +     * so layout doesn't need to worry about that.

"paints an opaque background that we want to be visible under the page, so layout should not force a default background."

Comment 48

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8951d7e2d564

Comment 49

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/8951d7e2d564

Comment 50

[George Wright (:gw280) (needinfo me!)]

Backout of previous hack: https://hg.mozilla.org/integration/mozilla-inbound/rev/6b917c007510

Comment 51

[Ed Morley [:emorley]]

(In reply to George Wright (:gw280) from comment #50)
> Backout of previous hack:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6b917c007510

https://hg.mozilla.org/mozilla-central/rev/6b917c007510

(Landed with the wrong bug number)

Comment 52

[Martijn Wargers (dead)]

Could this have cause bug 765677?