Skip to content

Commit 07d9122

Browse files
MyidShinsvillar
authored andcommitted
Fix DCHECK crashes by Password Manager impl
This patch fixes the DCHECK crash caused by the implementation of Passworkd Manager. - Added WolvicImageDecoder since IdentityManager requires a valid ImageDecoder instance when initializing. - Nullable Java string should check empty before calling ConvertJavaStringTo{UTF8/UTF16} - Avoid DCHECK through a fake url of credential since the update status requires the credential info or the saved PasswordForm when adding the new ID/PW via PasswordFormManagerForUI::Update - Remove AffiliationService to delegate the PasswordForm check in Wolvic Implementations
1 parent 22c3ecc commit 07d9122

9 files changed

+216
-28
lines changed

wolvic/BUILD.gn

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ shared_library("libcontent_native") {
3131
"../content/shell/common/shell_switches.h",
3232
"browser/autocomplete/wolvic_autofill_client.cc",
3333
"browser/autocomplete/wolvic_autofill_client.h",
34+
"browser/autocomplete/wolvic_image_decoder.cc",
35+
"browser/autocomplete/wolvic_image_decoder.h",
3436
"browser/autocomplete/wolvic_password_form_util.cc",
3537
"browser/autocomplete/wolvic_password_form_util.h",
3638
"browser/autocomplete/wolvic_password_manager_client.cc",
Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
// Copyright 2024 The Chromium Authors
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#include "wolvic/browser/autocomplete/wolvic_image_decoder.h"
6+
7+
#include <vector>
8+
9+
#include "base/functional/callback.h"
10+
#include "base/memory/raw_ptr.h"
11+
#include "base/ranges/algorithm.h"
12+
#include "base/task/sequenced_task_runner.h"
13+
#include "base/task/single_thread_task_runner.h"
14+
#include "ipc/ipc_channel.h"
15+
#include "services/data_decoder/public/cpp/data_decoder.h"
16+
#include "services/data_decoder/public/cpp/decode_image.h"
17+
#include "ui/gfx/geometry/size.h"
18+
#include "ui/gfx/image/image.h"
19+
20+
// Referred to image_decoder_impl.cc & image_decoder.cc in //chrome
21+
namespace wolvic {
22+
23+
namespace {
24+
25+
const int64_t kMaxImageSizeInBytes =
26+
static_cast<int64_t>(IPC::Channel::kMaximumMessageSize);
27+
28+
void RunDecodeCallbackOnTaskRunner(
29+
data_decoder::DecodeImageCallback callback,
30+
scoped_refptr<base::SequencedTaskRunner> task_runner,
31+
const SkBitmap& image) {
32+
task_runner->PostTask(FROM_HERE, base::BindOnce(std::move(callback), image));
33+
}
34+
35+
// Note that this is always called on the thread which initiated the
36+
// corresponding data_decoder::DecodeImage request.
37+
void OnDecodeImageDone(
38+
base::OnceCallback<void()> fail_callback,
39+
base::OnceCallback<void(const SkBitmap&)> success_callback,
40+
const SkBitmap& image) {
41+
if (!image.isNull() && !image.empty())
42+
std::move(success_callback).Run(image);
43+
else
44+
std::move(fail_callback).Run();
45+
}
46+
47+
} // namespace
48+
49+
// A request for decoding an image.
50+
class WolvicImageDecoder::DecodeImageRequest {
51+
public:
52+
DecodeImageRequest(WolvicImageDecoder* decoder,
53+
const std::string& image_data,
54+
data_decoder::DataDecoder* data_decoder,
55+
const gfx::Size& desired_image_frame_size,
56+
image_fetcher::ImageDecodedCallback callback)
57+
: task_runner_(base::SingleThreadTaskRunner::GetCurrentDefault()),
58+
data_decoder_(data_decoder),
59+
callback_(std::move(callback)) {
60+
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
61+
base::span<const uint8_t> image_data_span(
62+
base::as_bytes(base::make_span(image_data)));
63+
64+
auto decode_callback =
65+
base::BindOnce(&OnDecodeImageDone,
66+
base::BindOnce(&DecodeImageRequest::OnDecodeImageFailed,
67+
base::Unretained(this)),
68+
base::BindOnce(&DecodeImageRequest::OnDecodeImageSucceeded,
69+
base::Unretained(this)));
70+
71+
data_decoder::DecodeImage(
72+
data_decoder, image_data_span,
73+
data_decoder::mojom::ImageCodec::kDefault, /*shrink_to_fit=*/false,
74+
kMaxImageSizeInBytes, desired_image_frame_size,
75+
base::BindOnce(&RunDecodeCallbackOnTaskRunner, std::move(decode_callback),
76+
std::move(task_runner_)));
77+
}
78+
79+
DecodeImageRequest(const DecodeImageRequest&) = delete;
80+
DecodeImageRequest& operator=(const DecodeImageRequest&) = delete;
81+
82+
~DecodeImageRequest() = default;
83+
84+
private:
85+
SEQUENCE_CHECKER(sequence_checker_);
86+
87+
// Runs the callback and remove the request from the internal request queue.
88+
void RunCallbackAndRemoveRequest(const gfx::Image& image) {
89+
std::move(callback_).Run(image);
90+
91+
// This must be the last line in the method body.
92+
decoder_->RemoveDecodeImageRequest(this);
93+
}
94+
95+
void OnDecodeImageSucceeded(const SkBitmap& decoded_image) {
96+
DCHECK(task_runner_->RunsTasksInCurrentSequence());
97+
gfx::Image image(gfx::Image::CreateFrom1xBitmap(decoded_image));
98+
RunCallbackAndRemoveRequest(image);
99+
}
100+
101+
void OnDecodeImageFailed() {
102+
DCHECK(task_runner_->RunsTasksInCurrentSequence());
103+
RunCallbackAndRemoveRequest(gfx::Image());
104+
}
105+
106+
const scoped_refptr<base::SequencedTaskRunner> task_runner_;
107+
108+
raw_ptr<WolvicImageDecoder> decoder_;
109+
raw_ptr<data_decoder::DataDecoder> data_decoder_;
110+
// The callback to call after the request completed.
111+
image_fetcher::ImageDecodedCallback callback_;
112+
};
113+
114+
WolvicImageDecoder::WolvicImageDecoder() = default;
115+
116+
WolvicImageDecoder::~WolvicImageDecoder() = default;
117+
118+
void WolvicImageDecoder::DecodeImage(
119+
const std::string& image_data,
120+
const gfx::Size& desired_image_frame_size,
121+
data_decoder::DataDecoder* data_decoder,
122+
image_fetcher::ImageDecodedCallback callback) {
123+
auto decode_image_request = std::make_unique<DecodeImageRequest>(
124+
this, image_data, data_decoder, desired_image_frame_size, std::move(callback));
125+
126+
decode_image_requests_.push_back(std::move(decode_image_request));
127+
}
128+
129+
void WolvicImageDecoder::RemoveDecodeImageRequest(DecodeImageRequest* request) {
130+
// Remove the finished request from the request queue.
131+
auto request_it =
132+
base::ranges::find(decode_image_requests_, request,
133+
&std::unique_ptr<DecodeImageRequest>::get);
134+
DCHECK(request_it != decode_image_requests_.end());
135+
decode_image_requests_.erase(request_it);
136+
}
137+
138+
} // namespace wolvic
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// Copyright 2024 The Chromium Authors
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#ifndef WOLVIC_BROWSER_AUTOCOMPLETE_WOLVIC_IMAGE_DECODER_H_
6+
#define WOLVIC_BROWSER_AUTOCOMPLETE_WOLVIC_IMAGE_DECODER_H_
7+
8+
#include <string>
9+
10+
#include "base/functional/bind.h"
11+
#include "components/image_fetcher/core/image_decoder.h"
12+
13+
namespace gfx {
14+
class Size;
15+
} // namespace gfx
16+
17+
namespace wolvic {
18+
19+
// image_fetcher::ImageDecoder implementation.
20+
class WolvicImageDecoder : public image_fetcher::ImageDecoder {
21+
public:
22+
WolvicImageDecoder();
23+
24+
WolvicImageDecoder(const WolvicImageDecoder&) = delete;
25+
WolvicImageDecoder& operator=(const WolvicImageDecoder&) = delete;
26+
27+
~WolvicImageDecoder() override;
28+
29+
void DecodeImage(const std::string& image_data,
30+
const gfx::Size& desired_image_frame_size,
31+
data_decoder::DataDecoder* data_decoder,
32+
image_fetcher::ImageDecodedCallback callback) override;
33+
34+
private:
35+
class DecodeImageRequest;
36+
37+
// Removes the passed image decode |request| from the internal request queue.
38+
void RemoveDecodeImageRequest(DecodeImageRequest* request);
39+
40+
// All active image decoding requests.
41+
std::vector<std::unique_ptr<DecodeImageRequest>> decode_image_requests_;
42+
};
43+
44+
} // namespace wolvic
45+
46+
#endif // WOLVIC_BROWSER_AUTOCOMPLETE_WOLVIC_IMAGE_DECODER_H_

wolvic/browser/autocomplete/wolvic_password_form_util.cc

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "components/password_manager/core/browser/password_form.h"
1010
#include "components/password_manager/core/browser/password_manager_util.h"
1111
#include "wolvic/jni_headers/PasswordForm_jni.h"
12+
#include "url/origin.h"
1213

1314
using base::android::ConvertJavaStringToUTF8;
1415
using base::android::ConvertJavaStringToUTF16;
@@ -54,15 +55,29 @@ GetPasswordFormFromJavaObject(
5455
Java_PasswordForm_getPassword(env, j_password_form));
5556
form->url = GURL(ConvertJavaStringToUTF8(
5657
Java_PasswordForm_getOrigin(env, j_password_form)));
57-
form->action = GURL(ConvertJavaStringToUTF8(
58-
Java_PasswordForm_getFormActionOrigin(env, j_password_form)));
59-
form->signon_realm = ConvertJavaStringToUTF8(
60-
Java_PasswordForm_getHttpRealm(env, j_password_form));
61-
if (form->signon_realm.empty()) {
58+
59+
ScopedJavaLocalRef<jstring> jaction =
60+
Java_PasswordForm_getFormActionOrigin(env, j_password_form);
61+
if (jaction.obj() && env->GetStringLength(jaction.obj()) > 0) {
62+
form->action = GURL(ConvertJavaStringToUTF8(env, jaction));
63+
}
64+
65+
ScopedJavaLocalRef<jstring> jsignon_realm =
66+
Java_PasswordForm_getHttpRealm(env, j_password_form);
67+
if (jsignon_realm.obj() && env->GetStringLength(jsignon_realm.obj()) > 0) {
68+
form->signon_realm = ConvertJavaStringToUTF8(env, jsignon_realm);
69+
} else {
6270
form->signon_realm = password_manager::GetSignonRealm(form->url);
6371
}
64-
form->keychain_identifier = ConvertJavaStringToUTF8(
65-
Java_PasswordForm_getGuid(env, j_password_form));
72+
73+
ScopedJavaLocalRef<jstring> jguid =
74+
Java_PasswordForm_getGuid(env, j_password_form);
75+
if (jguid.obj() && env->GetStringLength(jguid.obj()) > 0) {
76+
form->keychain_identifier = ConvertJavaStringToUTF8(env, jguid);
77+
}
78+
79+
// We always use the profile store.
80+
form->in_store = password_manager::PasswordForm::Store::kProfileStore;
6681
return form;
6782
}
6883

wolvic/browser/autocomplete/wolvic_password_manager_client.cc

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,9 @@ void WolvicPasswordManagerClient::OnDismissed(JNIEnv* env) {
107107

108108
void WolvicPasswordManagerClient::HandleSavePassword(
109109
std::unique_ptr<password_manager::PasswordFormManagerForUI> form_to_save,
110-
const password_manager::PasswordForm& saved_form) {
110+
password_manager::PasswordForm& saved_form) {
111+
// Avoid DCHECK when adding the new ID/PW via PasswordFormManagerForUI::Update
112+
saved_form.federation_origin = url::Origin::Create(GURL(saved_form.url));
111113
form_to_save->Update(saved_form);
112114
form_to_save->Save();
113115
}

wolvic/browser/autocomplete/wolvic_password_manager_client.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ class WolvicPasswordManagerClient
157157

158158
void HandleSavePassword(
159159
std::unique_ptr<password_manager::PasswordFormManagerForUI> form_to_save,
160-
const password_manager::PasswordForm& saved_form);
160+
password_manager::PasswordForm& saved_form);
161161

162162
// content::WebContentsObserver overrides.
163163
void PrimaryPageChanged(content::Page& page) override;
@@ -178,7 +178,7 @@ class WolvicPasswordManagerClient
178178
const password_manager::SyncCredentialsFilter credentials_filter_;
179179

180180
// A callback to be invoked when user accept to save the password.
181-
base::OnceCallback<void(const password_manager::PasswordForm& saved_form)>
181+
base::OnceCallback<void(password_manager::PasswordForm& saved_form)>
182182
save_password_callback_;
183183

184184
// A callback to be invoked when user selects a credential.

wolvic/browser/autocomplete/wolvic_password_store_backend.cc

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,6 @@ void WolvicPasswordStoreBackend::FillMatchingLoginsAsync(
239239
void WolvicPasswordStoreBackend::GetGroupedMatchingLoginsAsync(
240240
const password_manager::PasswordFormDigest& form_digest,
241241
password_manager::LoginsOrErrorReply callback) {
242-
DCHECK(affiliated_match_helper_);
243242
GetLoginsWithAffiliationsRequestHandler(
244243
form_digest, this, affiliated_match_helper_.get(), std::move(callback));
245244
}

wolvic/wolvic_browser_context.cc

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@
5353
#include "content/test/mock_background_sync_controller.h"
5454
#include "content/test/mock_reduce_accept_language_controller_delegate.h"
5555
#include "third_party/blink/public/common/origin_trials/trial_token_validator.h"
56+
#include "wolvic/browser/autocomplete/wolvic_image_decoder.h"
5657
#include "wolvic/browser/autocomplete/wolvic_password_store_backend.h"
5758
#include "wolvic/browser/downloads/wolvic_download_manager_delegate.h"
5859
#include "wolvic/wolvic_permission_manager.h"
@@ -132,23 +133,9 @@ void WolvicBrowserContext::CreatePasswordStore() {
132133
->GetURLLoaderFactoryForBrowserProcess();
133134
auto backend_task_runner = base::ThreadPool::CreateSequencedTaskRunner(
134135
{base::MayBlock(), base::TaskPriority::USER_VISIBLE});
135-
affiliation_service_ =
136-
std::make_unique<password_manager::AffiliationServiceImpl>(
137-
url_loader_factory, backend_task_runner);
138-
139-
base::FilePath database_path =
140-
GetPrefStorePath().Append(
141-
password_manager::kAffiliationDatabaseFileName);
142-
affiliation_service_->Init(content::GetNetworkConnectionTracker(),
143-
database_path);
144-
145-
auto affiliated_match_helper =
146-
std::make_unique<password_manager::AffiliatedMatchHelper>(
147-
affiliation_service_.get());
148-
149136
password_store_ = new password_manager::PasswordStore(
150137
std::make_unique<wolvic::WolvicPasswordStoreBackend>());
151-
password_store_->Init(GetPrefService(), std::move(affiliated_match_helper));
138+
password_store_->Init(GetPrefService(), nullptr);
152139
}
153140

154141
void WolvicBrowserContext::CreateIdentityManger() {
@@ -161,6 +148,7 @@ void WolvicBrowserContext::CreateIdentityManger() {
161148
params.network_connection_tracker = content::GetNetworkConnectionTracker();
162149
params.pref_service = GetPrefService();
163150
params.profile_path = GetPrefStorePath();
151+
params.image_decoder = std::make_unique<wolvic::WolvicImageDecoder>();
164152
identity_manager_ = signin::BuildIdentityManager(&params);
165153
}
166154

wolvic/wolvic_browser_context.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
#include "base/files/file_path.h"
1111
#include "base/memory/raw_ptr.h"
1212
#include "components/autofill/core/browser/autocomplete_history_manager.h"
13-
#include "components/password_manager/core/browser/affiliation/affiliation_service_impl.h"
1413
#include "components/password_manager/core/browser/field_info_manager.h"
1514
#include "components/password_manager/core/browser/password_store.h"
1615
#include "components/prefs/pref_name_set.h"
@@ -134,7 +133,6 @@ class WolvicBrowserContext : public BrowserContext,
134133
base::FilePath path_;
135134
std::unique_ptr<SimpleFactoryKey> key_;
136135
std::unique_ptr<visitedlink::VisitedLinkWriter> visitedlink_writer_;
137-
std::unique_ptr<password_manager::AffiliationServiceImpl> affiliation_service_;
138136
std::unique_ptr<autofill::AutocompleteHistoryManager> autocomplete_history_manager_;
139137
scoped_refptr<password_manager::PasswordStore> password_store_;
140138
std::unique_ptr<password_manager::FieldInfoManager> field_info_manager_;

0 commit comments

Comments
 (0)