Bug 95003 - [Qt] Segmentation fault when closing QtTestBrowser
Summary: [Qt] Segmentation fault when closing QtTestBrowser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Roland Takacs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-25 05:33 PDT by Roland Takacs
Modified: 2012-09-12 08:36 PDT (History)
5 users (show)

See Also:


Attachments
patch (1.40 KB, patch)
2012-08-30 02:28 PDT, Roland Takacs
noam: review-
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
patch (9.07 KB, patch)
2012-09-11 08:24 PDT, Roland Takacs
no flags Details | Formatted Diff | Diff
patch (8.28 KB, patch)
2012-09-12 02:00 PDT, Roland Takacs
no flags Details | Formatted Diff | Diff
patch (8.29 KB, patch)
2012-09-12 07:46 PDT, Roland Takacs
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Takacs 2012-08-25 05:33:45 PDT
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)
Comment 1 Roland Takacs 2012-08-30 02:28:06 PDT
Created attachment 161431 [details]
patch

This patch eliminates the segmentation fault when closing QtTestBrowser at a WebGL using page.
Comment 2 Gyuyoung Kim 2012-08-30 02:58:52 PDT
Comment on attachment 161431 [details]
patch

Attachment 161431 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13685677
Comment 3 Simon Hausmann 2012-08-30 06:22:09 PDT
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.
Comment 4 Noam Rosenthal 2012-08-31 10:34:03 PDT
Comment on attachment 161431 [details]
patch

This is not the correct solution, see other comment :)
Comment 5 Roland Takacs 2012-09-11 08:24:19 PDT
Created attachment 163368 [details]
patch

What is your opinion about this?
Comment 6 Simon Hausmann 2012-09-11 10:40:47 PDT
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)
Comment 7 Roland Takacs 2012-09-12 02:00:34 PDT
Created attachment 163552 [details]
patch

There is no segfault with this patch :)
Comment 8 Simon Hausmann 2012-09-12 02:53:03 PDT
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)
Comment 9 Roland Takacs 2012-09-12 07:46:28 PDT
Created attachment 163626 [details]
patch
Comment 10 WebKit Review Bot 2012-09-12 08:36:22 PDT
Comment on attachment 163626 [details]
patch

Clearing flags on attachment: 163626

Committed r128315: <http://trac.webkit.org/changeset/128315>
Comment 11 WebKit Review Bot 2012-09-12 08:36:26 PDT
All reviewed patches have been landed.  Closing bug.