kicad-developers team mailing list archive

[jp charras <jp.charras@xxxxxxxxxx>]

Le 09/05/2016 à 17:20, Maciej Sumiński a écrit :
> On 05/05/2016 02:26 PM, jp charras wrote:
>> Hi, Orson.
>>
>> I made a few tests:
>>
>> The crash happens in OPENGL_GAL::~OPENGL_GAL().
>>
>> Exactly when executing delete OPENGL_GAL::glContext;
>>
>> if I comment the 2 lines
>> delete OPENGL_GAL::glContext;
>> glContext = NULL;
>>
>> I do not have any crash: all works fine (exit pcbnew and switch between Opengl and cairo).
>>
>> Hope this can help you.
> 
> Thank you Jean-Pierre. Code that deletes OpenGL context was added
> recently, and apparently it is the culprit. I would rather keep it
> there, as it seems necessary to free the context when it is not used
> anymore. If I cannot find another way, then I will simply remove the
> cleaning code, as it is called only on exit, so it should be freed anyway.
> 
> I am wondering though, do not you have any issues when closing 3D
> viewer? It deletes OpenGL context as well, and I wonder if it causes
> problems there.
> 
> Regards,
> Orson
> 

Hi, Orson,

About the crash which happens in OPENGL_GAL dtor, when executing delete OPENGL_GAL::glContext;, I am
now pretty sure the crash is due to an use after delete issue.

If it is called when exiting the application, no problem.

The issue certainly depends on the OS and the WM, so it explains why on some OS there is no crash.

I had a look at the wxWidgets opengl sample cube.cpp.

The sample deletes also the wxGLContext in wxApp:OnExit, not in wxGLCanvas dtor.
There is surely a reason.

I attached a patch which shows my changes. Please have a look at it and feel free to use and enhance it.

I noticed a wxClientDC initialized but never used in Opengl_gal class. I removed it.
I also noticed an other thing: in DRAW_PANEL_GAL::OnPaint event, a wxPaintDC is never created.
This is mandatory in OnPaint events, even if the wxDC is not used (this is in wxWidgets doc).
I added it.
(See the wxWidgets opengl sample cube.cpp for these problems).

Thanks.

-- 
Jean-Pierre CHARRAS

=== modified file 'common/draw_panel_gal.cpp'
--- common/draw_panel_gal.cpp	2016-05-20 10:06:08 +0000
+++ common/draw_panel_gal.cpp	2016-05-29 15:57:26 +0000
@@ -64,7 +64,7 @@
 
     SwitchBackend( aGalType );
     SetBackgroundStyle( wxBG_STYLE_CUSTOM );
-    
+
 // Scrollbars broken in GAL on OSX
 #ifdef __WXMAC__
     ShowScrollbars( wxSHOW_SB_NEVER, wxSHOW_SB_NEVER );
@@ -153,6 +153,9 @@
 
 void EDA_DRAW_PANEL_GAL::onPaint( wxPaintEvent& WXUNUSED( aEvent ) )
 {
+    // This is required even though dc is not used otherwise.
+    wxPaintDC dc(this);
+
     m_pendingRefresh = false;
 
     if( m_drawing )

=== modified file 'common/gal/opengl/opengl_gal.cpp'
--- common/gal/opengl/opengl_gal.cpp	2016-05-29 15:02:34 +0000
+++ common/gal/opengl/opengl_gal.cpp	2016-05-29 18:11:45 +0000
@@ -40,6 +40,7 @@
 #include <limits>
 #include <boost/bind.hpp>
 
+
 using namespace KIGFX;
 
 #include "bitmap_font_img.c"
@@ -52,6 +53,7 @@
 int OPENGL_GAL::instanceCounter = 0;
 bool OPENGL_GAL::isBitmapFontLoaded = false;
 
+
 OPENGL_GAL::OPENGL_GAL( wxWindow* aParent, wxEvtHandler* aMouseListener,
                         wxEvtHandler* aPaintListener, const wxString& aName ) :
     wxGLCanvas( aParent, wxID_ANY, (int*) glAttributes, wxDefaultPosition, wxDefaultSize,
@@ -146,13 +148,20 @@
             glDeleteTextures( 1, &fontTexture );
             isBitmapFontLoaded = false;
         }
-
-        delete OPENGL_GAL::glContext;
-        glContext = NULL;
     }
 }
 
 
+// This function must be called only when exit the application, not earlier
+// (depending on OS and window manager, calling it earlier can crash Kicad,
+// perhaps because the gl context is acceded far after the canvas are destroyed)
+void OPENGL_GAL::DeleteGLContext()
+{
+    delete OPENGL_GAL::glContext;
+    OPENGL_GAL::glContext = NULL;
+}
+
+
 void OPENGL_GAL::BeginDrawing()
 {
     if( !IsShownOnScreen() )
@@ -164,7 +173,6 @@
 #endif /* __WXDEBUG__ */
 
     SetCurrent( *glContext );
-    clientDC = new wxClientDC( this );
 
 #ifdef RETINA_OPENGL_PATCH
     const float scaleFactor = GetBackingScaleFactor();
@@ -300,8 +308,6 @@
 
     SwapBuffers();
 
-    delete clientDC;
-
 #ifdef __WXDEBUG__
     prof_end( &totalRealTime );
     wxLogTrace( "GAL_PROFILE", wxT( "OPENGL_GAL::EndDrawing(): %.1f ms" ), totalRealTime.msecs() );

=== modified file 'include/gal/opengl/opengl_gal.h'
--- include/gal/opengl/opengl_gal.h	2016-05-20 10:06:08 +0000
+++ include/gal/opengl/opengl_gal.h	2016-05-29 18:11:10 +0000
@@ -52,6 +52,7 @@
 {
 class SHADER;
 
+
 /**
  * @brief Class OpenGL_GAL is the OpenGL implementation of the Graphics Abstraction Layer.
  *
@@ -62,7 +63,6 @@
 class OPENGL_GAL : public GAL, public wxGLCanvas
 {
 public:
-
     /**
      * @brief Constructor OPENGL_GAL
      *
@@ -91,6 +91,16 @@
         return IsShownOnScreen();
     }
 
+    /**
+     * DeleteGLContext delete the wxGLContext used by the canvas.
+     * However, it cannot be called just when the canvas is no more in use
+     * (i.e. from the dtor) because this wxGLContext can be acceded by the OS after
+     * the canvas is deleted.
+     * The only one place to call this function is when exit the application
+     * So this function must be called only from wxAppl::Exit()
+     */
+    static void DeleteGLContext();
+
     // ---------------
     // Drawing methods
     // ---------------
@@ -273,7 +283,6 @@
     static const int    CIRCLE_POINTS   = 64;   ///< The number of points for circle approximation
     static const int    CURVE_POINTS    = 32;   ///< The number of points for curve approximation
 
-    wxClientDC*             clientDC;               ///< Drawing context
     static wxGLContext*     glContext;              ///< OpenGL context of wxWidgets
     wxEvtHandler*           mouseListener;
     wxEvtHandler*           paintListener;

=== modified file 'pcbnew/pcbnew.cpp'
--- pcbnew/pcbnew.cpp	2016-05-10 21:37:51 +0000
+++ pcbnew/pcbnew.cpp	2016-05-29 18:09:01 +0000
@@ -364,11 +364,18 @@
     return true;
 }
 
+#include <gal/opengl/opengl_gal.h>     // for KIGFX::OPENGL_GAL::DeleteGLContext() definition
 
 void IFACE::OnKifaceEnd()
 {
+    // This function cleans the OpenGL struct used (if any) by the wxGLCanvas.
+    // It can be called only when closing the application
+    // because it delete a OpenGL structure which can be used just before exiting application.
+    // Calling that earlier can crash the application, depending on the OS and the VM.
+    // There is no problem to call it even if the OpenGL canvas was not used
+    KIGFX::OPENGL_GAL::DeleteGLContext();
+
     end_common();
-
 #if defined( KICAD_SCRIPTING_WXPYTHON )
     // Restore the thread state and tell Python to cleanup after itself.
     // wxPython will do its own cleanup as part of that process.