[clangd][SwapIndex] ensure that the old index is alive while we are using it via the function returned by `SwapIndex::indexedFiles()` call

[ArcsinX]

Summary

Without this patch the old index could be freed, but there still could be tries to access it via the function returned by SwapIndex::indexedFiles() call.
This leads to hard to reproduce clangd crashes at code completion.
This patch keeps the old index alive until the function returned by SwapIndex::indexedFiles() call is alive.

[ArcsinX]

Fix format

[sammccall]

Thanks, I'd seen these crashes but not manage to track down yet!

LG with one fix

clang-tools-extra/clangd/index/Index.cpp

81–84

Calling snapshot() twice introduces a race - we may keep the wrong one alive.

Evaluate snapshot first into a variable, then call indexedFiles, then move both into the lambda?

[ArcsinX]

Call snapshot() only once to avoid possible race.

[ArcsinX]

Fix format

clang-tools-extra/clangd/index/Index.cpp

 void SwapIndex::relations(
     const RelationsRequest &R,
     llvm::function_ref<void(const SymbolID &, const Symbol &)> CB) const {
   return snapshot()->relations(R, CB);
 }
 
 llvm::unique_function<bool(llvm::StringRef) const>
 SwapIndex::indexedFiles() const {
-  return snapshot()->indexedFiles();
+  return [ KeepAlive = snapshot(),
+           IndexedFiles = snapshot()->indexedFiles() ](llvm::StringRef File) {
+    return IndexedFiles(File);
+  };
 }
 
 size_t SwapIndex::estimateMemoryUsage() const {
   return snapshot()->estimateMemoryUsage();
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/index/Merge.cpp

bool MergedIndex::fuzzyFind(
   unsigned MergedCount = 0;
   More |= Dynamic->fuzzyFind(Req, [&](const Symbol &S) {
     ++DynamicCount;
     DynB.insert(S);
   });
   SymbolSlab Dyn = std::move(DynB).build();
 
   llvm::DenseSet<SymbolID> SeenDynamicSymbols;
+  {
     auto DynamicContainsFile = Dynamic->indexedFiles();
     More |= Static->fuzzyFind(Req, [&](const Symbol &S) {
       // We expect the definition to see the canonical declaration, so it seems
       // to be enough to check only the definition if it exists.
       if (DynamicContainsFile(S.Definition ? S.Definition.FileURI
                                            : S.CanonicalDeclaration.FileURI))
         return;
       auto DynS = Dyn.find(S.ID);
       ++StaticCount;
       if (DynS == Dyn.end())
         return Callback(S);
       ++MergedCount;
       SeenDynamicSymbols.insert(S.ID);
       Callback(mergeSymbol(*DynS, S));
     });
+  }
   SPAN_ATTACH(Tracer, "dynamic", DynamicCount);
   SPAN_ATTACH(Tracer, "static", StaticCount);
   SPAN_ATTACH(Tracer, "merged", MergedCount);
   for (const Symbol &S : Dyn)
     if (!SeenDynamicSymbols.count(S.ID))
       Callback(S);
   return More;
 }
 
 void MergedIndex::lookup(
     const LookupRequest &Req,
     llvm::function_ref<void(const Symbol &)> Callback) const {
   trace::Span Tracer("MergedIndex lookup");
   SymbolSlab::Builder B;
 
   Dynamic->lookup(Req, [&](const Symbol &S) { B.insert(S); });
 
   auto RemainingIDs = Req.IDs;
+  {
     auto DynamicContainsFile = Dynamic->indexedFiles();
     Static->lookup(Req, [&](const Symbol &S) {
       // We expect the definition to see the canonical declaration, so it seems
       // to be enough to check only the definition if it exists.
       if (DynamicContainsFile(S.Definition ? S.Definition.FileURI
                                            : S.CanonicalDeclaration.FileURI))
         return;
       const Symbol *Sym = B.find(S.ID);
       RemainingIDs.erase(S.ID);
       if (!Sym)
         Callback(S);
       else
         Callback(mergeSymbol(*Sym, S));
     });
+  }
   for (const auto &ID : RemainingIDs)
     if (const Symbol *Sym = B.find(ID))
       Callback(*Sym);
 }
 
 bool MergedIndex::refs(const RefsRequest &Req,
                        llvm::function_ref<void(const Ref &)> Callback) const {
   trace::Span Tracer("MergedIndex refs");