Merge branch 'master' into mrschmidt-commitversion

Author: schmidt-sebastian

.github/CODEOWNERS

@@ -9,10 +9,10 @@ packages/database  @mikelehen @schmidt-sebastian
 packages/database-types  @mikelehen @schmidt-sebastian
 
 # Firestore Code
-packages/firestore  @mikelehen @schmidt-sebastian @wilhuff
-packages/webchannel-wrapper  @mikelehen @schmidt-sebastian @wilhuff
-packages/firestore-types  @mikelehen @schmidt-sebastian @wilhuff
-integration/firestore  @mikelehen @schmidt-sebastian @wilhuff
+packages/firestore  @mikelehen @schmidt-sebastian @wilhuff @gsoltis @var-const @rsgowman @zxu123
+packages/webchannel-wrapper  @mikelehen @schmidt-sebastian @wilhuff @gsoltis @var-const @rsgowman @zxu123
+packages/firestore-types  @mikelehen @schmidt-sebastian @wilhuff @gsoltis @var-const @rsgowman @zxu123
+integration/firestore  @mikelehen @schmidt-sebastian @wilhuff @gsoltis @var-const @rsgowman @zxu123
 
 # Storage Code
 packages/storage  @sphippen

packages/auth/src/error_withcredential.js

@@ -130,8 +130,9 @@ fireauth.AuthErrorWithCredential.fromPlainObject = function(response) {
       credentialInfo.email = response['email'];
     } else if (response['phoneNumber']) {
       credentialInfo.phoneNumber = response['phoneNumber'];
-    } else {
-      // Neither email nor phone number are set; return a generic error.
+    } else if (!credentialInfo.credential) {
+      // Neither email, phone number or credentials are set; return a generic
+      // error.
       return new fireauth.AuthError(code, response['message'] || undefined);
     }
 

packages/auth/src/utils.js

@@ -1459,6 +1459,7 @@ fireauth.util.setNoReferrer = function() {
 
 /** @return {?ServiceWorker} The servicerWorker controller if available. */
 fireauth.util.getServiceWorkerController = function() {
+  var navigator = goog.global['navigator'];
   return (navigator &&
           navigator.serviceWorker &&
           navigator.serviceWorker.controller) || null;
@@ -1477,13 +1478,17 @@ fireauth.util.getWorkerGlobalScope = function() {
  *     available. If no service worker is supported, it will resolve with null.
  */
 fireauth.util.getActiveServiceWorker = function() {
+  var navigator = goog.global['navigator'];
   if (navigator && navigator.serviceWorker) {
     return goog.Promise.resolve()
         .then(function() {
           return navigator.serviceWorker.ready;
         })
         .then(function(registration) {
           return /** @type {?ServiceWorker} */ (registration.active || null);
+        })
+        .thenCatch(function(error) {
+          return null;
         });
   }
   return goog.Promise.resolve(/** @type {?ServiceWorker} */ (null));

packages/auth/test/error_test.js

@@ -250,6 +250,26 @@ function testAuthErrorWithCredential_toPlainObject() {
   assertObjectEquals(
       errorObject3,
       error3.toPlainObject());
+
+  // AuthErrorWithCredential with just a credential and no email or phoneNumber.
+  var credential4 = fireauth.FacebookAuthProvider.credential('ACCESS_TOKEN');
+  var error4 = new fireauth.AuthErrorWithCredential(
+      fireauth.authenum.Error.CREDENTIAL_ALREADY_IN_USE,
+      {
+        credential: credential4
+      },
+      'This credential is already associated with a different user account.');
+  var errorObject4 = {
+    'code': 'auth/credential-already-in-use',
+    'message': 'This credential is already associated with a different user ' +
+        'account.',
+    'providerId': 'facebook.com',
+    'oauthAccessToken': 'ACCESS_TOKEN',
+    'signInMethod': fireauth.FacebookAuthProvider['FACEBOOK_SIGN_IN_METHOD']
+  };
+  assertObjectEquals(
+      errorObject4,
+      error4.toPlainObject());
 }
 
 
@@ -329,6 +349,36 @@ function testAuthErrorWithCredential_fromPlainObject() {
   assertObjectEquals(
       error3,
       fireauth.AuthErrorWithCredential.fromPlainObject(errorObject3NoPrefix));
+
+  // AuthErrorWithCredential with just a credential
+  var credential4 = fireauth.FacebookAuthProvider.credential('ACCESS_TOKEN');
+  var error4 = new fireauth.AuthErrorWithCredential(
+      fireauth.authenum.Error.CREDENTIAL_ALREADY_IN_USE,
+      {
+        credential: credential4
+      },
+      'This credential is already associated with a different user account.');
+  var errorObject4 = {
+    'code': 'auth/credential-already-in-use',
+    'message': 'This credential is already associated with a different user ' +
+        'account.',
+    'providerId': 'facebook.com',
+    'oauthAccessToken': 'ACCESS_TOKEN'
+  };
+  var errorObject4NoPrefix = {
+    'code': 'credential-already-in-use',
+    'message': 'This credential is already associated with a different user ' +
+        'account.',
+    'providerId': 'facebook.com',
+    'oauthAccessToken': 'ACCESS_TOKEN'
+  };
+  assertObjectEquals(
+      error4,
+      fireauth.AuthErrorWithCredential.fromPlainObject(errorObject4));
+  // If the error code prefix is missing.
+  assertObjectEquals(
+      error4,
+      fireauth.AuthErrorWithCredential.fromPlainObject(errorObject4NoPrefix));
 }
 
 

packages/firestore/src/core/sync_engine.ts

@@ -315,11 +315,13 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
       );
 
       if (!targetRemainsActive) {
-        this.sharedClientState.clearQueryState(queryView.targetId);
-        this.remoteStore.unlisten(queryView.targetId);
         await this.localStore
           .releaseQuery(query, /*keepPersistedQueryData=*/ false)
-          .then(() => this.removeAndCleanupQuery(queryView))
+          .then(() => {
+            this.sharedClientState.clearQueryState(queryView.targetId);
+            this.remoteStore.unlisten(queryView.targetId);
+            return this.removeAndCleanupQuery(queryView);
+          })
           .then(() => this.localStore.collectGarbage())
           .catch(err => this.ignoreIfPrimaryLeaseLoss(err));
       }
@@ -811,7 +813,6 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
     await Promise.all(queriesProcessed);
     this.syncEngineListener!.onWatchChange(newSnaps);
     this.localStore.notifyLocalViewChanges(docChangesInAllViews);
-    // TODO(multitab): Multitab garbage collection
     if (this.isPrimary) {
       await this.localStore
         .collectGarbage()
@@ -883,14 +884,22 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
       }
     } else if (isPrimary === false && this.isPrimary !== false) {
       this.isPrimary = false;
-      const activeQueries = await this.synchronizeQueryViewsAndRaiseSnapshots(
-        objUtils.indices(this.queryViewsByTarget)
-      );
+
+      const activeTargets: TargetId[] = [];
+
+      let p = Promise.resolve();
+      objUtils.forEachNumber(this.queryViewsByTarget, (targetId, queryView) => {
+        if (this.sharedClientState.isLocalQueryTarget(targetId)) {
+          activeTargets.push(targetId);
+        } else {
+          p = p.then(() => this.unlisten(queryView.query));
+        }
+        this.remoteStore.unlisten(queryView.targetId);
+      });
+      await p;
+
+      await this.synchronizeQueryViewsAndRaiseSnapshots(activeTargets);
       this.resetLimboDocuments();
-      for (const queryData of activeQueries) {
-        // TODO(multitab): Remove query views for non-local queries.
-        this.remoteStore.unlisten(queryData.targetId);
-      }
       await this.remoteStore.applyPrimaryState(false);
     }
   }
@@ -1039,10 +1048,12 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
       // Check that the query is still active since the query might have been
       // removed if it has been rejected by the backend.
       if (queryView) {
-        this.remoteStore.unlisten(targetId);
         await this.localStore
           .releaseQuery(queryView.query, /*keepPersistedQueryData=*/ false)
-          .then(() => this.removeAndCleanupQuery(queryView))
+          .then(() => {
+            this.remoteStore.unlisten(targetId);
+            return this.removeAndCleanupQuery(queryView);
+          })
           .catch(err => this.ignoreIfPrimaryLeaseLoss(err));
       }
     }

packages/firestore/src/core/view.ts

@@ -207,6 +207,7 @@ export class View {
       while (newDocumentSet.size > this.query.limit!) {
         const oldDoc = newDocumentSet.last();
         newDocumentSet = newDocumentSet.delete(oldDoc!.key);
+        newMutatedKeys = newMutatedKeys.delete(oldDoc!.key);
         changeSet.track({ type: ChangeType.Removed, doc: oldDoc! });
       }
     }

packages/firestore/src/local/indexeddb_persistence.ts

@@ -907,7 +907,6 @@ export class IndexedDbPersistence implements Persistence {
    */
   private markClientZombied(): void {
     try {
-      // TODO(multitab): Garbage Collect Local Storage
       this.window.localStorage.setItem(
         this.zombiedClientLocalStorageKey(this.clientId),
         String(Date.now())

packages/firestore/src/local/indexeddb_remote_document_cache.ts

@@ -106,7 +106,6 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
       }
 
       if (this.keepDocumentChangeLog) {
-        // TODO(multitab): GC the documentChanges store.
         promises.push(
           documentChangesStore(transaction).put({
             changes: this.serializer.toDbResourcePaths(changedKeys)

packages/firestore/src/local/persistence.ts

@@ -79,9 +79,6 @@ export type PrimaryStateListener = (isPrimary: boolean) => Promise<void>;
  * writes in order to avoid relying on being able to read back uncommitted
  * writes.
  */
-// TODO(multitab): Instead of marking methods as multi-tab safe, we should
-// point out (and maybe enforce) when methods cannot safely be used from
-// secondary tabs.
 export interface Persistence {
   /**
    * Whether or not this persistence instance has been started.

packages/firestore/src/local/shared_client_state.ts

@@ -105,6 +105,9 @@ export interface SharedClientState {
   /** Removes the Query Target ID association from the local client. */
   removeLocalQueryTarget(targetId: TargetId): void;
 
+  /** Checks whether the target is associated with the local client. */
+  isLocalQueryTarget(targetId: TargetId): boolean;
+
   /**
    * Processes an update to a query target.
    *
@@ -571,8 +574,6 @@ export class WebStorageSharedClientState implements SharedClientState {
     return !!(platform.window && platform.window.localStorage != null);
   }
 
-  // TOOD(multitab): Register the mutations that are already pending at client
-  // startup.
   async start(): Promise<void> {
     assert(!this.started, 'WebStorageSharedClientState already started');
     assert(
@@ -702,6 +703,10 @@ export class WebStorageSharedClientState implements SharedClientState {
     this.persistClientState();
   }
 
+  isLocalQueryTarget(targetId: TargetId): boolean {
+    return this.localClientState.activeTargetIds.has(targetId);
+  }
+
   clearQueryState(targetId: TargetId): void {
     this.removeItem(this.toLocalStorageQueryTargetMetadataKey(targetId));
   }
@@ -761,15 +766,15 @@ export class WebStorageSharedClientState implements SharedClientState {
 
   private handleLocalStorageEvent(event: StorageEvent): void {
     if (event.storageArea === this.storage) {
-      // TODO(multitab): This assert will likely become invalid as we add garbage
-      // collection.
-      assert(
-        event.key !== this.localClientStorageKey,
-        'Received LocalStorage notification for local change.'
-      );
-
       debug(LOG_TAG, 'EVENT', event.key, event.newValue);
 
+      if (event.key === this.localClientStorageKey) {
+        error(
+          'Received LocalStorage notification for local change. Another client might have garbage-collected our state'
+        );
+        return;
+      }
+
       this.queue.enqueueAndForget(async () => {
         if (!this.started) {
           this.earlyEvents.push(event);
@@ -1088,6 +1093,10 @@ export class MemorySharedClientState implements SharedClientState {
     this.localState.removeQueryTarget(targetId);
   }
 
+  isLocalQueryTarget(targetId: TargetId): boolean {
+    return this.localState.activeTargetIds.has(targetId);
+  }
+
   clearQueryState(targetId: TargetId): void {
     delete this.queryState[targetId];
   }

packages/firestore/src/util/obj.ts

@@ -39,13 +39,6 @@ export function size<V>(obj: Dict<V>): number {
   return count;
 }
 
-/** Extracts the numeric indices from a dictionary. */
-export function indices<V>(obj: { [numberKey: number]: V }): number[] {
-  return Object.keys(obj).map(key => {
-    return Number(key);
-  });
-}
-
 /** Returns the given value if it's defined or the defaultValue otherwise. */
 export function defaulted<V>(value: V | undefined, defaultValue: V): V {
   return value !== undefined ? value : defaultValue;

packages/firestore/test/unit/specs/describe_spec.ts

@@ -45,7 +45,7 @@ const KNOWN_TAGS = [
   DURABLE_PERSISTENCE_TAG
 ];
 
-// TOOD(mrschmidt): Make this configurable with mocha options.
+// TODO(mrschmidt): Make this configurable with mocha options.
 const RUN_BENCHMARK_TESTS = false;
 
 // The format of one describeSpec written to a JSON file.

packages/firestore/test/unit/specs/limit_spec.test.ts

@@ -41,6 +41,31 @@ describeSpec('Limits:', [], () => {
       });
   });
 
+  specTest(
+    "Documents outside of limit don't raise hasPendingWrites",
+    [],
+    () => {
+      const query1 = Query.atPath(path('collection')).withLimit(2);
+      const doc1 = doc('collection/a', 1000, { key: 'a' });
+      const doc2 = doc('collection/b', 1000, { key: 'b' });
+
+      return spec()
+        .withGCEnabled(false)
+        .userListens(query1)
+        .watchAcksFull(query1, 1000, doc1, doc2)
+        .expectEvents(query1, {
+          added: [doc1, doc2]
+        })
+        .userUnlistens(query1)
+        .userSets('collection/c', { key: 'c' })
+        .userListens(query1, 'resume-token-1000')
+        .expectEvents(query1, {
+          added: [doc1, doc2],
+          fromCache: true
+        });
+    }
+  );
+
   specTest('Deleted Document in limbo in full limit query', [], () => {
     const query = Query.atPath(path('collection')).withLimit(2);
     const doc1 = doc('collection/a', 1000, { key: 'a' });

packages/firestore/test/unit/specs/listen_spec.test.ts

@@ -1101,11 +1101,49 @@ describeSpec('Listens:', [], () => {
         .expectEvents(query, { added: [docC] })
         .client(0)
         .runTimer(TimerId.ClientMetadataRefresh)
+        // Client 0 recovers from its lease loss and applies the updates from
+        // client 1
         .expectPrimaryState(false)
         .expectEvents(query, { added: [docB, docC] })
     );
   });
 
+  specTest('Query bounces between primaries', ['multi-client'], () => {
+    const query = Query.atPath(path('collection'));
+    const docA = doc('collection/a', 1000, { key: 'a' });
+    const docB = doc('collection/b', 2000, { key: 'b' });
+    const docC = doc('collection/c', 3000, { key: 'c' });
+
+    // Client 0 listens to a query. Client 1 is the primary when the query is
+    // first listened to, then the query switches to client 0 and back to client
+    // 1.
+    return client(1)
+      .expectPrimaryState(true)
+      .client(0)
+      .userListens(query)
+      .client(1)
+      .expectListen(query)
+      .watchAcksFull(query, 1000, docA)
+      .client(0)
+      .expectEvents(query, { added: [docA] })
+      .client(2)
+      .stealPrimaryLease()
+      .expectListen(query, 'resume-token-1000')
+      .client(1)
+      .runTimer(TimerId.ClientMetadataRefresh)
+      .expectPrimaryState(false)
+      .client(2)
+      .watchAcksFull(query, 2000, docB)
+      .client(0)
+      .expectEvents(query, { added: [docB] })
+      .client(1)
+      .stealPrimaryLease()
+      .expectListen(query, 'resume-token-2000')
+      .watchAcksFull(query, 2000, docC)
+      .client(0)
+      .expectEvents(query, { added: [docC] });
+  });
+
   specTest(
     'Unresponsive primary ignores watch update',
     ['multi-client'],