[Paint Preview] Support versioning

If a change is made to the custom data or file structure there will
currently be a failure to composite. This failure will be recorded as
a generic deserialization failure. This CL provides a version number
recorded in the proto to allow compositing failures due to version
differences to be recorded independently.

This CL also deletes the paint preview if there was an issue starting
the compositor to avoid replicating the issue.

Bug: 1122015
Change-Id: I7b63a6491b497f78ec59852f0fc50996503e9f06
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2377109
Commit-Queue: Calder Kitagawa <ckitagawa@chromium.org>
Reviewed-by: Mehran Mahmoudi <mahmoudi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801878}

components/paint_preview/browser/paint_preview_client.cc

@@ -18,6 +18,7 @@
 #include "base/unguessable_token.h"
 #include "components/paint_preview/common/capture_result.h"
 #include "components/paint_preview/common/mojom/paint_preview_recorder.mojom-forward.h"
+#include "components/paint_preview/common/version.h"
 #include "components/ukm/content/source_url_recorder.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
@@ -283,8 +284,9 @@ void PaintPreviewClient::CapturePaintPreview(
   document_data.should_clean_up_files = true;
   document_data.persistence = params.persistence;
   document_data.root_dir = params.root_dir;
-  document_data.proto.mutable_metadata()->set_url(
-      render_frame_host->GetLastCommittedURL().spec());
+  auto* metadata = document_data.proto.mutable_metadata();
+  metadata->set_url(render_frame_host->GetLastCommittedURL().spec());
+  metadata->set_version(kPaintPreviewVersion);
   document_data.callback = std::move(callback);
   document_data.source_id =
       ukm::GetSourceIdForWebContentsDocument(web_contents());

components/paint_preview/browser/paint_preview_client_unittest.cc

@@ -18,6 +18,7 @@
 #include "components/paint_preview/common/mojom/paint_preview_recorder.mojom.h"
 #include "components/paint_preview/common/proto/paint_preview.pb.h"
 #include "components/paint_preview/common/test_utils.h"
+#include "components/paint_preview/common/version.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
@@ -161,7 +162,9 @@ TEST_P(PaintPreviewClientRenderViewHostTest, CaptureMainFrameMock) {
   response->scroll_offsets = gfx::Size(5, 10);
 
   PaintPreviewProto expected_proto;
-  expected_proto.mutable_metadata()->set_url(expected_url.spec());
+  auto* metadata = expected_proto.mutable_metadata();
+  metadata->set_url(expected_url.spec());
+  metadata->set_version(kPaintPreviewVersion);
   PaintPreviewFrameProto* main_frame = expected_proto.mutable_root_frame();
   main_frame->set_is_main_frame(true);
   main_frame->set_scroll_offset_x(5);

components/paint_preview/common/BUILD.gn

@@ -25,6 +25,8 @@ if (!is_ios) {
       "serialized_recording.h",
       "subset_font.cc",
       "subset_font.h",
+      "version.cc",
+      "version.h",
     ]
 
     deps = [

components/paint_preview/common/proto/paint_preview.proto

@@ -60,10 +60,15 @@ message PaintPreviewFrameProto {
 }
 
 // Metadata for the capture.
-// NEXT_TAG = 2
+// NEXT_TAG = 3
 message MetadataProto {
   // URL of the root frame.
   required string url = 1;
+
+  // Records the version number of the recording. Should be incremented if there
+  // is a breaking change to the custorm SkPicture deserialization or storage
+  // system.
+  optional uint64 version = 2;
 }
 
 // A paint preview of the entire page.

components/paint_preview/common/version.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/paint_preview/common/version.h"
+
+namespace paint_preview {
+
+const uint32_t kPaintPreviewVersion = 1;
+
+}  // namespace paint_preview

components/paint_preview/common/version.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_PAINT_PREVIEW_COMMON_VERSION_H_
+#define COMPONENTS_PAINT_PREVIEW_COMMON_VERSION_H_
+
+#include <stdint.h>
+
+namespace paint_preview {
+
+// Version of the paint preview. Should be incremented on breaking changes to
+// the storage format or the SkPicture deserialization process.
+extern const uint32_t kPaintPreviewVersion;
+
+}  // namespace paint_preview
+
+#endif  // COMPONENTS_PAINT_PREVIEW_COMMON_VERSION_H_

components/paint_preview/player/android/javatests/paint_preview_test_service.cc

@@ -18,6 +18,7 @@
 #include "components/paint_preview/common/file_stream.h"
 #include "components/paint_preview/common/file_utils.h"
 #include "components/paint_preview/common/proto/paint_preview.pb.h"
+#include "components/paint_preview/common/version.h"
 #include "components/paint_preview/player/android/javatests_jni_headers/PaintPreviewTestService_jni.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkColor.h"
@@ -237,6 +238,7 @@ jboolean PaintPreviewTestService::SerializeFrames(
   PaintPreviewProto paint_preview;
   auto* metadata = paint_preview.mutable_metadata();
   metadata->set_url(base::android::ConvertJavaStringToUTF8(env, j_url));
+  metadata->set_version(kPaintPreviewVersion);
 
   auto skp_path =
       path.AppendASCII(base::StrCat({root_it->second.guid.ToString(), ".skp"}));

components/paint_preview/player/android/player_compositor_delegate_android.cc

@@ -102,6 +102,12 @@ void PlayerCompositorDelegateAndroid::OnCompositorReady(
     LOG(ERROR) << "Compositor process failed to begin with code: "
                << static_cast<int>(compositor_status);
     std::move(compositor_error_).Run(static_cast<int>(compositor_status));
+
+    // If there was a problem, prevent it from happening again by deleting it.
+    auto file_manager = paint_preview_service_->GetFileManager();
+    file_manager->GetTaskRunner()->PostTask(
+        FROM_HERE,
+        base::BindOnce(&FileManager::DeleteArtifactSet, file_manager, key_));
     return;
   }
   auto delta = base::TimeTicks::Now() - startup_timestamp_;

components/paint_preview/player/compositor_status.h

@@ -7,6 +7,9 @@
 
 namespace paint_preview {
 
+// IMPORTANT: if CompositorStatus is updated, please update the corresponding
+// entry for TabbedPaintPreviewCompositorFailureReason in enums.xml.
+
 // GENERATED_JAVA_ENUM_PACKAGE: (
 //   org.chromium.components.paintpreview.player)
 enum class CompositorStatus : int {
@@ -18,6 +21,8 @@ enum class CompositorStatus : int {
   COMPOSITOR_DESERIALIZATION_ERROR,
   INVALID_ROOT_FRAME_SKP,
   INVALID_REQUEST,
+  OLD_VERSION,
+  UNEXPECTED_VERSION,
   COUNT,
 };
 

components/paint_preview/player/player_compositor_delegate.cc

@@ -26,6 +26,7 @@
 #include "components/paint_preview/common/proto/paint_preview.pb.h"
 #include "components/paint_preview/common/recording_map.h"
 #include "components/paint_preview/common/serialized_recording.h"
+#include "components/paint_preview/common/version.h"
 #include "components/paint_preview/public/paint_preview_compositor_client.h"
 #include "components/paint_preview/public/paint_preview_compositor_service.h"
 #include "components/services/paint_preview_compositor/public/mojom/paint_preview_compositor.mojom.h"
@@ -192,6 +193,23 @@ void PlayerCompositorDelegate::OnProtoAvailable(
     return;
   }
 
+  const uint32_t version = proto->metadata().version();
+  if (version < kPaintPreviewVersion) {
+    // If the version is old there was a breaking change to either;
+    // - The SkPicture encoding format
+    // - The storage structure
+    // In either case, the new code is likely unable to deserialize the result
+    // so we should early abort.
+    OnCompositorReady(CompositorStatus::OLD_VERSION, nullptr);
+    return;
+  } else if (version > kPaintPreviewVersion) {
+    // This shouldn't happen hence NOTREACHED(). However, in release we should
+    // treat this as a new failure type to catch any possible regressions.
+    OnCompositorReady(CompositorStatus::UNEXPECTED_VERSION, nullptr);
+    NOTREACHED();
+    return;
+  }
+
   auto proto_url = GURL(proto->metadata().url());
   if (expected_url != proto_url) {
     OnCompositorReady(CompositorStatus::URL_MISMATCH, nullptr);

components/paint_preview/player/player_compositor_delegate.h

@@ -61,6 +61,8 @@ class PlayerCompositorDelegate {
 
  protected:
   base::OnceCallback<void(int)> compositor_error_;
+  PaintPreviewBaseService* paint_preview_service_;
+  DirectoryKey key_;
 
  private:
   void OnCompositorReadyStatusAdapter(
@@ -78,8 +80,6 @@ class PlayerCompositorDelegate {
   void SendCompositeRequest(
       mojom::PaintPreviewBeginCompositeRequestPtr begin_composite_request);
 
-  PaintPreviewBaseService* paint_preview_service_;
-  DirectoryKey key_;
   bool compress_on_close_;
   std::unique_ptr<PaintPreviewCompositorService, base::OnTaskRunnerDeleter>
       paint_preview_compositor_service_;

components/paint_preview/player/player_compositor_delegate_unittest.cc

@@ -18,6 +18,7 @@
 #include "components/paint_preview/browser/file_manager.h"
 #include "components/paint_preview/browser/paint_preview_base_service.h"
 #include "components/paint_preview/common/proto/paint_preview.pb.h"
+#include "components/paint_preview/common/version.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gfx/geometry/rect.h"
 #include "url/gurl.h"
@@ -35,7 +36,9 @@ TEST(PlayerCompositorDelegate, OnClick) {
 
   GURL url("www.example.com");
   PaintPreviewProto proto;
-  proto.mutable_metadata()->set_url(url.spec());
+  auto* metadata = proto.mutable_metadata();
+  metadata->set_url(url.spec());
+  metadata->set_version(kPaintPreviewVersion);
 
   GURL root_frame_link("www.chromium.org");
   auto root_frame_id = base::UnguessableToken::Create();

tools/metrics/histograms/enums.xml

@@ -69358,6 +69358,8 @@ would be helpful to identify which type is being sent.
   <int value="5" label="Compositor Deserialization Error"/>
   <int value="6" label="Invalid Root Frame SKP"/>
   <int value="7" label="Invalid Request"/>
+  <int value="8" label="Old Version"/>
+  <int value="9" label="Unexpected Version"/>
 </enum>
 
 <enum name="TabbedPaintPreviewExitCause">