Modernize base::Value usage in IPC message utils.

Prefer using value-based instead of unique_ptr-based APIs where it is
not too intrusive. Use the new representation to more efficiently
translate list and dictionary values (per the documented
recommendations). Replace CreateWithCopiedBuffer with construction from
a span.

base::PickleIterator was augmented with a span-of-bytes ReadData API, to
make this a little cleaner, since this is now preferred over a pair of a
const char* and length.

Bug: 646113
Change-Id: I6361bfffe65e2d84aa8d50390648bb7ac194f205
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2241936
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#777516}

base/pickle.cc

@@ -199,6 +199,17 @@ bool PickleIterator::ReadData(const char** data, int* length) {
   return ReadBytes(data, *length);
 }
 
+bool PickleIterator::ReadData(base::span<const uint8_t>* data) {
+  const char* ptr;
+  int length;
+
+  if (!ReadData(&ptr, &length))
+    return false;
+
+  *data = base::as_bytes(base::make_span(ptr, length));
+  return true;
+}
+
 bool PickleIterator::ReadBytes(const char** data, int length) {
   const char* read_from = GetReadPointerAndAdvance(length);
   if (!read_from)

base/pickle.h

@@ -12,6 +12,7 @@
 
 #include "base/base_export.h"
 #include "base/compiler_specific.h"
+#include "base/containers/span.h"
 #include "base/gtest_prod_util.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
@@ -56,6 +57,9 @@ class BASE_EXPORT PickleIterator {
   // until the message data is mutated). Do not keep the pointer around!
   bool ReadData(const char** data, int* length) WARN_UNUSED_RESULT;
 
+  // Similar, but using base::span for convenience.
+  bool ReadData(base::span<const uint8_t>* data) WARN_UNUSED_RESULT;
+
   // A pointer to the data will be placed in |*data|. The caller specifies the
   // number of bytes to read, and ReadBytes will validate this length. The
   // pointer placed into |*data| points into the message's buffer so it will be

ipc/ipc_message_utils.cc

@@ -79,7 +79,7 @@ void LogBytes(const std::vector<CharType>& data, std::string* out) {
 
 bool ReadValue(const base::Pickle* m,
                base::PickleIterator* iter,
-               std::unique_ptr<base::Value>* value,
+               base::Value* value,
                int recursion);
 
 void WriteValue(base::Pickle* m, const base::Value* value, int recursion) {
@@ -166,15 +166,16 @@ bool ReadDictionaryValue(const base::Pickle* m,
   if (!ReadParam(m, iter, &size))
     return false;
 
-  for (int i = 0; i < size; ++i) {
-    std::string key;
-    std::unique_ptr<base::Value> subval;
-    if (!ReadParam(m, iter, &key) ||
-        !ReadValue(m, iter, &subval, recursion + 1))
+  std::vector<std::pair<std::string, std::unique_ptr<base::Value>>> entries;
+  entries.resize(size);
+  for (auto& entry : entries) {
+    entry.second = std::make_unique<base::Value>();
+    if (!ReadParam(m, iter, &entry.first) ||
+        !ReadValue(m, iter, entry.second.get(), recursion + 1))
       return false;
-    value->SetWithoutPathExpansion(key, std::move(subval));
   }
 
+  *value = base::DictionaryValue(base::Value::DictStorage(std::move(entries)));
   return true;
 }
 
@@ -188,19 +189,19 @@ bool ReadListValue(const base::Pickle* m,
   if (!ReadParam(m, iter, &size))
     return false;
 
-  for (int i = 0; i < size; ++i) {
-    std::unique_ptr<base::Value> subval;
+  base::Value::ListStorage list_storage;
+  list_storage.resize(size);
+  for (base::Value& subval : list_storage) {
     if (!ReadValue(m, iter, &subval, recursion + 1))
       return false;
-    value->Set(i, std::move(subval));
   }
-
+  *value = base::ListValue(std::move(list_storage));
   return true;
 }
 
 bool ReadValue(const base::Pickle* m,
                base::PickleIterator* iter,
-               std::unique_ptr<base::Value>* value,
+               base::Value* value,
                int recursion) {
   if (recursion > kMaxRecursionDepth) {
     LOG(ERROR) << "Max recursion depth hit in ReadValue.";
@@ -213,56 +214,55 @@ bool ReadValue(const base::Pickle* m,
 
   switch (static_cast<base::Value::Type>(type)) {
     case base::Value::Type::NONE:
-      *value = std::make_unique<base::Value>();
+      *value = base::Value();
       break;
     case base::Value::Type::BOOLEAN: {
       bool val;
       if (!ReadParam(m, iter, &val))
         return false;
-      *value = std::make_unique<base::Value>(val);
+      *value = base::Value(val);
       break;
     }
     case base::Value::Type::INTEGER: {
       int val;
       if (!ReadParam(m, iter, &val))
         return false;
-      *value = std::make_unique<base::Value>(val);
+      *value = base::Value(val);
       break;
     }
     case base::Value::Type::DOUBLE: {
       double val;
       if (!ReadParam(m, iter, &val))
         return false;
-      *value = std::make_unique<base::Value>(val);
+      *value = base::Value(val);
       break;
     }
     case base::Value::Type::STRING: {
       std::string val;
       if (!ReadParam(m, iter, &val))
         return false;
-      *value = std::make_unique<base::Value>(std::move(val));
+      *value = base::Value(std::move(val));
       break;
     }
     case base::Value::Type::BINARY: {
-      const char* data;
-      int length;
-      if (!iter->ReadData(&data, &length))
+      base::span<const uint8_t> data;
+      if (!iter->ReadData(&data))
         return false;
-      *value = base::Value::CreateWithCopiedBuffer(data, length);
+      *value = base::Value(data);
       break;
     }
     case base::Value::Type::DICTIONARY: {
       base::DictionaryValue val;
       if (!ReadDictionaryValue(m, iter, &val, recursion))
         return false;
-      *value = std::make_unique<base::Value>(std::move(val));
+      *value = std::move(val);
       break;
     }
     case base::Value::Type::LIST: {
       base::ListValue val;
       if (!ReadListValue(m, iter, &val, recursion))
         return false;
-      *value = std::make_unique<base::Value>(std::move(val));
+      *value = std::move(val);
       break;
     }
     default: