Release retired document sources outside the cache lock

internal/atspi/adapter.go

@@ -489,11 +489,30 @@ func (adapter *Adapter) retainedDocumentRoot(id ObjectID) ObjectID {
 // calls this on the GLib thread once the capture root is known, before
 // preparing the incremental-refresh cache.
 func (adapter *Adapter) rotateDocumentSlots(root ObjectID) {
+	adapter.rotateDocumentSlotsAndRelease(root, releaseDocumentSources)
+}
+
+func releaseDocumentSources(document stashedDocument) {
+	for _, source := range document.sources {
+		C.g_object_unref(C.gpointer(source))
+	}
+}
+
+// libatspi disposal synchronously emits defunct events. Publish the new slots
+// and unlock before releasing references, so reentrant invalidation sees a
+// consistent cache and can acquire treeMutex.
+func (adapter *Adapter) rotateDocumentSlotsAndRelease(root ObjectID, release func(stashedDocument)) {
 	if !root.Valid() {
 		return
 	}
+	var retired []stashedDocument
 	adapter.treeMutex.Lock()
-	defer adapter.treeMutex.Unlock()
+	defer func() {
+		adapter.treeMutex.Unlock()
+		for _, document := range retired {
+			release(document)
+		}
+	}()
 	if root == adapter.documentSlotRoot {
 		return
 	}
@@ -509,14 +528,12 @@ func (adapter *Adapter) rotateDocumentSlots(root ObjectID) {
 	}
 	if adapter.documentSlotRoot.Valid() &&
 		(adapter.documentTree != nil || adapter.documentTextTree != nil) {
-		adapter.stashDocumentLocked(adapter.documentSlotRoot, stashedDocument{
+		retired = adapter.stashDocumentLocked(adapter.documentSlotRoot, stashedDocument{
 			tree: adapter.documentTree, textTree: adapter.documentTextTree,
 			sources: adapter.nativeDocument,
 		})
-	} else {
-		for _, source := range adapter.nativeDocument {
-			C.g_object_unref(C.gpointer(source))
-		}
+	} else if len(adapter.nativeDocument) != 0 {
+		retired = append(retired, stashedDocument{sources: adapter.nativeDocument})
 	}
 	adapter.documentTree = stashed.tree
 	adapter.documentTextTree = stashed.textTree
@@ -524,14 +541,13 @@ func (adapter *Adapter) rotateDocumentSlots(root ObjectID) {
 	adapter.documentSlotRoot = root
 }
 
-func (adapter *Adapter) stashDocumentLocked(root ObjectID, document stashedDocument) {
+func (adapter *Adapter) stashDocumentLocked(root ObjectID, document stashedDocument) []stashedDocument {
+	var retired []stashedDocument
 	if adapter.stashedDocuments == nil {
 		adapter.stashedDocuments = make(map[ObjectID]stashedDocument)
 	}
 	if old, ok := adapter.stashedDocuments[root]; ok {
-		for _, source := range old.sources {
-			C.g_object_unref(C.gpointer(source))
-		}
+		retired = append(retired, old)
 		for index, id := range adapter.stashedDocumentIDs {
 			if id == root {
 				adapter.stashedDocumentIDs = append(adapter.stashedDocumentIDs[:index], adapter.stashedDocumentIDs[index+1:]...)
@@ -543,12 +559,11 @@ func (adapter *Adapter) stashDocumentLocked(root ObjectID, document stashedDocum
 	adapter.stashedDocumentIDs = append(adapter.stashedDocumentIDs, root)
 	for len(adapter.stashedDocumentIDs) > maxStashedDocuments {
 		oldest := adapter.stashedDocumentIDs[0]
-		for _, source := range adapter.stashedDocuments[oldest].sources {
-			C.g_object_unref(C.gpointer(source))
-		}
+		retired = append(retired, adapter.stashedDocuments[oldest])
 		delete(adapter.stashedDocuments, oldest)
 		adapter.stashedDocumentIDs = adapter.stashedDocumentIDs[1:]
 	}
+	return retired
 }
 
 // clearStashedDocuments releases every stashed document's native sources.

internal/atspi/adapter_tree_test.go

@@ -683,3 +683,67 @@ func TestCaptureFailsFastWhileProviderHung(t *testing.T) {
 		t.Fatalf("hung placement error = %v, want ErrProviderHung", err)
 	}
 }
+
+func TestDocumentSlotReleaseAllowsReentrantInvalidation(t *testing.T) {
+	for _, orphaned := range []bool{false, true} {
+		t.Run(fmt.Sprintf("orphaned=%v", orphaned), func(t *testing.T) {
+			adapter := New()
+			oldRoot := testID("/old")
+			newRoot := testID("/new")
+			oldTree, err := NewAccessibleTree(oldRoot, []TreeNode{{Snapshot: Snapshot{BusName: oldRoot.BusName, Path: oldRoot.Path}}})
+			if err != nil {
+				t.Fatal(err)
+			}
+			if orphaned {
+				adapter.nativeDocument = documentSourceKeys(adapter.nativeDocument, oldRoot)
+			} else {
+				adapter.documentSlotRoot = testID("/active")
+				adapter.documentTree = oldTree
+				adapter.stashedDocumentIDs = []ObjectID{oldRoot, testID("/second"), testID("/third")}
+				adapter.stashedDocuments = map[ObjectID]stashedDocument{oldRoot: {tree: oldTree}}
+			}
+			releases := 0
+			adapter.rotateDocumentSlotsAndRelease(newRoot, func(retired stashedDocument) {
+				releases++
+				if !adapter.treeMutex.TryLock() {
+					t.Fatal("native disposal would deadlock reentrant defunct invalidation")
+				}
+				adapter.treeMutex.Unlock()
+				if adapter.documentSlotRoot != newRoot || adapter.documentTree != nil {
+					t.Fatal("disposal observed partially rotated slots")
+				}
+				if _, retained := adapter.stashedDocuments[oldRoot]; retained {
+					t.Fatal("disposed document still retained")
+				}
+				adapter.invalidateTreeEvent(DefunctStateEvent, 1, oldRoot, ObjectID{})
+			})
+			if releases != 1 {
+				t.Fatalf("released %d documents, want 1", releases)
+			}
+		})
+	}
+}
+
+func BenchmarkDocumentSlotEviction(b *testing.B) {
+	adapter := New()
+	roots := make([]ObjectID, maxStashedDocuments+2)
+	trees := make([]*AccessibleTree, len(roots))
+	for i := range roots {
+		roots[i] = testID(fmt.Sprintf("/doc%d", i))
+		var err error
+		trees[i], err = NewAccessibleTree(roots[i], []TreeNode{{Snapshot: Snapshot{BusName: roots[i].BusName, Path: roots[i].Path}}})
+		if err != nil {
+			b.Fatal(err)
+		}
+	}
+	b.ReportAllocs()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		index := i % len(roots)
+		adapter.rotateDocumentSlots(roots[index])
+		if adapter.documentTree != nil {
+			b.Fatal("expected evicted document to need recapture")
+		}
+		adapter.documentTree = trees[index]
+	}
+}

internal/atspi/browser_native_test.go

@@ -390,6 +390,7 @@ waitReload:
 					if !activated {
 						t.Fatal("captured link action did not update the page")
 					}
+					testNativeBrowserDocumentEviction(t, adapter)
 					testNativeBrowserBackgroundLoad(t, adapter)
 					return
 				}
@@ -481,3 +482,34 @@ func testNativeBrowserBackgroundLoad(t *testing.T, adapter *Adapter) {
 		}
 	}
 }
+
+// More loads than the retained-slot limit force disposal of old native objects.
+// libatspi can synchronously deliver defunct callbacks during that disposal.
+func testNativeBrowserDocumentEviction(t *testing.T, adapter *Adapter) {
+	t.Helper()
+	for index := 0; index < maxStashedDocuments+2; index++ {
+		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+		if out, err := exec.CommandContext(ctx, "xdotool", "key", "--clearmodifiers", "ctrl+r").CombinedOutput(); err != nil {
+			cancel()
+			t.Fatalf("reload %d: %v %s", index, err, out)
+		}
+		var loaded ObjectID
+		for !loaded.Valid() {
+			select {
+			case event := <-adapter.Events():
+				if event.Type == DocumentLoadCompleteEvent && event.Source.Name == "Cthulhu Browser Probe" {
+					loaded = event.Source.ID()
+				}
+			case <-ctx.Done():
+				cancel()
+				t.Fatalf("reload %d: load completion missing", index)
+			}
+		}
+		tree, err := adapter.CaptureDocument(ctx, loaded)
+		cancel()
+		if err != nil {
+			t.Fatalf("capture after reload %d: %v", index, err)
+		}
+		findNamedReference(t, tree, "Browser foundation")
+	}
+}