From 4febd907583390a214d981c9d104aca2b01d8071 Mon Sep 17 00:00:00 2001
From: Dmitry S <dsagal+git@gmail.com>
Date: Fri, 6 Nov 2020 01:58:24 -0500
Subject: [PATCH] (core) Fix an insidious bug in RefCountMap, manifesting as JS
 errors some time after import.

Summary:
After an import from inside a document, one minute later, an important
QuerySet would get disposed, leaving the view section in a bad state,
and manifesting as JS errors on subsequent operations. (Might not
*always* happen because switching pages would prevent it from
manifesting, I think.)

Bad state that I've seen after transforms is probably explainable as
this bug, which is unrelated. Reproduction was hard because who knew one
had to wait a minute?!

Test Plan:
Added a unittest for the fix in QuerySet, and a browser test that
fails without the fix (JS errors, bad state), and passes with.

Reviewers: paulfitz

Reviewed By: paulfitz

Differential Revision: https://phab.getgrist.com/D2653
---
 app/client/models/QuerySet.ts |  6 ++++++
 app/common/RefCountMap.ts     | 20 ++++++++++++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/app/client/models/QuerySet.ts b/app/client/models/QuerySet.ts
index b64e9ef5..6e4e90d2 100644
--- a/app/client/models/QuerySet.ts
+++ b/app/client/models/QuerySet.ts
@@ -98,6 +98,12 @@ export class QuerySetManager extends Disposable {
   public purgeKey(queryKey: string) {
     this._queryMap.purgeKey(queryKey);
   }
+
+  // For testing: set gracePeriodMs, returning the previous value.
+  public testSetGracePeriodMs(ms: number): number {
+    return this._queryMap.testSetGracePeriodMs(ms);
+  }
+
 }
 
 /**
diff --git a/app/common/RefCountMap.ts b/app/common/RefCountMap.ts
index 4526076e..277ea47f 100644
--- a/app/common/RefCountMap.ts
+++ b/app/common/RefCountMap.ts
@@ -80,14 +80,18 @@ export class RefCountMap<Key, Value> implements IDisposable {
     this._map.clear();
   }
 
+  // For testing: set gracePeriodMs, returning the previous value.
+  public testSetGracePeriodMs(ms: number): number {
+    const prev = this._gracePeriodMs;
+    this._gracePeriodMs = ms;
+    return prev;
+  }
+
   private _useKey(key: Key): RefCountValue<Value> {
     const r = this._map.get(key);
     if (r) {
       r.count += 1;
-      if (r.disposeTimeout) {
-        clearTimeout(r.disposeTimeout);
-        r.disposeTimeout = undefined;
-      }
+      r.unsetTimeout();
       return r;
     }
     const value = this._createKey.call(null, key);
@@ -116,6 +120,7 @@ export class RefCountMap<Key, Value> implements IDisposable {
     if (r) {
       this._map.delete(key);
       r.count = 0;
+      r.unsetTimeout();   // Important, to avoid timeout triggering after the same key is re-added.
       this._disposeKey.call(null, key, r.value);
     }
   }
@@ -128,4 +133,11 @@ class RefCountValue<Value> {
   public count: number = 1;
   public disposeTimeout?: ReturnType<typeof setTimeout> = undefined;
   constructor(public value: Value) {}
+
+  public unsetTimeout() {
+    if (this.disposeTimeout) {
+      clearTimeout(this.disposeTimeout);
+      this.disposeTimeout = undefined;
+    }
+  }
 }