Fix stack overflow in endBatch() when disposing a Reaction from onBecomeUnobserved

.changeset/fix-endbatch-unobservation-reentrancy.md

@@ -0,0 +1,5 @@
+---
+"mobx": patch
+---
+
+Fix a stack overflow ("Maximum call stack size exceeded") that could occur when an `onBecomeUnobserved` handler disposes a `Reaction`. Disposing a `Reaction` re-enters `endBatch()`, which used to recurse into the same `pendingUnobservations` drain loop instead of letting the already-running outer loop pick up the newly queued items, causing unbounded stack depth for long enough chains.

packages/mobx/__tests__/base/become-observed.ts

@@ -510,6 +510,58 @@ test("#2667", () => {
     ])
 })
 
+test("#3954 - disposing a chain of reactions from onBecomeUnobserved doesn't overflow the stack", () => {
+    // Each box's onBecomeUnobserved handler disposes the next reaction in the
+    // chain. Disposing reaction[0] unobserves box[0], whose handler disposes
+    // reaction[1], which unobserves box[1], and so on. Each of those disposals
+    // re-enters endBatch() while the previous one is still draining
+    // pendingUnobservations, so this used to recurse N deep instead of
+    // looping, overflowing the stack for a large enough chain.
+    const N = 10000
+    const boxes = Array.from({ length: N }, () => observable.box(0))
+    const disposers = boxes.map(box => autorun(() => box.get()))
+    let unobservedCount = 0
+
+    boxes.forEach((box, i) => {
+        onBecomeUnobserved(box, () => {
+            unobservedCount++
+            if (i + 1 < N) {
+                disposers[i + 1]()
+            }
+        })
+    })
+
+    expect(() => disposers[0]()).not.toThrow()
+
+    // the whole chain should have unwound, not just the first link
+    expect(unobservedCount).toBe(N)
+})
+
+test("#3954 followup - isRunningUnobservations is released even if an onBecomeUnobserved handler throws", () => {
+    const boxA = observable.box(0)
+    const disposeA = autorun(() => boxA.get())
+    onBecomeUnobserved(boxA, () => {
+        throw new Error("boom")
+    })
+
+    // the handler's exception should still surface to the caller, not be swallowed
+    expect(() => disposeA()).toThrow("boom")
+
+    // if the internal guard were left stuck true after that exception, every
+    // future endBatch() would silently stop draining pendingUnobservations,
+    // so this completely unrelated disposal would never fire its own handler
+    const boxB = observable.box(0)
+    const disposeB = autorun(() => boxB.get())
+    let unobservedB = false
+    onBecomeUnobserved(boxB, () => {
+        unobservedB = true
+    })
+
+    disposeB()
+
+    expect(unobservedB).toBe(true)
+})
+
 test("works with ObservableSet #3595", () => {
     const onSetObserved = jest.fn()
     const onSetUnobserved = jest.fn()

packages/mobx/src/core/globalstate.ts

@@ -81,6 +81,14 @@ export class MobXGlobals {
      */
     isRunningReactions = false
 
+    /**
+     * Are we currently draining pendingUnobservations in endBatch?
+     * An onBecomeUnobserved handler can dispose a Reaction, which calls
+     * startBatch/endBatch again; this guards against re-entering the same
+     * drain loop recursively (see endBatch in observable.ts).
+     */
+    isRunningUnobservations = false
+
     /**
      * Is it allowed to change observables at this point?
      * In general, MobX doesn't allow that when running computations and React.render.

packages/mobx/src/core/observable.ts

@@ -111,24 +111,39 @@ export function endBatch() {
     if (--globalState.inBatch === 0) {
         runReactions()
         // the batch is actually about to finish, all unobserving should happen here.
-        const list = globalState.pendingUnobservations
-        for (let i = 0; i < list.length; i++) {
-            const observable = list[i]
-            observable.isPendingUnobservation = false
-            if (observable.observers_.size === 0) {
-                if (observable.isBeingObserved) {
-                    // if this observable had reactive observers, trigger the hooks
-                    observable.isBeingObserved = false
-                    observable.onBUO()
-                }
-                if (observable instanceof ComputedValue) {
-                    // computed values are automatically teared down when the last observer leaves
-                    // this process happens recursively, this computed might be the last observabe of another, etc..
-                    observable.suspend_()
+        // Guard against re-entering this loop: an onBUO handler can dispose a Reaction,
+        // which calls startBatch/endBatch again while we're still iterating. Bail out of
+        // the nested call instead of recursing; the outer loop re-reads list.length on
+        // every iteration, so it picks up anything the nested dispose() pushes onto the
+        // same pendingUnobservations array.
+        if (!globalState.isRunningUnobservations) {
+            globalState.isRunningUnobservations = true
+            try {
+                const list = globalState.pendingUnobservations
+                for (let i = 0; i < list.length; i++) {
+                    const observable = list[i]
+                    observable.isPendingUnobservation = false
+                    if (observable.observers_.size === 0) {
+                        if (observable.isBeingObserved) {
+                            // if this observable had reactive observers, trigger the hooks
+                            observable.isBeingObserved = false
+                            observable.onBUO()
+                        }
+                        if (observable instanceof ComputedValue) {
+                            // computed values are automatically teared down when the last observer leaves
+                            // this process happens recursively, this computed might be the last observabe of another, etc..
+                            observable.suspend_()
+                        }
+                    }
                 }
+                globalState.pendingUnobservations = []
+            } finally {
+                // Always release the guard, even if an onBUO handler (user code) threw,
+                // otherwise every future endBatch() would see isRunningUnobservations
+                // stuck true and silently stop draining pendingUnobservations forever.
+                globalState.isRunningUnobservations = false
             }
         }
-        globalState.pendingUnobservations = []
     }
 }