1059793 - Shutdown ABORT: X_FreePixmap: BadPixmap with OMTC and GL layers on Linux

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

Bug 1014355 cover the same assertion being hit with basic layers. Those are most likely two different problem.

below the relevant comments from Bug 1014355:

----

This reproduces fairly easily with in GLContextGLX::MakeCurrent (with MOZ_X_SYNC) on the compositor thread before destroying the remaining textures. At the same time, the main thread is waiting for the ImageBirdge::ShutDown to complete and is already past the destructor of nsWindow.
 
The GLContext used by the compositor doesn't own, the X Drawable is is using, the latter being owned by the nsWindow, so it very much looks like we need either:
* a way for GLContextGLX::MakeCurrent to early-return false if the widget owning the drawable has already been destroyed
* a way to keep the widget alive longer or a way to kill all textures before the part of ~nsWindow where the window drawable gets destroyed (very hard).

The Drawable I am referring to is nsWindow::mGdkWindow, which is passed to the compositor's GLContextGLX at creation time with deleteDrawable set to false (so the GLContext doesn't own the the drawable). So the Drawable is also GLContextGLX::mDrawable. The Drawable is destroyed somewhere in nsWindow::Destroy, which is called by ~nsWindow, all of this happening before all of the textures have had a chance to be destroyed (in particular, the ones used by async-video).

Would it be feasible to reference count the drawable and destroy it later (possibly on the compositor thread)? or would it be ok to notify all of the gl contexts that are created from a given widget that their drawable is gone?


And a comment from karlt:
I'm guessing we can assume that all inaccessible resources will be released by
the implementation, though I haven't verified that.

In the past we also needed to ensure that a context associated with a window was
not current when the window was destroyed, or we got errors when a different
context was made current.  Perhaps this was due to flushing when the context
changed, or perhaps the implementation always needs the window or its glx object when the context is released - I don't know.

Comment 1

[Karl Tomlinson (:karlt)]

(In reply to Nicolas Silva [:nical] from comment #0)
> Would it be feasible to reference count the drawable and destroy it later
> (possibly on the compositor thread)?

GDK is used to manage the drawable, and that must only be used from one thread, so the drawable will need to be destroyed on the main thread.

Perhaps it would be possible to hide immediately and destroy later.  I'm not sure.
It might get complicated

> or would it be ok to notify all of the
> gl contexts that are created from a given widget that their drawable is gone?

so if there is a way to sync notify the compositor thread, then I suspect that would be the simpler solution.

Comment 2

[Nicolas Silva [:nical]]

Attachment: Mark the GLContext destroyed when shutting down the compositor

CompositorOGL::CleanupResources is called in CompositorOGL::Destroy which happens in CompositorParent::RecvStop, which is a synchronous message happening just before the widget's drawable gets destroyed, so it sounds like a good place to signal the GLContext that it can't call xMakeCurrent anymore.

Jeff, is it a problem to force GLContext::MarkDestroyed before the context's destructor? The alternative is to add something like GLContext::NotifyWidgetDestroyed() and store a state that will prevent the context from calling xMakeCurrent.

Comment 3

[Kelsey Gilbert [:jgilbert]]

(In reply to Nicolas Silva [:nical] from comment #2)
> Created attachment 8481325 [details] [diff] [review]
> Mark the GLContext destroyed when shutting down the compositor
> 
> CompositorOGL::CleanupResources is called in CompositorOGL::Destroy which
> happens in CompositorParent::RecvStop, which is a synchronous message
> happening just before the widget's drawable gets destroyed, so it sounds
> like a good place to signal the GLContext that it can't call xMakeCurrent
> anymore.
> 
> Jeff, is it a problem to force GLContext::MarkDestroyed before the context's
> destructor? The alternative is to add something like
> GLContext::NotifyWidgetDestroyed() and store a state that will prevent the
> context from calling xMakeCurrent.

Calling MarkDestroyed early is safe, as long as you can guarantee that no GL functions will be called against that context after that point. (we zero the symbol table)

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8481325 [details] [diff] [review]
Mark the GLContext destroyed when shutting down the compositor

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

If you call gl->fFoo() after calling gl->MarkDestroyed(), you'll get a null deref. That's basically your assert, here.

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Gilbert [:jgilbert] from comment #3)
> (In reply to Nicolas Silva [:nical] from comment #2)
> > Created attachment 8481325 [details] [diff] [review]
> > Mark the GLContext destroyed when shutting down the compositor
> > 
> > CompositorOGL::CleanupResources is called in CompositorOGL::Destroy which
> > happens in CompositorParent::RecvStop, which is a synchronous message
> > happening just before the widget's drawable gets destroyed, so it sounds
> > like a good place to signal the GLContext that it can't call xMakeCurrent
> > anymore.
> > 
> > Jeff, is it a problem to force GLContext::MarkDestroyed before the context's
> > destructor? The alternative is to add something like
> > GLContext::NotifyWidgetDestroyed() and store a state that will prevent the
> > context from calling xMakeCurrent.
> 
> Calling MarkDestroyed early is safe, as long as you can guarantee that no GL
> functions will be called against that context after that point. (we zero the
> symbol table)

Wait, which GLContext is this? The compositor's GLContext? If so, this should be fine. (MarkDestroyed does call MakeCurrent though)

Comment 6

[Nicolas Silva [:nical]]

(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> Wait, which GLContext is this? The compositor's GLContext? If so, this
> should be fine. (MarkDestroyed does call MakeCurrent though)

Yes, the Compositor's. It's all Compositor-side code needs to check for the return value of MakeCurrent before doing things already, so it's fine that the function pointers are zeroed out as long as it doesn't mess up any particular ordering of the GLContext's destruction. In this patch we call MarkDestroyed just before it becomes dangerous to call xMakeCurrent (that is, when the sync ipc message returns to the main thread and we destroy the X11 Drawable), so things are looking good.

Comment 7

[Nicolas Silva [:nical]]

try looking good: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cca1c4776a6d

Comment 8

[Nicolas Silva [:nical]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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