882384 - URL tooltip layer is not destroyed

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Benoit Girard (:BenWa)]

STR:
1) Turn on OMTC + Layer border
2) Hover over a link to get a url tooltip layer
3) Unhover to have it fade out and be destroyed
4) Click on the page background

The layer reappears and remains as a transparent layer.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Screenshot 2013-11-20 12.08.21.png

Actually I think we need to up the priority of this bug. What happens is sometimes the tabbar needs to merge something with the layer. I believe the code has to allocate a big drawing buffer/texture to achieve this making this expensive and temporary.

Comment 2

[Benoit Girard (:BenWa)]

CC'ing mconley for something that could impact australis perf.

Comment 3

[Matt Woodrow (:mattwoodrow)]

I have never been able to reproduce this a debug build with a debugger attached :(

Comment 4

[Matt Woodrow (:mattwoodrow)]

Ok, managed to reproduce it using my default profile.

We have a ThebesLayer for the status bar, where the only item in it is an inactive nsDisplayOpacity, with an opacity of 0.

We always build these items, and instead just refuse to build a layer for them if they are opacity 0. I think this is to make it easier to make sure that we can still build a layer if they're starting an async animation.

We walk through the nsDisplayOpacity to compute visible areas/component alpha areas of the children, and come up with the size that you see. And decide that we need a component alpha layer.

Then we try build the inactive layer, it fails (since opacity==0), and we never actually put anything into our layer.

So it'll just sit there, taking up some memory. If we try draw it, it won't do anything.

I don't think this is a big problem, but we could probably avoid building the nsDisplayOpacity in the first place.

Comment 5

[Benoit Girard (:BenWa)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> I don't think this is a big problem, but we could probably avoid building
> the nsDisplayOpacity in the first place.

IMO it's a big problem because it forces a big layer temporarily because we also want to layerize something temporarily in the tabstrip and we're stuck creating a screen height buffer for something that will stick around half a second.

Comment 6

[Matt Woodrow (:mattwoodrow)]

What? This has nothing to do with the tabstrip, the layer only covers the tiny rectangle in the corner.

Comment 7

[Benoit Girard (:BenWa)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> What? This has nothing to do with the tabstrip, the layer only covers the
> tiny rectangle in the corner.

Are you rejecting the screenshot? The entire layer is represented in a single texture. When we add a tiny rect to the layer region for a fraction of a second we have to allocate a texture for it.

As Jeff said when I showed him this: This needs to die in a fire!

Comment 8

[Matt Woodrow (:mattwoodrow)]

I guess I can't really do that.

It certainly wasn't allocating a big layer when I reproduced it locally at least.

I'll try find time to arrange this being moved to the nearest fire today.

Comment 9

[Benoit Girard (:BenWa)]

haha :)

So it doesn't naturally happen. But it does happen when the tab is loading or some opacity change in the favicon. When the two things are true (and they aren't true for long) we go through this expensive step. I can force it but I've seen it happen by accident.

I flipped |layers.componentalpha.enabled;false| locally to see how badly we need componentalpha for retina and I don't see this bug anymore. Not sure if it's a coincidence or not but it could explain something.

Comment 10

[Matt Woodrow (:mattwoodrow)]

I think we need two fixes here.

We never want the content from the status bar to merge into the same layer as content from the UI, so we should add the xul 'layer' attribute to it to enforce it's own layer.

And then we should do what I suggested in comment 4 to stop the nsDisplayOpacity being created at all.

Comment 11

[Benoit Girard (:BenWa)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> I think we need two fixes here.
> 
> We never want the content from the status bar to merge into the same layer
> as content from the UI, so we should add the xul 'layer' attribute to it to
> enforce it's own layer.

IMO fixing this is the only 'high' priority issue. Maybe we should spin that off and fix that one?

> 
> And then we should do what I suggested in comment 4 to stop the
> nsDisplayOpacity being created at all.

This may be more annoying if it was mobile but it's only on desktop at the moment so meh.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Push the status bar into it's own layer so that it doesn't merge with other content

Comment 13

[Matt Woodrow (:mattwoodrow)]

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

Comment 14

[Carsten Book [:Tomcat]]

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