[clangd] Fix a potential race by copying the FixIts out of ASTManager before releasing the lock.

Also document the locking semantics a bit better.

llvm-svn: 296737

3 files changed, 12 insertions, 3 deletions

diff --git a/clang-tools-extra/clangd/ASTManager.cpp b/clang-tools-extra/clangd/ASTManager.cpp
index 7eccce7ee58..7353d135143 100644
--- a/clang-tools-extra/clangd/ASTManager.cpp
+++ b/clang-tools-extra/clangd/ASTManager.cpp
@@ -224,11 +224,11 @@ ASTManager::createASTUnitForFile(StringRef Uri, const DocumentStore &Docs) {
       /*AllowPCHWithCompilerErrors=*/true));
 }
 
-llvm::ArrayRef<clang::tooling::Replacement>
+std::vector<clang::tooling::Replacement>
 ASTManager::getFixIts(const clangd::Diagnostic &D) {
   std::lock_guard<std::mutex> Guard(FixItLock);
   auto I = FixIts.find(D);
   if (I != FixIts.end())
     return I->second;
-  return llvm::None;
+  return {};
 }
diff --git a/clang-tools-extra/clangd/ASTManager.h b/clang-tools-extra/clangd/ASTManager.h
index 129ee888b67..48539fed0d3 100644
--- a/clang-tools-extra/clangd/ASTManager.h
+++ b/clang-tools-extra/clangd/ASTManager.h
@@ -39,7 +39,10 @@ public:
   // FIXME: Implement codeComplete
 
   /// Get the fixes associated with a certain diagnostic as replacements.
-  llvm::ArrayRef<clang::tooling::Replacement>
+  ///
+  /// This function is thread-safe. It returns a copy to avoid handing out
+  /// references to unguarded data.
+  std::vector<clang::tooling::Replacement>
   getFixIts(const clangd::Diagnostic &D);
 
   DocumentStore &getStore() const { return Store; }
diff --git a/clang-tools-extra/clangd/DocumentStore.h b/clang-tools-extra/clangd/DocumentStore.h
index 45385cd58f5..6d396236dff 100644
--- a/clang-tools-extra/clangd/DocumentStore.h
+++ b/clang-tools-extra/clangd/DocumentStore.h
@@ -49,6 +49,9 @@ public:
       Listener->onDocumentRemove(Uri);
   }
   /// Retrieve a document from the store. Empty string if it's unknown.
+  ///
+  /// This function is thread-safe. It returns a copy to avoid handing out
+  /// references to unguarded data.
   std::string getDocument(StringRef Uri) const {
     // FIXME: This could be a reader lock.
     std::lock_guard<std::mutex> Guard(DocsMutex);
@@ -59,6 +62,9 @@ public:
   void addListener(DocumentStoreListener *DSL) { Listeners.push_back(DSL); }
 
   /// Get name and constents of all documents in this store.
+  ///
+  /// This function is thread-safe. It returns a copies to avoid handing out
+  /// references to unguarded data.
   std::vector<std::pair<std::string, std::string>> getAllDocuments() const {
     std::vector<std::pair<std::string, std::string>> AllDocs;
     std::lock_guard<std::mutex> Guard(DocsMutex);