Added MHTML loading status metric

This change causes results of attempts to load MHTML archives to be
logged with a UMA histogram, providing some previously missing insight into the
performance of MHTML loading.

Bug: 901025
Change-Id: If82fda621fcf8d4256edc20a0da9172d0cde2455
Reviewed-on: https://chromium-review.googlesource.com/c/1313669Reviewed-by: Philip Rogers <pdr@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Commit-Queue: Ian Wells <iwells@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609062}

third_party/blink/renderer/core/frame/mhtml_archive_test.cc

@@ -30,6 +30,7 @@
 
 #include <map>
 
+#include "base/test/metrics/histogram_tester.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/renderer/platform/date_components.h"
@@ -45,6 +46,7 @@
 #include "third_party/blink/renderer/platform/wtf/time.h"
 
 using blink::url_test_helpers::ToKURL;
+using LoadResult = blink::MHTMLArchive::LoadResult;
 
 namespace blink {
 namespace test {
@@ -144,7 +146,8 @@ class MHTMLArchiveTest : public testing::Test {
                          MHTMLArchive::EncodingPolicy encoding_policy,
                          const KURL& url,
                          const String& title,
-                         const String& mime_type) {
+                         const String& mime_type,
+                         bool validate) {
     // This boundary is as good as any other.  Plus it gets used in almost
     // all the examples in the MHTML spec - RFC 2557.
     String boundary = String::FromUTF8("boundary-example");
@@ -157,18 +160,21 @@ class MHTMLArchiveTest : public testing::Test {
     }
     MHTMLArchive::GenerateMHTMLFooterForTesting(boundary, mhtml_data_);
 
-    // Validate the generated MHTML.
-    MHTMLParser parser(
-        SharedBuffer::Create(mhtml_data_.data(), mhtml_data_.size()));
-    EXPECT_FALSE(parser.ParseArchive().IsEmpty())
-        << "Generated MHTML is malformed";
+    if (validate) {
+      // Validate the generated MHTML.
+      MHTMLParser parser(
+          SharedBuffer::Create(mhtml_data_.data(), mhtml_data_.size()));
+      EXPECT_FALSE(parser.ParseArchive().IsEmpty())
+          << "Generated MHTML is malformed";
+    }
   }
 
   void Serialize(const KURL& url,
                  const String& title,
                  const String& mime,
                  MHTMLArchive::EncodingPolicy encoding_policy) {
-    return GenerateMHTMLData(resources_, encoding_policy, url, title, mime);
+    return GenerateMHTMLData(resources_, encoding_policy, url, title, mime,
+                             true);
   }
 
   Vector<char>& mhtml_data() { return mhtml_data_; }
@@ -176,6 +182,24 @@ class MHTMLArchiveTest : public testing::Test {
   WTF::Time mhtml_date() const { return mhtml_date_; }
   const String& mhtml_date_header() const { return mhtml_date_header_; }
 
+  void CheckLoadResult(const KURL url,
+                       scoped_refptr<const SharedBuffer> data,
+                       LoadResult expected_result) {
+    // Set up histogram testing (takes snapshot of histogram data).
+    base::HistogramTester histogram_tester;
+
+    // Attempt loading the archive and check the returned pointer.
+    MHTMLArchive* archive = MHTMLArchive::Create(url, data);
+    if (expected_result == LoadResult::kSuccess)
+      EXPECT_NE(nullptr, archive);
+    else
+      EXPECT_EQ(nullptr, archive);
+
+    // Check that the correct count, and only the correct count, increased.
+    histogram_tester.ExpectUniqueSample(MHTMLArchive::kLoadResultUmaName,
+                                        expected_result, 1);
+  }
+
  private:
   scoped_refptr<SharedBuffer> ReadFile(const char* file_name) {
     String file_path = file_path_ + file_name;
@@ -339,16 +363,19 @@ TEST_F(MHTMLArchiveTest, MHTMLFromScheme) {
 
   // MHTMLArchives can only be initialized from local schemes, http/https
   // schemes, and content scheme(Android specific).
-  EXPECT_NE(nullptr, MHTMLArchive::Create(http_url, data.get()));
+  CheckLoadResult(http_url, data.get(), LoadResult::kSuccess);
+
 #if defined(OS_ANDROID)
-  EXPECT_NE(nullptr, MHTMLArchive::Create(content_url, data.get()));
+  CheckLoadResult(content_url, data.get(), LoadResult::kSuccess);
 #else
-  EXPECT_EQ(nullptr, MHTMLArchive::Create(content_url, data.get()));
+  CheckLoadResult(content_url, data.get(), LoadResult::kUrlSchemeNotAllowed);
 #endif
-  EXPECT_NE(nullptr, MHTMLArchive::Create(file_url, data.get()));
-  EXPECT_EQ(nullptr, MHTMLArchive::Create(special_scheme_url, data.get()));
+  CheckLoadResult(file_url, data.get(), LoadResult::kSuccess);
+  CheckLoadResult(special_scheme_url, data.get(),
+                  LoadResult::kUrlSchemeNotAllowed);
+
   SchemeRegistry::RegisterURLSchemeAsLocal("fooscheme");
-  EXPECT_NE(nullptr, MHTMLArchive::Create(special_scheme_url, data.get()));
+  CheckLoadResult(special_scheme_url, data.get(), LoadResult::kSuccess);
 }
 
 TEST_F(MHTMLArchiveTest, MHTMLDate) {
@@ -373,12 +400,15 @@ TEST_F(MHTMLArchiveTest, MHTMLDate) {
 }
 
 TEST_F(MHTMLArchiveTest, EmptyArchive) {
-  char* buf = nullptr;
+  // Test failure to load when |data| is null.
+  KURL http_url = ToKURL("http://www.example.com");
+  CheckLoadResult(http_url, nullptr, LoadResult::kEmptyFile);
+
+  // Test failure to load when |data| is non-null but empty.
+  const char* buf = "";
   scoped_refptr<SharedBuffer> data =
       SharedBuffer::Create(buf, static_cast<size_t>(0u));
-  KURL http_url = ToKURL("http://www.example.com");
-  MHTMLArchive* archive = MHTMLArchive::Create(http_url, data.get());
-  EXPECT_EQ(nullptr, archive);
+  CheckLoadResult(http_url, data.get(), LoadResult::kEmptyFile);
 }
 
 TEST_F(MHTMLArchiveTest, NoMainResource) {
@@ -393,9 +423,21 @@ TEST_F(MHTMLArchiveTest, NoMainResource) {
   scoped_refptr<SharedBuffer> data =
       SharedBuffer::Create(mhtml_data().data(), mhtml_data().size());
   KURL http_url = ToKURL("http://www.example.com");
-  MHTMLArchive* archive = MHTMLArchive::Create(http_url, data.get());
 
-  EXPECT_EQ(nullptr, archive);
+  CheckLoadResult(http_url, data.get(), LoadResult::kMissingMainResource);
+}
+
+TEST_F(MHTMLArchiveTest, InvalidMHTML) {
+  const char kURL[] = "http://www.example.com";
+  // Intentionally create MHTML data with no resources.
+  Vector<SerializedResource> resources;
+  GenerateMHTMLData(resources, MHTMLArchive::kUseDefaultEncoding, ToKURL(kURL),
+                    "Test invalid mhtml", "text/html", false);
+
+  scoped_refptr<SharedBuffer> data =
+      SharedBuffer::Create(mhtml_data().data(), mhtml_data().size());
+
+  CheckLoadResult(ToKURL(kURL), data.get(), LoadResult::kInvalidArchive);
 }
 
 }  // namespace test

third_party/blink/renderer/platform/mhtml/mhtml_archive.cc

@@ -31,6 +31,7 @@
 #include "third_party/blink/renderer/platform/mhtml/mhtml_archive.h"
 
 #include <stddef.h>
+#include "base/metrics/histogram_macros.h"
 #include "build/build_config.h"
 #include "third_party/blink/renderer/platform/date_components.h"
 #include "third_party/blink/renderer/platform/mhtml/archive_resource.h"
@@ -196,24 +197,38 @@ String ConvertToPrintableCharacters(const String& text) {
 
 }  // namespace
 
+const char* MHTMLArchive::kLoadResultUmaName =
+    "PageSerialization.MhtmlLoading.LoadResult";
+
 MHTMLArchive::MHTMLArchive() = default;
 
+// static
+void MHTMLArchive::ReportLoadResult(MHTMLArchive::LoadResult result) {
+  UMA_HISTOGRAM_ENUMERATION(kLoadResultUmaName, result);
+}
+
 MHTMLArchive* MHTMLArchive::Create(const KURL& url,
                                    scoped_refptr<const SharedBuffer> data) {
   // |data| may be null if archive file is empty.
-  if (!data)
+  if (!data || data->IsEmpty()) {
+    ReportLoadResult(LoadResult::kEmptyFile);
     return nullptr;
+  }
 
   // MHTML pages can only be loaded from local URLs, http/https URLs, and
   // content URLs(Android specific).  The latter is now allowed due to full
   // sandboxing enforcement on MHTML pages.
-  if (!CanLoadArchive(url))
+  if (!CanLoadArchive(url)) {
+    ReportLoadResult(LoadResult::kUrlSchemeNotAllowed);
     return nullptr;
+  }
 
   MHTMLParser parser(std::move(data));
   HeapVector<Member<ArchiveResource>> resources = parser.ParseArchive();
-  if (resources.IsEmpty())
+  if (resources.IsEmpty()) {
+    ReportLoadResult(LoadResult::kInvalidArchive);
     return nullptr;  // Invalid MHTML file.
+  }
 
   MHTMLArchive* archive = new MHTMLArchive;
   archive->date_ = parser.CreationDate();
@@ -246,8 +261,12 @@ MHTMLArchive* MHTMLArchive::Create(const KURL& url,
     else
       archive->AddSubresource(resource);
   }
-  if (archive->MainResource())
+  if (archive->MainResource()) {
+    ReportLoadResult(LoadResult::kSuccess);
     return archive;
+  }
+
+  ReportLoadResult(LoadResult::kMissingMainResource);
   return nullptr;
 }
 

third_party/blink/renderer/platform/mhtml/mhtml_archive.h

@@ -55,6 +55,19 @@ class PLATFORM_EXPORT MHTMLArchive final
  public:
   static MHTMLArchive* Create(const KURL&, scoped_refptr<const SharedBuffer>);
 
+  // Every outcome when loading an archive with MHTMLArchive::Create (mirroring
+  // MHTMLLoadResult in tools/metrics/histograms/enums.xml).
+  enum class LoadResult {
+    kSuccess,
+    kEmptyFile,
+    kUrlSchemeNotAllowed,
+    kInvalidArchive,
+    kMissingMainResource,
+
+    kMaxValue = kMissingMainResource
+  };
+  static const char* kLoadResultUmaName;
+
   // Binary encoding results in smaller MHTML files but they might not work in
   // other browsers.
   enum EncodingPolicy { kUseDefaultEncoding, kUseBinaryEncoding };
@@ -108,6 +121,8 @@ class PLATFORM_EXPORT MHTMLArchive final
  private:
   MHTMLArchive();
 
+  static void ReportLoadResult(LoadResult result);
+
   void SetMainResource(ArchiveResource*);
   void AddSubresource(ArchiveResource*);
   static bool CanLoadArchive(const KURL&);

tools/metrics/histograms/enums.xml

@@ -33602,6 +33602,22 @@ Called by update_use_counter_css.py.-->
   <int value="6" label="Render process no longer exists"/>
 </enum>
 
+<enum name="MhtmlLoadResult">
+  <int value="0" label="Success">Archive loaded successfully</int>
+  <int value="1" label="Empty file">
+    Failed to load archive because the file was empty
+  </int>
+  <int value="2" label="URL scheme not allowed">
+    Failed to load archive because the url wasn't http(s), file, or content
+  </int>
+  <int value="3" label="Invalid archive">
+    Failed to load archive because it was not valid MHTML
+  </int>
+  <int value="4" label="Missing main resource">
+    Failed to load archive because it had no main resource
+  </int>
+</enum>
+
 <enum name="MicrophoneMuteResult">
   <int value="0" label="Muted"/>
   <int value="1" label="Not muted"/>

tools/metrics/histograms/histograms.xml

@@ -75440,6 +75440,13 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram name="PageSerialization.MhtmlLoading.LoadResult"
+    enum="MhtmlLoadResult">
+  <owner>iwells@chromium.org</owner>
+  <owner>carlosk@chromium.org</owner>
+  <summary>Reports the result of an attempt to load an MHTML archive.</summary>
+</histogram>
+
 <histogram name="PageSerialization.ProblemDetection.LoadedCSSPercentage"
     units="%">
   <owner>romax@chromium.org</owner>