Commit 69425800 authored by Anqing Zhao's avatar Anqing Zhao Committed by Commit Bot

Add logs to fingure out the response code in the WebstoreDataFetcher

Some cases in the KioskAppManagerBrowsertest are flaky because
SimpleURLLoaderImpl::OnReceiveResponse may get non-500 failure response
code. I suspect they are actually 400 error, but don't have enough
evidence.

Add some additional logic to log this information to figure out the root
cause.

Bug: 1090937
Change-Id: Ia343519dbd06311dfde2bccd25d9ebf86bb72566
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2257857Reviewed-by: default avatarDavid Bertoni <dbertoni@chromium.org>
Commit-Queue: Anqing Zhao <anqing@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781510}
parent e1fb25ff
...@@ -30,6 +30,7 @@ ...@@ -30,6 +30,7 @@
#include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h" #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
#include "chrome/browser/chromeos/policy/device_local_account.h" #include "chrome/browser/chromeos/policy/device_local_account.h"
#include "chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h" #include "chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h"
#include "chrome/browser/extensions/webstore_data_fetcher.h"
#include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser.h"
#include "chrome/common/chrome_paths.h" #include "chrome/common/chrome_paths.h"
#include "chrome/common/chrome_switches.h" #include "chrome/common/chrome_switches.h"
...@@ -245,6 +246,9 @@ class KioskAppManagerTest : public InProcessBrowserTest { ...@@ -245,6 +246,9 @@ class KioskAppManagerTest : public InProcessBrowserTest {
base::PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir); base::PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
embedded_test_server()->ServeFilesFromDirectory(test_data_dir); embedded_test_server()->ServeFilesFromDirectory(test_data_dir);
// Log the response code for WebstoreDataFetcher instance if it is not 200.
extensions::WebstoreDataFetcher::SetLogResponseCodeForTesting(true);
// Don't spin up the IO thread yet since no threads are allowed while // Don't spin up the IO thread yet since no threads are allowed while
// spawning sandbox host process. See crbug.com/322732. // spawning sandbox host process. See crbug.com/322732.
ASSERT_TRUE(embedded_test_server()->InitializeAndListen()); ASSERT_TRUE(embedded_test_server()->InitializeAndListen());
......
...@@ -22,11 +22,14 @@ ...@@ -22,11 +22,14 @@
#include "services/network/public/cpp/resource_request.h" #include "services/network/public/cpp/resource_request.h"
#include "services/network/public/cpp/simple_url_loader.h" #include "services/network/public/cpp/simple_url_loader.h"
#include "services/network/public/mojom/url_loader_factory.mojom.h" #include "services/network/public/mojom/url_loader_factory.mojom.h"
#include "services/network/public/mojom/url_response_head.mojom.h"
namespace { namespace {
const char kInvalidWebstoreResponseError[] = "Invalid Chrome Web Store reponse"; const char kInvalidWebstoreResponseError[] = "Invalid Chrome Web Store reponse";
bool g_log_response_code_for_testing_ = false;
} // namespace } // namespace
namespace extensions { namespace extensions {
...@@ -41,6 +44,11 @@ WebstoreDataFetcher::WebstoreDataFetcher(WebstoreDataFetcherDelegate* delegate, ...@@ -41,6 +44,11 @@ WebstoreDataFetcher::WebstoreDataFetcher(WebstoreDataFetcherDelegate* delegate,
WebstoreDataFetcher::~WebstoreDataFetcher() {} WebstoreDataFetcher::~WebstoreDataFetcher() {}
// static
void WebstoreDataFetcher::SetLogResponseCodeForTesting(bool enabled) {
g_log_response_code_for_testing_ = enabled;
}
void WebstoreDataFetcher::Start( void WebstoreDataFetcher::Start(
network::mojom::URLLoaderFactory* url_loader_factory) { network::mojom::URLLoaderFactory* url_loader_factory) {
GURL webstore_data_url(extension_urls::GetWebstoreItemJsonDataURL(id_)); GURL webstore_data_url(extension_urls::GetWebstoreItemJsonDataURL(id_));
...@@ -90,12 +98,29 @@ void WebstoreDataFetcher::Start( ...@@ -90,12 +98,29 @@ void WebstoreDataFetcher::Start(
network::SimpleURLLoader::RETRY_ON_5XX | network::SimpleURLLoader::RETRY_ON_5XX |
network::SimpleURLLoader::RETRY_ON_NETWORK_CHANGE); network::SimpleURLLoader::RETRY_ON_NETWORK_CHANGE);
} }
if (g_log_response_code_for_testing_) {
simple_url_loader_->SetOnResponseStartedCallback(base::BindOnce(
&WebstoreDataFetcher::OnResponseStarted, base::Unretained(this)));
}
simple_url_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie( simple_url_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
url_loader_factory, url_loader_factory,
base::BindOnce(&WebstoreDataFetcher::OnSimpleLoaderComplete, base::BindOnce(&WebstoreDataFetcher::OnSimpleLoaderComplete,
base::Unretained(this))); base::Unretained(this)));
} }
void WebstoreDataFetcher::OnResponseStarted(
const GURL& final_url,
const network::mojom::URLResponseHead& response_head) {
if (!response_head.headers)
return;
int response_code = response_head.headers->response_code();
if (response_code != 200)
LOG(ERROR) << "Response_code: " << response_code;
}
void WebstoreDataFetcher::OnJsonParsed( void WebstoreDataFetcher::OnJsonParsed(
data_decoder::DataDecoder::ValueOrError result) { data_decoder::DataDecoder::ValueOrError result) {
if (!result.value) { if (!result.value) {
......
...@@ -17,6 +17,7 @@ namespace network { ...@@ -17,6 +17,7 @@ namespace network {
class SimpleURLLoader; class SimpleURLLoader;
namespace mojom { namespace mojom {
class URLLoaderFactory; class URLLoaderFactory;
class URLResponseHead;
} // namespace mojom } // namespace mojom
} // namespace network } // namespace network
...@@ -33,6 +34,8 @@ class WebstoreDataFetcher : public base::SupportsWeakPtr<WebstoreDataFetcher> { ...@@ -33,6 +34,8 @@ class WebstoreDataFetcher : public base::SupportsWeakPtr<WebstoreDataFetcher> {
const std::string webstore_item_id); const std::string webstore_item_id);
~WebstoreDataFetcher(); ~WebstoreDataFetcher();
static void SetLogResponseCodeForTesting(bool enabled);
void Start(network::mojom::URLLoaderFactory* url_loader_factory); void Start(network::mojom::URLLoaderFactory* url_loader_factory);
void set_max_auto_retries(int max_retries) { void set_max_auto_retries(int max_retries) {
...@@ -41,6 +44,8 @@ class WebstoreDataFetcher : public base::SupportsWeakPtr<WebstoreDataFetcher> { ...@@ -41,6 +44,8 @@ class WebstoreDataFetcher : public base::SupportsWeakPtr<WebstoreDataFetcher> {
private: private:
void OnJsonParsed(data_decoder::DataDecoder::ValueOrError result); void OnJsonParsed(data_decoder::DataDecoder::ValueOrError result);
void OnResponseStarted(const GURL& final_url,
const network::mojom::URLResponseHead& response_head);
void OnSimpleLoaderComplete(std::unique_ptr<std::string> response_body); void OnSimpleLoaderComplete(std::unique_ptr<std::string> response_body);
WebstoreDataFetcherDelegate* delegate_; WebstoreDataFetcherDelegate* delegate_;
......
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