Skip to content

Commit

Permalink
Inline Rename: Prevent crash when source control dialogs show
Browse files Browse the repository at this point in the history
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems?_a=edit&id=227513

InlineRenameSession.ApplyReplacementText is called in response to buffer changes. In response to these changes, we want to (1) kick off some background work to calculate the new ConflictResolutionTask, (2) propagate the new replacementText to all buffers on the UI thread, and then (3) apply the results of the ConflictResolutionTask on the UI thread once they're ready.

Prior to this change, steps (1) and (3) were always done together, in sequence, regardless of how (2) was progressing. Since the replacementText propagation started synchronously and stayed on the UI thread, this usually meant that the application of the results of the ConflictResolutionTask would have to wait until the replacementText propagation completed (because it would wait for the UI thread to be available). This worked great, most of the time.

But, if the propagation of the replacementText causes a buffer change in a source controlled document and "Prompt for Checkout" is enabled, then a modal dialog to checkout files appears. Then, the UI thread pumps and allows more work to be scheduled there. Our ConflictResolutionTask could finish calculating conflicts and the application of these conflicts could be scheduled on the UI thread while a dialog is still up (and we are still propagating the replacementText). It would then try to manipulate the undo stack and apply edits to buffers. Since we still have an open undo transaction from the replacementText propagation,
the attempt to manipulate the undo stack would crash.

This change splits the kicking off of the ConflictResolutionTask and the application of the replacements it computes into separately callable methods that aren't automatically chained together. In some cases they can be called in sequence, emulating the old behavior. But when a text buffer edit triggers ApplyReplacementText, we first kick off the ConflictResolutionTask, then propagate the new replacementText, and *then*, once the replacementText is fully propagated (including the handling of any source control modal dialogs), we finally apply the results of the ConflictResolutionTask on the UI thread.

This does not guarantee that *no* reentrancy will ever happen in Inline Rename, but it gets rid of the biggest, obvious culprit. Completely safeguarding would require a larger change, and I think this approach
is the best one given our current RTM escrow timeline, and it will fix the problem for the vast majority of users encountering this problem.
  • Loading branch information
David Poeschl committed Jan 31, 2017
1 parent f60c7fd commit 1bbe244
Showing 1 changed file with 44 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ private void UpdateReferenceLocationsTask(Task<IInlineRenameLocationSet> allRena
ForegroundTaskScheduler).CompletesAsyncOperation(asyncToken);

UpdateConflictResolutionTask();
QueueApplyReplacements();
}

public Workspace Workspace { get { return _workspace; } }
Expand Down Expand Up @@ -382,10 +383,15 @@ internal void ApplyReplacementText(string replacementText, bool propagateEditImm
VerifyNotDismissed();
this.ReplacementText = replacementText;

var asyncToken = _asyncListener.BeginAsyncOperation(nameof(ApplyReplacementText));

Action propagateEditAction = delegate
{
AssertIsForeground();

if (_dismissed)
{
asyncToken.Dispose();
return;
}

Expand All @@ -399,8 +405,29 @@ internal void ApplyReplacementText(string replacementText, bool propagateEditImm
}

_isApplyingEdit = false;

// We already kicked off UpdateConflictResolutionTask below (outside the delegate).
// Now that we are certain the replacement text has been propagated to all of the
// open buffers, it is safe to actually apply the replacements it has calculated.
// See https://devdiv.visualstudio.com/DevDiv/_workitems?_a=edit&id=227513
QueueApplyReplacements();

asyncToken.Dispose();
};

// Start the conflict resolution task but do not apply the results immediately. The
// buffer changes performed in propagateEditAction can cause source control modal
// dialogs to show. Those dialogs pump, and yield the UI thread to whatever work is
// waiting to be done there, including our ApplyReplacements work. If ApplyReplacements
// starts running on the UI thread while propagateEditAction is still updating buffers
// on the UI thread, we crash because we try to enumerate the undo stack while an undo
// transaction is still in process. Therefore, we defer QueueApplyReplacements until
// after the buffers have been edited, and any modal dialogs have been completed.
// In addition to avoiding the crash, this also ensures that the resolved conflict text
// is applied after the simple text change is propagated.
// See https://devdiv.visualstudio.com/DevDiv/_workitems?_a=edit&id=227513
UpdateConflictResolutionTask();

if (propagateEditImmediately)
{
propagateEditAction();
Expand All @@ -410,8 +437,6 @@ internal void ApplyReplacementText(string replacementText, bool propagateEditImm
// When responding to a text edit, we delay propagating the edit until the first transaction completes.
Dispatcher.CurrentDispatcher.BeginInvoke(propagateEditAction, DispatcherPriority.Send, null);
}

UpdateConflictResolutionTask();
}

private void UpdateConflictResolutionTask()
Expand All @@ -421,6 +446,8 @@ private void UpdateConflictResolutionTask()
_conflictResolutionTaskCancellationSource.Cancel();
_conflictResolutionTaskCancellationSource = new CancellationTokenSource();

// If the replacement text is empty, we do not update the results of the conflict
// resolution task. We instead wait for a non-empty identifier.
if (this.ReplacementText == string.Empty)
{
return;
Expand All @@ -430,30 +457,32 @@ private void UpdateConflictResolutionTask()
var optionSet = _optionSet;
var cancellationToken = _conflictResolutionTaskCancellationSource.Token;

var asyncToken = _asyncListener.BeginAsyncOperation(nameof(UpdateConflictResolutionTask));

_conflictResolutionTask = _allRenameLocationsTask.SafeContinueWithFromAsync(
t => UpdateConflictResolutionTask(t.Result, replacementText, optionSet, cancellationToken),
t => t.Result.GetReplacementsAsync(replacementText, optionSet, cancellationToken),
cancellationToken,
TaskContinuationOptions.OnlyOnRanToCompletion,
TaskScheduler.Default);

var asyncToken = _asyncListener.BeginAsyncOperation("UpdateConflictResolutionTask");
_conflictResolutionTask.CompletesAsyncOperation(asyncToken);
}

private Task<IInlineRenameReplacementInfo> UpdateConflictResolutionTask(IInlineRenameLocationSet locations, string replacementText, OptionSet optionSet, CancellationToken cancellationToken)
private void QueueApplyReplacements()
{
var conflictResolutionTask = Task.Run(async () =>
await locations.GetReplacementsAsync(replacementText, optionSet, cancellationToken).ConfigureAwait(false),
cancellationToken);

var asyncToken = _asyncListener.BeginAsyncOperation("UpdateConflictResolutionTask");
conflictResolutionTask.SafeContinueWith(
t => ApplyReplacements(t.Result, cancellationToken),
cancellationToken,
// If the replacement text is empty, we do not update the results of the conflict
// resolution task. We instead wait for a non-empty identifier.
if (this.ReplacementText == string.Empty)
{
return;
}

var asyncToken = _asyncListener.BeginAsyncOperation(nameof(QueueApplyReplacements));
_conflictResolutionTask.SafeContinueWith(
t => ApplyReplacements(t.Result, _conflictResolutionTaskCancellationSource.Token),
_conflictResolutionTaskCancellationSource.Token,
TaskContinuationOptions.OnlyOnRanToCompletion,
ForegroundTaskScheduler).CompletesAsyncOperation(asyncToken);

return conflictResolutionTask;
}

private void ApplyReplacements(IInlineRenameReplacementInfo replacementInfo, CancellationToken cancellationToken)
Expand Down

0 comments on commit 1bbe244

Please sign in to comment.