View Issue Details

IDProjectCategoryView StatusLast Update
0035706LazarusWidgetsetpublic2019-06-19 22:19
ReporterZoë PetersonAssigned ToDmitry Boyarintsev 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version2.0.3 (SVN)Product Build 
Target VersionFixed in Version 
Summary0035706: Cocoa crash from event handlers accessing freed memory
DescriptionI don't have a patch for this yet, because I believe a proper fix will require extensive changes and wanted to discuss it before making them.

Currently TLCLCommonCallback objects are freed in the widgetset DestroyHandle functions, immediately before the NS object is released. If a control is destroyed in response to an event where it's the target, the calling functions may access the callback object after it's been freed.

I think that the proper fix for this would be to free the callback in the NS object's dealloc, rather than DestroyHandle. Cocoa retains the NS object until the event is done being processed. DestroyHandle might need to still set the callback's Target to nil to terminate further processing, and if so, we would probably need to add additional checks of Assigned(Target).

We saw the issue because we're using FastMM4's full debug mode, which catches use of freed objects/interfaces and faults. An easier way to expose it is by applying the attached patch, which triggers a fault if the callback is freed in the middle of KeyEvBefore. Once you do, run the sample project, click on an item in the treeview a couple of times to make it enter rename mode, then press [Enter] or [Esc] to exit it.

I've verified that the issue was not caused by my key event patches. It affects mouse down events too, though that's been harder to trigger since they don't access the callback object as much as the key events do. In the case where we were seeing it, we were moving a TFrame between two forms and freeing one of the forms.
TagsNo tags attached.
Fixed in Revision
LazTarget-
WidgetsetCocoa
Attached Files
  • RenameCrash.zip (130,849 bytes)
  • ForceCrash.patch (841 bytes)
    diff --git a/lcl/interfaces/cocoa/cocoawscommon.pas b/lcl/interfaces/cocoa/cocoawscommon.pas
    index bb78bcecb9..de7cb9aa45 100644
    --- a/lcl/interfaces/cocoa/cocoawscommon.pas
    +++ b/lcl/interfaces/cocoa/cocoawscommon.pas
    @@ -32,6 +32,7 @@ type
           FBoundsReportedToChildren: boolean;
           FIsOpaque:boolean;
           FIsEventRouting:boolean;
    +      FFreed: Boolean;
       protected
         function GetHasCaret: Boolean;
         procedure SetHasCaret(AValue: Boolean);
    @@ -395,6 +396,7 @@ end;
     
     destructor TLCLCommonCallback.Destroy;
     begin
    +  FFreed := True;
       FContext.Free;
       FPropStorage.Free;
       FTarget := nil;
    @@ -774,6 +776,9 @@ begin
     
       if Event.type_ = NSFlagsChanged then
         AllowCocoaHandle := true;
    +
    +  if FFreed then
    +    raise EInvalidPointer.Create('Callback freed early');
     end;
     
     procedure TLCLCommonCallback.KeyEvAfter;
    
    ForceCrash.patch (841 bytes)

Activities

Zoë Peterson

2019-06-12 01:12

reporter  

RenameCrash.zip (130,849 bytes)
ForceCrash.patch (841 bytes)
diff --git a/lcl/interfaces/cocoa/cocoawscommon.pas b/lcl/interfaces/cocoa/cocoawscommon.pas
index bb78bcecb9..de7cb9aa45 100644
--- a/lcl/interfaces/cocoa/cocoawscommon.pas
+++ b/lcl/interfaces/cocoa/cocoawscommon.pas
@@ -32,6 +32,7 @@ type
       FBoundsReportedToChildren: boolean;
       FIsOpaque:boolean;
       FIsEventRouting:boolean;
+      FFreed: Boolean;
   protected
     function GetHasCaret: Boolean;
     procedure SetHasCaret(AValue: Boolean);
@@ -395,6 +396,7 @@ end;
 
 destructor TLCLCommonCallback.Destroy;
 begin
+  FFreed := True;
   FContext.Free;
   FPropStorage.Free;
   FTarget := nil;
@@ -774,6 +776,9 @@ begin
 
   if Event.type_ = NSFlagsChanged then
     AllowCocoaHandle := true;
+
+  if FFreed then
+    raise EInvalidPointer.Create('Callback freed early');
 end;
 
 procedure TLCLCommonCallback.KeyEvAfter;
ForceCrash.patch (841 bytes)

Dmitry Boyarintsev

2019-06-13 20:23

developer   ~0116713

Last edited: 2019-06-13 20:24

View 3 revisions

alternatives:
* manual "garbage collection" / delayed release. (instead of calling callbackobject.free - just put it into some list and free after the end of event processing)
* using COM interfaces, instead of Corba. lclClearcallback should take care of the references from Cocoa object.
* rewriting the code. Not to store a callback in a local variable and always try query the callback from Cocoa object.

A concerning part would be - not to call any messages for (LCL) Target. As target might not exist (right after DestroyHandle) as well.

Obviously, overriding "dealloc" for each possible control is undesired solution.

Zoë Peterson

2019-06-13 22:37

reporter   ~0116714

Dmitry,

Why is overriding dealloc undesirable? As far as I can tell it would just replace the existing lclClearCallback function everywhere.

Using COM interfaces won't work. The compiler doesn't support mixing them with Objective C objects.

Always querying for the callback isn't enough. The crash in the sample project occurs while it's executing a callback method, so 'Self' becomes invalid in in the middle of it.

Sticking them on a list and freeing them would mirror Objective C's existing AutoRelease behavior, so that might work, but I'm wary of the NS object and the callback's freed state getting out of sync, and of lclGetCallback returning a callback at the start of a function and not doing so at the end.

My impression is that COM objects would be best, but lacking those, I need to do a hybrid:

1) Callbacks, once assigned, should stay valid until both dealloc and the message loop run. Never clear the NS object's callback reference once it's assigned.
2) "Target" may become invalid, and should always be checked explicitly before accessing it.
3) Callback objects should stay alive until both dealloc runs and we return to the message loop. Add some sort of explicit reference count and free list.

I'll need to spend some time on the other widgetset interfaces too, to see how they handle this. It might not help, but the LCL does have its own refcounting used for DeliverMessage to give an error if the target is freed in the middle.

I can make the changes, but I'll have to audit basically all of the LCLCOCOA code, and the patch will probably touch a lot of areas. Unless you have a good reason not to, can you please merge the keyboard handling patch in 0035538 so I don't have deal with conflicts between them?

Dmitry Boyarintsev

2019-06-14 15:47

developer   ~0116721

please see changes in r61387

Issue History

Date Modified Username Field Change
2019-06-12 01:12 Zoë Peterson New Issue
2019-06-12 01:12 Zoë Peterson File Added: RenameCrash.zip
2019-06-12 01:12 Zoë Peterson File Added: ForceCrash.patch
2019-06-13 20:23 Dmitry Boyarintsev Note Added: 0116713
2019-06-13 20:24 Dmitry Boyarintsev Note Edited: 0116713 View Revisions
2019-06-13 20:24 Dmitry Boyarintsev Note Edited: 0116713 View Revisions
2019-06-13 20:25 Dmitry Boyarintsev Assigned To => Dmitry Boyarintsev
2019-06-13 20:25 Dmitry Boyarintsev Status new => feedback
2019-06-13 20:25 Dmitry Boyarintsev LazTarget => -
2019-06-13 22:37 Zoë Peterson Note Added: 0116714
2019-06-13 22:37 Zoë Peterson Status feedback => assigned
2019-06-14 15:47 Dmitry Boyarintsev Status assigned => feedback
2019-06-14 15:47 Dmitry Boyarintsev Note Added: 0116721
2019-06-19 22:06 Dmitry Boyarintsev Status feedback => resolved
2019-06-19 22:06 Dmitry Boyarintsev Resolution open => fixed
2019-06-19 22:06 Dmitry Boyarintsev Widgetset Cocoa => Cocoa
2019-06-19 22:19 Zoë Peterson Status resolved => closed