Bug 97394 – [TextureMapper] [WebKit2] Crash in WebCore::BitmapTextureGL::updateContents

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

You can see this by going back and forth from pages that have accelerated compositing to pages that do not.

#0  0x00007f17519d0c20 in WebCore::GraphicsContext3D::makeContextCurrent() ()
   from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#1  0x00007f175155a4b0 in WebCore::GraphicsContext3D::bindTexture(unsigned int, unsigned int) ()
   from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#2  0x00007f17519d1ff1 in WebCore::BitmapTextureGL::updateContents(void const*, WebCore::IntRect const&, WebCore::IntPoint const&, int) () from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#3  0x00007f17519dcca3 in WebCore::TextureMapperTile::updateContents(WebCore::TextureMapper*, WebCore::Image*, WebCore::IntRect const&) () from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#4  0x00007f17519dd5e5 in WebCore::TextureMapperTiledBackingStore::updateContents(WebCore::TextureMapper*, WebCore::Image*, WebCore::FloatSize const&, WebCore::IntRect const&) ()
   from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#5  0x00007f17519e110e in WebCore::TextureMapperLayer::updateBackingStore(WebCore::TextureMapper*, WebCore::GraphicsLayerTextureMapper*) () from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#6  0x00007f17519e1d05 in WebCore::TextureMapperLayer::syncCompositingState(WebCore::GraphicsLayerTextureMapper*, WebCore::TextureMapper*, int) () from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#7  0x00007f17519e1d6d in WebCore::TextureMapperLayer::syncCompositingState(WebCore::GraphicsLayerTextureMapper*, WebCore::TextureMapper*, int) () from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#8  0x00007f17519e1d6d in WebCore::TextureMapperLayer::syncCompositingState(WebCore::GraphicsLayerTextureMapper*, WebCore::TextureMapper*, int) () from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#9  0x00007f17519e1d6d in WebCore::TextureMapperLayer::syncCompositingState(WebCore::GraphicsLayerTextureMapper*, WebCore::TextureMapper*, int) () from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#10 0x00007f17519e1d6d in WebCore::TextureMapperLayer::syncCompositingState(WebCore::GraphicsLayerTextureMapper*, WebCore::TextureMapper*, int) () from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#11 0x00007f17519e1d6d in WebCore::TextureMapperLayer::syncCompositingState(WebCore::GraphicsLayerTextureMapper*, WebCore::TextureMapper*, int) () from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#12 0x00007f17519e1d6d in WebCore::TextureMapperLayer::syncCompositingState(WebCore::GraphicsLayerTextureMapper*, WebCore::TextureMapper*, int) () from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#13 0x00007f17519e1d6d in WebCore::TextureMapperLayer::syncCompositingState(WebCore::GraphicsLayerTextureMapper*, WebCore::TextureMapper*, int) () from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#14 0x00007f17519e1d6d in WebCore::TextureMapperLayer::syncCompositingState(WebCore::GraphicsLayerTextureMapper*, WebCore::TextureMapper*, int) () from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#15 0x00007f17519e1d6d in WebCore::TextureMapperLayer::syncCompositingState(WebCore::GraphicsLayerTextureMapper*, WebCore::TextureMapper*, int) () from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#16 0x00007f17519e1d6d in WebCore::TextureMapperLayer::syncCompositingState(WebCore::GraphicsLayerTextureMapper*, WebCore::TextureMapper*, int) () from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#17 0x00007f17516ea5f7 in WebCore::RenderLayerCompositor::flushPendingLayerChanges(bool) ()
   from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#18 0x00007f17514d6fb4 in WebCore::FrameView::syncCompositingStateForThisFrame(WebCore::Frame*) ()
   from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#19 0x00007f17514d7413 in WebCore::FrameView::syncCompositingStateIncludingSubframes() ()
   from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#20 0x00007f1750ead860 in WebKit::LayerTreeHostGtk::flushAndRenderLayers() ()
   from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#21 0x00007f1750ead8e0 in WebKit::LayerTreeHostGtk::layerFlushTimerFired() ()
   from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#22 0x00007f1750ead929 in WebKit::LayerTreeHostGtk::layerFlushTimerFiredCallback(WebKit::LayerTreeHostGtk*) ()
   from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#23 0x00007f174b200a1b in g_timeout_dispatch (source=<optimized out>, callback=<optimized out>, 
    user_data=<optimized out>) at gmain.c:3882
---Type <return> to continue, or q <return> to quit---
#24 0x00007f174b1ffe53 in g_main_dispatch (context=0x1966f40) at gmain.c:2539
#25 g_main_context_dispatch (context=0x1966f40) at gmain.c:3075
#26 0x00007f174b2001a0 in g_main_context_iterate (dispatch=1, block=<optimized out>, context=0x1966f40, 
    self=<optimized out>) at gmain.c:3146
#27 g_main_context_iterate (context=0x1966f40, block=<optimized out>, dispatch=1, self=<optimized out>) at gmain.c:3083
#28 0x00007f174b20059a in g_main_loop_run (loop=0x19a3000) at gmain.c:3340
#29 0x00007f1750e411ef in WebProcessMainGtk ()
   from /home/martin/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#30 0x00007f17505b376d in __libc_start_main () from /lib/x86_64-linux-gnu/libc.so.6

[Martin Robinson]

So far it looks like the issue is that while you can change a layer's TextureMapper, the m_textureMapper members of the BitmapTextureGL instances in the TextureMapperTiledBackingStore are stale. Continuing to investigate...

[Martin Robinson]

Created attachment 166368 [details]
Patch

[Martin Robinson]

I've uploaded a patch that I'm not entirely happy with for two reasons:

First, this causes unnecessary texture uploads even when the GraphicsContext3D doesn't change. For instance, it's always the current context in GTK+. A better fix might be to have the TexturePool independent of the TextureMapper, I think. This would allow textures to exist within the context of multiple TextureMappers.

Second, I think that this would be slightly simpler if instead of GraphicsLayerTextureMapper owning the backing store for Image* layers, this was just delegated to TexturMapperLayer and shared the "normal" layer path.

Both of these changes would require a much more complicated patch, so I've fixed the crash here despite these shortcomings. Fixing a crash takes priority over absolute beauty, but I would like to continue to develop these ideas in the future.

[Noam Rosenthal]

Comment on attachment 166368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166368&action=review

Wow, kind of complicated, but probably the only way to achieve what you're trying to do.
At least in coordinated graphics we don't really change the textureMapper from underneath...

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:67
> +void TextureMapper::notifyDestructionObserversEarly()

How about willBeDestroyed

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:69
> +    HashSet<TextureMapperDestructionObserver*>::iterator stop = m_destructionObservers.end();

stop -> end

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:203
> +    void observeTextureMapper(TextureMapper* textureMapper)
> +    {
> +        if (m_observedTextureMapper == textureMapper)
> +            return;
> +
> +        if (m_observedTextureMapper)
> +            m_observedTextureMapper->removeDestructionObserver(this);
> +
> +        m_observedTextureMapper = textureMapper;
> +
> +        if (m_observedTextureMapper)
> +            m_observedTextureMapper->addDestructionObserver(this);
> +    }

This is a bit verbose.
I'd rather this be an interface, with the virtual function something like willDestroyTextureMapper(TextureMapper*), and then the client can check if it's the right one.

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:211
> +    virtual void notifyTextureMapperDestroyed() = 0;

willDestroyTextureMapper

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:532
> +    // If our TextureMapper changed the backing store will be invalid, so we need to
> +    // create a new one and upload the layer contents again.
> +    if (m_backingStore && !m_backingStore->isValid()) {
> +        m_backingStore.clear();
> +        m_state.needsDisplay = true;
> +    }

Why not observe the textureMapper from TextureMapperLayer, and simply clear the backingStore when it's destroyed?

[Martin Robinson]

(In reply to comment #4)

> > Source/WebCore/platform/graphics/texmap/TextureMapper.h:203
> > +    void observeTextureMapper(TextureMapper* textureMapper)
> > +    {
> > +        if (m_observedTextureMapper == textureMapper)
> > +            return;
> > +
> > +        if (m_observedTextureMapper)
> > +            m_observedTextureMapper->removeDestructionObserver(this);
> > +
> > +        m_observedTextureMapper = textureMapper;
> > +
> > +        if (m_observedTextureMapper)
> > +            m_observedTextureMapper->addDestructionObserver(this);
> > +    }
> 
> This is a bit verbose.
> I'd rather this be an interface, with the virtual function something like willDestroyTextureMapper(TextureMapper*), and then the client can check if it's the right one.

One thing I worry about with that kind of approach is that the old TextureMapper could potentially be destroyed after setting the new one. We could guard against this by simply clearing the backing store when the texture mapper changes as well, but it's a bit less fool-proof.

[Noam Rosenthal]

Comment on attachment 166368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166368&action=review

> Source/WebCore/ChangeLog:12
> +        to textures obtained from that TextureMapper's texture pool. Since a GL
> +        BitmapTexture needs to use its owning TextureMapper in the destructor,
> +        we actively notify TextureMapperTiledBackingStores when their original
> +        TextureMappers are destroyed.

Actually, it just needs its GraphicsContext3D. Can't we keep a reference to that in BTGL, and then it's ok to do things in the regular destructor?

[Noam Rosenthal]

> One thing I worry about with that kind of approach is that the old TextureMapper could potentially be destroyed after setting the new one. We could guard against this by simply clearing the backing store when the texture mapper changes as well, but it's a bit less fool-proof.

We should still keep a reference for the TextureMapper in TextureMapperLayer and make sure it's the right one. I'd rather do that there though, and not inside the observer.

[Martin Robinson]

(In reply to comment #7)

> > Source/WebCore/ChangeLog:12
> > +        to textures obtained from that TextureMapper's texture pool. Since a GL
> > +        BitmapTexture needs to use its owning TextureMapper in the destructor,
> > +        we actively notify TextureMapperTiledBackingStores when their original
> > +        TextureMappers are destroyed.
> 
> Actually, it just needs its GraphicsContext3D. Can't we keep a reference to that in BTGL, and then it's ok to do things in the regular destructor?

I think this might be the right fix.

[Martin Robinson]

Created attachment 166380 [details]
Patch

[Martin Robinson]

(In reply to comment #9)
> Created an attachment (id=166380) [details]
> Patch

I've posted a new patch using your suggestion. I do not track when the TextureMapper changes. I don't think it should matter since all TextureMapperGLs are using the "current context" render style.

This still leaves a bug when the TextureMapper is used to render to some other GL context without a sharing context. That's not something that GTK+ ever does, but I think it should be handled, in a robust way, in another bug.

[Martin Robinson]

Comment on attachment 166380 [details]
Patch

Thanks for the review!

[WebKit Review Bot]

Comment on attachment 166380 [details]
Patch

Clearing flags on attachment: 166380

Committed r129993: <http://trac.webkit.org/changeset/129993>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.