diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index e88ab8baa82..b4248aeb5c0 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -1,5 +1,6 @@ # Unreleased * [feature] Add support for disjunctions in queries (`OR` queries). +* [fixed] Fixed stack overflow caused by deeply nested server timestamps (#4702). # 24.4.4 * [changed] Relaxed certain query validations performed by the SDK (#4231). diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ServerTimestamps.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ServerTimestamps.java index 7ae19eadcca..b3a47aa313e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ServerTimestamps.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ServerTimestamps.java @@ -62,6 +62,16 @@ public static Value valueOf(Timestamp localWriteTime, @Nullable Value previousVa .putFields(TYPE_KEY, encodedType) .putFields(LOCAL_WRITE_TIME_KEY, encodeWriteTime); + // We should avoid storing deeply nested server timestamp map values, because we never use the + // intermediate "previous values". + // For example: + // previous: 42L, add: t1, result: t1 -> 42L + // previous: t1, add: t2, result: t2 -> 42L (NOT t2 -> t1 -> 42L) + // previous: t2, add: t3, result: t3 -> 42L (NOT t3 -> t2 -> t1 -> 42L) + // previous: t3, add: t4, result: t4 -> 42L (NOT t4 -> t3 -> t2 -> t1 -> 42L) + // `getPreviousValue` recursively traverses server timestamps to find the least recent Value. + previousValue = + isServerTimestamp(previousValue) ? getPreviousValue(previousValue) : previousValue; if (previousValue != null) { mapRepresentation.putFields(PREVIOUS_VALUE_KEY, previousValue); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteLocalStoreTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteLocalStoreTest.java index 792c5e8f0ce..f522469ff18 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteLocalStoreTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteLocalStoreTest.java @@ -32,12 +32,29 @@ import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; +import com.google.firebase.Timestamp; import com.google.firebase.firestore.FieldValue; import com.google.firebase.firestore.core.Query; +import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.FieldIndex; +import com.google.firebase.firestore.model.FieldPath; +import com.google.firebase.firestore.model.ObjectValue; +import com.google.firebase.firestore.model.ServerTimestamps; +import com.google.firebase.firestore.model.mutation.FieldMask; +import com.google.firebase.firestore.model.mutation.FieldTransform; +import com.google.firebase.firestore.model.mutation.PatchMutation; +import com.google.firebase.firestore.model.mutation.Precondition; +import com.google.firebase.firestore.model.mutation.ServerTimestampOperation; +import com.google.firestore.v1.Value; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; import org.junit.Test; import org.junit.runner.RunWith; import org.robolectric.RobolectricTestRunner; @@ -290,4 +307,63 @@ public void testIndexesServerTimestamps() { assertOverlayTypes(keyMap("coll/a", CountingQueryEngine.OverlayType.Set)); assertQueryReturned("coll/a"); } + + @Test + public void testDeeplyNestedServerTimestamps() { + Timestamp timestamp = Timestamp.now(); + Value initialServerTimestamp = ServerTimestamps.valueOf(timestamp, null); + Map fields = + new HashMap() { + { + put("timestamp", ServerTimestamps.valueOf(timestamp, initialServerTimestamp)); + } + }; + FieldPath path = FieldPath.fromSingleSegment("timestamp"); + FieldMask mask = + FieldMask.fromSet( + new HashSet() { + { + add(path); + } + }); + FieldTransform fieldTransform = + new FieldTransform(path, ServerTimestampOperation.getInstance()); + List fieldTransforms = + new ArrayList() { + { + add(fieldTransform); + } + }; + // The purpose of this test is to ensure that deeply nested server timestamps do not result in + // a stack overflow error. Below we use a `Thread` object to create a large number of mutations + // because the `Thread` class allows us to specify the maximum stack size. + AtomicReference error = new AtomicReference<>(); + Thread thread = + new Thread( + Thread.currentThread().getThreadGroup(), + () -> { + try { + for (int i = 0; i < 1000; ++i) { + writeMutation( + new PatchMutation( + DocumentKey.fromPathString("some/object/for/test"), + ObjectValue.fromMap(fields), + mask, + Precondition.NONE, + fieldTransforms)); + } + } catch (Throwable e) { + error.set(e); + } + }, + /* name */ "test", + /* stackSize */ 1024 * 1024); + try { + thread.start(); + thread.join(); + } catch (InterruptedException e) { + throw new AssertionError(e); + } + assertThat(error.get()).isNull(); + } }