Skip to content

Do not retry on Clipboard API for StackTraceExplorer#59658

Merged
ryzngard merged 5 commits intodotnet:mainfrom
ryzngard:issues/stacktrace_explorer_clipboard_hang
Feb 28, 2022
Merged

Do not retry on Clipboard API for StackTraceExplorer#59658
ryzngard merged 5 commits intodotnet:mainfrom
ryzngard:issues/stacktrace_explorer_clipboard_hang

Conversation

@ryzngard
Copy link
Copy Markdown
Contributor

@ryzngard ryzngard commented Feb 18, 2022

Fixes AB#1467909

Removes usage of Clipboard APIs for StackTrace Explorer in cases where we are looking at the clipboard data on VS activation

@ryzngard ryzngard requested a review from a team as a code owner February 18, 2022 23:35
@ghost ghost added the Area-IDE label Feb 18, 2022
@ryzngard ryzngard marked this pull request as draft February 18, 2022 23:35
@ryzngard ryzngard changed the title [DRAFT] Use GetPriorityClipboardFormat [DRAFT] Do not retry on Clipboard API for StackTraceExplorer Feb 18, 2022
/// </summary>
public static bool CanGetText()
{
const uint TextFormatId = 1; // ID for clipboard text format (CF_TEXT)
Copy link
Copy Markdown

@ryanmolden ryanmolden Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSDN says this is the format to use for ANSI text, there is also a CF_UNICODETEXT, do we need to accept unicode text that can't be expressed via CF_TEXT with the (also MSDN stated) rule that a single null terminator ends the datastream?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're looking at an older commit? This was changed to use IDataObject

@ryzngard ryzngard marked this pull request as ready for review February 22, 2022 21:42
@ryzngard ryzngard changed the title [DRAFT] Do not retry on Clipboard API for StackTraceExplorer Do not retry on Clipboard API for StackTraceExplorer Feb 22, 2022
Copy link
Copy Markdown
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This'll work, but still very worried about the UI thread being blocked if the clipboard is owned by another process and it's going to take it's time. Would love to see us just transition the fetch to the clipboard to our OOP process as an asynchronous request where we can then avoid blocking a VS thread entirely.

IntPtr ptr;
object handleRefObj = new();

ptr = Win32GlobalLock(new HandleRef(handleRefObj, handle));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to be using HandleRef here versus just GlobalLock directly being an IntPtr? It looks like the clipboard code you're copying from was trying to use that to keep some other object alive, but it's not doing anything here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that just be putting a global lock on the handle at that point instead of associating a ref object with it?

@ryzngard ryzngard merged commit 2c861cf into dotnet:main Feb 28, 2022
@ryzngard ryzngard deleted the issues/stacktrace_explorer_clipboard_hang branch February 28, 2022 03:57
@ghost ghost added this to the Next milestone Feb 28, 2022
333fred added a commit that referenced this pull request Feb 28, 2022
…ures/required-members

* upstream/main: (187 commits)
  Add GlobalOptions.SetBackgroundAnalysisScope and PythiaGlobalOptions External Access API (#59794)
  Update source-build dependency to source-build-externals (#59549)
  Do not retry on Clipboard API for StackTraceExplorer (#59658)
  Remove unnecessary accesses on XML end tag (#59771)
  Threading
  lint
  Improve PDB source document project handling (#59643)
  Disable Auto-Open behavior for Stack Trace Explorer (#59785)
  REmove comment
  Push async up
  Simplify
  Remove stale remarks
  Update src/Tools/ExternalAccess/FSharp/Navigation/FSharpDocumentNavigationService.cs
  Remove unnecessary code
  Simplify threading
  Update tests
  Make the  IDocumentNavigationSerivice entirely async.
  Disable additional text comparer in generator driver (#59776)
  [LSP] Cache parsed xml snippets for razor (#59605)
  Make static
  ...
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants