Skip to content

Commit

Permalink
Fix crash on request retry in KidsChromeManagementClient
Browse files Browse the repository at this point in the history
Response request is moved out of request structure during creation
of SimpleURLLoader. If the request is retried later, which happens
in case of HTTP_UNAUTHORIZED error, nullptr is dereferenced in to
set header.
This fix recrates response request if it was moved out. Landing it
fast to verify that the crashes are fixed on ToT.

(cherry picked from commit 160b4bf)

Bug: 1192222
Test: Manually
Change-Id: I736e8332673e1796f8c42255a45d2e7ab9d48334
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2818943
Reviewed-by: Dan S <danan@chromium.org>
Commit-Queue: Aga Wronska <agawronska@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#871192}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2824169
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Aga Wronska <agawronska@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/4472@{#45}
Cr-Branched-From: 3d60439-refs/heads/master@{#870763}
  • Loading branch information
Aga Wronska authored and Chromium LUCI CQ committed Apr 13, 2021
1 parent 05f7e65 commit 6d0e98c
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,15 @@ GetClassifyURLResponseProto(const std::string& response) {
return response_proto;
}

std::unique_ptr<network::ResourceRequest>
CreateResourceRequestForUrlClassifier() {
auto resource_request = std::make_unique<network::ResourceRequest>();
resource_request->url = GURL(kClassifyUrlRequestApiPath);
resource_request->method = "POST";
resource_request->credentials_mode = network::mojom::CredentialsMode::kOmit;
return resource_request;
}

} // namespace

struct KidsChromeManagementClient::KidsChromeManagementRequest {
Expand Down Expand Up @@ -168,11 +177,6 @@ void KidsChromeManagementClient::ClassifyURL(
KidsChromeManagementCallback callback) {
DVLOG(1) << "Checking URL: " << request_proto->url();

auto resource_request = std::make_unique<network::ResourceRequest>();
resource_request->url = GURL(kClassifyUrlRequestApiPath);
resource_request->method = "POST";
resource_request->credentials_mode = network::mojom::CredentialsMode::kOmit;

const net::NetworkTrafficAnnotationTag traffic_annotation =
net::DefineNetworkTrafficAnnotation(
"kids_chrome_management_client_classify_url", R"(
Expand Down Expand Up @@ -202,7 +206,7 @@ void KidsChromeManagementClient::ClassifyURL(

auto kids_chrome_request = std::make_unique<KidsChromeManagementRequest>(
std::move(request_proto), std::move(callback),
std::move(resource_request), traffic_annotation,
CreateResourceRequestForUrlClassifier(), traffic_annotation,
kClassifyUrlOauthConsumerName, kClassifyUrlKidPermissionScope,
RequestMethod::kClassifyUrl);

Expand All @@ -220,6 +224,12 @@ void KidsChromeManagementClient::StartFetching(
KidsChromeRequestList::iterator it) {
KidsChromeManagementRequest* req = it->get();

// This is a quick fix for https://crbug.com/1192222. `resource_request` is
// moved during creation of SimpleURLLoader. Retrying the request causes
// dereferencing nullptr. To avoid that recreate the `resource_request` here.
if (!req->resource_request)
req->resource_request = CreateResourceRequestForUrlClassifier();

signin::ScopeSet scopes{req->scope};

req->access_token_fetcher =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ class KidsChromeManagementClient : public KeyedService {
ErrorCode error);

scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
signin::IdentityManager* identity_manager_;
signin::IdentityManager* identity_manager_ = nullptr;

// List of requests in execution.
KidsChromeRequestList requests_in_progress_;
Expand Down

0 comments on commit 6d0e98c

Please sign in to comment.