Bug 95003 – [Qt] Segmentation fault when closing QtTestBrowser

• Component: WebGL
• Status: RESOLVED FIXED

[Roland Takacs]

Segmentation fault occurs when somebody would like to close QtTestBrowser at a WebGL using page.

The reason is that we delete 'm_surface' at the ~GraphicsContext3DPrivate().
The 'm_surface' that located at GraphicsContext3DPrivate points to the window so we do not have to delete it.
The window will be destroyed at QWidgetPrivate::deleteTLSysExtra(). (qwidget_qpa.cpp)

[Roland Takacs]

Created attachment 161431 [details]
patch

This patch eliminates the segmentation fault when closing QtTestBrowser at a WebGL using page.

[Gyuyoung Kim]

Comment on attachment 161431 [details]
patch

Attachment 161431 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13685677

[Simon Hausmann]

Comment on attachment 161431 [details]
patch

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

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:213
> -        delete m_surface;
>          m_surface = 0;
> -        // Platform context is assumed to be owned by surface.
>          m_platformContext = 0;

Hmm, I think we have a bigger problem here.

There are different cases here when it comes to the creation of the surface:

   1) m_surface is allocated through createPlatformGraphicsContext3D, which in createPlatformGraphicsContext3DFromWidget creates a new QGLWidget, takes its windowHandle() and assigns it to surface. Somebody has to delete that QGLWidget, and with the current code it'll be deleted through the code path here. m_surface will be a QSurface and its virtual destructor will take care of deleting the QWindow, but I'm suddenly not sure anymore that deleting the QWidgetWindow will also delete the corresponding QGLWidget. This may require a change in Qt.

   2) The surface is adopted via renderStyle == GraphicsContext3D::RenderToCurrentGLContext, in which case deleting it is indeed totally wrong.

   3) The surface is created in the #if USE(GRAPHICS_SURFACE) block and an actual QWindow. Deleting m_surface is the right thing to do.

Besides m_surface and m_platformContext we could have a third member variable that points to the object we should delete in the destructor. In case (1) that should be the QGLWidget, in case 2 it should be a null pointer (delete 0 being fine) and in the third case it should be the surface.

[Noam Rosenthal]

Comment on attachment 161431 [details]
patch

This is not the correct solution, see other comment :)

[Roland Takacs]

Created attachment 163368 [details]
patch

What is your opinion about this?

[Simon Hausmann]

Comment on attachment 163368 [details]
patch

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

I think this patch is almost there :)

> Source/WebCore/ChangeLog:15
> +        No new tests (OOPS!).

You should remove this line :)

> Source/WebCore/platform/graphics/GraphicsContext3D.h:73
> +typedef QObject* PlatformGraphicsWindow3D;

You can remove this typedef, it's not necessary.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:87
> +    PlatformGraphicsWindow3D m_surfaceOwner;

Instead just change the type of this straight to a QPointer* :)

> Source/WebCore/platform/qt/QWebPageClient.h:112
> +                                                 PlatformGraphicsWindow3D* = 0) = 0;

And here the last output parameter would also be a QObject* and (similarly in the implementations)

[Roland Takacs]

Created attachment 163552 [details]
patch

There is no segfault with this patch :)

[Simon Hausmann]

Comment on attachment 163552 [details]
patch

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

> Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp:58
> +        if (surfaceOwner)
> +            surfaceOwner = glWidget;

I think the type of surfaceOwner needs to be a QObject** for this to work, otherwise surface = glWidget is just going to change the local stack pointer. Instead it needs to be a pointer to a pointer and then it needs to read *surfaceOwner = glWidget. (of course alternatively you could pass a reference to a pointer, that's fine, too)

[Roland Takacs]

Created attachment 163626 [details]
patch

[WebKit Review Bot]

Comment on attachment 163626 [details]
patch

Clearing flags on attachment: 163626

Committed r128315: <http://trac.webkit.org/changeset/128315>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.