Skip to content

Commit

Permalink
[Web Payments] Avoid early deletion of crawler for 2+ web app manifests
Browse files Browse the repository at this point in the history 
The previous fix[0] overlooked that
number_of_payment_method_manifest_to_download_ could be synchronously
reduced to 0 during the loop, as opposed to only in the last iteration.
This CL corrects that, by pre-allocating
number_of_payment_method_manifest_to_download_ ahead of the loop and
decrementing it every loop iteration. As such, only the last iteration
should be able to have it reduced to zero.

[0]: https://chromium-review.googlesource.com/c/chromium/src/+/3920030

Bug: 1378286
Change-Id: Ia2857a0775dc12aca1a83b3f3087836c9caad168
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3983012
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1063872}
  • Loading branch information
@stephenmcgruer
stephenmcgruer authored and Chromium LUCI CQ committed Oct 26, 2022
1 parent bfbd659 commit c5c1f9c
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 9 deletions.
43 changes: 37 additions & 6 deletions chrome/browser/payments/service_worker_payment_app_finder_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ class ServiceWorkerPaymentAppFinderBrowserTest : public InProcessBrowserTest {
ServiceWorkerPaymentAppFinderBrowserTest()
: alicepay_(net::EmbeddedTestServer::TYPE_HTTPS),
bobpay_(net::EmbeddedTestServer::TYPE_HTTPS),
charliepay_(net::EmbeddedTestServer::TYPE_HTTPS),
frankpay_(net::EmbeddedTestServer::TYPE_HTTPS),
georgepay_(net::EmbeddedTestServer::TYPE_HTTPS),
kylepay_(net::EmbeddedTestServer::TYPE_HTTPS),
Expand Down Expand Up @@ -88,6 +89,7 @@ class ServiceWorkerPaymentAppFinderBrowserTest : public InProcessBrowserTest {
void SetUpOnMainThread() override {
ASSERT_TRUE(StartTestServer("alicepay.com", &alicepay_));
ASSERT_TRUE(StartTestServer("bobpay.com", &bobpay_));
ASSERT_TRUE(StartTestServer("charliepay.com", &charliepay_));
ASSERT_TRUE(StartTestServer("frankpay.com", &frankpay_));
ASSERT_TRUE(StartTestServer("georgepay.com", &georgepay_));
ASSERT_TRUE(StartTestServer("kylepay.com", &kylepay_));
Expand Down Expand Up @@ -148,6 +150,8 @@ class ServiceWorkerPaymentAppFinderBrowserTest : public InProcessBrowserTest {
alicepay_.GetURL("alicepay.com", "/"));
downloader->AddTestServerURL("https://bobpay.com/",
bobpay_.GetURL("bobpay.com", "/"));
downloader->AddTestServerURL("https://charliepay.com/",
charliepay_.GetURL("charliepay.com", "/"));
downloader->AddTestServerURL("https://frankpay.com/",
frankpay_.GetURL("frankpay.com", "/"));
downloader->AddTestServerURL("https://georgepay.com/",
Expand Down Expand Up @@ -307,6 +311,11 @@ class ServiceWorkerPaymentAppFinderBrowserTest : public InProcessBrowserTest {
// payment method.
net::EmbeddedTestServer bobpay_;

// https://charliepay.com/webpay has a payment method manifest file that
// specifies multiple web app manifests (i.e., multiple payment handlers for
// the method).
net::EmbeddedTestServer charliepay_;

// https://frankpay.com/webpay supports payment apps from any origin.
net::EmbeddedTestServer frankpay_;

Expand Down Expand Up @@ -912,14 +921,36 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerPaymentAppFinderCSPCheckerBrowserTest,
}
}

// A range from 0 (inclusive) to 5 (exclusive) will test CSP checker reset:
// Variant of CSPCheckerResetDoesNotCrash, but with a payment method manifest
// that has multiple web applications specified (i.e., |default_applications|
// will have multiple entries).
IN_PROC_BROWSER_TEST_P(ServiceWorkerPaymentAppFinderCSPCheckerBrowserTest,
CSPCheckerResetDoesNotCrashWithTwoWebAppManifests) {
// Repeat lookups should have identical results, regardless of manifest cache
// state. To ensure that the cache has no effect, we repeat the test twice.
for (int i = 0; i < 2; ++i) {
reset_number_of_lookups();

GetAllPaymentAppsForMethods({"https://charliepay.com/webpay"});

EXPECT_TRUE(apps().empty());
EXPECT_TRUE(installable_apps().empty());
}
}

// A range from 0 (inclusive) to 6 (exclusive) will test CSP checker reset:
// 0: Before any CSP lookups.
// 1: After CSP lookup for https://kylepay.com/webpay.
// 2: After CSP lookup for https://kylepay.com/payment-method.json.
// 3: After CSP lookup for https://kylepay.com/app.json
// 4: No CSP checker reset at all, tested just in case.
// 1: After CSP lookup for payment method URL (e.g.,
// https://kylepay.com/webpay).
// 2: After CSP lookup for payment method manifest (e.g.,
// https://kylepay.com/payment-method.json).
// 3: After CSP lookup for first web app manifest (e.g.,
// https://kylepay.com/app.json).
// 4: After CSP lookup for second web app manifest, if it exists (e.g.,
// https://charliepay.com/prod.json).
// 5: No CSP checker reset at all, tested just in case.
INSTANTIATE_TEST_SUITE_P(Test,
ServiceWorkerPaymentAppFinderCSPCheckerBrowserTest,
testing::Range(0, 5));
testing::Range(0, 6));

} // namespace payments
16 changes: 13 additions & 3 deletions components/payments/content/installable_payment_app_crawler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ void InstallablePaymentAppCrawler::Start(
return;
}

// May cause this InstallablePaymentAppCrawler object to be synchronously
// deleted in the last iteration, so no code should come after the loop.
number_of_payment_method_manifest_to_download_ = manifests_to_download.size();
for (const auto& url : manifests_to_download) {
downloader_->DownloadPaymentMethodManifest(
Expand Down Expand Up @@ -173,17 +175,20 @@ void InstallablePaymentAppCrawler::OnPaymentMethodManifestParsed(
// to be deleted, so no code should be run after this loop.
//
// Note that only the last iteration of the loop can result in a deletion, as
// `number_of_web_app_manifest_to_download_` must be zero.
// `number_of_web_app_manifest_to_download_` must be zero for this to happen.
number_of_web_app_manifest_to_download_ += default_applications.size();
for (const auto& web_app_manifest_url : default_applications) {
if (downloaded_web_app_manifests_.find(web_app_manifest_url) !=
downloaded_web_app_manifests_.end()) {
// Do not download the same web app manifest again since a web app could
// be the default application of multiple payment methods.
number_of_web_app_manifest_to_download_--;
continue;
}

if (!IsSameOriginWith(method_manifest_url_after_redirects,
web_app_manifest_url)) {
number_of_web_app_manifest_to_download_--;
std::string error_message = base::ReplaceStringPlaceholders(
errors::kCrossOriginWebAppManifestNotAllowed,
{web_app_manifest_url.spec(),
Expand All @@ -199,21 +204,26 @@ void InstallablePaymentAppCrawler::OnPaymentMethodManifestParsed(
url::Origin::Create(web_app_manifest_url))
.status != blink::mojom::PermissionStatus::GRANTED) {
// Do not download the web app manifest if it is blocked.
number_of_web_app_manifest_to_download_--;
continue;
}

number_of_web_app_manifest_to_download_++;
downloaded_web_app_manifests_.insert(web_app_manifest_url);

if (method_manifest_url_after_redirects == web_app_manifest_url) {
// For this to happen, the payment manifest must have been valid which
// means its content should have been non-empty. If somehow we get here
// but content is empty, it would cause a synchronous deletion of 'this',
// so guard against that.
CHECK(!content.empty());
OnPaymentWebAppManifestDownloaded(
method_manifest_url, web_app_manifest_url, web_app_manifest_url,
content, /*error_message=*/"");
continue;
}

// May cause this InstallablePaymentAppCrawler object to be synchronously
// deleted.
// deleted in the last iteration of the loop.
downloader_->DownloadWebAppManifest(
url::Origin::Create(method_manifest_url_after_redirects),
web_app_manifest_url,
Expand Down

0 comments on commit c5c1f9c

Please sign in to comment.