Skip to content

Commit

Permalink
manta: IdentityManager shutdown safety
Browse files Browse the repository at this point in the history 
OrcaProvider and SnapperProvider can watch for
IdentityManager::Shutdown themselves. After detecting
shutdown, reply with empty error responses to all api calls.
Add new enum value MantaStatusCode::kNoIdentityManager for
this new case.

Bug: b:302151610
Test: components_unittests --gtest_filter=*Orca*:*Snapper*
Change-Id: Ib17348745b6a03d810a1a772689a55ccf37e1085
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4908370
Reviewed-by: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Darren Shen <shend@chromium.org>
Commit-Queue: Jeffrey Young <cowmoo@google.com>
Cr-Commit-Position: refs/heads/main@{#1210401}
  • Loading branch information
@cwmoo740
cwmoo740 authored and Chromium LUCI CQ committed Oct 16, 2023
1 parent 92d43bb commit a35ef42
Show file tree
Hide file tree
Showing 8 changed files with 135 additions and 64 deletions.
1 change: 1 addition & 0 deletions chrome/browser/ash/input_method/editor_text_query_provider.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ orca::mojom::TextQueryErrorCode ConvertErrorCode(
switch (status_code) {
case manta::MantaStatusCode::kGenericError:
case manta::MantaStatusCode::kMalformedResponse:
case manta::MantaStatusCode::kNoIdentityManager:
return orca::mojom::TextQueryErrorCode::kUnknown;
case manta::MantaStatusCode::kInvalidInput:
return orca::mojom::TextQueryErrorCode::kInvalidArgument;
Expand Down
5 changes: 4 additions & 1 deletion components/manta/manta_status.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ enum class MantaStatusCode {
kUnsupportedLanguage = 7,
kBlockedOutputs = 8,
kRestrictedCountry = 9,
kMax = kRestrictedCountry,
// Request was never sent due to missing IdentityManager. This is usually
// caused by a request being attempted while ChromeOS is shutting down.
kNoIdentityManager = 10,
kMax = kNoIdentityManager,
};

struct MantaStatus {
Expand Down
24 changes: 21 additions & 3 deletions components/manta/orca_provider.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@
#include <string>
#include <vector>

#include "base/check.h"
#include "base/containers/fixed_flat_map.h"
#include "base/functional/bind.h"
#include "base/time/time.h"
#include "base/values.h"
#include "components/endpoint_fetcher/endpoint_fetcher.h"
#include "components/manta/manta_status.h"
#include "components/manta/proto/manta.pb.h"
#include "components/signin/public/base/consent_level.h"
#include "components/signin/public/identity_manager/identity_manager.h"
Expand Down Expand Up @@ -116,13 +118,21 @@ void OnServerResponseOrErrorReceived(
OrcaProvider::OrcaProvider(
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
signin::IdentityManager* identity_manager)
: identity_manager_(identity_manager),
url_loader_factory_(url_loader_factory) {}
: url_loader_factory_(url_loader_factory) {
CHECK(identity_manager);
identity_manager_observation_.Observe(identity_manager);
}

OrcaProvider::~OrcaProvider() = default;

void OrcaProvider::Call(const std::map<std::string, std::string>& input,
MantaGenericCallback done_callback) {
if (!identity_manager_observation_.IsObserving()) {
std::move(done_callback)
.Run(base::Value::Dict(), {MantaStatusCode::kNoIdentityManager});
return;
}

absl::optional<proto::Request> request = ComposeRequest(input);
if (request == absl::nullopt) {
std::move(done_callback)
Expand All @@ -145,10 +155,18 @@ void OrcaProvider::Call(const std::map<std::string, std::string>& input,
std::move(fetcher)));
}

void OrcaProvider::OnIdentityManagerShutdown(
signin::IdentityManager* identity_manager) {
if (identity_manager_observation_.IsObservingSource(identity_manager)) {
identity_manager_observation_.Reset();
}
}

std::unique_ptr<EndpointFetcher> OrcaProvider::CreateEndpointFetcher(
const GURL& url,
const std::vector<std::string>& scopes,
const std::string& post_data) {
CHECK(identity_manager_observation_.IsObserving());
// TODO(b:288019728): MISSING_TRAFFIC_ANNOTATION should be resolved before
// launch.
return std::make_unique<EndpointFetcher>(
Expand All @@ -161,7 +179,7 @@ std::unique_ptr<EndpointFetcher> OrcaProvider::CreateEndpointFetcher(
/*timeout_ms=*/kTimeoutMs.InMilliseconds(),
/*post_data=*/post_data,
/*annotation_tag=*/MISSING_TRAFFIC_ANNOTATION,
/*identity_manager=*/identity_manager_,
/*identity_manager=*/identity_manager_observation_.GetSource(),
/*consent_level=*/signin::ConsentLevel::kSignin);
}

Expand Down
40 changes: 17 additions & 23 deletions components/manta/orca_provider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,36 +11,24 @@
#include <vector>

#include "base/component_export.h"
#include "base/memory/raw_ptr.h"
#include "base/memory/scoped_refptr.h"
#include "base/memory/weak_ptr.h"
#include "base/scoped_observation.h"
#include "components/endpoint_fetcher/endpoint_fetcher.h"
#include "components/manta/manta_service_callbacks.h"
#include "components/signin/public/identity_manager/identity_manager.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
#include "url/gurl.h"

namespace signin {
class IdentityManager;
} // namespace signin

namespace manta {

// The Orca provider for the Manta project. Provides a method for clients to
// call the relevant google API, handling OAuth and http fetching.
//
// IMPORTANT: This class depends on `IdentityManager`, a
// `ProfileKeyedServiceFactory`. You should ensure you do not call
// `OrcaProvider::Call` post `IdentityManager`'s destruction.
// There are several ways to ensure this. You can:
// 1. Make the owner of `OrcaProvider` a `ProfileKeyedServiceFactory` that
// `DependsOn` `IdentityManager`. See
// https://www.chromium.org/developers/design-documents/profile-architecture/#dependency-management-overview
// for more information.
// 2. Register an `IdentityManager::Observer` that listens to
// `OnIdentityManagerShutdown`.
// 3. Manually ensure OrcaProvider isn't used past `IdentityManager`'s
// lifetime.
class COMPONENT_EXPORT(MANTA) OrcaProvider {
// IMPORTANT: This class depends on `IdentityManager`.
// `OrcaProvider::Call` will return an empty response after `IdentityManager`
// destruction.
class COMPONENT_EXPORT(MANTA) OrcaProvider
: public signin::IdentityManager::Observer {
public:
// Returns a `OrcaProvider` instance tied to the profile of the passed
// arguments.
Expand All @@ -51,17 +39,20 @@ class COMPONENT_EXPORT(MANTA) OrcaProvider {
OrcaProvider(const OrcaProvider&) = delete;
OrcaProvider& operator=(const OrcaProvider&) = delete;

virtual ~OrcaProvider();
~OrcaProvider() override;

// Calls the google service endpoint with the http POST request payload
// populated with the `input` parameters.
// The fetched response is processed and returned to the caller via an
// `MantaGenericCallback` callback.
//
// NOTE: This methods internally depends on a valid `IdentityManager`.
// Will give an empty response if `IdentityManager` is no longer valid.
void Call(const std::map<std::string, std::string>& input,
MantaGenericCallback done_callback);

// signin::IdentityManager::Observer:
void OnIdentityManagerShutdown(
signin::IdentityManager* identity_manager) override;

private:
friend class FakeOrcaProvider;

Expand All @@ -73,9 +64,12 @@ class COMPONENT_EXPORT(MANTA) OrcaProvider {
const std::vector<std::string>& scopes,
const std::string& post_data);

const raw_ptr<signin::IdentityManager> identity_manager_;
const scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;

base::ScopedObservation<signin::IdentityManager,
signin::IdentityManager::Observer>
identity_manager_observation_{this};

base::WeakPtrFactory<OrcaProvider> weak_ptr_factory_{this};
};

Expand Down
32 changes: 26 additions & 6 deletions components/manta/orca_provider_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "base/time/time.h"
#include "components/manta/manta_status.h"
#include "components/signin/public/base/consent_level.h"
#include "components/signin/public/identity_manager/identity_manager.h"
#include "components/signin/public/identity_manager/identity_test_environment.h"
#include "net/base/net_errors.h"
#include "net/http/http_status_code.h"
Expand Down Expand Up @@ -49,6 +50,7 @@ class FakeOrcaProvider : public OrcaProvider {
const GURL& url,
const std::vector<std::string>& scopes,
const std::string& post_data) override {
CHECK(identity_manager_observation_.IsObserving());
return std::make_unique<EndpointFetcher>(
/*url_loader_factory=*/url_loader_factory_,
/*oauth_consumer_name=*/kMockOAuthConsumerName,
Expand All @@ -57,7 +59,7 @@ class FakeOrcaProvider : public OrcaProvider {
/*scopes=*/std::vector<std::string>{kMockScope},
/*timeout_ms=*/kMockTimeout.InMilliseconds(), /*post_data=*/post_data,
/*annotation_tag=*/TRAFFIC_ANNOTATION_FOR_TESTS,
/*identity_manager=*/identity_manager_,
/*identity_manager=*/identity_manager_observation_.GetSource(),
/*consent_level=*/signin::ConsentLevel::kSync);
}
};
Expand All @@ -73,9 +75,10 @@ class OrcaProviderTest : public testing::Test {
~OrcaProviderTest() override = default;

void SetUp() override {
identity_test_env_.MakePrimaryAccountAvailable(kEmail,
signin::ConsentLevel::kSync);
identity_test_env_.SetAutomaticIssueOfAccessTokens(true);
identity_test_env_ = std::make_unique<signin::IdentityTestEnvironment>();
identity_test_env_->MakePrimaryAccountAvailable(
kEmail, signin::ConsentLevel::kSync);
identity_test_env_->SetAutomaticIssueOfAccessTokens(true);
}

void SetEndpointMockResponse(const GURL& request_url,
Expand All @@ -99,14 +102,14 @@ class OrcaProviderTest : public testing::Test {
return std::make_unique<FakeOrcaProvider>(
base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
&test_url_loader_factory_),
identity_test_env_.identity_manager());
identity_test_env_->identity_manager());
}

protected:
base::test::TaskEnvironment task_environment_;
std::unique_ptr<signin::IdentityTestEnvironment> identity_test_env_;

private:
signin::IdentityTestEnvironment identity_test_env_;
network::TestURLLoaderFactory test_url_loader_factory_;
};

Expand Down Expand Up @@ -230,4 +233,21 @@ TEST_F(OrcaProviderTest, ParseSuccessfulResponse) {
task_environment_.RunUntilQuit();
}

TEST_F(OrcaProviderTest, EmptyResponseAfterIdentityManagerShutdown) {
std::unique_ptr<FakeOrcaProvider> orca_provider = CreateOrcaProvider();

identity_test_env_.reset();

orca_provider->Call(
{}, base::BindLambdaForTesting(
[quit_closure = task_environment_.QuitClosure()](
base::Value::Dict dict, MantaStatus manta_status) {
ASSERT_TRUE(dict.empty());
ASSERT_EQ(MantaStatusCode::kNoIdentityManager,
manta_status.status_code);
quit_closure.Run();
}));
task_environment_.RunUntilQuit();
}

} // namespace manta
22 changes: 19 additions & 3 deletions components/manta/snapper_provider.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "base/time/time.h"
#include "components/endpoint_fetcher/endpoint_fetcher.h"
#include "components/manta/manta_service_callbacks.h"
#include "components/manta/manta_status.h"
#include "components/manta/proto/manta.pb.h"
#include "components/signin/public/base/consent_level.h"
#include "components/signin/public/identity_manager/identity_manager.h"
Expand All @@ -34,13 +35,20 @@ constexpr base::TimeDelta kTimeoutMs = base::Seconds(90);
SnapperProvider::SnapperProvider(
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
signin::IdentityManager* identity_manager)
: identity_manager_(identity_manager),
url_loader_factory_(url_loader_factory) {}
: url_loader_factory_(url_loader_factory) {
CHECK(identity_manager);
identity_manager_observation_.Observe(identity_manager);
}

SnapperProvider::~SnapperProvider() = default;

void SnapperProvider::Call(const manta::proto::Request& request,
MantaProtoResponseCallback done_callback) {
if (!identity_manager_observation_.IsObserving()) {
std::move(done_callback)
.Run(nullptr, {MantaStatusCode::kNoIdentityManager});
return;
}
std::string serialized_request;
request.SerializeToString(&serialized_request);

Expand All @@ -53,18 +61,26 @@ void SnapperProvider::Call(const manta::proto::Request& request,
std::move(fetcher)));
}

void SnapperProvider::OnIdentityManagerShutdown(
signin::IdentityManager* identity_manager) {
if (identity_manager_observation_.IsObservingSource(identity_manager)) {
identity_manager_observation_.Reset();
}
}

std::unique_ptr<EndpointFetcher> SnapperProvider::CreateEndpointFetcher(
const GURL& url,
const std::vector<std::string>& scopes,
const std::string& post_data) {
CHECK(identity_manager_observation_.IsObserving());
return std::make_unique<EndpointFetcher>(
/*url_loader_factory=*/url_loader_factory_,
/*oauth_consumer_name=*/kOauthConsumerName, /*url=*/url,
/*http_method=*/kHttpMethod, /*content_type=*/kHttpContentType,
/*scopes=*/scopes,
/*timeout_ms=*/kTimeoutMs.InMilliseconds(), /*post_data=*/post_data,
/*annotation_tag=*/MISSING_TRAFFIC_ANNOTATION,
/*identity_manager=*/identity_manager_,
/*identity_manager=*/identity_manager_observation_.GetSource(),
/*consent_level=*/signin::ConsentLevel::kSignin);
}

Expand Down
40 changes: 18 additions & 22 deletions components/manta/snapper_provider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,37 +10,26 @@
#include <vector>

#include "base/component_export.h"
#include "base/memory/raw_ptr.h"
#include "base/memory/scoped_refptr.h"
#include "base/memory/weak_ptr.h"
#include "base/scoped_observation.h"
#include "components/endpoint_fetcher/endpoint_fetcher.h"
#include "components/manta/manta_service_callbacks.h"
#include "components/manta/proto/manta.pb.h"
#include "components/signin/public/identity_manager/identity_manager.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
#include "url/gurl.h"

namespace signin {
class IdentityManager;
} // namespace signin

namespace manta {

// The Snapper provider for the Manta project. Provides a method for clients to
// call the relevant google API, handling OAuth and http fetching.
//
// IMPORTANT: This class depends on `IdentityManager`, a
// `ProfileKeyedServiceFactory`. You should ensure you do not call
// `SnapperProvider::Call` post `IdentityManager`'s destruction.
// There are several ways to ensure this. You can:
// 1. Make the owner of `SnapperProvider` a `ProfileKeyedServiceFactory` that
// `DependsOn` `IdentityManager`. See
// https://www.chromium.org/developers/design-documents/profile-architecture/#dependency-management-overview
// for more information.
// 2. Register an `IdentityManager::Observer` that listens to
// `OnIdentityManagerShutdown`.
// 3. Manually ensure SnapperProvided isn't used past `IdentityManager`'s
// lifetime.
class COMPONENT_EXPORT(MANTA) SnapperProvider {
// IMPORTANT: This class depends on `IdentityManager`.
// `SnapperProvider::Call` will return an empty response after `IdentityManager`
// destruction.
class COMPONENT_EXPORT(MANTA) SnapperProvider
: public signin::IdentityManager::Observer {
public:
// Returns a `SnapperProvider` instance tied to the profile of the passed
// arguments.
Expand All @@ -51,16 +40,20 @@ class COMPONENT_EXPORT(MANTA) SnapperProvider {
SnapperProvider(const SnapperProvider&) = delete;
SnapperProvider& operator=(const SnapperProvider&) = delete;

virtual ~SnapperProvider();
~SnapperProvider() override;

// Calls the google service endpoint with the provided request as the http
// POST request payload. The fetched response is returned to the caller via a
// `MantaProtoResponseCallback` callback.
//
// NOTE: This methods internally depends on a valid `IdentityManager`.
// `done_callback` will be called with nullptr if `IdentityManager` is no
// longer valid.
virtual void Call(const manta::proto::Request& request,
MantaProtoResponseCallback done_callback);

// signin::IdentityManager::Observer:
void OnIdentityManagerShutdown(
signin::IdentityManager* identity_manager) override;

private:
friend class FakeSnapperProvider;

Expand All @@ -72,9 +65,12 @@ class COMPONENT_EXPORT(MANTA) SnapperProvider {
const std::vector<std::string>& scopes,
const std::string& post_data);

const raw_ptr<signin::IdentityManager> identity_manager_;
const scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;

base::ScopedObservation<signin::IdentityManager,
signin::IdentityManager::Observer>
identity_manager_observation_{this};

base::WeakPtrFactory<SnapperProvider> weak_ptr_factory_{this};
};

Expand Down

0 comments on commit a35ef42

Please sign in to comment.