Skip to content

Commit

Permalink
[debugger] Fix deadlock in DebugModuleCache
Browse files Browse the repository at this point in the history 
Event handlers in DebugModuleCache may try switching to the main thread, which is dangerous to do while holding a lock. This change address this in two ways:
1) move all event handler invocations outside of the `lock` section
2) don't switch to main thread for calling `IDebugEventCallback2.Send` as it doesn't actually need it.

GitOrigin-RevId: b0ceb1592299bd6513fba63578a1b57c291c33e6
  • Loading branch information
@werat @copybara-github
werat authored and copybara-github committed Aug 16, 2022
1 parent c624804 commit e66a6b5
Show file tree
Hide file tree
Showing 6 changed files with 89 additions and 72 deletions.
2 changes: 1 addition & 1 deletion YetiVSI.Shared/DebugEngine/DebugEngineFactoryCompRoot.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ public virtual IDebugEngineFactory CreateDebugEngineFactory()
new LldbAttachedProgram.Factory(
GetJoinableTaskContext(),
GetFactoryDecorator().Decorate<IDebugEngineHandlerFactory>(
new DebugEngineHandler.Factory(GetJoinableTaskContext())),
new DebugEngineHandler.Factory()),
GetTaskExecutor(), eventManagerFactory, debugProgramFactory,
debugModuleFactory, debugThreadAsyncFactory,
debugStackFrameFactory, lldbShell, breakpointManagerFactory,
Expand Down
53 changes: 20 additions & 33 deletions YetiVSI.Shared/DebugEngine/DebugEngineHandler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,11 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using DebuggerApi;
using Microsoft.VisualStudio.Debugger.Interop;
using Microsoft.VisualStudio;
using System.Diagnostics;
using Microsoft.VisualStudio.Debugger.Interop;
using YetiVSI.DebugEngine.Exit;
using Microsoft.VisualStudio.Threading;
using System.Threading.Tasks;

namespace YetiVSI.DebugEngine
{
Expand Down Expand Up @@ -124,26 +122,18 @@ public class DebugEngineHandler : IDebugEngineHandler
{
public class Factory : IDebugEngineHandlerFactory
{
readonly JoinableTaskContext _taskContext;

public Factory(JoinableTaskContext taskContext)
public IDebugEngineHandler Create(
IDebugEngine2 debugEngine, IDebugEventCallback2 eventCallback)
{
_taskContext = taskContext;
return new DebugEngineHandler(debugEngine, eventCallback);
}

public IDebugEngineHandler Create(IDebugEngine2 debugEngine,
IDebugEventCallback2 eventCallback) =>
new DebugEngineHandler(_taskContext, debugEngine, eventCallback);
}

readonly JoinableTaskContext _taskContext;
readonly IDebugEngine2 _debugEngine;
readonly IDebugEventCallback2 _eventCallback;

public DebugEngineHandler(JoinableTaskContext taskContext, IDebugEngine2 debugEngine,
IDebugEventCallback2 eventCallback)
public DebugEngineHandler(IDebugEngine2 debugEngine, IDebugEventCallback2 eventCallback)
{
_taskContext = taskContext;
_debugEngine = debugEngine;
_eventCallback = eventCallback;
}
Expand All @@ -155,30 +145,27 @@ public DebugEngineHandler(JoinableTaskContext taskContext, IDebugEngine2 debugEn
// https://docs.microsoft.com/en-us/visualstudio/extensibility/debugger/supported-event-types
public int SendEvent(IGgpDebugEvent evnt, IGgpDebugProgram program, IDebugThread2 thread)
{
return _taskContext.Factory.Run(async () =>
{
return await SendEventAsync(evnt, program, thread);
});
}

public int SendEvent(IGgpDebugEvent evnt, IGgpDebugProgram program,
RemoteThread thread) => SendEvent(evnt, program,
program.GetDebugThread(thread));

async Task<int> SendEventAsync(IGgpDebugEvent evnt, IGgpDebugProgram program,
IDebugThread2 thread)
{
await _taskContext.Factory.SwitchToMainThreadAsync();

if (((IDebugEvent2)evnt).GetAttributes(out uint attributes) != VSConstants.S_OK)
if (evnt.GetAttributes(out uint attributes) != VSConstants.S_OK)
{
Trace.WriteLine($"Could not get event attributes of event ({evnt})");
return VSConstants.E_FAIL;
}

Guid eventId = evnt.EventId;
return _eventCallback.Event(_debugEngine, null, program, thread, evnt, ref eventId,
attributes);

#pragma warning disable VSTHRD010 // Invoke single-threaded types on Main thread
// IDebugEventCallback2.Send doesn't actually require to be called on the main thread.
var ret = _eventCallback.Event(
_debugEngine, null, program, thread, evnt, ref eventId, attributes);
#pragma warning restore VSTHRD010 // Invoke single-threaded types on Main thread

return ret;
}

public int SendEvent(IGgpDebugEvent evnt, IGgpDebugProgram program, RemoteThread thread)
{
return SendEvent(evnt, program, program.GetDebugThread(thread));
}
}
}
2 changes: 1 addition & 1 deletion YetiVSI.Shared/DebugEngine/DebugModule.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public int GetInfo(enum_MODULE_INFO_FIELDS fields, MODULE_INFO[] moduleInfo)
info.m_addrPreferredLoadAddress = _lldbModule.GetCodeLoadAddress();
info.dwValidFields |= enum_MODULE_INFO_FIELDS.MIF_PREFFEREDADDRESS;
}

// is used to calculate address's range end
if (HasFlag(enum_MODULE_INFO_FIELDS.MIF_SIZE))
{
Expand Down
95 changes: 65 additions & 30 deletions YetiVSI.Shared/DebugEngine/DebugModuleCache.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,13 @@
// See the License for the specific language governing permissions and
// limitations under the License.

using DebuggerApi;
using Microsoft.VisualStudio.Debugger.Interop;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using DebuggerApi;
using Microsoft.VisualStudio.Debugger.Interop;
using YetiCommon.CastleAspects;
using YetiVSI.Util;

namespace YetiVSI.DebugEngine
{
Expand Down Expand Up @@ -106,8 +105,7 @@ public delegate IDebugModule3 ModuleCreator(SbModule lldbModule, uint loadOrder,
public event EventHandler<ModuleRemovedEventArgs> ModuleRemoved;

readonly ModuleCreator moduleCreator;

Dictionary<SbModule, IDebugModule3> cache;
readonly Dictionary<SbModule, IDebugModule3> cache;
uint nextLoadOrder = 0;

public DebugModuleCache(ModuleCreator moduleCreator)
Expand All @@ -118,59 +116,96 @@ public DebugModuleCache(ModuleCreator moduleCreator)

public IDebugModule3 GetOrCreate(SbModule lldbModule, IGgpDebugProgram program)
{
IDebugModule3 module;
bool added = false;

lock (cache)
{
if (!cache.TryGetValue(lldbModule, out IDebugModule3 module))
if (!cache.TryGetValue(lldbModule, out module))
{
module = moduleCreator(lldbModule, nextLoadOrder++, program);
cache.Add(lldbModule, module);
added = true;
}
}

try
{
ModuleAdded?.Invoke(Self, new ModuleAddedEventArgs(module));
}
catch (Exception e)
{
Trace.WriteLine(
$"Warning: ModuleAdded handler failed: {e.Demystify()}");
}
};
return module;
// Event handlers _may_ try to switch to the main thread, which is dangerous to do
// while holding a lock. Therefore we fire them after leaving the critical section.
if (added)
{
try
{
ModuleAdded?.Invoke(Self, new ModuleAddedEventArgs(module));
}
catch (Exception e)
{
Trace.WriteLine(
$"Warning: ModuleAdded handler failed: {e.Demystify()}");
}
}
return module;
}

public bool Remove(SbModule lldbModule)
{
IDebugModule3 module;
bool removed = false;

lock (cache)
{
if (cache.TryGetValue(lldbModule, out IDebugModule3 module))
if (cache.TryGetValue(lldbModule, out module))
{
cache.Remove(lldbModule);
try
{
ModuleRemoved?.Invoke(Self, new ModuleRemovedEventArgs(module));
}
catch (Exception e)
{
Trace.WriteLine(
$"Warning: ModuleRemoved handler failed: {e.Demystify()}");
}
return true;
removed = true;
}
return false;
}

// Event handlers _may_ try to switch to the main thread, which is dangerous to do
// while holding a lock. Therefore we fire them after leaving the critical section.
if (removed)
{
try
{
ModuleRemoved?.Invoke(Self, new ModuleRemovedEventArgs(module));
}
catch (Exception e)
{
Trace.WriteLine(
$"Warning: ModuleRemoved handler failed: {e.Demystify()}");
}
}
return removed;
}

public void RemoveAllExcept(IEnumerable<SbModule> liveModules)
{
var removedModules = new List<IDebugModule3>();

lock (cache)
{
var comparer = SbModuleEqualityComparer.Instance;
var deadModules = cache.Keys.Except(liveModules, comparer).ToList();

foreach (var module in deadModules)
{
Remove(module);
IDebugModule3 debugModule = cache[module];
cache.Remove(module);
removedModules.Add(debugModule);
}
}

// Event handlers _may_ try to switch to the main thread, which is dangerous to do
// while holding a lock. Therefore we fire them after leaving the critical section.
foreach (var module in removedModules)
{
try
{
ModuleRemoved?.Invoke(Self, new ModuleRemovedEventArgs(module));
}
catch (Exception e)
{
Trace.WriteLine(
$"Warning: ModuleRemoved handler failed: {e.Demystify()}");
}
}
}
Expand Down
7 changes: 1 addition & 6 deletions YetiVSI.Tests/DebugEngine/DebugEngineHandlerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
using DebuggerApi;
using Microsoft.VisualStudio;
using Microsoft.VisualStudio.Debugger.Interop;
using Microsoft.VisualStudio.Threading;
using NSubstitute;
using NUnit.Framework;
using System;
Expand Down Expand Up @@ -45,18 +44,14 @@ class DebugEngineHandlerTests
[SetUp]
public void SetUp()
{
#pragma warning disable VSSDK005 // Avoid instantiating JoinableTaskContext
var taskContext = new JoinableTaskContext();
#pragma warning restore VSSDK005 // Avoid instantiating JoinableTaskContext

logSpy = new LogSpy();
logSpy.Attach();

program = Substitute.For<IGgpDebugProgram>();
debugEngine = Substitute.For<IDebugEngine2>();
callback = Substitute.For<IDebugEventCallback2>();

handler = new DebugEngineHandler(taskContext, debugEngine, callback);
handler = new DebugEngineHandler(debugEngine, callback);

var idArg = Arg.Any<Guid>();
program.GetProgramId(out idArg)
Expand Down
2 changes: 1 addition & 1 deletion YetiVSI.Tests/DebugEngine/DebugSessionLauncherTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ DebugSessionLauncher.Factory CreateLauncherFactory(bool stadiaPlatformAvailable,
var symbolSettingsProvider = Substitute.For<ISymbolSettingsProvider>();

var attachedProgramFactory = new LldbAttachedProgram.Factory(
taskContext, new DebugEngineHandler.Factory(taskContext), taskExecutor,
taskContext, new DebugEngineHandler.Factory(), taskExecutor,
new LldbEventManager.Factory(new BoundBreakpointEnumFactory(), taskContext),
new DebugProgram.Factory(taskContext,
new DebugDisassemblyStream.Factory(
Expand Down

0 comments on commit e66a6b5

Please sign in to comment.