Bug 143214 – [EFL] Fix 18 crashing compositing tests after r182101

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Csaba Osztrogonác]

before:
- https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/20777
- 46 fail, only 1 crash from it

after:
- https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/20778
- 64 fails, 18 compositing test crashes from it

[Csaba Osztrogonác]

I got it, fix is coming.

[Csaba Osztrogonác]

nullptr check of m_private in GraphicsContext3D::makeContextCurrent()
(Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp) is missing.

GraphicsContext3DCairo.cpp and GraphicsContext3DWin.cpp implementations
already contain this nullptr check, EFL implementation should do the same.

I checked it fixes these crashes.

So r182101 is innocent, it only reveales this bug, 
sorry for blaming this change. :)

[Csaba Osztrogonác]

Created attachment 249823 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #2)
> nullptr check of m_private in GraphicsContext3D::makeContextCurrent()
> (Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp) is missing.
> 
> GraphicsContext3DCairo.cpp and GraphicsContext3DWin.cpp implementations
> already contain this nullptr check, EFL implementation should do the same.
> 
> I checked it fixes these crashes.
> 
> So r182101 is innocent, it only reveales this bug, 
> sorry for blaming this change. :)

Thanks, Ossy. This fix looks good for me.

For GraphicsContext3DCairo, the nullptr check looks redundant, because GraphicsContext3DCairo creates GraphicsContext3DPrivate unconditionally.

Unfortunately, I'm not a reviewer. We need another reviewer for stamp. :)

[WebKit Commit Bot]

Comment on attachment 249823 [details]
Patch

Clearing flags on attachment: 249823

Committed r182187: <http://trac.webkit.org/changeset/182187>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #5)
> Comment on attachment 249823 [details]
> Patch
> 
> Clearing flags on attachment: 249823
> 
> Committed r182187: <http://trac.webkit.org/changeset/182187>

I made something wrong, it seems it didn't fix anything. :(
I was sure it fixed the crashes locally, I'll check it soon.

[Csaba Osztrogonác]

Unfortunately it isn't easy to debug it due to this bug:
https://bugs.webkit.org/show_bug.cgi?id=123397#c5

[Csaba Osztrogonác]

I managed to get a better backtrace for this crash. Unfortunately
the original one was a little confusing because of the optimizations.

The crash happens because m_context3D is nullptr here:
https://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp?rev=182101#L116

I'm not familiar with this part of the source. Could you give me a hint
where should I continue debugging, and how can we fix it properly?

[Csaba Osztrogonác]

I think the problem is instantiating the BitmapTexturePool with the default
constructor here:
https://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp?rev=182101#L32

The default constructor doesn't initialize m_context3D:
https://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/texmap/BitmapTexturePool.cpp?rev=182101#L42

[Gwang Yoon Hwang]

(In reply to comment #10)
> I think the problem is instantiating the BitmapTexturePool with the default
> constructor here:
> https://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/
> texmap/TextureMapperImageBuffer.cpp?rev=182101#L32
>

TextureMapperImageBuffer is not used in TEXTURE_MAPPER_GL. How about to check
passed GraphicsContext3D to BitmapTextureGL is valid or not? Every GraphicsContext3D
should be valid when we are creating BitmapTextureGL.

> The default constructor doesn't initialize m_context3D:
> https://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/
> texmap/BitmapTexturePool.cpp?rev=182101#L42

The default constructor of BitmapTexturePool should not be called in TEXTURE_MAPPER_GL
It would be good to disable the default constructor using !USE(TEXTURE_MAPPER_GL)

[Csaba Osztrogonác]

After more debugging, the problem is that the default constructor of 
BitmapTexturePool::BitmapTexturePool() is called which doesn't initialize
m_context3D.

[Csaba Osztrogonác]

(In reply to comment #11)
> (In reply to comment #10)
> > I think the problem is instantiating the BitmapTexturePool with the default
> > constructor here:
> > https://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/
> > texmap/TextureMapperImageBuffer.cpp?rev=182101#L32
> >
> 
> TextureMapperImageBuffer is not used in TEXTURE_MAPPER_GL. How about to check
> passed GraphicsContext3D to BitmapTextureGL is valid or not? Every
> GraphicsContext3D
> should be valid when we are creating BitmapTextureGL.

Let's see the crash again.

m_context3D is nullptr in BitmapTextureGL::BitmapTextureGL(PassRefPtr<GraphicsContext3D> context3D),
because it is called with nullptr from PassRefPtr<BitmapTexture> BitmapTexturePool::createTexture(). And it is nullptr here, because
it wasn't initialezed by the default constructor - BitmapTexturePool::BitmapTexturePool() .

[Csaba Osztrogonác]

(In reply to comment #11)
> (In reply to comment #10)
> > I think the problem is instantiating the BitmapTexturePool with the default
> > constructor here:
> > https://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/
> > texmap/TextureMapperImageBuffer.cpp?rev=182101#L32
> >
> 
> TextureMapperImageBuffer is not used in TEXTURE_MAPPER_GL. How about to check
> passed GraphicsContext3D to BitmapTextureGL is valid or not? Every
> GraphicsContext3D
> should be valid when we are creating BitmapTextureGL.

I don't know anything about this part of the code. I don't understand what
you mean "TextureMapperImageBuffer is not used in TEXTURE_MAPPER_GL".
ifdefink TextureMapperImageBuffer.cpp and h out caused build failure
for me, because it is used in TextureMapper.cpp
 
> > The default constructor doesn't initialize m_context3D:
> > https://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/
> > texmap/BitmapTexturePool.cpp?rev=182101#L42
> 
> The default constructor of BitmapTexturePool should not be called in
> TEXTURE_MAPPER_GL
> It would be good to disable the default constructor using
> !USE(TEXTURE_MAPPER_GL)

It is called from TextureMapperImageBuffer.cpp .

[Csaba Osztrogonác]

any additional hint where should I continue debugging?

[Csaba Osztrogonác]

Just to note, I marked these tests crash in http://trac.webkit.org/changeset/182312 . We should remove these ecpectations once the bug is fixed.

[Csaba Osztrogonác]

(In reply to comment #16)
> Just to note, I marked these tests crash in
> http://trac.webkit.org/changeset/182312 . We should remove these
> ecpectations once the bug is fixed.

I had to comment out the original expectations, because the new
ones aren't overriden them - http://trac.webkit.org/changeset/182313.

[Csaba Osztrogonác]

(In reply to comment #15)
> any additional hint where should I continue debugging?

ping?

[Csaba Osztrogonác]

(In reply to comment #18)
> (In reply to comment #15)
> > any additional hint where should I continue debugging?
> ping?

ping2?

[Gwang Yoon Hwang]

(In reply to comment #14)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > I think the problem is instantiating the BitmapTexturePool with the default
> > > constructor here:
> > > https://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/
> > > texmap/TextureMapperImageBuffer.cpp?rev=182101#L32
> > >
> > 
> > TextureMapperImageBuffer is not used in TEXTURE_MAPPER_GL. How about to check
> > passed GraphicsContext3D to BitmapTextureGL is valid or not? Every
> > GraphicsContext3D
> > should be valid when we are creating BitmapTextureGL.
> 
> I don't know anything about this part of the code. I don't understand what
> you mean "TextureMapperImageBuffer is not used in TEXTURE_MAPPER_GL".
> ifdefink TextureMapperImageBuffer.cpp and h out caused build failure
> for me, because it is used in TextureMapper.cpp
>  

After taking a look at EFL ports, it uses TextureMapperImageBuffer only if there is no OpenGL surface available.

Basically, if we use TEXTURE_MAPPER_GL, it tries to use TextureMapperGL first, and then
if it was not successful (this is what I'm facing, efl refuses llvmpipe as a GPU backend), it uses TextureMapper + TextureMapperImageBuffer.

But the TextureMapperImageBuffer is not maintained for a while, and I'm planning to remove this.

If EFL ports uses OpenGL backend, it uses TextureMapperGL instead of TextureMapperImageBuffer. Because it is done at runtime, you faced build failure when you remove TextureMapperImageBuffer.

If you to remove TextureMapperImageBuffer and some related codes, (it is
enough to remove line 43,44 in TextureMapper.cpp) and it should be run properly.




> > > The default constructor doesn't initialize m_context3D:
> > > https://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/
> > > texmap/BitmapTexturePool.cpp?rev=182101#L42
> > 
> > The default constructor of BitmapTexturePool should not be called in
> > TEXTURE_MAPPER_GL
> > It would be good to disable the default constructor using
> > !USE(TEXTURE_MAPPER_GL)
> 
> It is called from TextureMapperImageBuffer.cpp .

[Csaba Osztrogonác]

Thanks for the info, I filed a new bug report to remove TextureMapperImageBuffer
- bug143561. It fixes the crashes and many other compositing failures on EFL.

[Gwang Yoon Hwang]

(In reply to comment #21)
> Thanks for the info, I filed a new bug report to remove
> TextureMapperImageBuffer
> - bug143561. It fixes the crashes and many other compositing failures on EFL.

It's quite interesting result. It means WebKitEFL uses TextureMapperImageBuffer and TextureMapperGL at same time, which should not be happened.

[Csaba Osztrogonác]

(In reply to comment #22)
> (In reply to comment #21)
> > Thanks for the info, I filed a new bug report to remove
> > TextureMapperImageBuffer
> > - bug143561. It fixes the crashes and many other compositing failures on EFL.
> 
> It's quite interesting result. It means WebKitEFL uses
> TextureMapperImageBuffer and TextureMapperGL at same time, which should not
> be happened.

After digging the bug a little bit deeper. The problem is that EFL still 
refuses using GL backend. It seemed that removing the software based
TextureMapperImageBuffer solved the compositing tests crashes ... but
we got 72 another crashes instead. :-/

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.