Skip to content

Commit

Permalink
Clean up error handling code in WebBundleURLLoaderFactory
Browse files Browse the repository at this point in the history 
This is in preparation of crbug.com/1176493 and makes it easier to add
more error conditions.

Pure refactoring, no behavior changes.

Bug: 1176493
Change-Id: I47b62b774e5f0345d20a6dc969a7ef6bce21a0c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2690768
Reviewed-by: Hayato Ito <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Tsuyoshi Horo <[email protected]>
Commit-Queue: Kunihiko Sakamoto <[email protected]>
Cr-Commit-Position: refs/heads/master@{#853937}
  • Loading branch information
@irori
irori authored and Chromium LUCI CQ committed Feb 15, 2021
1 parent 3dacebd commit d929f2a
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 57 deletions.
89 changes: 36 additions & 53 deletions services/network/web_bundle_url_loader_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,6 @@ namespace {

constexpr size_t kBlockedBodyAllocationSize = 1;

void RecordLoadResult(
WebBundleURLLoaderFactory::SubresourceWebBundleLoadResult result) {
base::UmaHistogramEnumeration("SubresourceWebBundles.LoadResult", result);
}

void DeleteProducerAndRunCallback(
std::unique_ptr<mojo::DataPipeProducer> producer,
base::OnceCallback<void(MojoResult result)> callback,
Expand Down Expand Up @@ -423,6 +418,11 @@ base::WeakPtr<WebBundleURLLoaderFactory> WebBundleURLLoaderFactory::GetWeakPtr()
return weak_ptr_factory_.GetWeakPtr();
}

bool WebBundleURLLoaderFactory::HasError() const {
return load_result_.has_value() &&
*load_result_ != SubresourceWebBundleLoadResult::kSuccess;
}

void WebBundleURLLoaderFactory::SetBundleStream(
mojo::ScopedDataPipeConsumerHandle body) {
mojo::PendingRemote<web_package::mojom::BundleDataSource> data_source;
Expand Down Expand Up @@ -461,11 +461,7 @@ void WebBundleURLLoaderFactory::StartSubresourceRequest(
URLLoader* loader =
new URLLoader(std::move(receiver), url_request, std::move(client),
request_initiator_origin_lock_);
if (metadata_error_) {
loader->OnFail(net::ERR_INVALID_WEB_BUNDLE);
return;
}
if (quota_exceeded_error_) {
if (HasError()) {
loader->OnFail(net::ERR_INVALID_WEB_BUNDLE);
return;
}
Expand Down Expand Up @@ -499,29 +495,36 @@ void WebBundleURLLoaderFactory::StartLoad(URLLoader* loader) {
weak_ptr_factory_.GetWeakPtr(), loader->GetWeakPtr()));
}

void WebBundleURLLoaderFactory::ReportErrorAndCancelPendingLoaders(
SubresourceWebBundleLoadResult result,
mojom::WebBundleErrorType error,
const std::string& message) {
MaybeRecordLoadResult(result);
web_bundle_handle_->OnWebBundleError(error, message);
auto pending_loaders = std::move(pending_loaders_);
for (auto loader : pending_loaders) {
if (loader)
loader->OnFail(net::ERR_INVALID_WEB_BUNDLE);
}

source_.reset();
parser_.reset();
}

void WebBundleURLLoaderFactory::OnMetadataParsed(
web_package::mojom::BundleMetadataPtr metadata,
web_package::mojom::BundleMetadataParseErrorPtr error) {
TRACE_EVENT0("loading", "WebBundleURLLoaderFactory::OnMetadataParsed");
if (error) {
metadata_error_ = std::move(error);
MaybeRecordLoadResult();
web_bundle_handle_->OnWebBundleError(
mojom::WebBundleErrorType::kMetadataParseError,
metadata_error_->message);
auto pending_loaders = std::move(pending_loaders_);
for (auto loader : pending_loaders) {
if (loader)
loader->OnFail(net::ERR_INVALID_WEB_BUNDLE);
}

source_.reset();
parser_.reset();
ReportErrorAndCancelPendingLoaders(
SubresourceWebBundleLoadResult::kMetadataParseError,
mojom::WebBundleErrorType::kMetadataParseError, error->message);
return;
}

metadata_ = std::move(metadata);
MaybeRecordLoadResult();
if (data_completed_)
MaybeRecordLoadResult(SubresourceWebBundleLoadResult::kSuccess);
for (auto loader : pending_loaders_)
StartLoad(loader.get());
pending_loaders_.clear();
Expand Down Expand Up @@ -586,46 +589,26 @@ void WebBundleURLLoaderFactory::OnResponseParsed(

void WebBundleURLLoaderFactory::OnMemoryQuotaExceeded() {
TRACE_EVENT0("loading", "WebBundleURLLoaderFactory::OnMemoryQuotaExceeded");
quota_exceeded_error_ = true;
MaybeRecordLoadResult();
web_bundle_handle_->OnWebBundleError(
ReportErrorAndCancelPendingLoaders(
SubresourceWebBundleLoadResult::kMemoryQuotaExceeded,
mojom::WebBundleErrorType::kMemoryQuotaExceeded,
"Memory quota exceeded. Currently, there is an upper limit on the total "
"size of subresource web bundles in a process. See "
"https://crbug.com/1154140 for more details.");
auto pending_loaders = std::move(pending_loaders_);
for (auto loader : pending_loaders) {
if (loader) {
loader->OnFail(net::ERR_INVALID_WEB_BUNDLE);
}
}
source_.reset();
parser_.reset();
}

void WebBundleURLLoaderFactory::OnDataCompleted() {
data_completed_ = true;
MaybeRecordLoadResult();
if (metadata_)
MaybeRecordLoadResult(SubresourceWebBundleLoadResult::kSuccess);
}

void WebBundleURLLoaderFactory::MaybeRecordLoadResult() {
if (load_result_recorded_)
return;
if (quota_exceeded_error_) {
RecordLoadResult(SubresourceWebBundleLoadResult::kMemoryQuotaExceeded);
load_result_recorded_ = true;
return;
}
if (metadata_error_) {
RecordLoadResult(SubresourceWebBundleLoadResult::kMetadataParseError);
load_result_recorded_ = true;
void WebBundleURLLoaderFactory::MaybeRecordLoadResult(
SubresourceWebBundleLoadResult result) {
if (load_result_.has_value())
return;
}
if (metadata_ && data_completed_) {
RecordLoadResult(SubresourceWebBundleLoadResult::kSuccess);
load_result_recorded_ = true;
return;
}
load_result_ = result;
base::UmaHistogramEnumeration("SubresourceWebBundles.LoadResult", result);
}

} // namespace network
10 changes: 6 additions & 4 deletions services/network/web_bundle_url_loader_factory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) WebBundleURLLoaderFactory {
base::WeakPtr<WebBundleURLLoaderFactory> GetWeakPtr() const;

void SetBundleStream(mojo::ScopedDataPipeConsumerHandle body);
void ReportErrorAndCancelPendingLoaders(SubresourceWebBundleLoadResult result,
mojom::WebBundleErrorType error,
const std::string& message);
mojo::PendingRemote<mojom::URLLoaderClient> WrapURLLoaderClient(
mojo::PendingRemote<mojom::URLLoaderClient> wrapped);

Expand All @@ -53,6 +56,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) WebBundleURLLoaderFactory {
class BundleDataSource;
class URLLoader;

bool HasError() const;
void StartLoad(URLLoader* loader);
void OnMetadataParsed(web_package::mojom::BundleMetadataPtr metadata,
web_package::mojom::BundleMetadataParseErrorPtr error);
Expand All @@ -61,7 +65,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) WebBundleURLLoaderFactory {
web_package::mojom::BundleResponseParseErrorPtr error);
void OnMemoryQuotaExceeded();
void OnDataCompleted();
void MaybeRecordLoadResult();
void MaybeRecordLoadResult(SubresourceWebBundleLoadResult result);

GURL bundle_url_;
mojo::Remote<mojom::WebBundleHandle> web_bundle_handle_;
Expand All @@ -71,10 +75,8 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) WebBundleURLLoaderFactory {
std::unique_ptr<BundleDataSource> source_;
mojo::Remote<web_package::mojom::WebBundleParser> parser_;
web_package::mojom::BundleMetadataPtr metadata_;
web_package::mojom::BundleMetadataParseErrorPtr metadata_error_;
bool quota_exceeded_error_ = false;
base::Optional<SubresourceWebBundleLoadResult> load_result_;
bool data_completed_ = false;
bool load_result_recorded_ = false;
std::vector<base::WeakPtr<URLLoader>> pending_loaders_;

base::WeakPtrFactory<WebBundleURLLoaderFactory> weak_ptr_factory_{this};
Expand Down

0 comments on commit d929f2a

Please sign in to comment.