Skip to content

Commit c5f5ffb

Browse files
Charles ZhaoCommit Bot
Charles Zhao
authored and
Commit Bot
committed
Fix HandwritingModelLoader.
(1) calling HandwritingModelLoader(spec, receiver, callback).Load() is a bad idea because the .Load() is async and the temporal object could be released very soon. This Cl changes the whole class to be stateless, so that calling HandwritingModelLoader::Load(spec, receiver, callback) won't suffer the same problem. (2) unit tests are fixed accordingly. Bug: 1054628 Change-Id: I56459b14e1fc4a6608d5ed264e6c650d1e076f69 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2415914 Commit-Queue: Charles . <[email protected]> Reviewed-by: Andrew Moylan <[email protected]> Reviewed-by: Charles . <[email protected]> Reviewed-by: Xinglong Luan <[email protected]> Cr-Commit-Position: refs/heads/master@{#810102}
1 parent 08c2656 commit c5f5ffb

File tree

3 files changed

+99
-125
lines changed

3 files changed

+99
-125
lines changed

‎chromeos/services/machine_learning/public/cpp/handwriting_model_loader.cc

Lines changed: 70 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,12 @@ namespace chromeos {
1313
namespace machine_learning {
1414
namespace {
1515

16-
using chromeos::machine_learning::mojom::LoadHandwritingModelResult;
16+
using ::chromeos::machine_learning::mojom::HandwritingRecognizerSpecPtr;
17+
using ::chromeos::machine_learning::mojom::LoadHandwritingModelResult;
18+
using HandwritingRecognizer =
19+
mojo::PendingReceiver<mojom::HandwritingRecognizer>;
20+
using LoadHandwritingModelCallback = ::chromeos::machine_learning::mojom::
21+
MachineLearningService::LoadHandwritingModelCallback;
1722

1823
// Records CrOSActionRecorder event.
1924
void RecordLoadHandwritingModelResult(const LoadHandwritingModelResult val) {
@@ -22,6 +27,8 @@ void RecordLoadHandwritingModelResult(const LoadHandwritingModelResult val) {
2227
LoadHandwritingModelResult::LOAD_MODEL_FILES_ERROR);
2328
}
2429

30+
constexpr char kOndeviceHandwritingSwitch[] = "ondevice_handwriting";
31+
constexpr char kLibHandwritingDlcId[] = "libhandwriting";
2532
// A list of supported language code.
2633
constexpr char kLanguageCodeEn[] = "en";
2734
constexpr char kLanguageCodeGesture[] = "gesture_in_context";
@@ -30,10 +37,8 @@ constexpr char kLanguageCodeGesture[] = "gesture_in_context";
3037
// kOndeviceHandwritingSwitch.
3138
bool HandwritingSwitchHasValue(const std::string& value) {
3239
base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
33-
return command_line->HasSwitch(
34-
HandwritingModelLoader::kOndeviceHandwritingSwitch) &&
35-
command_line->GetSwitchValueASCII(
36-
HandwritingModelLoader::kOndeviceHandwritingSwitch) == value;
40+
return command_line->HasSwitch(kOndeviceHandwritingSwitch) &&
41+
command_line->GetSwitchValueASCII(kOndeviceHandwritingSwitch) == value;
3742
}
3843

3944
// Returns true if switch kOndeviceHandwritingSwitch is set to use_rootfs.
@@ -46,69 +51,45 @@ bool IsLibHandwritingDlcEnabled() {
4651
return HandwritingSwitchHasValue("use_dlc");
4752
}
4853

49-
} // namespace
50-
51-
constexpr char HandwritingModelLoader::kOndeviceHandwritingSwitch[];
52-
constexpr char HandwritingModelLoader::kLibHandwritingDlcId[];
53-
54-
HandwritingModelLoader::HandwritingModelLoader(
55-
mojom::HandwritingRecognizerSpecPtr spec,
56-
mojo::PendingReceiver<mojom::HandwritingRecognizer> receiver,
57-
mojom::MachineLearningService::LoadHandwritingModelCallback callback)
58-
: dlc_client_(chromeos::DlcserviceClient::Get()),
59-
spec_(std::move(spec)),
60-
receiver_(std::move(receiver)),
61-
callback_(std::move(callback)),
62-
weak_ptr_factory_(this) {}
63-
64-
HandwritingModelLoader::~HandwritingModelLoader() = default;
65-
66-
void HandwritingModelLoader::Load() {
67-
// Returns FEATURE_NOT_SUPPORTED_ERROR if both rootfs and dlc are not enabled.
68-
if (!IsLibHandwritingRootfsEnabled() && !IsLibHandwritingDlcEnabled()) {
69-
RecordLoadHandwritingModelResult(
70-
LoadHandwritingModelResult::FEATURE_NOT_SUPPORTED_ERROR);
71-
std::move(callback_).Run(
72-
LoadHandwritingModelResult::FEATURE_NOT_SUPPORTED_ERROR);
73-
return;
74-
}
75-
76-
// Returns LANGUAGE_NOT_SUPPORTED_ERROR if the language is not supported yet.
77-
if (spec_->language != kLanguageCodeEn &&
78-
spec_->language != kLanguageCodeGesture) {
79-
RecordLoadHandwritingModelResult(
80-
LoadHandwritingModelResult::LANGUAGE_NOT_SUPPORTED_ERROR);
81-
std::move(callback_).Run(
82-
LoadHandwritingModelResult::LANGUAGE_NOT_SUPPORTED_ERROR);
83-
return;
84-
}
85-
86-
// Load from rootfs if enabled.
87-
if (IsLibHandwritingRootfsEnabled()) {
54+
// Called when InstallDlc completes.
55+
// Returns an error if the `result.error` is not dlcservice::kErrorNone.
56+
// Calls mlservice to LoadHandwritingModel otherwise.
57+
void OnInstallDlcComplete(
58+
HandwritingRecognizerSpecPtr spec,
59+
HandwritingRecognizer receiver,
60+
LoadHandwritingModelCallback callback,
61+
const chromeos::DlcserviceClient::InstallResult& result) {
62+
// Call LoadHandwritingModelWithSpec if no error was found.
63+
if (result.error == dlcservice::kErrorNone) {
8864
ServiceConnection::GetInstance()->LoadHandwritingModel(
89-
std::move(spec_), std::move(receiver_), std::move(callback_));
65+
std::move(spec), std::move(receiver), std::move(callback));
9066
return;
9167
}
9268

93-
// Gets existing dlc list and based on the presence of libhandwriting
94-
// either returns an error or installs the libhandwriting dlc.
95-
dlc_client_->GetExistingDlcs(
96-
base::BindOnce(&HandwritingModelLoader::OnGetExistingDlcsComplete,
97-
weak_ptr_factory_.GetWeakPtr()));
69+
RecordLoadHandwritingModelResult(
70+
LoadHandwritingModelResult::DLC_INSTALL_ERROR);
71+
std::move(callback).Run(LoadHandwritingModelResult::DLC_INSTALL_ERROR);
9872
}
9973

100-
void HandwritingModelLoader::OnGetExistingDlcsComplete(
74+
// Called when the existing-dlc-list is returned.
75+
// Returns an error if libhandwriting is not in the existing-dlc-list.
76+
// Calls InstallDlc otherwise.
77+
void OnGetExistingDlcsComplete(
78+
HandwritingRecognizerSpecPtr spec,
79+
HandwritingRecognizer receiver,
80+
LoadHandwritingModelCallback callback,
81+
DlcserviceClient* const dlc_client,
10182
const std::string& err,
10283
const dlcservice::DlcsWithContent& dlcs_with_content) {
10384
// Loop over dlcs_with_content, and installs libhandwriting if already exists.
10485
// Since we don't want to trigger downloading here, we only install(mount)
10586
// the handwriting dlc if it is already on device.
10687
for (const auto& dlc_info : dlcs_with_content.dlc_infos()) {
107-
if (dlc_info.id() == HandwritingModelLoader::kLibHandwritingDlcId) {
108-
dlc_client_->Install(
88+
if (dlc_info.id() == kLibHandwritingDlcId) {
89+
dlc_client->Install(
10990
kLibHandwritingDlcId,
110-
base::BindOnce(&HandwritingModelLoader::OnInstallDlcComplete,
111-
weak_ptr_factory_.GetWeakPtr()),
91+
base::BindOnce(&OnInstallDlcComplete, std::move(spec),
92+
std::move(receiver), std::move(callback)),
11293
chromeos::DlcserviceClient::IgnoreProgress);
11394
return;
11495
}
@@ -117,21 +98,46 @@ void HandwritingModelLoader::OnGetExistingDlcsComplete(
11798
// Returns error if the handwriting dlc is not on the device.
11899
RecordLoadHandwritingModelResult(
119100
LoadHandwritingModelResult::DLC_DOES_NOT_EXIST);
120-
std::move(callback_).Run(LoadHandwritingModelResult::DLC_DOES_NOT_EXIST);
101+
std::move(callback).Run(LoadHandwritingModelResult::DLC_DOES_NOT_EXIST);
121102
}
122103

123-
void HandwritingModelLoader::OnInstallDlcComplete(
124-
const chromeos::DlcserviceClient::InstallResult& result) {
125-
// Call LoadHandwritingModelWithSpec if no error was found.
126-
if (result.error == dlcservice::kErrorNone) {
104+
} // namespace
105+
106+
void LoadHandwritingModelFromRootfsOrDlc(HandwritingRecognizerSpecPtr spec,
107+
HandwritingRecognizer receiver,
108+
LoadHandwritingModelCallback callback,
109+
DlcserviceClient* const dlc_client) {
110+
// Returns FEATURE_NOT_SUPPORTED_ERROR if both rootfs and dlc are not enabled.
111+
if (!IsLibHandwritingRootfsEnabled() && !IsLibHandwritingDlcEnabled()) {
112+
RecordLoadHandwritingModelResult(
113+
LoadHandwritingModelResult::FEATURE_NOT_SUPPORTED_ERROR);
114+
std::move(callback).Run(
115+
LoadHandwritingModelResult::FEATURE_NOT_SUPPORTED_ERROR);
116+
return;
117+
}
118+
119+
// Returns LANGUAGE_NOT_SUPPORTED_ERROR if the language is not supported yet.
120+
if (spec->language != kLanguageCodeEn &&
121+
spec->language != kLanguageCodeGesture) {
122+
RecordLoadHandwritingModelResult(
123+
LoadHandwritingModelResult::LANGUAGE_NOT_SUPPORTED_ERROR);
124+
std::move(callback).Run(
125+
LoadHandwritingModelResult::LANGUAGE_NOT_SUPPORTED_ERROR);
126+
return;
127+
}
128+
129+
// Load from rootfs if enabled.
130+
if (IsLibHandwritingRootfsEnabled()) {
127131
ServiceConnection::GetInstance()->LoadHandwritingModel(
128-
std::move(spec_), std::move(receiver_), std::move(callback_));
132+
std::move(spec), std::move(receiver), std::move(callback));
129133
return;
130134
}
131135

132-
RecordLoadHandwritingModelResult(
133-
LoadHandwritingModelResult::DLC_INSTALL_ERROR);
134-
std::move(callback_).Run(LoadHandwritingModelResult::DLC_INSTALL_ERROR);
136+
// Gets existing dlc list and based on the presence of libhandwriting
137+
// either returns an error or installs the libhandwriting dlc.
138+
dlc_client->GetExistingDlcs(
139+
base::BindOnce(&OnGetExistingDlcsComplete, std::move(spec),
140+
std::move(receiver), std::move(callback), dlc_client));
135141
}
136142

137143
} // namespace machine_learning

‎chromeos/services/machine_learning/public/cpp/handwriting_model_loader.h

Lines changed: 12 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -14,64 +14,31 @@
1414
namespace chromeos {
1515
namespace machine_learning {
1616

17-
// Class that decides either to load handwriting model from rootfs or dlc.
17+
// Helper function decides either to load handwriting model from rootfs or dlc.
1818
// New Handwriting clients should call this helper instead of calling
1919
// ServiceConnection::GetInstance()->LoadHandwritingModel.
2020
// Three typical examples of the callstack are:
2121
// Case 1: handwriting in enabled on rootfs.
22-
// client calls HandwritingModelLoader("en", receiver, callback).Load()
22+
// client calls LoadHandwritingModelFromRootfsOrDlc("en", receiver, callback)
2323
// which calls LoadHandwritingModel -> handwriting model loaded from rootfs.
2424
// Case 2: handwriting is enabled for dlc and dlc is already on the device.
25-
// client calls HandwritingModelLoader("en", receiver, callback).Load()
25+
// client calls LoadHandwritingModelFromRootfsOrDlc("en", receiver, callback)
2626
// which calls -> GetExistingDlcs -> libhandwriting dlc already exists
2727
// -> InstallDlc -> LoadHandwritingModel
2828
// The correct handwriting model will be loaded and bond to the receiver.
2929
// Case 3: handwriting is enabled for dlc and dlc is not on the device yet.
30-
// client calls HandwritingModelLoader("en", receiver, callback).Load()
30+
// client calls LoadHandwritingModelFromRootfsOrDlc("en", receiver, callback)
3131
// which calls -> GetExistingDlcs -> NO libhandwriting dlc exists
3232
// -> Return error DLC_NOT_EXISTED.
3333
// Then it will be the client's duty to install the dlc and then calls
34-
// HandwritingModelLoader("en", receiver, callback).Load() again.
35-
class HandwritingModelLoader {
36-
public:
37-
HandwritingModelLoader(
38-
mojom::HandwritingRecognizerSpecPtr spec,
39-
mojo::PendingReceiver<mojom::HandwritingRecognizer> receiver,
40-
mojom::MachineLearningService::LoadHandwritingModelCallback callback);
41-
42-
~HandwritingModelLoader();
43-
44-
// Load handwriting model based on the comandline flag and language.
45-
void Load();
46-
47-
static constexpr char kOndeviceHandwritingSwitch[] = "ondevice_handwriting";
48-
static constexpr char kLibHandwritingDlcId[] = "libhandwriting";
49-
50-
private:
51-
friend class HandwritingModelLoaderTest;
52-
53-
// Called when the existing-dlc-list is returned.
54-
// Returns an error if libhandwriting is not in the existing-dlc-list.
55-
// Calls InstallDlc otherwise.
56-
void OnGetExistingDlcsComplete(
57-
const std::string& err,
58-
const dlcservice::DlcsWithContent& dlcs_with_content);
59-
60-
// Called when InstallDlc completes.
61-
// Returns an error if the `result.error` is not dlcservice::kErrorNone.
62-
// Calls mlservice to LoadHandwritingModel otherwise.
63-
void OnInstallDlcComplete(
64-
const chromeos::DlcserviceClient::InstallResult& result);
65-
66-
DlcserviceClient* dlc_client_;
67-
mojom::HandwritingRecognizerSpecPtr spec_;
68-
mojo::PendingReceiver<mojom::HandwritingRecognizer> receiver_;
69-
mojom::MachineLearningService::LoadHandwritingModelCallback callback_;
70-
71-
base::WeakPtrFactory<HandwritingModelLoader> weak_ptr_factory_;
72-
73-
DISALLOW_COPY_AND_ASSIGN(HandwritingModelLoader);
74-
};
34+
// LoadHandwritingModelFromRootfsOrDlc("en", receiver, callback) again.
35+
//
36+
// `dlc_client` should only be replaced with non-default value in unit tests.
37+
void LoadHandwritingModelFromRootfsOrDlc(
38+
mojom::HandwritingRecognizerSpecPtr spec,
39+
mojo::PendingReceiver<mojom::HandwritingRecognizer> receiver,
40+
mojom::MachineLearningService::LoadHandwritingModelCallback callback,
41+
DlcserviceClient* dlc_client = chromeos::DlcserviceClient::Get());
7542

7643
} // namespace machine_learning
7744
} // namespace chromeos

‎chromeos/services/machine_learning/public/cpp/handwriting_model_loader_unittest.cc

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,20 +18,16 @@ namespace chromeos {
1818
namespace machine_learning {
1919
using chromeos::machine_learning::mojom::LoadHandwritingModelResult;
2020

21+
constexpr char kOndeviceHandwritingSwitch[] = "ondevice_handwriting";
22+
constexpr char kLibHandwritingDlcId[] = "libhandwriting";
23+
2124
class HandwritingModelLoaderTest : public testing::Test {
2225
protected:
2326
void SetUp() override {
2427
ServiceConnection::UseFakeServiceConnectionForTesting(
2528
&fake_service_connection_);
2629
result_ = LoadHandwritingModelResult::DEPRECATED_MODEL_SPEC_ERROR;
27-
28-
loader_ = std::make_unique<HandwritingModelLoader>(
29-
mojom::HandwritingRecognizerSpec::New("en"),
30-
recognizer_.BindNewPipeAndPassReceiver(),
31-
base::BindOnce(
32-
&HandwritingModelLoaderTest::OnHandwritingModelLoaderComplete,
33-
base::Unretained(this)));
34-
loader_->dlc_client_ = &fake_client_;
30+
language_ = "en";
3531
}
3632

3733
// Callback that called when loader_->Load() is over to save the returned
@@ -44,14 +40,19 @@ class HandwritingModelLoaderTest : public testing::Test {
4440
// Runs loader_->Load() and check the returned result as expected.
4541
void ExpectLoadHandwritingModelResult(
4642
const LoadHandwritingModelResult expected_result) {
47-
loader_->Load();
43+
LoadHandwritingModelFromRootfsOrDlc(
44+
mojom::HandwritingRecognizerSpec::New(language_),
45+
recognizer_.BindNewPipeAndPassReceiver(),
46+
base::BindOnce(
47+
&HandwritingModelLoaderTest::OnHandwritingModelLoaderComplete,
48+
base::Unretained(this)),
49+
&fake_client_);
50+
4851
base::RunLoop().RunUntilIdle();
4952
EXPECT_EQ(result_, expected_result);
5053
}
5154

52-
void SetLanguage(const std::string& language) {
53-
loader_->spec_->language = language;
54-
}
55+
void SetLanguage(const std::string& language) { language_ = language; }
5556

5657
// Creates a dlc list with one dlc inside.
5758
void AddDlcsWithContent(const std::string& dlc_id) {
@@ -69,7 +70,7 @@ class HandwritingModelLoaderTest : public testing::Test {
6970
// Sets "ondevice_handwriting" value.
7071
void SetSwitchValue(const std::string& switch_value) {
7172
base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
72-
HandwritingModelLoader::kOndeviceHandwritingSwitch, switch_value);
73+
kOndeviceHandwritingSwitch, switch_value);
7374
}
7475

7576
private:
@@ -80,8 +81,8 @@ class HandwritingModelLoaderTest : public testing::Test {
8081
FakeDlcserviceClient fake_client_;
8182
FakeServiceConnectionImpl fake_service_connection_;
8283
LoadHandwritingModelResult result_;
84+
std::string language_;
8385
mojo::Remote<mojom::HandwritingRecognizer> recognizer_;
84-
std::unique_ptr<HandwritingModelLoader> loader_;
8586
};
8687

8788
TEST_F(HandwritingModelLoaderTest, HandwritingNotEnabled) {
@@ -122,7 +123,7 @@ TEST_F(HandwritingModelLoaderTest, LoadingWithoutDlcOnDevice) {
122123
TEST_F(HandwritingModelLoaderTest, DlcInstalledWithError) {
123124
SetSwitchValue("use_dlc");
124125

125-
AddDlcsWithContent(HandwritingModelLoader::kLibHandwritingDlcId);
126+
AddDlcsWithContent(kLibHandwritingDlcId);
126127
SetInstallError("random error");
127128

128129
// InstallDlc error should return DLC_INSTALL_ERROR.
@@ -133,7 +134,7 @@ TEST_F(HandwritingModelLoaderTest, DlcInstalledWithError) {
133134
TEST_F(HandwritingModelLoaderTest, DlcInstalledWithoutError) {
134135
SetSwitchValue("use_dlc");
135136

136-
AddDlcsWithContent(HandwritingModelLoader::kLibHandwritingDlcId);
137+
AddDlcsWithContent(kLibHandwritingDlcId);
137138
SetInstallError(dlcservice::kErrorNone);
138139

139140
// InstallDlc without an error should return success.

0 commit comments

Comments
 (0)