Clipboard: Add more detailed comments to ClipboardWriter.

Bug: 1145787
Change-Id: I8d4b1edfbdf2dc739d2b545cb828c35b62a5cfd5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2520321
Auto-Submit: Darwin Huang <huangdarwin@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828101}

third_party/blink/renderer/modules/clipboard/clipboard_reader.h

@@ -20,8 +20,10 @@ class ClipboardPromise;
 // Reading a type from the system clipboard to a Blob is accomplished by:
 // (1) Reading - the format from the system clipboard.
 // (2) Encoding - the system clipboard's contents for a format. Encoding may be
-//     time-consuming, and so is done on a background thread whenever possible.
-// (3) Writing - the decoded contents to a Blob.
+//     time-consuming, so it is done on a background thread whenever possible.
+//     An example where encoding is done on the main thread is HTML, where
+//     Blink's HTML encoder can only be used on the main thread.
+// (3) Writing - the encoded contents to a Blob.
 //
 // ClipboardReader takes as input a ClipboardPromise, from which it expects
 // a clipboard format, and to which it provides a Blob containing an encoded
@@ -59,9 +61,12 @@ class ClipboardReader : public GarbageCollected<ClipboardReader> {
   virtual void NextRead(Vector<uint8_t> utf8_bytes) = 0;
 
   SystemClipboard* system_clipboard() { return system_clipboard_; }
-  // This ClipboardPromise owns this ClipboardReader.
+  // This ClipboardPromise owns this ClipboardReader. Subclasses use `promise_`
+  // to report the Blob output, or to obtain the execution context.
   Member<ClipboardPromise> promise_;
 
+  // Every subclass method that runs on the main thread should
+  // DCHECK_CALLED_ON_VALID_SEQUENCE with this checker.
   SEQUENCE_CHECKER(sequence_checker_);
 
  private:

third_party/blink/renderer/modules/clipboard/clipboard_writer.cc

@@ -28,7 +28,7 @@ namespace blink {
 
 namespace {  // anonymous namespace for ClipboardWriter's derived classes.
 
-// Writes a blob with image/png content to the System Clipboard.
+// Writes a Blob with image/png content to the System Clipboard.
 class ClipboardImageWriter final : public ClipboardWriter {
  public:
   ClipboardImageWriter(SystemClipboard* system_clipboard,
@@ -59,7 +59,7 @@ class ClipboardImageWriter final : public ClipboardWriter {
         true, ImageDecoder::kAlphaPremultiplied, ImageDecoder::kDefaultBitDepth,
         ColorBehavior::Tag());
     sk_sp<SkImage> image = nullptr;
-    // |decoder| is nullptr if |png_data| doesn't begin with the PNG signature.
+    // `decoder` is nullptr if `png_data` doesn't begin with the PNG signature.
     if (decoder)
       image = ImageBitmap::GetSkImageFromDecoder(std::move(decoder));
 
@@ -81,7 +81,7 @@ class ClipboardImageWriter final : public ClipboardWriter {
   }
 };
 
-// Writes a blob with text/plain content to the System Clipboard.
+// Writes a Blob with text/plain content to the System Clipboard.
 class ClipboardTextWriter final : public ClipboardWriter {
  public:
   ClipboardTextWriter(SystemClipboard* system_clipboard,
@@ -173,8 +173,6 @@ class ClipboardSvgWriter final : public ClipboardWriter {
   ~ClipboardSvgWriter() override = default;
 
  private:
-  // This must be called on the main thread because XML DOM nodes can
-  // only be used on the main thread
   void StartWrite(
       DOMArrayBuffer* svg_data,
       scoped_refptr<base::SingleThreadTaskRunner> task_runner) override {
@@ -184,7 +182,8 @@ class ClipboardSvgWriter final : public ClipboardWriter {
         String::FromUTF8(reinterpret_cast<const LChar*>(svg_data->Data()),
                          svg_data->ByteLength());
 
-    // Now sanitize the SVG string.
+    // Sanitizing on the main thread because SVG/XML DOM nodes can only be used
+    // on the main thread.
     KURL url;
     unsigned fragment_start = 0;
     unsigned fragment_end = svg_string.length();
@@ -204,7 +203,7 @@ class ClipboardSvgWriter final : public ClipboardWriter {
   }
 };
 
-// Writes a blob with arbitrary, unsanitized content to the System Clipboard.
+// Writes a Blob with arbitrary, unsanitized content to the System Clipboard.
 class ClipboardRawDataWriter final : public ClipboardWriter {
  public:
   ClipboardRawDataWriter(RawSystemClipboard* raw_system_clipboard,

third_party/blink/renderer/modules/clipboard/clipboard_writer.h

@@ -19,15 +19,33 @@ class FileReaderLoader;
 class SystemClipboard;
 class RawSystemClipboard;
 
-// Interface for writing async-clipboard-compatible types as a Blob to the
-// System Clipboard, asynchronously.
+// Interface for writing an individual Clipboard API format as a Blob to the
+// System Clipboard, safely and asynchronously.
+//
+// ClipboardWriter takes as input a ClipboardPromise, which manages writing
+// multiple formats and passes in unsanitized clipboard payloads.
+// ClipboardWriter then sanitizes a Blob payload and writes it onto the
+// underlying system clipboard. All System Clipboard operations should be
+// called from the main thread.
 //
 // Writing a Blob's data to the system clipboard is accomplished by:
-// (1) Reading the blob's contents using a FileReaderLoader.
-// (2) Decoding the blob's contents to avoid RCE in native applications that may
-//     take advantage of vulnerabilities in their decoders. In
-//     ClipboardRawDataWriter, this decoding is skipped.
-// (3) Writing the blob's decoded contents to the system clipboard.
+// (1) Reading - the Blob's contents using a FileReaderLoader.
+// (2) Decoding - or sanitizing the Blob's contents to avoid RCE in native
+//     applications that may take advantage of vulnerabilities in their
+//     decoders, whenever possible. Decoding may be time-consuming, so it
+//     is done on a background thread whenever possible. An example where
+//     decoding is done on the main thread is HTML, where Blink's HTML decoder
+//     can only be used on the main thread.
+// (3) Writing - the Blob's decoded contents to the system clipboard.
+//
+// Subclasses of ClipboardWriter should be implemented for each supported
+// format. Subclasses should:
+// (1) Begin execution by implementing ClipboardWriter::StartWrite().
+// (2) Decode the payload on a background thread (if possible) by implementing
+//     a static DecodeOnBackgroundThread() function. This function is called by
+//     StartWrite() via worker_pool::PostTask().
+// (3) Write the decoded content to the system clipboard by implementing
+//     ClipboardWriter::Write();
 //
 // ClipboardWriter is owned only by itself and ClipboardPromise. It keeps
 // itself alive for the duration of FileReaderLoader's async operations using
@@ -36,20 +54,23 @@ class RawSystemClipboard;
 class ClipboardWriter : public GarbageCollected<ClipboardWriter>,
                         public FileReaderLoaderClient {
  public:
+  // For writing sanitized MIME types.
   static ClipboardWriter* Create(SystemClipboard* system_clipboard,
                                  const String& mime_type,
                                  ClipboardPromise* promise);
+  // For writing unsanitized types.
   static ClipboardWriter* Create(RawSystemClipboard* raw_system_clipboard,
                                  const String& mime_type,
                                  ClipboardPromise* promise);
-  static ClipboardWriter* Create(SystemClipboard* system_clipboard,
-                                 const String& mime_type,
-                                 ClipboardPromise* promise,
-                                 LocalFrame* frame);
   ~ClipboardWriter() override;
 
-  static bool IsValidType(const String& type, bool is_raw);
-  void WriteToSystem(Blob*);
+  // Returns whether ClipboardWriter has implemented support for this type.
+  //
+  // If this API returns false for a `mime_type`, Create() must not be called
+  // with that `mime_type`.
+  static bool IsValidType(const String& mime_type, bool is_raw);
+  // Begins the sequence of writing the Blob to the system clipbaord.
+  void WriteToSystem(Blob* blob);
 
   // FileReaderLoaderClient.
   void DidStartLoading() override;
@@ -69,15 +90,18 @@ class ClipboardWriter : public GarbageCollected<ClipboardWriter>,
   virtual void StartWrite(
       DOMArrayBuffer* raw_data,
       scoped_refptr<base::SingleThreadTaskRunner> task_runner) = 0;
-  // This ClipboardPromise owns this ClipboardWriter.
-  Member<ClipboardPromise> promise_;
-
-  // Ensure that System Clipboard operations occur on the main thread.
-  SEQUENCE_CHECKER(sequence_checker_);
 
   SystemClipboard* system_clipboard() { return system_clipboard_; }
   RawSystemClipboard* raw_system_clipboard() { return raw_system_clipboard_; }
 
+  // This ClipboardPromise owns this ClipboardWriter. Subclasses use `promise_`
+  // to report success or failure, or to obtain the execution context.
+  Member<ClipboardPromise> promise_;
+
+  // Every subclass method that runs on the main thread should
+  // DCHECK_CALLED_ON_VALID_SEQUENCE with this checker.
+  SEQUENCE_CHECKER(sequence_checker_);
+
  private:
   ClipboardWriter(SystemClipboard* system_clipboard,
                   RawSystemClipboard* raw_system_clipboard,