Provide warning and potential workaround for iOS Safari bug described by #1670. #1854

mikelehen · 2019-06-07T15:09:16Z

On iOS 12.2 we proactively warn about the "internal error was encountered in
Indexed Database Server" Bug.
When the bug is actually encountered on iOS >= 12.2 an < 13 we log a custom
error message.

Both log message and error link to https://stackoverflow.com/q/56496296/110915 which
provides potential error handling / recovery example.

… by #1670. * On iOS 12.2 we proactively warn about the "internal error was encountered in Indexed Database Server" Bug. * When the bug is actually encountered on iOS >= 12.2 an < 13 we log a custom error message. Both log message and error link to https://stackoverflow.com/q/56496296/110915 which provides potential error handling / recovery example.

schmidt-sebastian

Some suggestions for you :)

packages/firestore/CHANGELOG.md

schmidt-sebastian · 2019-06-07T16:11:38Z

packages/firestore/src/local/simple_db.ts

@@ -15,9 +15,10 @@
 * limitations under the License.
 */

+import { getUA } from '@firebase/util';


I take it we don't have API reviews for @firebase/util? :)

The @firebase/* packages are technically considered internal and aren't meant for public consumption. 🤷‍♂

We should use Go-style variable names everywhere :)

packages/firestore/src/local/simple_db.ts

schmidt-sebastian · 2019-06-07T16:16:47Z

packages/firestore/src/local/simple_db.ts

+          .split('_')
+          .slice(0, 2)
+          .join('.')
+      : '-1';
    return Number(version);


My gut feeling is that version numbers are better represented as strings.

Yeah, potentially... though strings don't necessarily compare properly. In any case, all I did is make getIOSVersion() match the existing getAndroidVersion() function, so I'm inclined to keep it for now.

Sounds good. Will bug you about it again when we need to block 12.3.1.

schmidt-sebastian · 2019-06-07T16:18:36Z

packages/firestore/src/local/simple_db.ts

@@ -578,7 +599,9 @@ export class SimpleDbStore<
    const cursorRequest = this.cursor({});
    return new PersistencePromise((resolve, reject) => {
      cursorRequest.onerror = (event: Event) => {
-        reject((event.target as IDBRequest).error!);
+        const error = (event.target as IDBRequest).error!;


You could move this cast into checkForAndReportiOSError, which could then do an instanceof check..

we need the cast result for reject(error) too though... and IDBRequest is an interface not a class, so no instanceof checks. :-/

schmidt-sebastian · 2019-06-07T16:21:02Z

packages/firestore/src/local/simple_db.ts

+    return;
+  }
+  const iOSVersion = SimpleDb.getIOSVersion(getUA());
+  if (iOSVersion >= 12.2 && iOSVersion < 13) {


Now I understand why we use a version number and not a version number string.

Do we need to guard this check on the iOS version? We might just want to log the console message whenever we encounter the matching error string.

Given the iOS error message is so generic, I am a little worried about making this check too open-ended and possibly having it hide future bugs in iOS Safari's IndexedDB implementation (I also named this "IOS_INDEXEDDB_BUG1" to allow for future bugs in the future 😛)... So I prefer keeping the version scoped for now.

Sounds good. Glad you didn't use IOS_INDEXEDDB_BUG_001 :)

schmidt-sebastian · 2019-06-07T16:21:35Z

packages/firestore/src/local/simple_db.ts

+      reportedIOSError = true;
+      // Throw a global exception outside of this promise chain, for the user to
+      // potentially catch.
+      setTimeout(() => {


This could be setImmediate.

https://developer.mozilla.org/en-US/docs/Web/API/Window/setImmediate

This feature is non-standard and is not on a standards track. Do not use it on production sites facing the Web: it will not work for every user. There may also be large incompatibilities between implementations and the behavior may change in the future.

Hm. I learned something today.

(Never develop JS for browsers).

schmidt-sebastian · 2019-06-07T16:22:16Z

packages/firestore/src/local/simple_db.ts

+      setTimeout(() => {
+        throw new FirestoreError(
+          'internal',
+          `IOS_INDEXEDDB_BUG1: IndexedDb has thrown '${IOS_ERROR}'. This is likely ` +


Is IOS_INDEXEDDB_BUG1 intentional?

Yup. Future proofing for the next iOS bug that bites us. 😬

mikelehen

Thanks! PTAL

mikelehen · 2019-06-07T22:57:37Z

packages/firestore/src/local/simple_db.ts

@@ -15,9 +15,10 @@
 * limitations under the License.
 */

+import { getUA } from '@firebase/util';


The @firebase/* packages are technically considered internal and aren't meant for public consumption. 🤷‍♂

packages/firestore/CHANGELOG.md

mikelehen · 2019-06-07T22:59:10Z

packages/firestore/src/local/simple_db.ts

+          .split('_')
+          .slice(0, 2)
+          .join('.')
+      : '-1';
    return Number(version);


Yeah, potentially... though strings don't necessarily compare properly. In any case, all I did is make getIOSVersion() match the existing getAndroidVersion() function, so I'm inclined to keep it for now.

packages/firestore/src/local/simple_db.ts

mikelehen · 2019-06-07T23:01:35Z

packages/firestore/src/local/simple_db.ts

@@ -578,7 +599,9 @@ export class SimpleDbStore<
    const cursorRequest = this.cursor({});
    return new PersistencePromise((resolve, reject) => {
      cursorRequest.onerror = (event: Event) => {
-        reject((event.target as IDBRequest).error!);
+        const error = (event.target as IDBRequest).error!;


we need the cast result for reject(error) too though... and IDBRequest is an interface not a class, so no instanceof checks. :-/

mikelehen · 2019-06-07T23:04:31Z

packages/firestore/src/local/simple_db.ts

+    return;
+  }
+  const iOSVersion = SimpleDb.getIOSVersion(getUA());
+  if (iOSVersion >= 12.2 && iOSVersion < 13) {


Given the iOS error message is so generic, I am a little worried about making this check too open-ended and possibly having it hide future bugs in iOS Safari's IndexedDB implementation (I also named this "IOS_INDEXEDDB_BUG1" to allow for future bugs in the future 😛)... So I prefer keeping the version scoped for now.

mikelehen · 2019-06-07T23:05:15Z

packages/firestore/src/local/simple_db.ts

+      reportedIOSError = true;
+      // Throw a global exception outside of this promise chain, for the user to
+      // potentially catch.
+      setTimeout(() => {


https://developer.mozilla.org/en-US/docs/Web/API/Window/setImmediate

This feature is non-standard and is not on a standards track. Do not use it on production sites facing the Web: it will not work for every user. There may also be large incompatibilities between implementations and the behavior may change in the future.

mikelehen · 2019-06-07T23:05:44Z

packages/firestore/src/local/simple_db.ts

+      setTimeout(() => {
+        throw new FirestoreError(
+          'internal',
+          `IOS_INDEXEDDB_BUG1: IndexedDb has thrown '${IOS_ERROR}'. This is likely ` +


Yup. Future proofing for the next iOS bug that bites us. 😬

schmidt-sebastian

Ship it.

schmidt-sebastian · 2019-06-08T04:34:23Z

packages/firestore/src/local/simple_db.ts

@@ -15,9 +15,10 @@
 * limitations under the License.
 */

+import { getUA } from '@firebase/util';


We should use Go-style variable names everywhere :)

schmidt-sebastian · 2019-06-08T04:35:02Z

packages/firestore/src/local/simple_db.ts

+          .split('_')
+          .slice(0, 2)
+          .join('.')
+      : '-1';
    return Number(version);


Sounds good. Will bug you about it again when we need to block 12.3.1.

schmidt-sebastian · 2019-06-08T04:36:47Z

packages/firestore/src/local/simple_db.ts

+    return;
+  }
+  const iOSVersion = SimpleDb.getIOSVersion(getUA());
+  if (iOSVersion >= 12.2 && iOSVersion < 13) {


Sounds good. Glad you didn't use IOS_INDEXEDDB_BUG_001 :)

schmidt-sebastian · 2019-06-08T04:37:30Z

packages/firestore/src/local/simple_db.ts

+      reportedIOSError = true;
+      // Throw a global exception outside of this promise chain, for the user to
+      // potentially catch.
+      setTimeout(() => {


Hm. I learned something today.

(Never develop JS for browsers).

mikelehen added the api: firestore label Jun 7, 2019

mikelehen requested a review from schmidt-sebastian June 7, 2019 15:09

mikelehen assigned schmidt-sebastian Jun 7, 2019

Fix lint.

be524ff

schmidt-sebastian reviewed Jun 7, 2019

View reviewed changes

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Jun 7, 2019

CR tweaks.

bc2b78e

mikelehen commented Jun 7, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Jun 7, 2019

schmidt-sebastian approved these changes Jun 8, 2019

View reviewed changes

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Jun 8, 2019

mikelehen merged commit 6673a92 into master Jun 10, 2019

mikelehen deleted the mikelehen/indexeddb-error branch June 10, 2019 14:05

firebase locked and limited conversation to collaborators Oct 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide warning and potential workaround for iOS Safari bug described by #1670. #1854

Provide warning and potential workaround for iOS Safari bug described by #1670. #1854

mikelehen commented Jun 7, 2019

schmidt-sebastian left a comment

schmidt-sebastian Jun 7, 2019

mikelehen Jun 7, 2019

schmidt-sebastian Jun 8, 2019

schmidt-sebastian Jun 7, 2019

mikelehen Jun 7, 2019

schmidt-sebastian Jun 8, 2019

schmidt-sebastian Jun 7, 2019

mikelehen Jun 7, 2019

schmidt-sebastian Jun 7, 2019

mikelehen Jun 7, 2019

schmidt-sebastian Jun 8, 2019

schmidt-sebastian Jun 7, 2019

mikelehen Jun 7, 2019

schmidt-sebastian Jun 8, 2019

schmidt-sebastian Jun 7, 2019

mikelehen Jun 7, 2019

mikelehen left a comment

mikelehen Jun 7, 2019

mikelehen Jun 7, 2019

mikelehen Jun 7, 2019

mikelehen Jun 7, 2019

mikelehen Jun 7, 2019

mikelehen Jun 7, 2019

schmidt-sebastian left a comment

schmidt-sebastian Jun 8, 2019

schmidt-sebastian Jun 8, 2019

schmidt-sebastian Jun 8, 2019

schmidt-sebastian Jun 8, 2019

Provide warning and potential workaround for iOS Safari bug described by #1670. #1854

Provide warning and potential workaround for iOS Safari bug described by #1670. #1854

Conversation

mikelehen commented Jun 7, 2019

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment