Access violation in TEvent.InvokeEventHandlerStub/InternalInvoke when adding a eventhandler while in an eventhandler

[Deeem2031]

• the problem first appeared with commit event optimizations
• still persists with current commit 070b8ae (the line numbers in the text refer to this state)

In our main project the symptom is an access violation read at 0x00000000 at line 466 of Spring.Events, which is “pop edx” in TEvent.InvokeEventHandlerStub (that's what the IDE shows). I could pinpoint the actual problem to line 675 of TEvent.InternalInvoke at which handlers.Code is nil.

In my attempts to replicate the issue in a smaller project I could at least get a read access violation at 0x80808080 (freed memory) with both handlers.Code and .Data being 0x80808080.

The prerequisites seem to be quite complex with an event having multiple handlers, which have to access the container (?) and adding another handler in a sub-thread.

Note that the code does leak one instance of TC, which is expected.

program Project41;

uses
  FastMM4,
  System.Classes,
  Spring,
  Spring.Container;

type
  TEvent = procedure() of object;

type
  TC = class
  public class var
    Event: Event<TEvent>;
  public
    class procedure EventHandler1();
    class procedure EventHandler2();
    procedure EventHandler3();
    procedure EventHandler4();
  end;

class procedure TC.EventHandler1();
begin
  GlobalContainer.Resolve<TC>().Free;
end;

class procedure TC.EventHandler2();
begin
  TThread.CreateAnonymousThread(
    procedure
    begin
      var x := TC.Create;
      TC.Event.Add(x.EventHandler4);
    end).Start;
  TThread.Sleep(1000);
end;

procedure TC.EventHandler3();
begin
  //dummy
end;

procedure TC.EventHandler4();
begin
  //dummy
end;

var c: TC;
begin
  GlobalContainer.RegisterType<TC>;
  GlobalContainer.Build;
  c := TC.Create;
  c.Event.Add(c.EventHandler1);
  c.Event.Add(c.EventHandler2);
  c.Event.Add(c.EventHandler3);
  TC.Event.Invoke();
  c.Free;
end.

‌

[Stefan Glienke]

And I almost thought it’s ready for release

I’ll look into it later. Thanks for testing though! Much appreciated.

[Stefan Glienke]

fixed #361

→ <<cset 3687a86a0825>>

[Deeem2031]

While the MCVE does run without issues with the latest changes I can still replicate the issue in our main project. The only difference being that the address changed (from access to 0x00000000 to some random location).

I was not able to create a new MCVE yet, just wanted to let you know that this issue doesn't seem to be fixed.

[Deeem2031]

Even adding logging to all the public functions of Spring.HazardEra does alter the execution enough that the problem disappears. So it seems to be a race condition. I’m currently out of ideas on how to recreate the issue. Any ideas on your side?

[Stefan Glienke]

Is the situation when the AV occurs still the same? I mean still during Invoke and some other thread modifies the invoked event handler list or at other times?

Update: I managed to produce a stress test scenario that involves invoking and adding/removing event handlers which causes a leak that might as well be the cause for an AV in your scenario. Will investigate.

[Stefan Glienke]

Found and fixed the issue I mentioned - while I could only reliably reproduce a memory leak caused by this I suspect this could also have been the cause for AVs.

Please let me know if this works now for you.

[Deeem2031]

The latest change doesn't seem to help. Everything still the same as I descripted initially. On the plus side turning my example into a "stress test" does result in an access violation even with the most recent changes.

[Stefan Glienke]

Well then I need some code with this behavior to analyze further.

[Deeem2031]

That’s why i attached the code “Project41.dpr”

[Stefan Glienke]

Sorry, I missed that

Thanks, will look into

[Stefan Glienke]

Next attempt - this should finally do it

The issue was that the internal control blocks were reused and overridden when a guard was acquired while the current thread already had a guard acquired (during Invoke). This caused the prior guarded pointer to become unguarded and was potentially freed and being reused by the memory manager causing those AVs. Every guard now uses its own control block.

Please keep in mind that your latest project might still raise an AV which is being caused by the application already terminating while the anonymous threads are still running.

Thanks again for testing and being patient.

[Deeem2031]

That fixed it! Thanks.

[Stefan Glienke]

fixed #361

→ <<cset a7d543927ceb>>