Mojo: Require nullable outputs for optional fields

This makes it so optional mojom values can only be read from their
DataView into a destination type that supports nullability when read
with the common Read[FieldName] methods. Attempting to violate this
constraint will result in a compile-time assertion failure at the site
of the Read[FieldName] call. Prior to this change, such instances were
non-obvious and could instead result in surprising runtime behavior.

Types that support nullability include anything wrapped with
base::Optional, or any type for which there exists an appropriate
{Struct,Union,Array,String}Traits definition with a SetToNull method.

Pre-existing violations of the new constraint are corrected here either
by making fields non-optional, deserializing to a base::Optional, or
doing something more specialized to fix traits logic.

Finally, this also removes the IsNull and SetToNull methods from
StringTraits<std::string> since: (a) optional string fields are
generated as base::Optional<std::string> and (b) it was not intentional
for null strings and empty strings to be treated as equivalent in
meaning.

Fixed: 1124639
Change-Id: I0569b4b8420b4b416bd889417d9723307a880454
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2404478
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807146}

cc/paint/filter_operation.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/check_op.h"
+#include "base/containers/span.h"
 #include "cc/paint/paint_export.h"
 #include "cc/paint/paint_filter.h"
 #include "third_party/skia/include/core/SkColor.h"
@@ -210,7 +211,7 @@ class CC_PAINT_EXPORT FilterOperation {
     image_filter_ = std::move(image_filter);
   }
 
-  void set_matrix(const Matrix& matrix) {
+  void set_matrix(base::span<const SkScalar, 20> matrix) {
     DCHECK_EQ(type_, COLOR_MATRIX);
     for (unsigned i = 0; i < 20; ++i)
       matrix_[i] = matrix[i];

components/arc/intent_helper/intent_filter_mojom_traits.cc

@@ -8,6 +8,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/optional.h"
 #include "base/strings/string_util.h"
 
 namespace mojo {
@@ -23,7 +24,7 @@ bool StructTraits<arc::mojom::IntentFilterDataView, arc::IntentFilter>::Read(
   if (!data.ReadDataPaths(&paths))
     return false;
 
-  std::string package_name;
+  base::Optional<std::string> package_name;
   if (!data.ReadPackageName(&package_name))
     return false;
 
@@ -39,15 +40,17 @@ bool StructTraits<arc::mojom::IntentFilterDataView, arc::IntentFilter>::Read(
   if (!data.ReadMimeTypes(&mime_types))
     return false;
 
-  std::string activity_name;
+  base::Optional<std::string> activity_name;
   if (!data.ReadActivityName(&activity_name))
     return false;
 
-  std::string activity_label;
+  base::Optional<std::string> activity_label;
   if (!data.ReadActivityLabel(&activity_label))
     return false;
 
-  *out = arc::IntentFilter(package_name, activity_name, activity_label,
+  *out = arc::IntentFilter(std::move(package_name).value_or(std::string()),
+                           std::move(activity_name).value_or(std::string()),
+                           std::move(activity_label).value_or(std::string()),
                            std::move(actions), std::move(authorities),
                            std::move(paths), std::move(schemes),
                            std::move(mime_types));

media/mojo/mojom/status_mojom_traits.cc

@@ -24,8 +24,10 @@ bool StructTraits<media::mojom::StatusDataView, media::Status>::Read(
   if (media::StatusCode::kOk == code)
     return true;
 
-  if (!data.ReadMessage(&message))
+  base::Optional<std::string> optional_message;
+  if (!data.ReadMessage(&optional_message))
     return false;
+  message = std::move(optional_message).value_or(std::string());
 
   output->data_ =
       std::make_unique<media::Status::StatusInternal>(code, std::move(message));
@@ -36,8 +38,10 @@ bool StructTraits<media::mojom::StatusDataView, media::Status>::Read(
   if (!data.ReadCauses(&output->data_->causes))
     return false;
 
-  if (!data.ReadData(&output->data_->data))
+  base::Optional<base::Value> optional_data;
+  if (!data.ReadData(&optional_data))
     return false;
+  output->data_->data = std::move(optional_data).value_or(base::Value());
 
   return true;
 }

mojo/public/cpp/bindings/array_traits_span.h

@@ -16,10 +16,6 @@ template <typename T, size_t Extent>
 struct ArrayTraits<base::span<T, Extent>> {
   using Element = T;
 
-  // There is no concept of a null span, as it is indistinguishable from the
-  // empty span.
-  static bool IsNull(const base::span<T>& input) { return false; }
-
   static size_t GetSize(const base::span<T>& input) { return input.size(); }
 
   static T* GetData(base::span<T>& input) { return input.data(); }

mojo/public/cpp/bindings/lib/serialization_util.h

@@ -9,11 +9,14 @@
 #include <stdint.h>
 
 #include <queue>
+#include <type_traits>
 
 #include "base/logging.h"
 #include "base/macros.h"
 #include "mojo/public/cpp/bindings/lib/bindings_internal.h"
 #include "mojo/public/cpp/bindings/lib/serialization_context.h"
+#include "mojo/public/cpp/bindings/lib/serialization_forward.h"
+#include "mojo/public/cpp/bindings/lib/template_util.h"
 
 namespace mojo {
 namespace internal {
@@ -71,10 +74,61 @@ template <typename Traits,
           typename std::enable_if<!HasSetToNullMethod<Traits>::value>::type* =
               nullptr>
 bool CallSetToNullIfExists(UserType* output) {
-  LOG(ERROR) << "A null value is received. But the Struct/Array/StringTraits "
-             << "class doesn't define a SetToNull() function and therefore is "
-             << "unable to deserialize the value.";
-  return false;
+  // Note that it is not considered an error to attempt to read a null value
+  // into a non-nullable `output` object. In such cases, the caller must have
+  // used a DataView's corresponding MaybeRead[FieldName] method, thus
+  // explicitly choosing to ignore null values without affecting `output`.
+  //
+  // If instead a caller unwittingly attempts to use a corresponding
+  // Read[FieldName] method to read an optional value into the same non-nullable
+  // UserType, it will result in a compile-time error. As such, we don't need to
+  // account for that case here.
+  return true;
+}
+
+template <typename MojomType, typename UserType, typename = void>
+struct TraitsFinder {
+  using Traits = StructTraits<MojomType, UserType>;
+};
+
+template <typename MojomType, typename UserType>
+struct TraitsFinder<
+    MojomType,
+    UserType,
+    std::enable_if_t<BelongsTo<MojomType, MojomTypeCategory::kUnion>::value>> {
+  using Traits = UnionTraits<MojomType, UserType>;
+};
+
+template <typename UserType>
+struct TraitsFinder<StringDataView, UserType> {
+  using Traits = StringTraits<UserType>;
+};
+
+template <typename UserType, typename ElementType>
+struct TraitsFinder<ArrayDataView<ElementType>, UserType> {
+  using Traits = ArrayTraits<UserType>;
+};
+
+template <typename UserType, typename KeyType, typename ValueType>
+struct TraitsFinder<MapDataView<KeyType, ValueType>, UserType> {
+  using Traits = MapTraits<UserType>;
+};
+
+template <typename MojomType,
+          typename UserType,
+          typename std::enable_if<IsOptionalWrapper<UserType>::value>::type* =
+              nullptr>
+constexpr bool IsValidUserTypeForOptionalValue() {
+  return true;
+}
+
+template <typename MojomType,
+          typename UserType,
+          typename std::enable_if<!IsOptionalWrapper<UserType>::value>::type* =
+              nullptr>
+constexpr bool IsValidUserTypeForOptionalValue() {
+  using Traits = typename TraitsFinder<MojomType, UserType>::Traits;
+  return HasSetToNullMethod<Traits>::value;
 }
 
 template <typename T, typename MaybeConstUserType>

mojo/public/cpp/bindings/string_traits_stl.h

@@ -13,16 +13,6 @@ namespace mojo {
 
 template <>
 struct StringTraits<std::string> {
-  static bool IsNull(const std::string& input) {
-    // std::string is always converted to non-null mojom string.
-    return false;
-  }
-
-  static void SetToNull(std::string* output) {
-    // std::string doesn't support null state. Set it to empty instead.
-    output->clear();
-  }
-
   static const std::string& GetUTF8(const std::string& input) { return input; }
 
   static bool Read(StringDataView input, std::string* output) {

mojo/public/tools/bindings/generators/cpp_templates/struct_data_view_declaration.tmpl

+{%- import "struct_macros.tmpl" as struct_macros %}
+
 class {{struct.name}}DataView {
  public:
   {{struct.name}}DataView() {}
@@ -22,6 +24,7 @@ class {{struct.name}}DataView {
 
   template <typename UserType>
   WARN_UNUSED_RESULT bool Read{{name|under_to_camel}}(UserType* output) {
+    {{struct_macros.assert_nullable_output_type_if_necessary(kind, name)}}
 {%-     if pf.min_version != 0 %}
     auto* pointer = data_->header_.version >= {{pf.min_version}} && !data_->{{name}}.is_null()
                     ? &data_->{{name}} : nullptr;
@@ -38,6 +41,7 @@ class {{struct.name}}DataView {
 
   template <typename UserType>
   WARN_UNUSED_RESULT bool Read{{name|under_to_camel}}(UserType* output) {
+    {{struct_macros.assert_nullable_output_type_if_necessary(kind, name)}}
 {%-     if pf.min_version != 0 %}
     auto* pointer = data_->header_.version >= {{pf.min_version}}
                     ? data_->{{name}}.Get() : nullptr;

mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl

@@ -125,3 +125,18 @@
 {%-     endif %}
 {%-   endfor %}
 {%- endmacro %}
+
+{%- macro assert_nullable_output_type_if_necessary(kind, name) -%}
+{%-   if kind|is_nullable_kind and not kind|is_native_only_kind %}
+static_assert(
+    mojo::internal::IsValidUserTypeForOptionalValue<
+        {{kind|unmapped_type_for_serializer}}, UserType>(),
+    "Attempting to read the optional `{{name}}` field into a type which "
+    "cannot represent a null value. Either wrap the destination object "
+    "with base::Optional, ensure that any corresponding "
+    "{Struct/Union/Array/String}Traits define the necessary IsNull and "
+    "SetToNull methods, or use `MaybeRead{{name|under_to_camel}}` instead "
+    "of `Read{{name|under_to_camel}} if you're fine with null values being "
+    "silently ignored in this case.");
+{%-   endif %}
+{%- endmacro %}

mojo/public/tools/bindings/generators/cpp_templates/union_data_view_declaration.tmpl

+{%- import "struct_macros.tmpl" as struct_macros %}
+
 class {{union.name}}DataView {
  public:
   using Tag = internal::{{union.name}}_Data::{{union.name}}_Tag;
@@ -32,6 +34,7 @@ class {{union.name}}DataView {
 
   template <typename UserType>
   WARN_UNUSED_RESULT bool Read{{name|under_to_camel}}(UserType* output) {
+    {{struct_macros.assert_nullable_output_type_if_necessary(kind, name)}}
     DCHECK(is_{{name}}());
     return mojo::internal::Deserialize<{{kind|unmapped_type_for_serializer}}>(
         data_->data.f_{{name}}.Get(), output, context_);

services/network/public/mojom/url_loader.mojom

@@ -423,7 +423,7 @@ struct DataElement {
   mojo_base.mojom.File? file;
   // For kBlob
   // TODO(richard.li): Deprecate this once NetworkService is fully shipped.
-  string? blob_uuid;
+  string blob_uuid;
   // For kDataPipe
   pending_remote<network.mojom.DataPipeGetter>? data_pipe_getter;
   // For kChunkedDataPipe

services/viz/public/cpp/compositing/compositor_render_pass_mojom_traits.cc

@@ -7,6 +7,7 @@
 #include "base/numerics/safe_conversions.h"
 #include "components/viz/common/quads/compositor_render_pass.h"
 #include "services/viz/public/cpp/compositing/compositor_render_pass_id_mojom_traits.h"
+#include "services/viz/public/cpp/compositing/shared_quad_state_mojom_traits.h"
 #include "services/viz/public/cpp/crash_keys.h"
 #include "ui/gfx/mojom/display_color_spaces_mojom_traits.h"
 
@@ -60,11 +61,13 @@ bool StructTraits<viz::mojom::CompositorRenderPassDataView,
     // Read the SharedQuadState.
     viz::mojom::SharedQuadStateDataView sqs_data_view;
     quad_data_view.GetSqsDataView(&sqs_data_view);
-    // If there is no seralized SharedQuadState then used the last deseriaized
+    // If there is no serialized SharedQuadState then use the last deserialized
     // one.
     if (!sqs_data_view.is_null()) {
+      using SqsTraits = StructTraits<viz::mojom::SharedQuadStateDataView,
+                                     viz::SharedQuadState>;
       last_sqs = (*out)->CreateAndAppendSharedQuadState();
-      if (!quad_data_view.ReadSqs(last_sqs))
+      if (!SqsTraits::Read(sqs_data_view, last_sqs))
         return false;
     }
     quad->shared_quad_state = last_sqs;

services/viz/public/cpp/compositing/filter_operation_mojom_traits.h

@@ -197,13 +197,14 @@ struct StructTraits<viz::mojom::FilterOperationDataView, cc::FilterOperation> {
         return true;
       }
       case cc::FilterOperation::COLOR_MATRIX: {
-        // TODO(fsamuel): It would be nice to modify cc::FilterOperation to
-        // avoid this extra copy.
-        cc::FilterOperation::Matrix matrix_buffer = {};
-        base::span<float> matrix(matrix_buffer);
-        if (!data.ReadMatrix(&matrix))
-          return false;
-        out->set_matrix(matrix_buffer);
+        mojo::ArrayDataView<float> matrix;
+        data.GetMatrixDataView(&matrix);
+        if (!matrix.is_null()) {
+          // Guaranteed by prior validation of the FilterOperation struct
+          // because this array specifies a fixed size in the mojom.
+          DCHECK_EQ(matrix.size(), 20u);
+          out->set_matrix(base::make_span<20>(matrix));
+        }
         return true;
       }
       case cc::FilterOperation::ZOOM: {

third_party/blink/common/fetch/fetch_api_request_body_mojom_traits.cc

@@ -26,11 +26,13 @@ bool StructTraits<
     blink::mojom::FetchAPIDataElementDataView,
     network::DataElement>::Read(blink::mojom::FetchAPIDataElementDataView data,
                                 network::DataElement* out) {
+  base::Optional<std::string> blob_uuid;
   if (!data.ReadPath(&out->path_) || !data.ReadFile(&out->file_) ||
-      !data.ReadBlobUuid(&out->blob_uuid_) ||
+      !data.ReadBlobUuid(&blob_uuid) ||
       !data.ReadExpectedModificationTime(&out->expected_modification_time_)) {
     return false;
   }
+  out->blob_uuid_ = std::move(blob_uuid).value_or(std::string());
   if (data.type() == network::mojom::DataElementType::kBytes) {
     if (!data.ReadBuf(&out->buf_))
       return false;

third_party/blink/common/manifest/manifest_mojom_traits.cc

@@ -162,8 +162,10 @@ bool StructTraits<blink::mojom::ManifestRelatedApplicationDataView,
     return false;
   out->platform = std::move(string.string);
 
-  if (!data.ReadUrl(&out->url))
+  base::Optional<GURL> url;
+  if (!data.ReadUrl(&url))
     return false;
+  out->url = std::move(url).value_or(GURL());
 
   if (!data.ReadId(&string))
     return false;

third_party/blink/common/notifications/notification_mojom_traits.cc

@@ -101,10 +101,11 @@ bool StructTraits<blink::mojom::NotificationDataDataView,
   // platform_notification_data.data once it stores a vector of ints not chars.
   std::vector<uint8_t> data;
 
+  base::Optional<std::string> lang;
   if (!notification_data.ReadTitle(&platform_notification_data->title) ||
       !notification_data.ReadDirection(
           &platform_notification_data->direction) ||
-      !notification_data.ReadLang(&platform_notification_data->lang) ||
+      !notification_data.ReadLang(&lang) ||
       !notification_data.ReadBody(&platform_notification_data->body) ||
       !notification_data.ReadTag(&platform_notification_data->tag) ||
       !notification_data.ReadImage(&platform_notification_data->image) ||
@@ -119,6 +120,8 @@ bool StructTraits<blink::mojom::NotificationDataDataView,
     return false;
   }
 
+  platform_notification_data->lang = std::move(lang).value_or(std::string());
+
   platform_notification_data->data.assign(data.begin(), data.end());
 
   platform_notification_data->timestamp =

third_party/blink/public/mojom/manifest/manifest.mojom

@@ -186,7 +186,7 @@ struct ManifestShareTarget {
 struct ManifestFileHandler {
   // The URL that will be opened when the file handler is invoked.
   url.mojom.Url action;
-  mojo_base.mojom.String16? name;
+  mojo_base.mojom.String16 name;
   // A mapping of a MIME types to a corresponding list of file extensions.
   map<mojo_base.mojom.String16, array<mojo_base.mojom.String16>> accept;
 };

third_party/blink/renderer/modules/manifest/manifest_parser.cc

@@ -152,7 +152,7 @@ base::Optional<String> ManifestParser::ParseString(const JSONObject* object,
     return base::nullopt;
 
   String value;
-  if (!json_value->AsString(&value)) {
+  if (!json_value->AsString(&value) || value.IsNull()) {
     AddErrorInfo("property '" + key + "' ignored, type " + "string expected.");
     return base::nullopt;
   }

ui/events/mojom/event_mojom_traits.cc

@@ -477,11 +477,11 @@ bool StructTraits<ui::mojom::EventDataView, EventUniquePtr>::Read(
   if (!event.ReadLatency((*out)->latency()))
     return false;
 
-  ui::Event::Properties properties;
+  base::Optional<ui::Event::Properties> properties;
   if (!event.ReadProperties(&properties))
     return false;
-  if (!properties.empty())
-    (*out)->SetProperties(properties);
+  if (properties && !properties->empty())
+    (*out)->SetProperties(std::move(*properties));
 
   return true;
 }