630346 - [Linux] With layers acceleration, doorhangers have black bar at the top

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: screenshot of bug

STEPS TO REPRODUCE:
 1. Enable layers acceleration.
 2. Visit any site that uses geolocation.  e.g.:
        http://htmlfive.appspot.com/static/whereami.html

ACTUAL RESULTS:  The doorhanger that appears has a black strip at the top.
EXPECTED RESULTS: No black strip.

I've also seen this with "remember password" doorhangers.
With layers acceleration disabled, the bug doesn't occur.

Mozilla/5.0 (X11; Linux x86_64; rv:2.0b11pre) Gecko/20110131 Firefox/4.0b11pre
OS: Ubuntu 10.10 x86_64 (w/ compiz enabled, as it is by default)
Graphics card: GeForce 9800 GT
NVIDIA proprietary driver version (from nvidia-settings): 260.19.06

Comment 1

[Daniel Holbert [:dholbert]]

WFM on Mac. This must be Linux-only.

Comment 2

[Daniel Holbert [:dholbert]]

(unsurprisingly, this also affects the "Larry" site info doorhangers.)

Comment 3

[Karl Tomlinson (:karlt)]

Clicking on the black portion leaves the doorhanger open indicating that the doorhanger window that has the wrong shape.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: use basic layer manager on shaped windows

The LAYERS_OPENGL path doesn't handle analysing the alpha channel to set the shape.  As long as we use the SHAPE extension, there's not a lot of point in drawing on the GPU as we'll have to read back the alpha channel anyway.

Still need to sort out mLayerManager destruction properly (see nsWindow::Destroy()), and do all on all child windows.  Don't know yet whether ClearCachedResources() is necessary after LayerManager::Destroy().

Comment 6

[Marco Castelluccio [:marco]]

ClearCachedResources is weird because in nsWindow::Destroy it's called AFTER setting mLayerManager to nullptr, so it only calls the ClearCachedResources of the child windows that still have mLayerManager (because they are destroyed after).

(We're actually calling ClearCachedResources two times on child windows on nsWindow destroy, but I guess it isn't a problem)

However it shouldn't be needed, because BasicLayerManager (that is the only layer manager to have ClearCachedResources) calls that function from its destructor.
So I think we could even remove the call to ClearCachedResources from nsWindow::Display.

Comment 7

[Marco Castelluccio [:marco]]

Attachment: patch with child windows cleaning

Still has some problems.
The first time (only the first time) I open a doorhanger notification, there's a bar at the top (not black).
The issue occurs even if I create only basic layers for the nsWindow.

Comment 8

[Marco Castelluccio [:marco]]

Comment on attachment 657950 [details] [diff] [review]
patch with child windows cleaning

Actually, the problem of the non-black bar isn't related to layers acceleration, so the patch works correctly.
Looks like Firefox 15 is unaffected.

Comment 9

[Marco Castelluccio [:marco]]

Filed bug 788170.

Comment 10

[Karl Tomlinson (:karlt)]

There is some awkward code in the GTK nsWindow::Destroy to get the
LayerManager's GLContext and call MarkDestroyed on it.  It seems ugly to have
the widget code do that as the widget knows nothing about the GLContext.

LayerManager::Destroy() "Should do any cleanup necessary in preparation for
its widget going away" and so its implementation should call MarkDestroyed on
its GLContext, if appropriate.

Bug 574481 comment 7 added "MarkDestroyed() which explcitly nulls out all GL
function params, so that any misuses of a dead GLContext can get caught as
null deref crashes."

Apparently that is not necessary on Mac so perhaps GLContexts there are not
dead when their window is destroyed, but I guess calling MarkDestroyed there too would be harmless at worst?

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to Marco Castelluccio [:marco] from comment #6)
> However [ClearCachedResources] shouldn't be needed, because BasicLayerManager
> (that is the only layer manager to have ClearCachedResources) calls that function
> from its destructor.

We probably do want ClearCachedResources to get called because there may be other references to the LayerManager and so it may not get deleted until well after Destroyed() is called on it.

However, as you point out that is ineffective during destruction unless the parent nsWindow is destroyed first, which may be the case.

I expect LayerManager::Destroy() should call ClearCachedResources(), but I'm happy to leave that for another time and not add any more messy ClearCachedResources calls in this patch.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 657950 [details] [diff] [review]
patch with child windows cleaning

This looks pretty good to me but I'd like the cleanup code to match
the relevant parts of nsWindow::Destroy().

I think there should be a DestroyCompositor() call in
CleanLayerManagerRecursive.

The checkin comment should describe what the patch changes, not necessarily the symptoms that are resolved.

Can you create a separate patch please to move the GLContext::MarkDestroyed
code from nsWindow::Destroy to LayerManagerOGL::Destroy?  Then that won't need to be duplicated here.

LayerManagerOGL::CleanupResources is only used by Destroy and so can be moved
out of the public API and merged into Destroy.  By changing the if (!mDestroyed)
in Destroy to an if (mDestroyed) early return, the cleanup code won't need to
be indented.

>+    virtual LayerManager*   GetLayerManager(PLayersChild* aShadowManager = nullptr,
>+                                            LayersBackend aBackendHint = mozilla::layers::LAYERS_NONE,
>+                                            LayerManagerPersistence aPersistence = LAYER_MANAGER_CURRENT,
>+                                            bool* aAllowRetaining = nullptr) MOZ_OVERRIDE;

This is not expected to be used externally and so should be private.

Can you close up the gap between * and GetLayerManager to one space, please?

Comment 13

[Marco Castelluccio [:marco]]

Attachment: Part 1: Use basic layers for windows with transparency

Comment 14

[Marco Castelluccio [:marco]]

Attachment: Part 2: Remove MarkDestroyed from LayerManagerOGL::Destroy

(In reply to Karl Tomlinson (:karlt) from comment #12)
> Can you create a separate patch please to move the GLContext::MarkDestroyed
> code from nsWindow::Destroy to LayerManagerOGL::Destroy?  Then that won't
> need to be duplicated here.

MarkDestroyed is called only if LayerManagerOGL::mGLContext is not null, but mGLContext is always null after LayerManagerOGL::Destroy.

Comment 15

[Marco Castelluccio [:marco]]

Attachment: Part 3: Merge LayerManagerOGL::CleanupResources into LayerManagerOGL::Destroy

(In reply to Karl Tomlinson (:karlt) from comment #12) 
> LayerManagerOGL::CleanupResources is only used by Destroy and so can be moved
> out of the public API and merged into Destroy.

Done, and removed an useless |if (mRoot) { ... }| as there mRoot is always null.

Comment 16

[Marco Castelluccio [:marco]]

(In reply to Marco Castelluccio [:marco] from comment #14)
> MarkDestroyed is called only if LayerManagerOGL::mGLContext is not null, but
> mGLContext is always null after LayerManagerOGL::Destroy.

I forgot to say that MarkDestroyed is called by the GLContextGLX destructor: http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderGLX.cpp#813

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 663610 [details] [diff] [review]
Part 3: Merge LayerManagerOGL::CleanupResources into LayerManagerOGL::Destroy

(In reply to Marco Castelluccio [:marco] from comment #15)
> Done, and removed an useless |if (mRoot) { ... }| as there mRoot is always
> null.

Great, thanks.

(And interesting because that means that ContainerCleanupResources in
ContainerCleanupResources is/was never called.)

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 663606 [details] [diff] [review]
Part 1: Use basic layers for windows with transparency

>+    void CleanLayerManagerRecursive();
>+
> protected:

Please make this method private, also.

>+    // It is safe to call DestroyeCompositor several times (here and 
>+    // in the parent class) since it will take effect only once.
>+    // The reason we call it here is because on gtk platforms we need 
>+    // to destroy the compositor before we destroy the gdk window (which
>+    // destroys the the gl context attached to it).
>+    DestroyCompositor();

This comment doesn't make so much sense in the CleanLayerManagerRecursive context.  I don't think we need a comment here anyway as someone can refer to Destroy, so please remove the comment from here.

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 663608 [details] [diff] [review]
Part 2: Remove MarkDestroyed from LayerManagerOGL::Destroy

The LayerManagerOGL::Destroy changes are good, but I want to keep the
explicit MarkDestroyed call for now.

(In reply to Marco Castelluccio [:marco] from comment #14)
> MarkDestroyed is called only if LayerManagerOGL::mGLContext is not null, but
> mGLContext is always null after LayerManagerOGL::Destroy.

(In reply to Marco Castelluccio [:marco] from comment #16)
> I forgot to say that MarkDestroyed is called by the GLContextGLX destructor:
> http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderGLX.
> cpp#813

The reason for the explicit MarkDestroyed call from widget code was to ensure
that the GLContext is marked destroyed even if there is something else holding
a reference to the GLContext.  I suspect there currently isn't anything else that might hold a reference, but the purpose of the method was to provide for this possibility.

If LayerManagerOGL::Destroy were to call MarkDestroyed, it could do so before setting mGLContext to null.  However, because this is currently the last reference to the GLContext, I no longer think we need to call MarkDestroyed during CleanLayerManagerRecursive for now.  I'm currently trying to work out how best to tidy up this Destroy process for bug 763449 and bug 782505, so I can sort this out there.

Probably tidiest to fold the remaining LayerManagerOGL::Destroy changes into the
CleanupResources-merge patch.

Comment 20

[Marco Castelluccio [:marco]]

Attachment: Part 1: Use basic layers for windows with transparency

Carrying forward r+.

Comment 21

[Marco Castelluccio [:marco]]

Attachment: Part 2: Merge LayerManagerOGL::CleanupResources into LayerManagerOGL::Destroy

Comment 22

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e19ca9b2b5a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb29b2cc631

Comment 23

[Karl Tomlinson (:karlt)]

https://tbpl.mozilla.org/?tree=Try&rev=c84ffc9700ae
https://tbpl.mozilla.org/?tree=Try&rev=ec6076ebefde
https://tbpl.mozilla.org/?tree=Try&rev=b1b1a81df544

Comment 24

[u279076]

RE: in-testsuite-
Is this something that cannot go into the testsuite or is it something that will not? In either case, does this need a manual MozTrap test?

Comment 25

[Karl Tomlinson (:karlt)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #24)

This bug in visual appearance is not something that mochitests and reftests can detect.  It may be possible to write a unit test to query the OS, but the reward to effort ratio is not high there.

This was an issue due to a missing part in a still incomplete implementation of a non-default configuration.  Any manual test would have only noticed this should the configuration have been selected through the layers.acceleration.force-enabled pref, so there may be existing manual tests that cover this.  If not, it would be good to have some coverage of door hangers in manual tests.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e19ca9b2b5a9
https://hg.mozilla.org/mozilla-central/rev/9fb29b2cc631