All Projects FPC fpcprojects fpprofilerfpGUI Lazarus Packages PatchesLazarus CCR Mantis pas2js

View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0035395LazarusWidgetsetpublic2019-04-16 10:042020-02-13 10:09
ReporterBoris Glavin Assigned ToJuha Manninen  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionreopened 
PlatformLinuxOSAll 
Product Version2.1 (SVN) 
Fixed in Version2.0.4 
Summary0035395: Race condition in TGtk2WidgetSet (TGtkMessageQueue)
DescriptionIn methods Lock/UnLock of TGtkMessageQueue
```
procedure TGtkMessageQueue.Lock;
begin
  inc(fLock);
  if fLock=1 then
    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
    EnterCriticalsection(FCritSec);
    {$ELSE}
    g_main_context_acquire(FMainContext);
    {$ENDIF}
end;

procedure TGtkMessageQueue.UnLock;
begin
  dec(fLock);
  if fLock=0 then
    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
    LeaveCriticalsection(FCritSec);
    {$ELSE}
    g_main_context_release(FMainContext);
    {$ENDIF}
end;
```
race status is possible
Steps To ReproduceExecute multiple call PostMessage from another threads
Additional InformationIt is possible to add critical section for solution of the problems:
```
procedure TGtkMessageQueue.Lock;
begin
  EnterCriticalSection(FLockCritSec);
  inc(fLock);
  if fLock=1 then
    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
    EnterCriticalsection(FCritSec);
    {$ELSE}
    g_main_context_acquire(FMainContext);
    {$ENDIF}
  LeaveCriticalSection(FLockCritSec);
end;

procedure TGtkMessageQueue.UnLock;
begin
  EnterCriticalSection(FLockCritSec);
  dec(fLock);
  if fLock=0 then
    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
    LeaveCriticalsection(FCritSec);
    {$ELSE}
    g_main_context_release(FMainContext);
    {$ENDIF}
  LeaveCriticalSection(FLockCritSec);
end;
```
TagsNo tags attached.
Fixed in Revisionr61026, r62219
LazTarget-
WidgetsetGTK 2
Attached Files

Activities

Zeljan Rikalo

2019-04-17 17:14

developer   ~0115598

Please create valid patch

Martin Friebe

2019-04-17 18:19

manager   ~0115603

Does it need another critical section?

Or would a InterLockedIncrement() / InterLockedDecrement() do?

Boris Glavin

2019-04-17 18:53

reporter   ~0115605

Last edited: 2019-04-17 22:38

View 2 revisions

 Yes, InterlockedIncrement/InterlockedDecrement will solve a problem with in excess calls of g_main_context_release. Patch added.

Boris Glavin

2019-04-17 22:21

reporter  

gtk2msgqueue.path (683 bytes)    
Index: lcl/interfaces/gtk2/gtk2msgqueue.pp
===================================================================
--- lcl/interfaces/gtk2/gtk2msgqueue.pp	(revision 61001)
+++ lcl/interfaces/gtk2/gtk2msgqueue.pp	(working copy)
@@ -143,8 +143,7 @@
 
 procedure TGtkMessageQueue.Lock;
 begin
-  inc(fLock);
-  if fLock=1 then
+  if InterlockedIncrement(fLock)=1 then
     {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
     EnterCriticalsection(FCritSec);
     {$ELSE}
@@ -154,8 +153,7 @@
 
 procedure TGtkMessageQueue.UnLock;
 begin
-  dec(fLock);
-  if fLock=0 then
+  if InterlockedDecrement(fLock)=1 then
     {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
     LeaveCriticalsection(FCritSec);
     {$ELSE}
gtk2msgqueue.path (683 bytes)   

Boris Glavin

2019-04-17 22:47

reporter  

gtk2msgqueue1.path (683 bytes)    
Index: lcl/interfaces/gtk2/gtk2msgqueue.pp
===================================================================
--- lcl/interfaces/gtk2/gtk2msgqueue.pp	(revision 61001)
+++ lcl/interfaces/gtk2/gtk2msgqueue.pp	(working copy)
@@ -143,8 +143,7 @@
 
 procedure TGtkMessageQueue.Lock;
 begin
-  inc(fLock);
-  if fLock=1 then
+  if InterlockedIncrement(fLock)=1 then
     {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
     EnterCriticalsection(FCritSec);
     {$ELSE}
@@ -154,8 +153,7 @@
 
 procedure TGtkMessageQueue.UnLock;
 begin
-  dec(fLock);
-  if fLock=0 then
+  if InterlockedDecrement(fLock)=0 then
     {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
     LeaveCriticalsection(FCritSec);
     {$ELSE}
gtk2msgqueue1.path (683 bytes)   

Boris Glavin

2019-04-19 22:51

reporter   ~0115683

Macroes LOCK_CONTEXT/UNLOCK_CONTEXT - thread save?
https://github.com/GNOME/glib/blob/master/glib/gmain.c
InterlockedIncrement/InterlockedDecrement not working

Martin Friebe

2019-04-20 22:26

manager   ~0115693

Please test, and close if ok.

Boris Glavin

2019-04-21 22:39

reporter   ~0115713

My old patch does not solve a problem because it is deeper. The call of g_main_context_iteration removes blocking and leads to a race during adding of events in queue. It is necessary to think and understand what the author of a solution tried to make. An example for reproduction of an error in the attached archive. In the analysis of the code only what g_main_context_acquire/g_main_context_release it makes sense to cause only from the main flow is evident (is caused implicitly through TGtkMessageQueue.Lock/TGtkMessageQueue.UnLock in TGtk2WidgetSet.AppProcessMessages). The call from other flows g_main_context_acquire/g_main_context_release does not make any sense (it is a part of glib, and GTK not thread safe).
gtk2msgqueue3.path (1,357 bytes)    
Index: lcl/interfaces/gtk2/gtk2msgqueue.pp
===================================================================
--- lcl/interfaces/gtk2/gtk2msgqueue.pp	(revision 61030)
+++ lcl/interfaces/gtk2/gtk2msgqueue.pp	(working copy)
@@ -54,8 +54,8 @@
     FMainContext: PGMainContext;
     {$ELSE}
     FCritSec: TRTLCriticalSection;
+    fLock: integer;
     {$ENDIF}
-    fLock: integer;
   protected
     function CreateItem : TLinkListItem;override;
     function CalculateHash(ParWnd : Hwnd):integer;
@@ -86,6 +86,9 @@
 
 implementation
 
+uses
+  Classes;
+
 {---(TGtkMessageQueueItem)----------------------}
 
 function TGtkMessageQueueItem.IsPaintMessage: Boolean;
@@ -143,10 +146,11 @@
 
 procedure TGtkMessageQueue.Lock;
 begin
+    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
   if InterlockedIncrement(fLock)=1 then
-    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
     EnterCriticalsection(FCritSec);
     {$ELSE}
+  if GetCurrentThreadId = MainThreadID then
     g_main_context_acquire(FMainContext);
     {$ENDIF}
 end;
@@ -153,10 +157,11 @@
 
 procedure TGtkMessageQueue.UnLock;
 begin
+    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
   if InterlockedDecrement(fLock)=0 then
-    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
     LeaveCriticalsection(FCritSec);
     {$ELSE}
+  if GetCurrentThreadId = MainThreadID then
     g_main_context_release(FMainContext);
     {$ENDIF}
 end;
gtk2msgqueue3.path (1,357 bytes)   
test.zip (128,468 bytes)

Boris Glavin

2019-04-22 09:03

reporter   ~0115718

Update path and debug project
testgtk2msgqueue.zip (3,691 bytes)
gtk2msgqueue4.path (2,354 bytes)    
Index: lcl/interfaces/gtk2/gtk2msgqueue.pp
===================================================================
--- lcl/interfaces/gtk2/gtk2msgqueue.pp	(revision 61030)
+++ lcl/interfaces/gtk2/gtk2msgqueue.pp	(working copy)
@@ -50,12 +50,12 @@
   TGtkMessageQueue=class(TLinkList)
   private
     FPaintMessages: TDynHashArray; // Hash for paint messages
+    FCritSec: TRTLCriticalSection;
     {$IFNDEF USE_GTK_MAIN_OLD_ITERATION}
     FMainContext: PGMainContext;
     {$ELSE}
-    FCritSec: TRTLCriticalSection;
+    fLock: integer;
     {$ENDIF}
-    fLock: integer;
   protected
     function CreateItem : TLinkListItem;override;
     function CalculateHash(ParWnd : Hwnd):integer;
@@ -86,6 +86,9 @@
 
 implementation
 
+uses
+  Classes;
+
 {---(TGtkMessageQueueItem)----------------------}
 
 function TGtkMessageQueueItem.IsPaintMessage: Boolean;
@@ -121,9 +124,8 @@
   inherited Create;
   FPaintMessages := TDynHashArray.Create(-1);
   FPaintMessages.OwnerHashFunction := @HashPaintMessage;
-  {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
   InitCriticalSection(FCritSec);
-  {$ELSE}
+  {$IFNDEF USE_GTK_MAIN_OLD_ITERATION}
   FMainContext := g_main_context_new;
   g_main_context_ref(FMainContext);
   {$ENDIF}
@@ -133,31 +135,36 @@
 begin
   inherited Destroy;
   fPaintMessages.destroy;
-  {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
-  DoneCriticalsection(FCritSec);
-  {$ELSE}
+  {$IFNDEF USE_GTK_MAIN_OLD_ITERATION}
   g_main_context_unref(FMainContext);
   FMainContext := nil;
   {$ENDIF}
+  DoneCriticalsection(FCritSec);
 end;
 
 procedure TGtkMessageQueue.Lock;
 begin
+    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
   if InterlockedIncrement(fLock)=1 then
-    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
     EnterCriticalsection(FCritSec);
     {$ELSE}
-    g_main_context_acquire(FMainContext);
+  if GetCurrentThreadId = MainThreadID then
+    g_main_context_acquire(FMainContext)
+  else
+    EnterCriticalsection(FCritSec);
     {$ENDIF}
 end;
 
 procedure TGtkMessageQueue.UnLock;
 begin
+    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
   if InterlockedDecrement(fLock)=0 then
-    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
     LeaveCriticalsection(FCritSec);
     {$ELSE}
-    g_main_context_release(FMainContext);
+  if GetCurrentThreadId = MainThreadID then
+    g_main_context_release(FMainContext)
+  else
+    LeaveCriticalsection(FCritSec)
     {$ENDIF}
 end;
gtk2msgqueue4.path (2,354 bytes)   

Martin Friebe

2019-04-22 12:26

manager   ~0115724

re-open, and unassigned => needs to go to one of the gtk maintainers

Juha Manninen

2019-04-24 20:34

developer   ~0115779

Boris, please give your patches a suffix ".patch" instead of ".path". They can be viewed directly from a web browser when you do so.

Boris Glavin

2019-04-24 20:42

reporter   ~0115780

ok
gtk2msgqueue4.patch (2,354 bytes)    
Index: lcl/interfaces/gtk2/gtk2msgqueue.pp
===================================================================
--- lcl/interfaces/gtk2/gtk2msgqueue.pp	(revision 61030)
+++ lcl/interfaces/gtk2/gtk2msgqueue.pp	(working copy)
@@ -50,12 +50,12 @@
   TGtkMessageQueue=class(TLinkList)
   private
     FPaintMessages: TDynHashArray; // Hash for paint messages
+    FCritSec: TRTLCriticalSection;
     {$IFNDEF USE_GTK_MAIN_OLD_ITERATION}
     FMainContext: PGMainContext;
     {$ELSE}
-    FCritSec: TRTLCriticalSection;
+    fLock: integer;
     {$ENDIF}
-    fLock: integer;
   protected
     function CreateItem : TLinkListItem;override;
     function CalculateHash(ParWnd : Hwnd):integer;
@@ -86,6 +86,9 @@
 
 implementation
 
+uses
+  Classes;
+
 {---(TGtkMessageQueueItem)----------------------}
 
 function TGtkMessageQueueItem.IsPaintMessage: Boolean;
@@ -121,9 +124,8 @@
   inherited Create;
   FPaintMessages := TDynHashArray.Create(-1);
   FPaintMessages.OwnerHashFunction := @HashPaintMessage;
-  {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
   InitCriticalSection(FCritSec);
-  {$ELSE}
+  {$IFNDEF USE_GTK_MAIN_OLD_ITERATION}
   FMainContext := g_main_context_new;
   g_main_context_ref(FMainContext);
   {$ENDIF}
@@ -133,31 +135,36 @@
 begin
   inherited Destroy;
   fPaintMessages.destroy;
-  {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
-  DoneCriticalsection(FCritSec);
-  {$ELSE}
+  {$IFNDEF USE_GTK_MAIN_OLD_ITERATION}
   g_main_context_unref(FMainContext);
   FMainContext := nil;
   {$ENDIF}
+  DoneCriticalsection(FCritSec);
 end;
 
 procedure TGtkMessageQueue.Lock;
 begin
+    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
   if InterlockedIncrement(fLock)=1 then
-    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
     EnterCriticalsection(FCritSec);
     {$ELSE}
-    g_main_context_acquire(FMainContext);
+  if GetCurrentThreadId = MainThreadID then
+    g_main_context_acquire(FMainContext)
+  else
+    EnterCriticalsection(FCritSec);
     {$ENDIF}
 end;
 
 procedure TGtkMessageQueue.UnLock;
 begin
+    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
   if InterlockedDecrement(fLock)=0 then
-    {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
     LeaveCriticalsection(FCritSec);
     {$ELSE}
-    g_main_context_release(FMainContext);
+  if GetCurrentThreadId = MainThreadID then
+    g_main_context_release(FMainContext)
+  else
+    LeaveCriticalsection(FCritSec)
     {$ENDIF}
 end;
gtk2msgqueue4.patch (2,354 bytes)   

Martin Friebe

2019-04-27 22:51

manager   ~0115859

Note on the Interlocked... I may have been wrong with that suggestion.

It is possible than:

T1: InterLockedDec reaches 0
T2: Does an InterlockInc
T2: enters critical section // g_main_context_acquire(FMainContext) => but it has not yet been released
T1: enters critical section // g_main_release_acquire(FMainContext)

- So either the entire code must be in a critical section.
- Or if a critical section is entered, the Flock value must be checked again, and rather complex handling would be needed. => Probably better to do all in a critical section.

Sorry for the misleading idea.

Andre Kappert

2019-11-07 10:56

reporter   ~0119130

I still had problems in 2.0.4. Updated to Lazarus 2.0.6. Still error, with on stack view “lock” from file gtk2msgqueue.pp after postmessage . Expected to be solved in 2.0.4?
Used the patch gtk2msgqueue4.patch, which still worked on the new gtk2msgqueue.pp. Found that I had to rebuild lazarus to update gtk2msgqueue.ppu (“sudo make bigide” in lazarus directory).
After this patch, the problems where solved.

Juha Manninen

2019-11-09 11:21

developer   ~0119172

LCL-GTK2 has no active maintainer who really knows the code. Thus I am applying the patch. How big speed penalty does it cause? I guess r61026 by Martin is still OK.
First I though USE_GTK_MAIN_OLD_ITERATION can be removed as an old leftover but apparently it is still used for Windows. Can somebody please test if it really is needed there still. The comment about hanging is very old.
  {$define USE_GTK_MAIN_OLD_ITERATION} // in other case it hangs

Zeljan Rikalo

2019-11-09 12:33

developer   ~0119174

Please do not remove USE_GTK_MAIN_OLD_ITERATION since it is still used by users with older linux installations (eg Debian with gtk2-2.14)

Andre Kappert

2019-11-09 16:19

reporter   ~0119175

I had the problems on Linux Mint 19.2 Tina 64-bits, Mate 1.22.0. That’s the last version, so not very old. Because the problem is in a gtk part, I thought it was obvious that Linux was the OS. I had tested my program with Windows 10 (default LCLwidgettype) , but not that extensively (My windows computer was very slow). I had no problems with Windows, but that testing was much shorter.

Juha Manninen

2019-11-10 13:14

developer   ~0119184

I applied the patch. Thanks.

> Because the problem is in a gtk part, I thought it was obvious that Linux was the OS.
No, it is not obvious. GTK2 works under many systems including Windows although I don't think many people use it there any more.
Anyway, apparently USE_GTK_MAIN_OLD_ITERATION is still needed also with Linux.

Please look at file gtk2defines.inc :

{$ifdef Unix}
  ...
{$else}
  {$define GTK_2_10}
  {$define USE_GTK_MAIN_OLD_ITERATION} // in other case it hangs
{$endif}

Issue History

Date Modified Username Field Change
2019-04-16 10:04 Boris Glavin New Issue
2019-04-17 17:14 Zeljan Rikalo Note Added: 0115598
2019-04-17 18:19 Martin Friebe Note Added: 0115603
2019-04-17 18:53 Boris Glavin Note Added: 0115605
2019-04-17 22:21 Boris Glavin File Added: gtk2msgqueue.path
2019-04-17 22:38 Boris Glavin Note Edited: 0115605 View Revisions
2019-04-17 22:47 Boris Glavin File Added: gtk2msgqueue1.path
2019-04-19 22:51 Boris Glavin Note Added: 0115683
2019-04-20 12:41 Martin Friebe Assigned To => Martin Friebe
2019-04-20 12:41 Martin Friebe Status new => assigned
2019-04-20 22:26 Martin Friebe Status assigned => resolved
2019-04-20 22:26 Martin Friebe Resolution open => fixed
2019-04-20 22:26 Martin Friebe Fixed in Version => 2.0.4
2019-04-20 22:26 Martin Friebe Fixed in Revision => 61026
2019-04-20 22:26 Martin Friebe LazTarget => 2.0.4
2019-04-20 22:26 Martin Friebe Widgetset GTK 2 => GTK 2
2019-04-20 22:26 Martin Friebe Note Added: 0115693
2019-04-21 22:39 Boris Glavin File Added: gtk2msgqueue3.path
2019-04-21 22:39 Boris Glavin File Added: test.zip
2019-04-21 22:39 Boris Glavin Note Added: 0115713
2019-04-22 09:03 Boris Glavin File Added: testgtk2msgqueue.zip
2019-04-22 09:03 Boris Glavin File Added: gtk2msgqueue4.path
2019-04-22 09:03 Boris Glavin Note Added: 0115718
2019-04-22 12:26 Martin Friebe Assigned To Martin Friebe =>
2019-04-22 12:26 Martin Friebe Status resolved => assigned
2019-04-22 12:26 Martin Friebe Resolution fixed => reopened
2019-04-22 12:26 Martin Friebe LazTarget 2.0.4 => -
2019-04-22 12:26 Martin Friebe Note Added: 0115724
2019-04-22 12:26 Martin Friebe Status assigned => new
2019-04-22 12:26 Martin Friebe Widgetset GTK 2 => GTK 2
2019-04-24 20:34 Juha Manninen Note Added: 0115779
2019-04-24 20:42 Boris Glavin File Added: gtk2msgqueue4.patch
2019-04-24 20:42 Boris Glavin Note Added: 0115780
2019-04-27 22:51 Martin Friebe Note Added: 0115859
2019-11-07 10:56 Andre Kappert Note Added: 0119130
2019-11-09 11:08 Juha Manninen Assigned To => Juha Manninen
2019-11-09 11:08 Juha Manninen Status new => assigned
2019-11-09 11:21 Juha Manninen Note Added: 0119172
2019-11-09 11:37 Juha Manninen Status assigned => feedback
2019-11-09 12:33 Zeljan Rikalo Note Added: 0119174
2019-11-09 16:19 Andre Kappert Note Added: 0119175
2019-11-10 13:14 Juha Manninen Status feedback => resolved
2019-11-10 13:14 Juha Manninen Fixed in Revision 61026 => r61026, r62219
2019-11-10 13:14 Juha Manninen Widgetset GTK 2 => GTK 2
2019-11-10 13:14 Juha Manninen Note Added: 0119184
2019-11-25 17:44 Juha Manninen Relationship added related to 0036359
2019-12-13 23:19 Juha Manninen Relationship deleted related to 0036359