refactor: make node Buffers more friendly to base::span / std::span (#46778)

* refactor: add electron::Buffer namespace; move the Buffer as_byte_span() into it

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* feat: add electron::Buffer::Copy()

a span-friendly version of node::Buffer::Copy()

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* refactor: use electron::Buffer::Copy() in electron_api_base_window.cc

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* refactor: use electron::Buffer::Copy() in electron_api_data_pipe_holder.cc

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* refactor: use electron::Buffer::Copy() in electron_api_safe_storage.cc

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* refactor: use electron::Buffer::Copy() in electron_api_clipboard.cc

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* refactor: use electron::Buffer::Copy() in osr_converter.cc

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* refactor: use electron::Buffer::Copy() in electron_api_native_image.cc

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* refactor: use electron::Buffer::Copy() in net_converter.cc

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* refactor: use electron::Buffer::Copy() in electron_api_web_contents.cc

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* refactor: make NewEmptyBuffer() return a Local<Value>

Co-authored-by: Charles Kerr <charles@charleskerr.com>

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Charles Kerr <charles@charleskerr.com>

shell/browser/api/electron_api_base_window.cc

@@ -31,6 +31,7 @@
 #include "shell/common/gin_helper/object_template_builder.h"
 #include "shell/common/gin_helper/persistent_dictionary.h"
 #include "shell/common/node_includes.h"
+#include "shell/common/node_util.h"
 #include "shell/common/options_switches.h"
 
 #if defined(TOOLKIT_VIEWS)
@@ -72,8 +73,9 @@ namespace {
 
 #if !BUILDFLAG(IS_MAC)
 // Converts binary data to Buffer.
-v8::Local<v8::Value> ToBuffer(v8::Isolate* isolate, void* val, int size) {
+v8::Local<v8::Value> ToBuffer(v8::Isolate* isolate,
-  auto buffer = node::Buffer::Copy(isolate, static_cast<char*>(val), size);
+                              const base::span<const uint8_t> val) {
+  auto buffer = electron::Buffer::Copy(isolate, val);
   if (buffer.IsEmpty())
     return v8::Null(isolate);
   else
@@ -348,8 +350,8 @@ void BaseWindow::OnWindowMessage(UINT message, WPARAM w_param, LPARAM l_param) {
     v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
     v8::HandleScope scope(isolate);
     messages_callback_map_[message].Run(
-        ToBuffer(isolate, static_cast<void*>(&w_param), sizeof(WPARAM)),
+        ToBuffer(isolate, base::byte_span_from_ref(w_param)),
-        ToBuffer(isolate, static_cast<void*>(&l_param), sizeof(LPARAM)));
+        ToBuffer(isolate, base::byte_span_from_ref(l_param)));
   }
 }
 #endif
@@ -786,7 +788,7 @@ v8::Local<v8::Value> BaseWindow::GetNativeWindowHandle() {
   // https://chromium-review.googlesource.com/c/chromium/src/+/1253094/ has
   // landed
   NativeWindowHandle handle = window_->GetNativeWindowHandle();
-  return ToBuffer(isolate(), &handle, sizeof(handle));
+  return ToBuffer(isolate(), base::byte_span_from_ref(handle));
 }
 #endif
 

shell/browser/api/electron_api_data_pipe_holder.cc

@@ -17,6 +17,7 @@
 #include "net/base/net_errors.h"
 #include "shell/common/gin_helper/promise.h"
 #include "shell/common/key_weak_map.h"
+#include "shell/common/node_util.h"
 
 #include "shell/common/node_includes.h"
 
@@ -114,8 +115,7 @@ class DataPipeReader {
     // inside the sandbox
     v8::HandleScope handle_scope(promise_.isolate());
     v8::Local<v8::Value> buffer =
-        node::Buffer::Copy(promise_.isolate(), &buffer_.front(), buffer_.size())
+        electron::Buffer::Copy(promise_.isolate(), buffer_).ToLocalChecked();
-            .ToLocalChecked();
     promise_.Resolve(buffer);
 
     // Destroy data pipe.

shell/browser/api/electron_api_safe_storage.cc

@@ -11,6 +11,7 @@
 #include "shell/common/gin_converters/callback_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/node_includes.h"
+#include "shell/common/node_util.h"
 
 namespace {
 
@@ -72,8 +73,7 @@ v8::Local<v8::Value> EncryptString(v8::Isolate* isolate,
     return {};
   }
 
-  return node::Buffer::Copy(isolate, ciphertext.c_str(), ciphertext.size())
+  return electron::Buffer::Copy(isolate, ciphertext).ToLocalChecked();
-      .ToLocalChecked();
 }
 
 std::string DecryptString(v8::Isolate* isolate, v8::Local<v8::Value> buffer) {

shell/browser/api/electron_api_web_contents.cc

@@ -2979,9 +2979,7 @@ void OnPDFCreated(gin_helper::Promise<v8::Local<v8::Value>> promise,
       v8::Local<v8::Context>::New(isolate, promise.GetContext()));
 
   v8::Local<v8::Value> buffer =
-      node::Buffer::Copy(isolate, reinterpret_cast<const char*>(data->front()),
+      electron::Buffer::Copy(isolate, *data).ToLocalChecked();
-                         data->size())
-          .ToLocalChecked();
 
   promise.Resolve(buffer);
 }

shell/common/api/electron_api_clipboard.cc

@@ -13,6 +13,7 @@
 #include "shell/common/gin_converters/image_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/node_includes.h"
+#include "shell/common/node_util.h"
 #include "shell/common/process_util.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/clipboard/clipboard_format_type.h"
@@ -99,8 +100,7 @@ std::string Clipboard::Read(const std::string& format_string) {
 v8::Local<v8::Value> Clipboard::ReadBuffer(const std::string& format_string,
                                            gin_helper::Arguments* args) {
   std::string data = Read(format_string);
-  return node::Buffer::Copy(args->isolate(), data.data(), data.length())
+  return electron::Buffer::Copy(args->isolate(), data).ToLocalChecked();
-      .ToLocalChecked();
 }
 
 void Clipboard::WriteBuffer(const std::string& format,

shell/common/api/electron_api_native_image.cc

@@ -122,6 +122,10 @@ base::win::ScopedGDIObject<HICON> ReadICOFromPath(int size,
 }
 #endif
 
+[[nodiscard]] v8::Local<v8::Value> NewEmptyBuffer(v8::Isolate* isolate) {
+  return node::Buffer::New(isolate, 0).ToLocalChecked();
+}
+
 }  // namespace
 
 NativeImage::NativeImage(v8::Isolate* isolate, const gfx::Image& image)
@@ -226,57 +230,48 @@ HICON NativeImage::GetHICON(int size) {
 #endif
 
 v8::Local<v8::Value> NativeImage::ToPNG(gin::Arguments* args) {
+  v8::Isolate* const isolate = args->isolate();
   float scale_factor = GetScaleFactorFromOptions(args);
 
   if (scale_factor == 1.0f) {
     // Use raw 1x PNG bytes when available
-    scoped_refptr<base::RefCountedMemory> png = image_.As1xPNGBytes();
+    const scoped_refptr<base::RefCountedMemory> png = image_.As1xPNGBytes();
-    if (png->size() > 0) {
+    const base::span<const uint8_t> png_span = *png;
-      const char* data = reinterpret_cast<const char*>(png->front());
+    if (!png_span.empty())
-      size_t size = png->size();
+      return electron::Buffer::Copy(isolate, png_span).ToLocalChecked();
-      return node::Buffer::Copy(args->isolate(), data, size).ToLocalChecked();
-    }
   }
 
   const SkBitmap bitmap =
       image_.AsImageSkia().GetRepresentation(scale_factor).GetBitmap();
-  std::optional<std::vector<uint8_t>> encoded =
+  const std::optional<std::vector<uint8_t>> encoded =
       gfx::PNGCodec::EncodeBGRASkBitmap(bitmap, false);
   if (!encoded.has_value())
-    return node::Buffer::New(args->isolate(), 0).ToLocalChecked();
+    return NewEmptyBuffer(isolate);
-  const char* data = reinterpret_cast<char*>(encoded->data());
+
-  size_t size = encoded->size();
+  return electron::Buffer::Copy(isolate, *encoded).ToLocalChecked();
-  return node::Buffer::Copy(args->isolate(), data, size).ToLocalChecked();
 }
 
 v8::Local<v8::Value> NativeImage::ToBitmap(gin::Arguments* args) {
-  float scale_factor = GetScaleFactorFromOptions(args);
+  v8::Isolate* const isolate = args->isolate();
 
-  const SkBitmap bitmap =
+  const float scale = GetScaleFactorFromOptions(args);
-      image_.AsImageSkia().GetRepresentation(scale_factor).GetBitmap();
+  const auto src = image_.AsImageSkia().GetRepresentation(scale).GetBitmap();
 
-  SkImageInfo info =
+  const auto dst_info = SkImageInfo::MakeN32Premul(src.dimensions());
-      SkImageInfo::MakeN32Premul(bitmap.width(), bitmap.height());
+  const size_t dst_n_bytes = dst_info.computeMinByteSize();
+  auto dst_buf = v8::ArrayBuffer::New(isolate, dst_n_bytes);
 
-  auto array_buffer =
+  if (!src.readPixels(dst_info, dst_buf->Data(), dst_info.minRowBytes(), 0, 0))
-      v8::ArrayBuffer::New(args->isolate(), info.computeMinByteSize());
+    return NewEmptyBuffer(isolate);
-  if (bitmap.readPixels(info, array_buffer->Data(), info.minRowBytes(), 0, 0)) {
+  return node::Buffer::New(isolate, dst_buf, 0, dst_n_bytes).ToLocalChecked();
-    return node::Buffer::New(args->isolate(), array_buffer, 0,
-                             info.computeMinByteSize())
-        .ToLocalChecked();
-  }
-  return node::Buffer::New(args->isolate(), 0).ToLocalChecked();
 }
 
 v8::Local<v8::Value> NativeImage::ToJPEG(v8::Isolate* isolate, int quality) {
-  std::optional<std::vector<uint8_t>> encoded_image =
+  const std::optional<std::vector<uint8_t>> encoded_image =
       gfx::JPEG1xEncodedDataFromImage(image_, quality);
-  if (!encoded_image.has_value())
+  if (!encoded_image)
-    return node::Buffer::New(isolate, 0).ToLocalChecked();
+    return NewEmptyBuffer(isolate);
-  return node::Buffer::Copy(
+  return electron::Buffer::Copy(isolate, *encoded_image).ToLocalChecked();
-             isolate, reinterpret_cast<const char*>(&encoded_image->front()),
-             encoded_image->size())
-      .ToLocalChecked();
 }
 
 std::string NativeImage::ToDataURL(gin::Arguments* args) {
@@ -301,17 +296,17 @@ v8::Local<v8::Value> NativeImage::GetBitmap(gin::Arguments* args) {
 
 v8::Local<v8::Value> NativeImage::GetNativeHandle(
     gin_helper::ErrorThrower thrower) {
+  v8::Isolate* const isolate = thrower.isolate();
 #if BUILDFLAG(IS_MAC)
   if (IsEmpty())
-    return node::Buffer::New(thrower.isolate(), 0).ToLocalChecked();
+    return NewEmptyBuffer(isolate);
 
   NSImage* ptr = image_.AsNSImage();
-  return node::Buffer::Copy(thrower.isolate(), reinterpret_cast<char*>(ptr),
+  return electron::Buffer::Copy(isolate, base::byte_span_from_ref(ptr))
-                            sizeof(void*))
       .ToLocalChecked();
 #else
   thrower.ThrowError("Not implemented");
-  return v8::Undefined(thrower.isolate());
+  return v8::Undefined(isolate);
 #endif
 }
 
@@ -402,7 +397,7 @@ void NativeImage::AddRepresentation(const gin_helper::Dictionary& options) {
   GURL url;
   if (options.Get("buffer", &buffer) && node::Buffer::HasInstance(buffer)) {
     skia_rep_added = electron::util::AddImageSkiaRepFromBuffer(
-        &image_skia, electron::util::as_byte_span(buffer), width, height,
+        &image_skia, electron::Buffer::as_byte_span(buffer), width, height,
         scale_factor);
   } else if (options.Get("dataURL", &url)) {
     std::string mime_type, charset, data;
@@ -511,7 +506,7 @@ gin::Handle<NativeImage> NativeImage::CreateFromBitmap(
   auto info = SkImageInfo::MakeN32(width, height, kPremul_SkAlphaType);
   auto size_bytes = info.computeMinByteSize();
 
-  const auto buffer_data = electron::util::as_byte_span(buffer);
+  const auto buffer_data = electron::Buffer::as_byte_span(buffer);
   if (size_bytes != buffer_data.size()) {
     thrower.ThrowError("invalid buffer size");
     return {};
@@ -552,7 +547,7 @@ gin::Handle<NativeImage> NativeImage::CreateFromBuffer(
 
   gfx::ImageSkia image_skia;
   electron::util::AddImageSkiaRepFromBuffer(
-      &image_skia, electron::util::as_byte_span(buffer), width, height,
+      &image_skia, electron::Buffer::as_byte_span(buffer), width, height,
       scale_factor);
   return Create(args->isolate(), gfx::Image(image_skia));
 }

shell/common/gin_converters/net_converter.cc

@@ -34,6 +34,7 @@
 #include "shell/common/gin_converters/value_converter.h"
 #include "shell/common/gin_helper/promise.h"
 #include "shell/common/node_includes.h"
+#include "shell/common/node_util.h"
 #include "shell/common/v8_util.h"
 
 namespace gin {
@@ -524,11 +525,11 @@ v8::Local<v8::Value> Converter<network::ResourceRequestBody>::ToV8(
       }
       case network::mojom::DataElement::Tag::kBytes: {
         upload_data.Set("type", "rawData");
-        const auto& bytes = element.As<network::DataElementBytes>().bytes();
-        const char* data = reinterpret_cast<const char*>(bytes.data());
         upload_data.Set(
             "bytes",
-            node::Buffer::Copy(isolate, data, bytes.size()).ToLocalChecked());
+            electron::Buffer::Copy(
+                isolate, element.As<network::DataElementBytes>().bytes())
+                .ToLocalChecked());
         break;
       }
       case network::mojom::DataElement::Tag::kDataPipe: {

shell/common/gin_converters/osr_converter.cc

@@ -111,12 +111,10 @@ v8::Local<v8::Value> Converter<electron::OffscreenSharedTextureValue>::ToV8(
   dict.Set("metadata", ConvertToV8(isolate, metadata));
 
 #if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)
-  auto handle_buf = node::Buffer::Copy(
+  dict.Set("sharedTextureHandle",
-      isolate,
+           electron::Buffer::Copy(
-      reinterpret_cast<char*>(
+               isolate, base::byte_span_from_ref(val.shared_texture_handle))
-          const_cast<uintptr_t*>(&val.shared_texture_handle)),
+               .ToLocalChecked());
-      sizeof(val.shared_texture_handle));
-  dict.Set("sharedTextureHandle", handle_buf.ToLocalChecked());
 #elif BUILDFLAG(IS_LINUX)
   auto v8_planes = base::ToVector(val.planes, [isolate](const auto& plane) {
     gin::Dictionary v8_plane(isolate, v8::Object::New(isolate));

shell/common/node_util.cc

@@ -75,16 +75,6 @@ void EmitWarning(v8::Isolate* isolate,
   emit_warning.Run(warning_msg, warning_type, "");
 }
 
-// SAFETY: There is no node::Buffer API that passes the UNSAFE_BUFFER_USAGE
-// test, so let's isolate the unsafe API use into this function. Instead of
-// calling `Buffer::Data()` and `Buffer::Length()` directly, the rest of our
-// code should prefer to use spans returned by this function.
-base::span<uint8_t> as_byte_span(v8::Local<v8::Value> node_buffer) {
-  auto* data = reinterpret_cast<uint8_t*>(node::Buffer::Data(node_buffer));
-  const auto size = node::Buffer::Length(node_buffer);
-  return UNSAFE_BUFFERS(base::span{data, size});
-}
-
 node::Environment* CreateEnvironment(v8::Isolate* isolate,
                                      node::IsolateData* isolate_data,
                                      v8::Local<v8::Context> context,
@@ -133,3 +123,28 @@ node::Environment* CreateEnvironment(v8::Isolate* isolate,
 }
 
 }  // namespace electron::util
+
+namespace electron::Buffer {
+
+// SAFETY: There is no node::Buffer API that passes the UNSAFE_BUFFER_USAGE
+// test, so let's isolate the unsafe API use into this function. Instead of
+// calling `Buffer::Data()` and `Buffer::Length()` directly, the rest of our
+// code should prefer to use spans returned by this function.
+base::span<uint8_t> as_byte_span(v8::Local<v8::Value> node_buffer) {
+  auto* data = reinterpret_cast<uint8_t*>(node::Buffer::Data(node_buffer));
+  const auto size = node::Buffer::Length(node_buffer);
+  return UNSAFE_BUFFERS(base::span{data, size});
+}
+
+v8::MaybeLocal<v8::Object> Copy(v8::Isolate* isolate,
+                                const base::span<const char> data) {
+  // SAFETY: span-friendly version of node::Buffer::Copy()
+  return UNSAFE_BUFFERS(node::Buffer::Copy(isolate, data.data(), data.size()));
+}
+
+v8::MaybeLocal<v8::Object> Copy(v8::Isolate* isolate,
+                                const base::span<const uint8_t> data) {
+  return Copy(isolate, base::as_chars(data));
+}
+
+}  // namespace electron::Buffer

shell/common/node_util.h

@@ -53,11 +53,23 @@ node::Environment* CreateEnvironment(v8::Isolate* isolate,
                                      node::EnvironmentFlags::Flags env_flags,
                                      std::string_view process_type = "");
 
+}  // namespace electron::util
+
+namespace electron::Buffer {
+
 // Convenience function to view a Node buffer's data as a base::span().
 // Analogous to base::as_byte_span()
 [[nodiscard]] base::span<uint8_t> as_byte_span(
     v8::Local<v8::Value> node_buffer);
 
-}  // namespace electron::util
+// span-friendly version of node::Buffer::Copy()
+[[nodiscard]] v8::MaybeLocal<v8::Object> Copy(v8::Isolate* isolate,
+                                              base::span<const char> data);
+
+// span-friendly version of node::Buffer::Copy()
+[[nodiscard]] v8::MaybeLocal<v8::Object> Copy(v8::Isolate* isolate,
+                                              base::span<const uint8_t> data);
+
+}  // namespace electron::Buffer
 
 #endif  // ELECTRON_SHELL_COMMON_NODE_UTIL_H_