Add error handler for serializer (for deserializing cases) (#1181)

This is more interesting than the serializing case, as we should expect
to see occasional corruption of our input byte vector.

3 files changed, 316 insertions, 69 deletions

diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc
index b5a0720..2a89515 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.cc
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc
@@ -34,6 +34,7 @@ namespace remote {
 using firebase::firestore::model::FieldValue;
 using firebase::firestore::model::ObjectValue;
 using firebase::firestore::util::Status;
+using firebase::firestore::util::StatusOr;
 
 namespace {
 
@@ -43,7 +44,7 @@ class Reader;
 
 void EncodeObject(Writer* writer, const ObjectValue& object_value);
 
-ObjectValue DecodeObject(Reader* reader);
+ObjectValue::Map DecodeObject(Reader* reader);
 
 /**
  * Represents a nanopb tag.
@@ -196,6 +197,14 @@ class Reader {
     return stream_.bytes_left;
   }
 
+  Status status() const {
+    return status_;
+  }
+
+  void set_status(Status status) {
+    status_ = status;
+  }
+
  private:
   /**
    * Creates a new Reader, based on the given nanopb pb_istream_t. Note that
@@ -220,6 +229,8 @@ class Reader {
    */
   uint64_t ReadVarint();
 
+  Status status_ = Status::OK();
+
   pb_istream_t stream_;
 };
 
@@ -270,12 +281,17 @@ void Writer::WriteTag(Tag tag) {
 
 Tag Reader::ReadTag() {
   Tag tag;
+  if (!status_.ok()) return tag;
+
   bool eof;
-  bool ok = pb_decode_tag(&stream_, &tag.wire_type, &tag.field_number, &eof);
-  if (!ok || eof) {
-    // TODO(rsgowman): figure out error handling
-    abort();
+  if (!pb_decode_tag(&stream_, &tag.wire_type, &tag.field_number, &eof)) {
+    status_ = Status(FirestoreErrorCode::DataLoss, PB_GET_ERROR(&stream_));
+    return tag;
   }
+
+  // nanopb code always returns a false status when setting eof.
+  FIREBASE_ASSERT_MESSAGE(!eof, "nanopb set both ok status and eof to true");
+
   return tag;
 }
 
@@ -301,10 +317,11 @@ void Writer::WriteVarint(uint64_t value) {
  * @return The decoded varint as a uint64_t.
  */
 uint64_t Reader::ReadVarint() {
-  uint64_t varint_value;
+  if (!status_.ok()) return 0;
+
+  uint64_t varint_value = 0;
   if (!pb_decode_varint(&stream_, &varint_value)) {
-    // TODO(rsgowman): figure out error handling
-    abort();
+    status_ = Status(FirestoreErrorCode::DataLoss, PB_GET_ERROR(&stream_));
   }
   return varint_value;
 }
@@ -315,9 +332,11 @@ void Writer::WriteNull() {
 
 void Reader::ReadNull() {
   uint64_t varint = ReadVarint();
+  if (!status_.ok()) return;
+
   if (varint != google_protobuf_NullValue_NULL_VALUE) {
-    // TODO(rsgowman): figure out error handling
-    abort();
+    status_ = Status(FirestoreErrorCode::DataLoss,
+                     "Input proto bytes cannot be parsed (invalid null value)");
   }
 }
 
@@ -327,14 +346,18 @@ void Writer::WriteBool(bool bool_value) {
 
 bool Reader::ReadBool() {
   uint64_t varint = ReadVarint();
+  if (!status_.ok()) return false;
+
   switch (varint) {
     case 0:
       return false;
     case 1:
       return true;
     default:
-      // TODO(rsgowman): figure out error handling
-      abort();
+      status_ =
+          Status(FirestoreErrorCode::DataLoss,
+                 "Input proto bytes cannot be parsed (invalid bool value)");
+      return false;
   }
 }
 
@@ -357,17 +380,21 @@ void Writer::WriteString(const std::string& string_value) {
 }
 
 std::string Reader::ReadString() {
+  if (!status_.ok()) return "";
+
   pb_istream_t substream;
   if (!pb_make_string_substream(&stream_, &substream)) {
-    // TODO(rsgowman): figure out error handling
-    abort();
+    status_ = Status(FirestoreErrorCode::DataLoss, PB_GET_ERROR(&stream_));
+    pb_close_string_substream(&stream_, &substream);
+    return "";
   }
 
   std::string result(substream.bytes_left, '\0');
   if (!pb_read(&substream, reinterpret_cast<pb_byte_t*>(&result[0]),
                substream.bytes_left)) {
-    // TODO(rsgowman): figure out error handling
-    abort();
+    status_ = Status(FirestoreErrorCode::DataLoss, PB_GET_ERROR(&stream_));
+    pb_close_string_substream(&stream_, &substream);
+    return "";
   }
 
   // NB: future versions of nanopb read the remaining characters out of the
@@ -375,10 +402,9 @@ std::string Reader::ReadString() {
   // check within pb_close_string_substream. Unfortunately, that's not present
   // in the current version (0.38).  We'll make a stronger assertion and check
   // to make sure there *are* no remaining characters in the substream.
-  if (substream.bytes_left != 0) {
-    // TODO(rsgowman): figure out error handling
-    abort();
-  }
+  FIREBASE_ASSERT_MESSAGE(
+      substream.bytes_left == 0,
+      "Bytes remaining in substream after supposedly reading all of them.");
 
   pb_close_string_substream(&stream_, &substream);
 
@@ -431,29 +457,52 @@ void EncodeFieldValueImpl(Writer* writer, const FieldValue& field_value) {
 
 FieldValue DecodeFieldValueImpl(Reader* reader) {
   Tag tag = reader->ReadTag();
+  if (!reader->status().ok()) return FieldValue::NullValue();
 
   // Ensure the tag matches the wire type
-  // TODO(rsgowman): figure out error handling
   switch (tag.field_number) {
     case google_firestore_v1beta1_Value_null_value_tag:
     case google_firestore_v1beta1_Value_boolean_value_tag:
     case google_firestore_v1beta1_Value_integer_value_tag:
       if (tag.wire_type != PB_WT_VARINT) {
-        abort();
+        reader->set_status(
+            Status(FirestoreErrorCode::DataLoss,
+                   "Input proto bytes cannot be parsed (mismatch between "
+                   "the wiretype and the field number (tag))"));
       }
       break;
 
     case google_firestore_v1beta1_Value_string_value_tag:
     case google_firestore_v1beta1_Value_map_value_tag:
       if (tag.wire_type != PB_WT_STRING) {
-        abort();
+        reader->set_status(
+            Status(FirestoreErrorCode::DataLoss,
+                   "Input proto bytes cannot be parsed (mismatch between "
+                   "the wiretype and the field number (tag))"));
       }
       break;
 
     default:
-      abort();
+      // We could get here for one of two reasons; either because the input
+      // bytes are corrupt, or because we're attempting to parse a tag that we
+      // haven't implemented yet. Long term, the latter reason should become
+      // less likely (especially in production), so we'll assume former.
+
+      // TODO(rsgowman): While still in development, we'll contradict the above
+      // and assume the latter. Remove the following assertion when we're
+      // confident that we're handling all the tags in the protos.
+      FIREBASE_ASSERT_MESSAGE(
+          false,
+          "Unhandled message field number (tag): %i. (Or possibly "
+          "corrupt input bytes)",
+          tag.field_number);
+      reader->set_status(Status(
+          FirestoreErrorCode::DataLoss,
+          "Input proto bytes cannot be parsed (invalid field number (tag))"));
   }
 
+  if (!reader->status().ok()) return FieldValue::NullValue();
+
   switch (tag.field_number) {
     case google_firestore_v1beta1_Value_null_value_tag:
       reader->ReadNull();
@@ -465,12 +514,15 @@ FieldValue DecodeFieldValueImpl(Reader* reader) {
     case google_firestore_v1beta1_Value_string_value_tag:
       return FieldValue::StringValue(reader->ReadString());
     case google_firestore_v1beta1_Value_map_value_tag:
-      return FieldValue::ObjectValueFromMap(
-          DecodeObject(reader).internal_value);
+      return FieldValue::ObjectValueFromMap(DecodeObject(reader));
 
     default:
-      // TODO(rsgowman): figure out error handling
-      abort();
+      // This indicates an internal error as we've already ensured that this is
+      // a valid field_number.
+      FIREBASE_ASSERT_MESSAGE(
+          false,
+          "Somehow got an unexpected field number (tag) after verifying that "
+          "the field number was expected.");
   }
 }
 
@@ -533,24 +585,34 @@ template <typename T>
 T Reader::ReadNestedMessage(const std::function<T(Reader*)>& read_message_fn) {
   // Implementation note: This is roughly modeled on pb_decode_delimited,
   // adjusted to account for the oneof in FieldValue.
+
+  if (!status_.ok()) return T();
+
   pb_istream_t raw_substream;
   if (!pb_make_string_substream(&stream_, &raw_substream)) {
-    // TODO(rsgowman): figure out error handling
-    abort();
+    status_ = Status(FirestoreErrorCode::DataLoss, PB_GET_ERROR(&stream_));
+    pb_close_string_substream(&stream_, &raw_substream);
+    return T();
   }
   Reader substream(raw_substream);
 
+  // If this fails, we *won't* return right away so that we can cleanup the
+  // substream (although technically, that turns out not to matter; no resource
+  // leaks occur if we don't do this.)
+  // TODO(rsgowman): Consider RAII here. (Watch out for Reader class which also
+  // wraps streams.)
   T message = read_message_fn(&substream);
+  status_ = substream.status();
 
   // NB: future versions of nanopb read the remaining characters out of the
   // substream (and return false if that fails) as an additional safety
   // check within pb_close_string_substream. Unfortunately, that's not present
   // in the current version (0.38).  We'll make a stronger assertion and check
   // to make sure there *are* no remaining characters in the substream.
-  if (substream.bytes_left() != 0) {
-    // TODO(rsgowman): figure out error handling
-    abort();
-  }
+  FIREBASE_ASSERT_MESSAGE(
+      substream.bytes_left() == 0,
+      "Bytes remaining in substream after supposedly reading all of them.");
+
   pb_close_string_substream(&stream_, &substream.stream_);
 
   return message;
@@ -591,6 +653,7 @@ void EncodeFieldsEntry(Writer* writer, const ObjectValue::Map::value_type& kv) {
 
 ObjectValue::Map::value_type DecodeFieldsEntry(Reader* reader) {
   Tag tag = reader->ReadTag();
+  if (!reader->status().ok()) return {};
 
   // TODO(rsgowman): figure out error handling: We can do better than a failed
   // assertion.
@@ -600,6 +663,7 @@ ObjectValue::Map::value_type DecodeFieldsEntry(Reader* reader) {
   std::string key = reader->ReadString();
 
   tag = reader->ReadTag();
+  if (!reader->status().ok()) return {};
   FIREBASE_ASSERT(tag.field_number ==
                   google_firestore_v1beta1_MapValue_FieldsEntry_value_tag);
   FIREBASE_ASSERT(tag.wire_type == PB_WT_STRING);
@@ -607,7 +671,7 @@ ObjectValue::Map::value_type DecodeFieldsEntry(Reader* reader) {
   FieldValue value =
       reader->ReadNestedMessage<FieldValue>(DecodeFieldValueImpl);
 
-  return {key, value};
+  return ObjectValue::Map::value_type{key, value};
 }
 
 void EncodeObject(Writer* writer, const ObjectValue& object_value) {
@@ -622,12 +686,17 @@ void EncodeObject(Writer* writer, const ObjectValue& object_value) {
   });
 }
 
-ObjectValue DecodeObject(Reader* reader) {
-  ObjectValue::Map internal_value = reader->ReadNestedMessage<ObjectValue::Map>(
+ObjectValue::Map DecodeObject(Reader* reader) {
+  if (!reader->status().ok()) return ObjectValue::Map();
+
+  return reader->ReadNestedMessage<ObjectValue::Map>(
       [](Reader* reader) -> ObjectValue::Map {
         ObjectValue::Map result;
+        if (!reader->status().ok()) return result;
+
         while (reader->bytes_left()) {
           Tag tag = reader->ReadTag();
+          if (!reader->status().ok()) return result;
           FIREBASE_ASSERT(tag.field_number ==
                           google_firestore_v1beta1_MapValue_fields_tag);
           FIREBASE_ASSERT(tag.wire_type == PB_WT_STRING);
@@ -640,6 +709,7 @@ ObjectValue DecodeObject(Reader* reader) {
           // map.
           // TODO(rsgowman): figure out error handling: We can do better than a
           // failed assertion.
+          if (!reader->status().ok()) return result;
           FIREBASE_ASSERT(result.find(fv.first) == result.end());
 
           // Add this key,fieldvalue to the results map.
@@ -647,7 +717,6 @@ ObjectValue DecodeObject(Reader* reader) {
         }
         return result;
       });
-  return ObjectValue{internal_value};
 }
 
 }  // namespace
@@ -659,9 +728,15 @@ Status Serializer::EncodeFieldValue(const FieldValue& field_value,
   return writer.status();
 }
 
-FieldValue Serializer::DecodeFieldValue(const uint8_t* bytes, size_t length) {
+StatusOr<FieldValue> Serializer::DecodeFieldValue(const uint8_t* bytes,
+                                                  size_t length) {
   Reader reader = Reader::Wrap(bytes, length);
-  return DecodeFieldValueImpl(&reader);
+  FieldValue fv = DecodeFieldValueImpl(&reader);
+  if (reader.status().ok()) {
+    return fv;
+  } else {
+    return reader.status();
+  }
 }
 
 }  // namespace remote
diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.h b/Firestore/core/src/firebase/firestore/remote/serializer.h
index 7f08f7d..7d13583 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.h
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.h
@@ -24,6 +24,7 @@
 #include "Firestore/core/src/firebase/firestore/model/field_value.h"
 #include "Firestore/core/src/firebase/firestore/util/firebase_assert.h"
 #include "Firestore/core/src/firebase/firestore/util/status.h"
+#include "Firestore/core/src/firebase/firestore/util/statusor.h"
 #include "absl/base/attributes.h"
 
 namespace firebase {
@@ -66,8 +67,9 @@ class Serializer {
    * @param field_value the model to convert.
    * @param[out] out_bytes A buffer to place the output. The bytes will be
    * appended to this vector.
+   * @return A Status, which if not ok(), indicates what went wrong. Note that
+   * errors during encoding generally indicate a serious/fatal error.
    */
-  // TODO(rsgowman): error handling, incl return code.
   // TODO(rsgowman): If we never support any output except to a vector, it may
   // make sense to have Serializer own the vector and provide an accessor rather
   // than asking the user to create it first.
@@ -80,20 +82,22 @@ class Serializer {
    *
    * @param bytes The bytes to convert. It's assumed that exactly all of the
    * bytes will be used by this conversion.
-   * @return The model equivalent of the bytes.
+   * @return The model equivalent of the bytes or a Status indicating
+   * what went wrong.
    */
-  // TODO(rsgowman): error handling.
-  model::FieldValue DecodeFieldValue(const uint8_t* bytes, size_t length);
+  util::StatusOr<model::FieldValue> DecodeFieldValue(const uint8_t* bytes,
+                                                     size_t length);
 
   /**
    * @brief Converts from bytes to the model FieldValue format.
    *
    * @param bytes The bytes to convert. It's assumed that exactly all of the
    * bytes will be used by this conversion.
-   * @return The model equivalent of the bytes.
+   * @return The model equivalent of the bytes or a Status indicating
+   * what went wrong.
    */
-  // TODO(rsgowman): error handling.
-  model::FieldValue DecodeFieldValue(const std::vector<uint8_t>& bytes) {
+  util::StatusOr<model::FieldValue> DecodeFieldValue(
+      const std::vector<uint8_t>& bytes) {
     return DecodeFieldValue(bytes.data(), bytes.size());
   }
 
diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
index 96125f7..a2036a8 100644
--- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
+++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
@@ -33,18 +33,27 @@
 #include <vector>
 
 #include "Firestore/Protos/cpp/google/firestore/v1beta1/document.pb.h"
+#include "Firestore/core/include/firebase/firestore/firestore_errors.h"
 #include "Firestore/core/src/firebase/firestore/model/field_value.h"
 #include "Firestore/core/src/firebase/firestore/util/status.h"
+#include "Firestore/core/src/firebase/firestore/util/statusor.h"
 #include "google/protobuf/stubs/common.h"
 #include "google/protobuf/util/message_differencer.h"
 #include "gtest/gtest.h"
 
+using firebase::firestore::FirestoreErrorCode;
 using firebase::firestore::model::FieldValue;
 using firebase::firestore::model::ObjectValue;
 using firebase::firestore::remote::Serializer;
 using firebase::firestore::util::Status;
+using firebase::firestore::util::StatusOr;
 using google::protobuf::util::MessageDifferencer;
 
+#define ASSERT_OK(status) ASSERT_TRUE(StatusOk(status))
+#define ASSERT_NOT_OK(status) ASSERT_FALSE(StatusOk(status))
+#define EXPECT_OK(status) EXPECT_TRUE(StatusOk(status))
+#define EXPECT_NOT_OK(status) EXPECT_FALSE(StatusOk(status))
+
 TEST(Serializer, CanLinkToNanopb) {
   // This test doesn't actually do anything interesting as far as actually using
   // nanopb is concerned but that it can run at all is proof that all the
@@ -74,22 +83,69 @@ class SerializerTest : public ::testing::Test {
     ExpectDeserializationRoundTrip(model, proto, type);
   }
 
+  /**
+   * Checks the status. Don't use directly; use one of the relevant macros
+   * instead. eg:
+   *
+   *   Status good_status = ...;
+   *   ASSERT_OK(good_status);
+   *
+   *   Status bad_status = ...;
+   *   EXPECT_NOT_OK(bad_status);
+   */
+  testing::AssertionResult StatusOk(const Status& status) {
+    if (!status.ok()) {
+      return testing::AssertionFailure()
+             << "Status should have been ok, but instead contained "
+             << status.ToString();
+    }
+    return testing::AssertionSuccess();
+  }
+
+  template <typename T>
+  testing::AssertionResult StatusOk(const StatusOr<T>& status) {
+    return StatusOk(status.status());
+  }
+
+  /**
+   * Ensures that decoding fails with the given status.
+   *
+   * @param status the expected (failed) status. Only the code() is verified.
+   */
+  void ExpectFailedStatusDuringDecode(Status status,
+                                      const std::vector<uint8_t>& bytes) {
+    StatusOr<FieldValue> bad_status = serializer.DecodeFieldValue(bytes);
+    ASSERT_NOT_OK(bad_status);
+    EXPECT_EQ(status.code(), bad_status.status().code());
+  }
+
   google::firestore::v1beta1::Value ValueProto(nullptr_t) {
-    std::vector<uint8_t> bytes;
-    Status status =
-        serializer.EncodeFieldValue(FieldValue::NullValue(), &bytes);
-    EXPECT_TRUE(status.ok());
+    std::vector<uint8_t> bytes =
+        EncodeFieldValue(&serializer, FieldValue::NullValue());
     google::firestore::v1beta1::Value proto;
     bool ok = proto.ParseFromArray(bytes.data(), bytes.size());
     EXPECT_TRUE(ok);
     return proto;
   }
 
-  google::firestore::v1beta1::Value ValueProto(bool b) {
+  std::vector<uint8_t> EncodeFieldValue(Serializer* serializer,
+                                        const FieldValue& fv) {
     std::vector<uint8_t> bytes;
-    Status status =
-        serializer.EncodeFieldValue(FieldValue::BooleanValue(b), &bytes);
-    EXPECT_TRUE(status.ok());
+    Status status = serializer->EncodeFieldValue(fv, &bytes);
+    EXPECT_OK(status);
+    return bytes;
+  }
+
+  void Mutate(uint8_t* byte,
+              uint8_t expected_initial_value,
+              uint8_t new_value) {
+    ASSERT_EQ(*byte, expected_initial_value);
+    *byte = new_value;
+  }
+
+  google::firestore::v1beta1::Value ValueProto(bool b) {
+    std::vector<uint8_t> bytes =
+        EncodeFieldValue(&serializer, FieldValue::BooleanValue(b));
     google::firestore::v1beta1::Value proto;
     bool ok = proto.ParseFromArray(bytes.data(), bytes.size());
     EXPECT_TRUE(ok);
@@ -97,10 +153,8 @@ class SerializerTest : public ::testing::Test {
   }
 
   google::firestore::v1beta1::Value ValueProto(int64_t i) {
-    std::vector<uint8_t> bytes;
-    Status status =
-        serializer.EncodeFieldValue(FieldValue::IntegerValue(i), &bytes);
-    EXPECT_TRUE(status.ok());
+    std::vector<uint8_t> bytes =
+        EncodeFieldValue(&serializer, FieldValue::IntegerValue(i));
     google::firestore::v1beta1::Value proto;
     bool ok = proto.ParseFromArray(bytes.data(), bytes.size());
     EXPECT_TRUE(ok);
@@ -112,10 +166,8 @@ class SerializerTest : public ::testing::Test {
   }
 
   google::firestore::v1beta1::Value ValueProto(const std::string& s) {
-    std::vector<uint8_t> bytes;
-    Status status =
-        serializer.EncodeFieldValue(FieldValue::StringValue(s), &bytes);
-    EXPECT_TRUE(status.ok());
+    std::vector<uint8_t> bytes =
+        EncodeFieldValue(&serializer, FieldValue::StringValue(s));
     google::firestore::v1beta1::Value proto;
     bool ok = proto.ParseFromArray(bytes.data(), bytes.size());
     EXPECT_TRUE(ok);
@@ -128,9 +180,7 @@ class SerializerTest : public ::testing::Test {
       const google::firestore::v1beta1::Value& proto,
       FieldValue::Type type) {
     EXPECT_EQ(type, model.type());
-    std::vector<uint8_t> bytes;
-    Status status = serializer.EncodeFieldValue(model, &bytes);
-    EXPECT_TRUE(status.ok());
+    std::vector<uint8_t> bytes = EncodeFieldValue(&serializer, model);
     google::firestore::v1beta1::Value actual_proto;
     bool ok = actual_proto.ParseFromArray(bytes.data(), bytes.size());
     EXPECT_TRUE(ok);
@@ -145,7 +195,10 @@ class SerializerTest : public ::testing::Test {
     std::vector<uint8_t> bytes(size);
     bool status = proto.SerializeToArray(bytes.data(), size);
     EXPECT_TRUE(status);
-    FieldValue actual_model = serializer.DecodeFieldValue(bytes);
+    StatusOr<FieldValue> actual_model_status =
+        serializer.DecodeFieldValue(bytes);
+    EXPECT_OK(actual_model_status);
+    FieldValue actual_model = actual_model_status.ValueOrDie();
     EXPECT_EQ(type, actual_model.type());
     EXPECT_EQ(model, actual_model);
   }
@@ -258,7 +311,122 @@ TEST_F(SerializerTest, WritesNestedObjects) {
   ExpectRoundTrip(model, proto, FieldValue::Type::Object);
 }
 
+TEST_F(SerializerTest, BadNullValue) {
+  std::vector<uint8_t> bytes =
+      EncodeFieldValue(&serializer, FieldValue::NullValue());
+
+  // Alter the null value from 0 to 1.
+  Mutate(&bytes[1], /*expected_initial_value=*/0, /*new_value=*/1);
+
+  ExpectFailedStatusDuringDecode(
+      Status(FirestoreErrorCode::DataLoss, "ignored"), bytes);
+}
+
+TEST_F(SerializerTest, BadBoolValue) {
+  std::vector<uint8_t> bytes =
+      EncodeFieldValue(&serializer, FieldValue::BooleanValue(true));
+
+  // Alter the bool value from 1 to 2. (Value values are 0,1)
+  Mutate(&bytes[1], /*expected_initial_value=*/1, /*new_value=*/2);
+
+  ExpectFailedStatusDuringDecode(
+      Status(FirestoreErrorCode::DataLoss, "ignored"), bytes);
+}
+
+TEST_F(SerializerTest, BadIntegerValue) {
+  // Encode 'maxint'. This should result in 9 0xff bytes, followed by a 1.
+  std::vector<uint8_t> bytes = EncodeFieldValue(
+      &serializer,
+      FieldValue::IntegerValue(std::numeric_limits<uint64_t>::max()));
+  ASSERT_EQ(11u, bytes.size());
+  for (size_t i = 1; i < bytes.size() - 1; i++) {
+    ASSERT_EQ(0xff, bytes[i]);
+  }
+
+  // make the number a bit bigger
+  Mutate(&bytes[10], /*expected_initial_value=*/1, /*new_value=*/0xff);
+  bytes.resize(12);
+  bytes[11] = 0x7f;
+
+  ExpectFailedStatusDuringDecode(
+      Status(FirestoreErrorCode::DataLoss, "ignored"), bytes);
+}
+
+TEST_F(SerializerTest, BadStringValue) {
+  std::vector<uint8_t> bytes =
+      EncodeFieldValue(&serializer, FieldValue::StringValue("a"));
+
+  // Claim that the string length is 5 instead of 1. (The first two bytes are
+  // used by the encoded tag.)
+  Mutate(&bytes[2], /*expected_initial_value=*/1, /*new_value=*/5);
+
+  ExpectFailedStatusDuringDecode(
+      Status(FirestoreErrorCode::DataLoss, "ignored"), bytes);
+}
+
+TEST_F(SerializerTest, BadTag) {
+  std::vector<uint8_t> bytes =
+      EncodeFieldValue(&serializer, FieldValue::NullValue());
+
+  // The google::firestore::v1beta1::Value value_type oneof currently has tags
+  // up to 18. For this test, we'll pick a tag that's unlikely to be added in
+  // the near term but still fits within a uint8_t even when encoded.
+  // Specifically 31. 0xf8 represents field number 31 encoded as a varint.
+  Mutate(&bytes[0], /*expected_initial_value=*/0x58, /*new_value=*/0xf8);
+
+  // TODO(rsgowman): The behaviour is *temporarily* slightly different during
+  // development; this will cause a failed assertion rather than a failed
+  // status. Remove this EXPECT_ANY_THROW statement (and reenable the
+  // following commented out statement) once the corresponding assert has been
+  // removed from serializer.cc.
+  EXPECT_ANY_THROW(ExpectFailedStatusDuringDecode(
+      Status(FirestoreErrorCode::DataLoss, "ignored"), bytes));
+  // ExpectFailedStatusDuringDecode(
+  //    Status(FirestoreErrorCode::DataLoss, "ignored"), bytes);
+}
+
+TEST_F(SerializerTest, TagVarintWiretypeStringMismatch) {
+  std::vector<uint8_t> bytes =
+      EncodeFieldValue(&serializer, FieldValue::BooleanValue(true));
+
+  // 0x0a represents a bool value encoded as a string. (We're using a
+  // boolean_value tag here, but any tag that would be represented by a varint
+  // would do.)
+  Mutate(&bytes[0], /*expected_initial_value=*/0x08, /*new_value=*/0x0a);
+
+  ExpectFailedStatusDuringDecode(
+      Status(FirestoreErrorCode::DataLoss, "ignored"), bytes);
+}
+
+TEST_F(SerializerTest, TagStringWiretypeVarintMismatch) {
+  std::vector<uint8_t> bytes =
+      EncodeFieldValue(&serializer, FieldValue::StringValue("foo"));
+
+  // 0x88 represents a string value encoded as a varint.
+  Mutate(&bytes[0], /*expected_initial_value=*/0x8a, /*new_value=*/0x88);
+
+  ExpectFailedStatusDuringDecode(
+      Status(FirestoreErrorCode::DataLoss, "ignored"), bytes);
+}
+
+TEST_F(SerializerTest, IncompleteFieldValue) {
+  std::vector<uint8_t> bytes =
+      EncodeFieldValue(&serializer, FieldValue::NullValue());
+  ASSERT_EQ(2u, bytes.size());
+
+  // Remove the (null) payload
+  ASSERT_EQ(0x00, bytes[1]);
+  bytes.pop_back();
+
+  ExpectFailedStatusDuringDecode(
+      Status(FirestoreErrorCode::DataLoss, "ignored"), bytes);
+}
+
+TEST_F(SerializerTest, IncompleteTag) {
+  std::vector<uint8_t> bytes;
+  ExpectFailedStatusDuringDecode(
+      Status(FirestoreErrorCode::DataLoss, "ignored"), bytes);
+}
+
 // TODO(rsgowman): Test [en|de]coding multiple protos into the same output
 // vector.
-
-// TODO(rsgowman): Death test for decoding invalid bytes.