BundledExchanges: Update SafeBundledExchangesParser interface

This class should hold the BundledExchangesParserFactory instance
so to keep the utility process alive. This fix leads a design change
to pass a service connector at constructor rather than other open
methods.

Also, this adds an error check to SafeBundledExchangesParser::OpenFile.
We could return a file open error and so on when
BundledExchangesReader::ReadMetadata is called.

Bug: 966753
Change-Id: I3174a88d0236f4afe0ebbad8348626800096b68d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1732518Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683923}

content/browser/web_package/bundled_exchanges_reader.cc

@@ -137,7 +137,10 @@ class BundledExchangesReader::SharedFileDataSource final
 
 BundledExchangesReader::BundledExchangesReader(
     const BundledExchangesSource& source)
-    : file_(base::MakeRefCounted<SharedFile>(source.file_path)) {}
+    : parser_(ServiceManagerConnection::GetForProcess()
+                  ? ServiceManagerConnection::GetForProcess()->GetConnector()
+                  : nullptr),
+      file_(base::MakeRefCounted<SharedFile>(source.file_path)) {}
 
 BundledExchangesReader::~BundledExchangesReader() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -202,8 +205,7 @@ const GURL& BundledExchangesReader::GetStartURL() const {
 }
 
 void BundledExchangesReader::SetBundledExchangesParserFactoryForTesting(
-    std::unique_ptr<data_decoder::mojom::BundledExchangesParserFactory>
-        factory) {
+    mojo::Remote<data_decoder::mojom::BundledExchangesParserFactory> factory) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   parser_.SetBundledExchangesParserFactoryForTesting(std::move(factory));
@@ -211,15 +213,17 @@ void BundledExchangesReader::SetBundledExchangesParserFactoryForTesting(
 
 void BundledExchangesReader::ReadMetadataInternal(MetadataCallback callback,
                                                   base::File file) {
-  parser_.OpenFile(
-      ServiceManagerConnection::GetForProcess()
-          ? ServiceManagerConnection::GetForProcess()->GetConnector()
-          : nullptr,
-      std::move(file));
-
-  parser_.ParseMetadata(
-      base::BindOnce(&BundledExchangesReader::OnMetadataParsed,
-                     base::Unretained(this), std::move(callback)));
+  base::File::Error error = parser_.OpenFile(std::move(file));
+  if (base::File::FILE_OK != error) {
+    PostTask(FROM_HERE,
+             base::BindOnce(std::move(callback),
+                            data_decoder::mojom::BundleMetadataParseError::New(
+                                base::File::ErrorToString(error))));
+  } else {
+    parser_.ParseMetadata(
+        base::BindOnce(&BundledExchangesReader::OnMetadataParsed,
+                       base::Unretained(this), std::move(callback)));
+  }
 }
 
 void BundledExchangesReader::OnMetadataParsed(

content/browser/web_package/bundled_exchanges_reader.h

@@ -68,8 +68,7 @@ class CONTENT_EXPORT BundledExchangesReader final {
   const GURL& GetStartURL() const;
 
   void SetBundledExchangesParserFactoryForTesting(
-      std::unique_ptr<data_decoder::mojom::BundledExchangesParserFactory>
-          factory);
+      mojo::Remote<data_decoder::mojom::BundledExchangesParserFactory> factory);
 
  private:
   // A simple wrapper class to share a single base::File instance among multiple

content/browser/web_package/bundled_exchanges_reader_unittest.cc

@@ -15,6 +15,10 @@
 #include "base/test/scoped_task_environment.h"
 #include "content/browser/web_package/bundled_exchanges_source.h"
 #include "mojo/public/c/system/data_pipe.h"
+#include "mojo/public/cpp/bindings/pending_receiver.h"
+#include "mojo/public/cpp/bindings/pending_remote.h"
+#include "mojo/public/cpp/bindings/remote.h"
+#include "mojo/public/cpp/bindings/self_owned_receiver.h"
 #include "services/data_decoder/public/mojom/bundled_exchanges_parser.mojom.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -133,7 +137,10 @@ class BundledExchangesReaderTest : public testing::Test {
     std::unique_ptr<MockParserFactory> factory =
         std::make_unique<MockParserFactory>();
     factory_ = factory.get();
-    reader_->SetBundledExchangesParserFactoryForTesting(std::move(factory));
+    mojo::Remote<data_decoder::mojom::BundledExchangesParserFactory> remote;
+    mojo::MakeSelfOwnedReceiver(std::move(factory),
+                                remote.BindNewPipeAndPassReceiver());
+    reader_->SetBundledExchangesParserFactoryForTesting(std::move(remote));
   }
 
  protected:

services/data_decoder/public/cpp/safe_bundled_exchanges_parser.cc

@@ -15,45 +15,42 @@ constexpr char kConnectionError[] =
     "Cannot connect to the remote parser service";
 }  // namespace
 
-SafeBundledExchangesParser::SafeBundledExchangesParser() = default;
+SafeBundledExchangesParser::SafeBundledExchangesParser(
+    service_manager::Connector* connector) {
+  if (connector) {
+    connector->Connect(mojom::kServiceName,
+                       factory_.BindNewPipeAndPassReceiver());
+  }
+}
+
 SafeBundledExchangesParser::~SafeBundledExchangesParser() = default;
 
-void SafeBundledExchangesParser::OpenFile(service_manager::Connector* connector,
-                                          base::File file) {
+base::File::Error SafeBundledExchangesParser::OpenFile(base::File file) {
   DCHECK(disconnected_);
-  if (factory_for_testing_) {
-    factory_for_testing_->GetParserForFile(parser_.BindNewPipeAndPassReceiver(),
-                                           std::move(file));
-  } else {
-    mojo::Remote<mojom::BundledExchangesParserFactory> factory;
-    connector->Connect(mojom::kServiceName,
-                       factory.BindNewPipeAndPassReceiver());
+  DCHECK(factory_.is_connected());
 
-    factory->GetParserForFile(parser_.BindNewPipeAndPassReceiver(),
-                              std::move(file));
-  }
+  if (!file.IsValid())
+    return file.error_details();
+
+  factory_->GetParserForFile(parser_.BindNewPipeAndPassReceiver(),
+                             std::move(file));
   parser_.set_disconnect_handler(base::BindOnce(
       &SafeBundledExchangesParser::OnDisconnect, base::Unretained(this)));
+
   disconnected_ = false;
+
+  return base::File::FILE_OK;
 }
 
 void SafeBundledExchangesParser::OpenDataSource(
-    service_manager::Connector* connector,
     mojo::PendingRemote<mojom::BundleDataSource> data_source) {
   DCHECK(disconnected_);
-  if (factory_for_testing_) {
-    factory_for_testing_->GetParserForDataSource(
-        parser_.BindNewPipeAndPassReceiver(), std::move(data_source));
-  } else {
-    mojo::Remote<mojom::BundledExchangesParserFactory> factory;
-    connector->Connect(mojom::kServiceName,
-                       factory.BindNewPipeAndPassReceiver());
-
-    factory->GetParserForDataSource(parser_.BindNewPipeAndPassReceiver(),
-                                    std::move(data_source));
-  }
+  DCHECK(factory_.is_connected());
+  factory_->GetParserForDataSource(parser_.BindNewPipeAndPassReceiver(),
+                                   std::move(data_source));
   parser_.set_disconnect_handler(base::BindOnce(
       &SafeBundledExchangesParser::OnDisconnect, base::Unretained(this)));
+
   disconnected_ = false;
 }
 
@@ -122,8 +119,8 @@ void SafeBundledExchangesParser::OnResponseParsed(
 }
 
 void SafeBundledExchangesParser::SetBundledExchangesParserFactoryForTesting(
-    std::unique_ptr<mojom::BundledExchangesParserFactory> factory) {
-  factory_for_testing_ = std::move(factory);
+    mojo::Remote<mojom::BundledExchangesParserFactory> factory) {
+  factory_ = std::move(factory);
 }
 
 }  // namespace data_decoder

services/data_decoder/public/cpp/safe_bundled_exchanges_parser.h

@@ -25,20 +25,18 @@ namespace data_decoder {
 // mojom::BundledExchangesParser service.
 class SafeBundledExchangesParser {
  public:
-  SafeBundledExchangesParser();
+  // |connector| can be null if SetBundledExchangesParserForTesting() will be
+  // called before calling Open*().
+  explicit SafeBundledExchangesParser(service_manager::Connector* connector);
   // Remaining callbacks on flight will be dropped.
   ~SafeBundledExchangesParser();
 
   // Binds |this| instance to the given |file| for subsequent parse calls.
-  // |connector| can be null if SetBundledExchangesParserForTesting() was
-  // called.
-  void OpenFile(service_manager::Connector* connector, base::File file);
+  base::File::Error OpenFile(base::File file);
 
   // Binds |this| instance to the given |data_source| for subsequent parse
-  // calls. |connector| can be null if SetBundledExchangesParserForTesting()
-  // was called.
-  void OpenDataSource(service_manager::Connector* connector,
-                      mojo::PendingRemote<mojom::BundleDataSource> data_source);
+  // calls.
+  void OpenDataSource(mojo::PendingRemote<mojom::BundleDataSource> data_source);
 
   // Parses metadata. See mojom::BundledExchangesParser::ParseMetadata for
   // details. This method fails when it's called before the previous call
@@ -56,7 +54,7 @@ class SafeBundledExchangesParser {
   // Sets alternative BundledExchangesParserFactory that will be used to create
   // BundledExchangesParser for testing purpose.
   void SetBundledExchangesParserFactoryForTesting(
-      std::unique_ptr<mojom::BundledExchangesParserFactory> factory);
+      mojo::Remote<mojom::BundledExchangesParserFactory> factory);
 
  private:
   void OnDisconnect();
@@ -66,14 +64,13 @@ class SafeBundledExchangesParser {
                         mojom::BundleResponsePtr response,
                         mojom::BundleResponseParseErrorPtr error);
 
+  mojo::Remote<mojom::BundledExchangesParserFactory> factory_;
   mojo::Remote<mojom::BundledExchangesParser> parser_;
   mojom::BundledExchangesParser::ParseMetadataCallback metadata_callback_;
   base::flat_map<size_t, mojom::BundledExchangesParser::ParseResponseCallback>
       response_callbacks_;
   size_t response_callback_next_id_ = 0;
   bool disconnected_ = true;
-  std::unique_ptr<mojom::BundledExchangesParserFactory> factory_for_testing_ =
-      nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(SafeBundledExchangesParser);
 };

services/data_decoder/public/cpp/safe_bundled_exchanges_parser_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/path_service.h"
 #include "base/run_loop.h"
 #include "base/test/scoped_task_environment.h"
+#include "mojo/public/cpp/bindings/self_owned_receiver.h"
 #include "services/data_decoder/public/cpp/test_data_decoder_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -65,7 +66,10 @@ class MockFactory final : public mojom::BundledExchangesParserFactory {
   };
 
   MockFactory() {}
-  MockParser* GetCreatedParser() { return parser_.get(); }
+  MockParser* GetCreatedParser() {
+    base::RunLoop().RunUntilIdle();
+    return parser_.get();
+  }
   void DeleteParser() { parser_.reset(); }
 
  private:
@@ -107,17 +111,26 @@ class SafeBundledExchangesParserTest : public testing::Test {
  public:
   service_manager::Connector* GetConnector() { return service_.connector(); }
 
+  MockFactory* InitializeMockFactory(SafeBundledExchangesParser* parser) {
+    std::unique_ptr<MockFactory> factory = std::make_unique<MockFactory>();
+    MockFactory* raw_factory = factory.get();
+    mojo::Remote<mojom::BundledExchangesParserFactory> remote;
+    mojo::MakeSelfOwnedReceiver(std::move(factory),
+                                remote.BindNewPipeAndPassReceiver());
+    parser->SetBundledExchangesParserFactoryForTesting(std::move(remote));
+    return raw_factory;
+  }
+
  private:
   base::test::ScopedTaskEnvironment task_environment_;
   TestDataDecoderService service_;
 };
 
 TEST_F(SafeBundledExchangesParserTest, ParseGoldenFile) {
-  SafeBundledExchangesParser parser;
+  SafeBundledExchangesParser parser(GetConnector());
   base::File test_file =
       OpenTestFile(base::FilePath(FILE_PATH_LITERAL("hello.wbn")));
-
-  parser.OpenFile(GetConnector(), std::move(test_file));
+  ASSERT_EQ(base::File::FILE_OK, parser.OpenFile(std::move(test_file)));
 
   mojom::BundleMetadataPtr metadata_result;
   {
@@ -172,8 +185,13 @@ TEST_F(SafeBundledExchangesParserTest, ParseGoldenFile) {
   EXPECT_EQ(index[3]->request_url, "https://test.example.org/script.js");
 }
 
+TEST_F(SafeBundledExchangesParserTest, OpenInvalidFile) {
+  SafeBundledExchangesParser parser(GetConnector());
+  EXPECT_EQ(base::File::FILE_ERROR_FAILED, parser.OpenFile(base::File()));
+}
+
 TEST_F(SafeBundledExchangesParserTest, CallWithoutOpen) {
-  SafeBundledExchangesParser parser;
+  SafeBundledExchangesParser parser(GetConnector());
   bool metadata_parsed = false;
   parser.ParseMetadata(base::BindOnce(
       [](bool* metadata_parsed, mojom::BundleMetadataPtr metadata,
@@ -204,13 +222,13 @@ TEST_F(SafeBundledExchangesParserTest, CallWithoutOpen) {
 }
 
 TEST_F(SafeBundledExchangesParserTest, UseMockFactory) {
-  SafeBundledExchangesParser parser;
-  std::unique_ptr<MockFactory> factory = std::make_unique<MockFactory>();
-  MockFactory* raw_factory = factory.get();
-  parser.SetBundledExchangesParserFactoryForTesting(std::move(factory));
+  SafeBundledExchangesParser parser(GetConnector());
+  MockFactory* raw_factory = InitializeMockFactory(&parser);
 
   EXPECT_FALSE(raw_factory->GetCreatedParser());
-  parser.OpenFile(GetConnector(), base::File());
+  base::File test_file =
+      OpenTestFile(base::FilePath(FILE_PATH_LITERAL("hello.wbn")));
+  ASSERT_EQ(base::File::FILE_OK, parser.OpenFile(std::move(test_file)));
   ASSERT_TRUE(raw_factory->GetCreatedParser());
   EXPECT_FALSE(raw_factory->GetCreatedParser()->IsParseMetadataCalled());
   EXPECT_FALSE(raw_factory->GetCreatedParser()->IsParseResponseCalled());
@@ -227,15 +245,13 @@ TEST_F(SafeBundledExchangesParserTest, UseMockFactory) {
 }
 
 TEST_F(SafeBundledExchangesParserTest, ConnectionError) {
-  SafeBundledExchangesParser parser;
-  std::unique_ptr<MockFactory> factory = std::make_unique<MockFactory>();
-  MockFactory* raw_factory = factory.get();
-  parser.SetBundledExchangesParserFactoryForTesting(std::move(factory));
+  SafeBundledExchangesParser parser(GetConnector());
+  MockFactory* raw_factory = InitializeMockFactory(&parser);
 
   mojo::PendingRemote<mojom::BundleDataSource> remote_data_source;
   auto data_source = std::make_unique<MockDataSource>(
       remote_data_source.InitWithNewPipeAndPassReceiver());
-  parser.OpenDataSource(GetConnector(), std::move(remote_data_source));
+  parser.OpenDataSource(std::move(remote_data_source));
   ASSERT_TRUE(raw_factory->GetCreatedParser());
 
   base::RunLoop run_loop;