Commit 16db4fb5 authored by Danyao Wang's avatar Danyao Wang Committed by Commit Bot

Revert "[Payment Request] Fixed potential use-after-free of RenderFrameHost."

This reverts commit 7f5e20cc.

Reason for revert: caused a new crash: crbug.com/1063048

Original change's description:
> [Payment Request] Fixed potential use-after-free of RenderFrameHost.
> 
> InstallablePaymentAppCrawler stores a RenderFrameHost raw pointer and
> uses it in asynchronously when DownloadAndDecodeWebAppIcon() is run as
> a callback for downloading the web app manifest. This creates a possible
> use-after-free situation.
> 
> This CL applies a limited fix: the RenderFrameHost* is converted to its
> GlobalFrameRoutingId to be stored inside InstallablePaymentAppCrawler.
> 
> A better fix is to use WeakPtr<RenderFrameHost> everywhere in payments
> code. However, this requires changing the content public API to expose
> a RenderFrameHost::GetWeakPtr() method. Since this CL is intended to be
> merged to M82, this more risky work is left as a followup for
> crbug.com/1058840.
> 
> The instantiation of InstallablePaymentAppCrawler happens synchronously
> with the instantiation of PaymentRequest, the risk of use-after-free as
> a result of PaymentRequest's storage of RenderFrameHost* is small.
> 
> Bug: 1061110
> Change-Id: I1428c6006201834ee341dd4546bd297116a5f380
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2106387
> Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
> Commit-Queue: Danyao Wang <danyao@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#751454}

TBR=rouslan@chromium.org,danyao@chromium.org

Change-Id: Iacb8ff851af05e01503630d671547f8d19dda05c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1061110
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2109828Reviewed-by: default avatarDanyao Wang <danyao@chromium.org>
Commit-Queue: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751709}
parent b64aa834
......@@ -33,6 +33,7 @@
namespace payments {
// TODO(crbug.com/782270): Use cache to accelerate crawling procedure.
// TODO(crbug.com/782270): Add integration tests for this class.
InstallablePaymentAppCrawler::InstallablePaymentAppCrawler(
const url::Origin& merchant_origin,
content::RenderFrameHost* initiator_render_frame_host,
......@@ -43,13 +44,7 @@ InstallablePaymentAppCrawler::InstallablePaymentAppCrawler(
: WebContentsObserver(web_contents),
log_(web_contents),
merchant_origin_(merchant_origin),
initiator_frame_routing_id_(
initiator_render_frame_host &&
initiator_render_frame_host->GetProcess()
? content::GlobalFrameRoutingId(
initiator_render_frame_host->GetProcess()->GetID(),
initiator_render_frame_host->GetRoutingID())
: content::GlobalFrameRoutingId()),
initiator_render_frame_host_(initiator_render_frame_host),
downloader_(downloader),
parser_(parser),
number_of_payment_method_manifest_to_download_(0),
......@@ -412,21 +407,13 @@ void InstallablePaymentAppCrawler::DownloadAndDecodeWebAppIcon(
number_of_web_app_icons_to_download_and_decode_++;
// If the initiator frame doesn't exists any more, e.g. the frame has
// navigated away, don't download the icon.
// TODO(crbug.com/1058840): Move this sanity check to ManifestIconDownloader
// after DownloadImage refactor is done.
content::RenderFrameHost* render_frame_host =
content::RenderFrameHost::FromID(initiator_frame_routing_id_);
if (!render_frame_host || !render_frame_host->IsCurrent() ||
content::WebContents::FromRenderFrameHost(render_frame_host) !=
web_contents()) {
// Shortcircuit to the done callback with an empty bitmap.
OnPaymentWebAppIconDownloadAndDecoded(method_manifest_url,
web_app_manifest_url, SkBitmap());
return;
content::GlobalFrameRoutingId frame_routing_id;
if (initiator_render_frame_host_ &&
initiator_render_frame_host_->GetProcess()) {
frame_routing_id = content::GlobalFrameRoutingId(
initiator_render_frame_host_->GetProcess()->GetID(),
initiator_render_frame_host_->GetRoutingID());
}
bool can_download_icon = content::ManifestIconDownloader::Download(
web_contents(), downloader_->FindTestServerURL(best_icon_url),
IconSizeCalculator::IdealIconHeight(native_view),
......@@ -436,7 +423,7 @@ void InstallablePaymentAppCrawler::DownloadAndDecodeWebAppIcon(
weak_ptr_factory_.GetWeakPtr(), method_manifest_url,
web_app_manifest_url),
false, /* square_only */
initiator_frame_routing_id_);
frame_routing_id);
DCHECK(can_download_icon);
}
......
......@@ -19,7 +19,6 @@
#include "components/payments/content/utility/payment_manifest_parser.h"
#include "components/payments/content/web_app_manifest.h"
#include "components/payments/core/payment_manifest_downloader.h"
#include "content/public/browser/global_routing_id.h"
#include "content/public/browser/web_contents_observer.h"
#include "third_party/blink/public/mojom/payments/payment_request.mojom.h"
#include "url/origin.h"
......@@ -112,7 +111,7 @@ class InstallablePaymentAppCrawler : public content::WebContentsObserver {
DeveloperConsoleLogger log_;
const url::Origin merchant_origin_;
const content::GlobalFrameRoutingId initiator_frame_routing_id_;
content::RenderFrameHost* initiator_render_frame_host_;
PaymentManifestDownloader* downloader_;
PaymentManifestParser* parser_;
FinishedCrawlingCallback callback_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment