From 867417cd7256bb751e565e341afc3d29efbcacc6 Mon Sep 17 00:00:00 2001
From: Wu-Hui <wu.hui.github@gmail.com>
Date: Mon, 4 May 2020 11:29:49 -0400
Subject: [PATCH 1/6] Reland "Use crypo RNG for auto ID generation to reduce
 conflicts (#2872)"

---
 packages/firestore/src/platform/platform.ts   |  6 +++++
 .../src/platform_browser/browser_platform.ts  | 23 +++++++++++++++++++
 .../src/platform_node/node_platform.ts        |  9 ++++++++
 packages/firestore/src/util/misc.ts           | 14 +++++++++--
 packages/firestore/test/util/test_platform.ts |  4 ++++
 5 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/packages/firestore/src/platform/platform.ts b/packages/firestore/src/platform/platform.ts
index f206ab1a77e..22349732aba 100644
--- a/packages/firestore/src/platform/platform.ts
+++ b/packages/firestore/src/platform/platform.ts
@@ -43,6 +43,12 @@ export interface Platform {
   /** Converts a binary string to a Base64 encoded string. */
   btoa(raw: string): string;
 
+  /**
+   * Generates `nBytes` of random bytes. If `nBytes` is negative, an empty array
+   * will be returned.
+   */
+  randomBytes(nBytes: number): Uint8Array;
+
   /** The Platform's 'window' implementation or null if not available. */
   readonly window: Window | null;
 
diff --git a/packages/firestore/src/platform_browser/browser_platform.ts b/packages/firestore/src/platform_browser/browser_platform.ts
index 48b19339e29..8a5669ab211 100644
--- a/packages/firestore/src/platform_browser/browser_platform.ts
+++ b/packages/firestore/src/platform_browser/browser_platform.ts
@@ -20,13 +20,19 @@ import { Platform } from '../platform/platform';
 import { Connection } from '../remote/connection';
 import { JsonProtoSerializer } from '../remote/serializer';
 import { ConnectivityMonitor } from './../remote/connectivity_monitor';
+
 import { NoopConnectivityMonitor } from '../remote/connectivity_monitor_noop';
 import { BrowserConnectivityMonitor } from './browser_connectivity_monitor';
 import { WebChannelConnection } from './webchannel_connection';
 
+// Polyfill for IE and WebWorker
+// eslint-disable-next-line no-restricted-globals
+const crypto = window['crypto'] || window['msCrypto'] || self['crypto'];
+
 // Implements the Platform API for browsers and some browser-like environments
 // (including ReactNative).
 export class BrowserPlatform implements Platform {
+  readonly useProto3Json = true;
   readonly base64Available: boolean;
 
   constructor() {
@@ -72,4 +78,21 @@ export class BrowserPlatform implements Platform {
   btoa(raw: string): string {
     return btoa(raw);
   }
+
+  randomBytes(nBytes: number): Uint8Array {
+    if (nBytes <= 0) {
+      return new Uint8Array();
+    }
+
+    const v = new Uint8Array(nBytes);
+    if(!!crypto) {
+      crypto.getRandomValues(v);
+    } else {
+      // Falls back to Math.random
+      for(let i = 0; i < nBytes; i++) {
+        v[i] = Math.floor(Math.random() * 256);
+      }
+    }
+    return v;
+  }
 }
diff --git a/packages/firestore/src/platform_node/node_platform.ts b/packages/firestore/src/platform_node/node_platform.ts
index 927209acd5a..8bb675fd3cf 100644
--- a/packages/firestore/src/platform_node/node_platform.ts
+++ b/packages/firestore/src/platform_node/node_platform.ts
@@ -15,6 +15,7 @@
  * limitations under the License.
  */
 
+import { randomBytes } from 'crypto';
 import { inspect } from 'util';
 
 import { DatabaseId, DatabaseInfo } from '../core/database_info';
@@ -75,4 +76,12 @@ export class NodePlatform implements Platform {
   btoa(raw: string): string {
     return new Buffer(raw, 'binary').toString('base64');
   }
+
+  randomBytes(nBytes: number): Uint8Array {
+    if (nBytes <= 0) {
+      return new Uint8Array();
+    }
+
+    return randomBytes(nBytes);
+  }
 }
diff --git a/packages/firestore/src/util/misc.ts b/packages/firestore/src/util/misc.ts
index 8a46a11292c..901f09a7394 100644
--- a/packages/firestore/src/util/misc.ts
+++ b/packages/firestore/src/util/misc.ts
@@ -16,6 +16,7 @@
  */
 
 import { debugAssert } from './assert';
+import { PlatformSupport } from '../platform/platform';
 
 export type EventHandler<E> = (value: E) => void;
 export interface Indexable {
@@ -28,8 +29,17 @@ export class AutoId {
     const chars =
       'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
     let autoId = '';
-    for (let i = 0; i < 20; i++) {
-      autoId += chars.charAt(Math.floor(Math.random() * chars.length));
+    while (autoId.length < 20) {
+      const bytes = PlatformSupport.getPlatform().randomBytes(40);
+      for (const b of Array.from(bytes)) {
+        // Length of `chars` is 62. We only take bytes between 0 and 62*4-1
+        // (both inclusive). The value is then evenly mapped to indices of `char`
+        // via a modulo operation.
+        const maxValue = 62 * 4 - 1;
+        if (autoId.length < 20 && b <= maxValue) {
+          autoId += chars.charAt(b % 62);
+        }
+      }
     }
     debugAssert(autoId.length === 20, 'Invalid auto ID: ' + autoId);
     return autoId;
diff --git a/packages/firestore/test/util/test_platform.ts b/packages/firestore/test/util/test_platform.ts
index 1d1f8346d77..cd9d458f63d 100644
--- a/packages/firestore/test/util/test_platform.ts
+++ b/packages/firestore/test/util/test_platform.ts
@@ -266,6 +266,10 @@ export class TestPlatform implements Platform {
   btoa(raw: string): string {
     return this.basePlatform.btoa(raw);
   }
+
+  randomBytes(nBytes: number): Uint8Array {
+    return this.basePlatform.randomBytes(nBytes);
+  }
 }
 
 /** Returns true if we are running under Node. */

From a2b89c2730e66adb8dbab1ed5cd0aa7e7051ea05 Mon Sep 17 00:00:00 2001
From: Wu-Hui <wu.hui.github@gmail.com>
Date: Mon, 4 May 2020 17:09:37 -0400
Subject: [PATCH 2/6] Reland "Use crypo RNG for auto ID generation to reduce
 conflicts (#2872)"

---
 .../src/platform_browser/browser_platform.ts       | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/packages/firestore/src/platform_browser/browser_platform.ts b/packages/firestore/src/platform_browser/browser_platform.ts
index 8a5669ab211..c5f289f32f7 100644
--- a/packages/firestore/src/platform_browser/browser_platform.ts
+++ b/packages/firestore/src/platform_browser/browser_platform.ts
@@ -25,10 +25,6 @@ import { NoopConnectivityMonitor } from '../remote/connectivity_monitor_noop';
 import { BrowserConnectivityMonitor } from './browser_connectivity_monitor';
 import { WebChannelConnection } from './webchannel_connection';
 
-// Polyfill for IE and WebWorker
-// eslint-disable-next-line no-restricted-globals
-const crypto = window['crypto'] || window['msCrypto'] || self['crypto'];
-
 // Implements the Platform API for browsers and some browser-like environments
 // (including ReactNative).
 export class BrowserPlatform implements Platform {
@@ -84,12 +80,18 @@ export class BrowserPlatform implements Platform {
       return new Uint8Array();
     }
 
+    // Polyfill for IE and WebWorker
+    // eslint-disable-next-line no-restricted-globals
+    const crypto =
+      (this.window &&
+        (this.window.crypto || (this.window as any)['msCrypto'])) ||
+      (self && self.crypto);
     const v = new Uint8Array(nBytes);
-    if(!!crypto) {
+    if (!!crypto) {
       crypto.getRandomValues(v);
     } else {
       // Falls back to Math.random
-      for(let i = 0; i < nBytes; i++) {
+      for (let i = 0; i < nBytes; i++) {
         v[i] = Math.floor(Math.random() * 256);
       }
     }

From 02592ec3fd93141789003d63c650030e3cb4ec48 Mon Sep 17 00:00:00 2001
From: Wu-Hui <wu.hui.github@gmail.com>
Date: Mon, 4 May 2020 17:11:22 -0400
Subject: [PATCH 3/6] Fix merge error

---
 packages/firestore/src/platform_browser/browser_platform.ts | 1 -
 1 file changed, 1 deletion(-)

diff --git a/packages/firestore/src/platform_browser/browser_platform.ts b/packages/firestore/src/platform_browser/browser_platform.ts
index c5f289f32f7..b75a3928107 100644
--- a/packages/firestore/src/platform_browser/browser_platform.ts
+++ b/packages/firestore/src/platform_browser/browser_platform.ts
@@ -28,7 +28,6 @@ import { WebChannelConnection } from './webchannel_connection';
 // Implements the Platform API for browsers and some browser-like environments
 // (including ReactNative).
 export class BrowserPlatform implements Platform {
-  readonly useProto3Json = true;
   readonly base64Available: boolean;
 
   constructor() {

From 17b6bc535d6a0a18fdb45c1b9c5ffa158b6353bf Mon Sep 17 00:00:00 2001
From: Wu-Hui <wu.hui.github@gmail.com>
Date: Tue, 5 May 2020 14:48:51 -0400
Subject: [PATCH 4/6] Address comments

---
 packages/firestore/src/platform/platform.ts          |  5 +++--
 .../src/platform_browser/browser_platform.ts         | 12 ++++--------
 .../firestore/src/platform_node/node_platform.ts     |  5 ++---
 packages/firestore/src/util/misc.ts                  |  6 +++---
 4 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/packages/firestore/src/platform/platform.ts b/packages/firestore/src/platform/platform.ts
index 22349732aba..262a0261af2 100644
--- a/packages/firestore/src/platform/platform.ts
+++ b/packages/firestore/src/platform/platform.ts
@@ -44,8 +44,9 @@ export interface Platform {
   btoa(raw: string): string;
 
   /**
-   * Generates `nBytes` of random bytes. If `nBytes` is negative, an empty array
-   * will be returned.
+   * Generates `nBytes` of random bytes.
+   *
+   * If `nBytes <= 0` , an error will be thrown.
    */
   randomBytes(nBytes: number): Uint8Array;
 
diff --git a/packages/firestore/src/platform_browser/browser_platform.ts b/packages/firestore/src/platform_browser/browser_platform.ts
index b75a3928107..686c6ce7cb8 100644
--- a/packages/firestore/src/platform_browser/browser_platform.ts
+++ b/packages/firestore/src/platform_browser/browser_platform.ts
@@ -24,6 +24,7 @@ import { ConnectivityMonitor } from './../remote/connectivity_monitor';
 import { NoopConnectivityMonitor } from '../remote/connectivity_monitor_noop';
 import { BrowserConnectivityMonitor } from './browser_connectivity_monitor';
 import { WebChannelConnection } from './webchannel_connection';
+import { validatePositiveNumber } from '../util/input_validation';
 
 // Implements the Platform API for browsers and some browser-like environments
 // (including ReactNative).
@@ -75,18 +76,13 @@ export class BrowserPlatform implements Platform {
   }
 
   randomBytes(nBytes: number): Uint8Array {
-    if (nBytes <= 0) {
-      return new Uint8Array();
-    }
+    validatePositiveNumber('randomBytes', 1, nBytes);
 
     // Polyfill for IE and WebWorker
-    // eslint-disable-next-line no-restricted-globals
     const crypto =
-      (this.window &&
-        (this.window.crypto || (this.window as any)['msCrypto'])) ||
-      (self && self.crypto);
+      typeof self !== 'undefined' && (self.crypto || (self as any)['msCrypto']);
     const v = new Uint8Array(nBytes);
-    if (!!crypto) {
+    if (crypto) {
       crypto.getRandomValues(v);
     } else {
       // Falls back to Math.random
diff --git a/packages/firestore/src/platform_node/node_platform.ts b/packages/firestore/src/platform_node/node_platform.ts
index 8bb675fd3cf..34401e2e45c 100644
--- a/packages/firestore/src/platform_node/node_platform.ts
+++ b/packages/firestore/src/platform_node/node_platform.ts
@@ -28,6 +28,7 @@ import { NoopConnectivityMonitor } from './../remote/connectivity_monitor_noop';
 
 import { GrpcConnection } from './grpc_connection';
 import { loadProtos } from './load_protos';
+import { validatePositiveNumber } from '../util/input_validation';
 
 export class NodePlatform implements Platform {
   readonly base64Available = true;
@@ -78,9 +79,7 @@ export class NodePlatform implements Platform {
   }
 
   randomBytes(nBytes: number): Uint8Array {
-    if (nBytes <= 0) {
-      return new Uint8Array();
-    }
+    validatePositiveNumber('randomBytes', 1, nBytes);
 
     return randomBytes(nBytes);
   }
diff --git a/packages/firestore/src/util/misc.ts b/packages/firestore/src/util/misc.ts
index 901f09a7394..91dc87d641e 100644
--- a/packages/firestore/src/util/misc.ts
+++ b/packages/firestore/src/util/misc.ts
@@ -31,13 +31,13 @@ export class AutoId {
     let autoId = '';
     while (autoId.length < 20) {
       const bytes = PlatformSupport.getPlatform().randomBytes(40);
-      for (const b of Array.from(bytes)) {
+      for (let i = 0; i < bytes.length; ++i) {
         // Length of `chars` is 62. We only take bytes between 0 and 62*4-1
         // (both inclusive). The value is then evenly mapped to indices of `char`
         // via a modulo operation.
         const maxValue = 62 * 4 - 1;
-        if (autoId.length < 20 && b <= maxValue) {
-          autoId += chars.charAt(b % 62);
+        if (autoId.length < 20 && bytes[i] <= maxValue) {
+          autoId += chars.charAt(bytes[i] % 62);
         }
       }
     }

From ea4967a16e2be239bc8a00c78a1c43c82481bf8c Mon Sep 17 00:00:00 2001
From: Wu-Hui <wu.hui.github@gmail.com>
Date: Tue, 5 May 2020 15:55:29 -0400
Subject: [PATCH 5/6] Suppress lint

---
 packages/firestore/src/platform_browser/browser_platform.ts | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/packages/firestore/src/platform_browser/browser_platform.ts b/packages/firestore/src/platform_browser/browser_platform.ts
index 686c6ce7cb8..8634b46dbe7 100644
--- a/packages/firestore/src/platform_browser/browser_platform.ts
+++ b/packages/firestore/src/platform_browser/browser_platform.ts
@@ -24,7 +24,7 @@ import { ConnectivityMonitor } from './../remote/connectivity_monitor';
 import { NoopConnectivityMonitor } from '../remote/connectivity_monitor_noop';
 import { BrowserConnectivityMonitor } from './browser_connectivity_monitor';
 import { WebChannelConnection } from './webchannel_connection';
-import { validatePositiveNumber } from '../util/input_validation';
+import { debugAssert } from '../util/assert';
 
 // Implements the Platform API for browsers and some browser-like environments
 // (including ReactNative).
@@ -76,10 +76,11 @@ export class BrowserPlatform implements Platform {
   }
 
   randomBytes(nBytes: number): Uint8Array {
-    validatePositiveNumber('randomBytes', 1, nBytes);
+    debugAssert(nBytes >= 0, 'Number of random bytes must not be negative');
 
     // Polyfill for IE and WebWorker
     const crypto =
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
       typeof self !== 'undefined' && (self.crypto || (self as any)['msCrypto']);
     const v = new Uint8Array(nBytes);
     if (crypto) {

From 434e6504e3643b0673e2673e8d13f61146dc83fa Mon Sep 17 00:00:00 2001
From: Wu-Hui <wu.hui.github@gmail.com>
Date: Thu, 7 May 2020 12:29:01 -0400
Subject: [PATCH 6/6] Address comments

---
 packages/firestore/src/platform/platform.ts   |  2 +-
 .../src/platform_browser/browser_platform.ts  | 12 +++++-----
 .../src/platform_node/node_platform.ts        |  4 ++--
 packages/firestore/src/util/misc.ts           | 23 ++++++++++++-------
 4 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/packages/firestore/src/platform/platform.ts b/packages/firestore/src/platform/platform.ts
index 262a0261af2..1ab582ea05f 100644
--- a/packages/firestore/src/platform/platform.ts
+++ b/packages/firestore/src/platform/platform.ts
@@ -46,7 +46,7 @@ export interface Platform {
   /**
    * Generates `nBytes` of random bytes.
    *
-   * If `nBytes <= 0` , an error will be thrown.
+   * If `nBytes < 0` , an error will be thrown.
    */
   randomBytes(nBytes: number): Uint8Array;
 
diff --git a/packages/firestore/src/platform_browser/browser_platform.ts b/packages/firestore/src/platform_browser/browser_platform.ts
index 8634b46dbe7..38133db4df5 100644
--- a/packages/firestore/src/platform_browser/browser_platform.ts
+++ b/packages/firestore/src/platform_browser/browser_platform.ts
@@ -76,21 +76,21 @@ export class BrowserPlatform implements Platform {
   }
 
   randomBytes(nBytes: number): Uint8Array {
-    debugAssert(nBytes >= 0, 'Number of random bytes must not be negative');
+    debugAssert(nBytes >= 0, `Expecting non-negative nBytes, got: ${nBytes}`);
 
-    // Polyfill for IE and WebWorker
+    // Polyfills for IE and WebWorker by using `self` and `msCrypto` when `crypto` is not available.
     const crypto =
       // eslint-disable-next-line @typescript-eslint/no-explicit-any
       typeof self !== 'undefined' && (self.crypto || (self as any)['msCrypto']);
-    const v = new Uint8Array(nBytes);
+    const bytes = new Uint8Array(nBytes);
     if (crypto) {
-      crypto.getRandomValues(v);
+      crypto.getRandomValues(bytes);
     } else {
       // Falls back to Math.random
       for (let i = 0; i < nBytes; i++) {
-        v[i] = Math.floor(Math.random() * 256);
+        bytes[i] = Math.floor(Math.random() * 256);
       }
     }
-    return v;
+    return bytes;
   }
 }
diff --git a/packages/firestore/src/platform_node/node_platform.ts b/packages/firestore/src/platform_node/node_platform.ts
index 34401e2e45c..2fe74f7d68b 100644
--- a/packages/firestore/src/platform_node/node_platform.ts
+++ b/packages/firestore/src/platform_node/node_platform.ts
@@ -28,7 +28,7 @@ import { NoopConnectivityMonitor } from './../remote/connectivity_monitor_noop';
 
 import { GrpcConnection } from './grpc_connection';
 import { loadProtos } from './load_protos';
-import { validatePositiveNumber } from '../util/input_validation';
+import { debugAssert } from '../util/assert';
 
 export class NodePlatform implements Platform {
   readonly base64Available = true;
@@ -79,7 +79,7 @@ export class NodePlatform implements Platform {
   }
 
   randomBytes(nBytes: number): Uint8Array {
-    validatePositiveNumber('randomBytes', 1, nBytes);
+    debugAssert(nBytes >= 0, `Expecting non-negative nBytes, got: ${nBytes}`);
 
     return randomBytes(nBytes);
   }
diff --git a/packages/firestore/src/util/misc.ts b/packages/firestore/src/util/misc.ts
index 91dc87d641e..35a7d40baf4 100644
--- a/packages/firestore/src/util/misc.ts
+++ b/packages/firestore/src/util/misc.ts
@@ -28,20 +28,27 @@ export class AutoId {
     // Alphanumeric characters
     const chars =
       'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
+    // The largest byte value that is a multiple of `char.length`.
+    const maxMultiple = Math.floor(256 / chars.length) * chars.length;
+    debugAssert(
+      0 < maxMultiple && maxMultiple < 256,
+      `Expect maxMultiple to be (0, 256), but got ${maxMultiple}`
+    );
+
     let autoId = '';
-    while (autoId.length < 20) {
+    const targetLength = 20;
+    while (autoId.length < targetLength) {
       const bytes = PlatformSupport.getPlatform().randomBytes(40);
       for (let i = 0; i < bytes.length; ++i) {
-        // Length of `chars` is 62. We only take bytes between 0 and 62*4-1
-        // (both inclusive). The value is then evenly mapped to indices of `char`
-        // via a modulo operation.
-        const maxValue = 62 * 4 - 1;
-        if (autoId.length < 20 && bytes[i] <= maxValue) {
-          autoId += chars.charAt(bytes[i] % 62);
+        // Only accept values that are [0, maxMultiple), this ensures they can
+        // be evenly mapped to indices of `chars` via a modulo operation.
+        if (autoId.length < targetLength && bytes[i] < maxMultiple) {
+          autoId += chars.charAt(bytes[i] % chars.length);
         }
       }
     }
-    debugAssert(autoId.length === 20, 'Invalid auto ID: ' + autoId);
+    debugAssert(autoId.length === targetLength, 'Invalid auto ID: ' + autoId);
+
     return autoId;
   }
 }