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.

2 files changed, 126 insertions, 47 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());
   }