View Issue Details

IDProjectCategoryView StatusLast Update
0032623PatchesLCLpublic2018-02-28 11:15
ReporterC Western Assigned ToJuha Manninen  
PrioritynormalSeverityminorReproducibilityrandom
Status resolvedResolutionfixed 
Platformi386OSMACOSX 
Product Version1.9 (SVN) 
Summary0032623: IDE crash on opening Tools, Options, User Defined Markup
DescriptionOn MacOSX/Carbon, Tools, Options, User Defined Markup crashes the IDE.
Investigating this it seems the crash occurs in TCustomGrid.EditorPos because a bad value (from an unset variable) is used in
FEditor.BoundsRect := CellR.
I think the grid is empty at the time of the call, so CellR:=CellRect(FCol,FRow); simply does not set CellR. The attached patch is one possible workaround, forcing sane values. (An alternative aproach would be to act on the fail returns in ColRowToOffset.)
TagsNo tags attached.
Fixed in Revisionr56235
LazTarget-
WidgetsetCarbon
Attached Files

Relationships

related to 0033273 closedJuha Manninen Lazarus After revision 56235 a exception is called when a app with a DBGrid is closed 

Activities

C Western

2017-10-29 09:00

reporter  

grid.patch (667 bytes)   
diff -uwNr --exclude=.svn --exclude=Makefile --exclude=Makefile.fpc --exclude=Makefile.compiled --exclude='*.rsj' --exclude='*.bak' --exclude='*.po' lazarus/lcl/grids.pas lazarus.w/lcl/grids.pas
--- lazarus/lcl/grids.pas	2017-10-28 12:40:36.000000000 +0100
+++ lazarus.w/lcl/grids.pas	2017-10-28 19:21:39.000000000 +0100
@@ -3425,6 +3425,7 @@
 function TCustomGrid.CellRect(ACol, ARow: Integer): TRect;
 begin
   //Result:=ColRowToClientCellRect(aCol,aRow);
+  Result := Rect(0,0,0,0); // Force sane values in case calls below fail
   ColRowToOffset(True, True, ACol, Result.Left, Result.Right);
   ColRowToOffSet(False,True, ARow, Result.Top, Result.Bottom);
 end;

grid.patch (667 bytes)   

Juha Manninen

2017-10-29 11:03

developer   ~0103804

Last edited: 2017-10-29 11:03

View 2 revisions

Widgetset = Carbon. However the patch is for LCL Grids code. The issue does not look like widgetset dependent.(?)

C Western

2017-10-29 11:07

reporter   ~0103806

I think it is indeed widgetset independent. (I selected Carbon because the crash only happens on a Mac for me.)

Juha Manninen

2017-10-29 13:01

developer   ~0103807

ACol and ARow in CellRect must be out of range then. I think their validity should be checked explicitly and adjust the result accordingly.
Another option is to add an Assert() for their validity and then make sure the method is not called with wrong values. It would be more robust.

C Western

2017-10-29 20:58

reporter   ~0103811

The root cause of the original crash may be that EditorPos shouldn't be called at all if there are no cells in the grid, but I am less sure about that. I attach an alternate patch that is more robust.

C Western

2017-10-29 20:59

reporter  

grid.alt.patch (2,739 bytes)   
diff -uwNr '--exclude=.svn' '--exclude=Makefile' '--exclude=Makefile.fpc' '--exclude=Makefile.compiled' '--exclude=*.rsj' '--exclude=*.bak' '--exclude=*.po' lazarus/lcl/grids.pas lazarus.w/lcl/grids.pas
--- lazarus/lcl/grids.pas	2017-09-25 08:48:02.487059888 +0100
+++ lazarus.w/lcl/grids.pas	2017-10-29 18:21:32.018523519 +0000
@@ -1258,6 +1258,7 @@
     procedure AutoAdjustColumns; virtual;
     procedure BeginUpdate;
     function  CellRect(ACol, ARow: Integer): TRect;
+    function  CellRectValid(ACol, ARow: Integer; out ARect: TRect): Boolean;
     function  CellToGridZone(aCol,aRow: Integer): TGridZone;
     procedure CheckPosition;
     function ClearCols: Boolean;
@@ -3421,12 +3422,16 @@
   end;
 end;
 
-{ Returns a reactagle corresponding to a fisical cell[aCol,aRow] }
+{ Returns a reactagle corresponding to a physical cell[aCol,aRow] }
 function TCustomGrid.CellRect(ACol, ARow: Integer): TRect;
 begin
-  //Result:=ColRowToClientCellRect(aCol,aRow);
-  ColRowToOffset(True, True, ACol, Result.Left, Result.Right);
-  ColRowToOffSet(False,True, ARow, Result.Top, Result.Bottom);
+  CellRectValid(ACol, ARow, Result);
+end;
+
+function TCustomGrid.CellRectValid(ACol, ARow: Integer; out ARect: TRect): Boolean;
+begin
+  Result := ColRowToOffset(True, True, ACol, ARect.Left, ARect.Right)
+            and ColRowToOffSet(False,True, ARow, ARect.Top, ARect.Bottom)
 end;
 
 // The visible grid Depends on  TopLeft and ClientWidht,ClientHeight,
@@ -5291,7 +5296,7 @@
       end;
 end;
 
-function TCustomGrid.GetDefColWidth: integer;
+function TCustomGrid.GetDefColWidth: Integer;
 begin
   if FDefColWidth<0 then
   begin
@@ -5302,7 +5307,7 @@
     Result := FDefColWidth;
 end;
 
-function TCustomGrid.GetDefRowHeight: integer;
+function TCustomGrid.GetDefRowHeight: Integer;
 begin
   if FDefRowHeight<0 then
   begin
@@ -8207,6 +8220,7 @@
 var
   msg: TGridMessage;
   CellR: TRect;
+  PosValid: Boolean;
 begin
   {$ifdef dbgGrid} DebugLn('Grid.EditorPos INIT');{$endif}
   if HandleAllocated and (FEditor<>nil) then begin
@@ -8219,9 +8233,11 @@
     FEditor.Dispatch(Msg);
 
     // send editor bounds
-    CellR:=CellRect(FCol,FRow);
+    PosValid := CellRectValid(FCol, FRow, CellR);
+    if not PosValid then // Can't position editor; ensure sane values
+      CellR := Rect(0,0,FEditor.Width, FEditor.Height);
 
-    if (CellR.Top<FGCache.FixedHeight) or (CellR.Top>FGCache.ClientHeight) or
+    if not PosValid or (CellR.Top<FGCache.FixedHeight) or (CellR.Top>FGCache.ClientHeight) or
        (UseRightToLeftAlignment and ((CellR.Right-1>FlipX(FGCache.FixedWidth)) or (CellR.Right<0))) or
        (not UseRightToLeftAlignment and ((CellR.Left<FGCache.FixedWidth) or (CellR.Left>FGCache.ClientWidth)))
     then
grid.alt.patch (2,739 bytes)   

Juha Manninen

2017-10-29 22:22

developer   ~0103814

Thanks. I applied the patch with minor changes.
I added an Assert() to CellRect and it did not trigger during my tests.

Issue History

Date Modified Username Field Change
2017-10-29 09:00 C Western New Issue
2017-10-29 09:00 C Western File Added: grid.patch
2017-10-29 11:01 Juha Manninen LazTarget => -
2017-10-29 11:01 Juha Manninen Description Updated View Revisions
2017-10-29 11:03 Juha Manninen Note Added: 0103804
2017-10-29 11:03 Juha Manninen Note Edited: 0103804 View Revisions
2017-10-29 11:07 C Western Note Added: 0103806
2017-10-29 13:01 Juha Manninen Note Added: 0103807
2017-10-29 20:58 C Western Note Added: 0103811
2017-10-29 20:59 C Western File Added: grid.alt.patch
2017-10-29 22:11 Juha Manninen Assigned To => Juha Manninen
2017-10-29 22:11 Juha Manninen Status new => assigned
2017-10-29 22:22 Juha Manninen Fixed in Revision => r56235
2017-10-29 22:22 Juha Manninen Note Added: 0103814
2017-10-29 22:22 Juha Manninen Status assigned => resolved
2017-10-29 22:22 Juha Manninen Resolution open => fixed
2018-02-28 11:15 Michl Relationship added related to 0033273