Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1001)

Unified Diff: content/browser/service_worker/service_worker_write_to_cache_job.cc

Issue 1467603002: URLRequestJob: change ReadRawData contract (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: add done_reading_called_ to MockNetworkTransaction Created 5 years, 1 month ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: content/browser/service_worker/service_worker_write_to_cache_job.cc
diff --git a/content/browser/service_worker/service_worker_write_to_cache_job.cc b/content/browser/service_worker/service_worker_write_to_cache_job.cc
index 24bcd0f441a1602d8a43f93cbba54bd7d928899e..ba605c06c9d91b16189fcc88117a4c148d5369a3 100644
--- a/content/browser/service_worker/service_worker_write_to_cache_job.cc
+++ b/content/browser/service_worker/service_worker_write_to_cache_job.cc
@@ -7,6 +7,7 @@
#include "base/bind.h"
#include "base/callback.h"
#include "base/strings/stringprintf.h"
+#include "base/thread_task_runner_handle.h"
#include "base/trace_event/trace_event.h"
#include "content/browser/service_worker/service_worker_cache_writer.h"
#include "content/browser/service_worker/service_worker_context_core.h"
@@ -47,7 +48,7 @@ const char kServiceWorkerAllowed[] = "Service-Worker-Allowed";
// developers.
// TODO(falken): Redesign this class so we don't have to fail at the network
// stack layer just to cancel the update.
-const int kIdenticalScriptError = net::ERR_FILE_EXISTS;
+const net::Error kIdenticalScriptError = net::ERR_FILE_EXISTS;
} // namespace
@@ -79,11 +80,18 @@ ServiceWorkerWriteToCacheJob::~ServiceWorkerWriteToCacheJob() {
}
void ServiceWorkerWriteToCacheJob::Start() {
+ base::ThreadTaskRunnerHandle::Get()->PostTask(
+ FROM_HERE, base::Bind(&ServiceWorkerWriteToCacheJob::StartAsync,
+ weak_factory_.GetWeakPtr()));
+}
+
+void ServiceWorkerWriteToCacheJob::StartAsync() {
TRACE_EVENT_ASYNC_BEGIN1("ServiceWorker",
"ServiceWorkerWriteToCacheJob::ExecutingJob",
this,
"URL", request_->url().spec());
if (!context_) {
+ // NotifyStartError is not safe to call synchronously in Start().
NotifyStartError(
net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_FAILED));
return;
@@ -108,8 +116,9 @@ void ServiceWorkerWriteToCacheJob::Kill() {
has_been_killed_ = true;
net_request_.reset();
if (did_notify_started_) {
- NotifyFinishedCaching(net::URLRequestStatus::FromError(net::ERR_ABORTED),
- kKilledError);
+ net::Error error = NotifyFinishedCaching(
+ net::URLRequestStatus::FromError(net::ERR_ABORTED), kKilledError);
+ DCHECK_EQ(net::ERR_ABORTED, error);
}
writer_.reset();
context_.reset();
@@ -156,40 +165,20 @@ void ServiceWorkerWriteToCacheJob::SetExtraRequestHeaders(
net_request_->SetExtraRequestHeaders(headers);
}
-bool ServiceWorkerWriteToCacheJob::ReadRawData(net::IOBuffer* buf,
- int buf_size,
- int* bytes_read) {
- net::URLRequestStatus status = ReadNetData(buf, buf_size, bytes_read);
- SetStatus(status);
+int ServiceWorkerWriteToCacheJob::ReadRawData(net::IOBuffer* buf,
+ int buf_size) {
+ int bytes_read = 0;
+ net::URLRequestStatus status = ReadNetData(buf, buf_size, &bytes_read);
if (status.is_io_pending())
- return false;
-
- if (!status.is_success()) {
- NotifyDoneHelper(status, kFetchScriptError);
- return false;
- }
-
- HandleNetData(*bytes_read);
- status = GetStatus();
-
- // Synchronous EOFs that do not replace the incumbent entry are considered
- // failures. Since normally the URLRequestJob's status would be set by
- // ReadNetData or HandleNetData, this code has to manually fix up the status
- // to match the failure this function is about to return.
- if (status.status() == net::URLRequestStatus::SUCCESS && *bytes_read == 0 &&
- !cache_writer_->did_replace()) {
- status = net::URLRequestStatus::FromError(kIdenticalScriptError);
- }
+ return net::ERR_IO_PENDING;
if (!status.is_success()) {
- NotifyDoneHelper(status, "");
- return false;
+ net::Error error = NotifyFinishedCaching(status, kFetchScriptError);
+ DCHECK_EQ(status.error(), error);
+ return error;
}
- // Since URLRequestStatus::is_success() means "SUCCESS or IO_PENDING", but the
- // contract of this function is "return true for synchronous successes only",
- // it is important to test against SUCCESS explicitly here.
- return status.status() == net::URLRequestStatus::SUCCESS;
+ return HandleNetData(bytes_read);
}
const net::HttpResponseInfo* ServiceWorkerWriteToCacheJob::http_info() const {
@@ -244,9 +233,9 @@ void ServiceWorkerWriteToCacheJob::OnReceivedRedirect(
TRACE_EVENT0("ServiceWorker",
"ServiceWorkerWriteToCacheJob::OnReceivedRedirect");
// Script resources can't redirect.
- NotifyDoneHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
- net::ERR_UNSAFE_REDIRECT),
- kRedirectError);
+ NotifyStartErrorHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
+ net::ERR_UNSAFE_REDIRECT),
+ kRedirectError);
}
void ServiceWorkerWriteToCacheJob::OnAuthRequired(
@@ -256,7 +245,7 @@ void ServiceWorkerWriteToCacheJob::OnAuthRequired(
TRACE_EVENT0("ServiceWorker",
"ServiceWorkerWriteToCacheJob::OnAuthRequired");
// TODO(michaeln): Pass this thru to our jobs client.
- NotifyDoneHelper(
+ NotifyStartErrorHelper(
net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_FAILED),
kClientAuthenticationError);
}
@@ -269,7 +258,7 @@ void ServiceWorkerWriteToCacheJob::OnCertificateRequested(
"ServiceWorkerWriteToCacheJob::OnCertificateRequested");
// TODO(michaeln): Pass this thru to our jobs client.
// see NotifyCertificateRequested.
- NotifyDoneHelper(
+ NotifyStartErrorHelper(
net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_FAILED),
kClientAuthenticationError);
}
@@ -283,9 +272,9 @@ void ServiceWorkerWriteToCacheJob::OnSSLCertificateError(
"ServiceWorkerWriteToCacheJob::OnSSLCertificateError");
// TODO(michaeln): Pass this thru to our jobs client,
// see NotifySSLCertificateError.
- NotifyDoneHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
- net::ERR_INSECURE_RESPONSE),
- kSSLError);
+ NotifyStartErrorHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
+ net::ERR_INSECURE_RESPONSE),
+ kSSLError);
}
void ServiceWorkerWriteToCacheJob::OnBeforeNetworkStart(
@@ -301,15 +290,15 @@ void ServiceWorkerWriteToCacheJob::OnResponseStarted(
net::URLRequest* request) {
DCHECK_EQ(net_request_.get(), request);
if (!request->status().is_success()) {
- NotifyDoneHelper(request->status(), kFetchScriptError);
+ NotifyStartErrorHelper(request->status(), kFetchScriptError);
return;
}
if (request->GetResponseCode() / 100 != 2) {
std::string error_message =
base::StringPrintf(kBadHTTPResponseError, request->GetResponseCode());
- NotifyDoneHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
- net::ERR_INVALID_RESPONSE),
- error_message);
+ NotifyStartErrorHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
+ net::ERR_INVALID_RESPONSE),
+ error_message);
// TODO(michaeln): Instead of error'ing immediately, send the net
// response to our consumer, just don't cache it?
return;
@@ -320,9 +309,10 @@ void ServiceWorkerWriteToCacheJob::OnResponseStarted(
const net::HttpNetworkSession::Params* session_params =
request->context()->GetNetworkSessionParams();
if (!session_params || !session_params->ignore_certificate_errors) {
- NotifyDoneHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
- net::ERR_INSECURE_RESPONSE),
- kSSLError);
+ NotifyStartErrorHelper(
+ net::URLRequestStatus(net::URLRequestStatus::FAILED,
+ net::ERR_INSECURE_RESPONSE),
+ kSSLError);
return;
}
}
@@ -337,9 +327,10 @@ void ServiceWorkerWriteToCacheJob::OnResponseStarted(
mime_type.empty()
? kNoMIMEError
: base::StringPrintf(kBadMIMEError, mime_type.c_str());
- NotifyDoneHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
- net::ERR_INSECURE_RESPONSE),
- error_message);
+ NotifyStartErrorHelper(
+ net::URLRequestStatus(net::URLRequestStatus::FAILED,
+ net::ERR_INSECURE_RESPONSE),
+ error_message);
return;
}
@@ -375,52 +366,31 @@ void ServiceWorkerWriteToCacheJob::OnWriteHeadersComplete(net::Error error) {
NotifyHeadersComplete();
}
-void ServiceWorkerWriteToCacheJob::HandleNetData(int bytes_read) {
- io_buffer_bytes_ = bytes_read;
- net::Error error = cache_writer_->MaybeWriteData(
- io_buffer_.get(), bytes_read,
- base::Bind(&ServiceWorkerWriteToCacheJob::OnWriteDataComplete,
- weak_factory_.GetWeakPtr()));
- SetStatus(net::URLRequestStatus::FromError(error));
-
- // In case of ERR_IO_PENDING, this logic is done in OnWriteDataComplete.
- if (error != net::ERR_IO_PENDING && bytes_read == 0) {
- NotifyFinishedCaching(net::URLRequestStatus::FromError(error),
- std::string());
- }
-}
-
void ServiceWorkerWriteToCacheJob::OnWriteDataComplete(net::Error error) {
SetStatus(net::URLRequestStatus::FromError(error));
DCHECK_NE(net::ERR_IO_PENDING, error);
- if (io_buffer_bytes_ == 0) {
- NotifyDoneHelper(net::URLRequestStatus::FromError(error), std::string());
- }
- NotifyReadComplete(error == net::OK ? io_buffer_bytes_ : error);
+ if (io_buffer_bytes_ == 0)
+ error = NotifyFinishedCaching(net::URLRequestStatus::FromError(error), "");
+ ReadRawDataComplete(error == net::OK ? io_buffer_bytes_ : error);
}
void ServiceWorkerWriteToCacheJob::OnReadCompleted(net::URLRequest* request,
int bytes_read) {
DCHECK_EQ(net_request_.get(), request);
+
+ int result;
if (bytes_read < 0) {
DCHECK(!request->status().is_success());
- NotifyDoneHelper(request->status(), kFetchScriptError);
- return;
- }
- HandleNetData(bytes_read);
- // HandleNetData can cause status of this job to change. If the status changes
- // to IO_PENDING, that means HandleNetData has pending IO, and
- // NotifyReadComplete will be called later by the appropriate callback.
- if (!GetStatus().is_io_pending()) {
- int result = GetStatus().status() == net::URLRequestStatus::SUCCESS
- ? bytes_read
- : GetStatus().error();
- // If bytes_read is 0, HandleNetData synchronously completed and this job is
- // at EOF.
- if (bytes_read == 0)
- NotifyDoneHelper(GetStatus(), std::string());
- NotifyReadComplete(result);
+ result = NotifyFinishedCaching(request->status(), kFetchScriptError);
+ } else {
+ result = HandleNetData(bytes_read);
}
+
+ // ReadRawDataComplete will be called in OnWriteDataComplete, so return early.
+ if (result == net::ERR_IO_PENDING)
+ return;
+
+ ReadRawDataComplete(result);
}
bool ServiceWorkerWriteToCacheJob::CheckPathRestriction(
@@ -434,43 +404,57 @@ bool ServiceWorkerWriteToCacheJob::CheckPathRestriction(
if (!ServiceWorkerUtils::IsPathRestrictionSatisfied(
version_->scope(), url_,
has_header ? &service_worker_allowed : nullptr, &error_message)) {
- NotifyDoneHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
- net::ERR_INSECURE_RESPONSE),
- error_message);
+ NotifyStartErrorHelper(net::URLRequestStatus(net::URLRequestStatus::FAILED,
+ net::ERR_INSECURE_RESPONSE),
+ error_message);
return false;
}
return true;
}
-void ServiceWorkerWriteToCacheJob::NotifyDoneHelper(
+int ServiceWorkerWriteToCacheJob::HandleNetData(int bytes_read) {
+ io_buffer_bytes_ = bytes_read;
+ net::Error error = cache_writer_->MaybeWriteData(
+ io_buffer_.get(), bytes_read,
+ base::Bind(&ServiceWorkerWriteToCacheJob::OnWriteDataComplete,
+ weak_factory_.GetWeakPtr()));
+
+ // In case of ERR_IO_PENDING, this logic is done in OnWriteDataComplete.
+ if (error != net::ERR_IO_PENDING && bytes_read == 0) {
+ error = NotifyFinishedCaching(net::URLRequestStatus::FromError(error),
+ std::string());
+ }
+ return error == net::OK ? bytes_read : error;
+}
+
+void ServiceWorkerWriteToCacheJob::NotifyStartErrorHelper(
const net::URLRequestStatus& status,
const std::string& status_message) {
DCHECK(!status.is_io_pending());
- // Note that NotifyFinishedCaching has logic in it to detect the special case
- // mentioned below as well.
- NotifyFinishedCaching(status, status_message);
+ net::Error error = NotifyFinishedCaching(status, status_message);
+ // The special case mentioned in NotifyFinishedCaching about script being
+ // identical does not apply here, since the entire body needs to be read
+ // before this is relevant.
+ DCHECK_EQ(status.error(), error);
net::URLRequestStatus reported_status = status;
std::string reported_status_message = status_message;
- // A strange special case: requests that successfully fetch the entire
- // ServiceWorker and write it back, but which did not replace the incumbent
- // script because the new script was identical, are considered to have failed.
- if (status.is_success() && !cache_writer_->did_replace()) {
- reported_status = net::URLRequestStatus::FromError(kIdenticalScriptError);
- reported_status_message = "";
- }
-
SetStatus(reported_status);
- NotifyDone(reported_status);
+ NotifyStartError(reported_status);
}
-void ServiceWorkerWriteToCacheJob::NotifyFinishedCaching(
+net::Error ServiceWorkerWriteToCacheJob::NotifyFinishedCaching(
net::URLRequestStatus status,
const std::string& status_message) {
+ net::Error result = static_cast<net::Error>(status.error());
if (did_notify_finished_)
- return;
+ return result;
+
+ int size = -1;
+ if (status.is_success())
+ size = cache_writer_->bytes_written();
// If all the calls to MaybeWriteHeaders/MaybeWriteData succeeded, but the
// incumbent entry wasn't actually replaced because the new entry was
@@ -478,17 +462,18 @@ void ServiceWorkerWriteToCacheJob::NotifyFinishedCaching(
// exists.
if (status.status() == net::URLRequestStatus::SUCCESS &&
!cache_writer_->did_replace()) {
- status = net::URLRequestStatus::FromError(kIdenticalScriptError);
+ result = kIdenticalScriptError;
+ status = net::URLRequestStatus::FromError(result);
version_->SetStartWorkerStatusCode(SERVICE_WORKER_ERROR_EXISTS);
+ version_->script_cache_map()->NotifyFinishedCaching(url_, size, status,
+ std::string());
+ } else {
+ version_->script_cache_map()->NotifyFinishedCaching(url_, size, status,
+ status_message);
}
- int size = -1;
- if (status.is_success())
- size = cache_writer_->bytes_written();
-
- version_->script_cache_map()->NotifyFinishedCaching(url_, size, status,
- status_message);
did_notify_finished_ = true;
+ return result;
}
scoped_ptr<ServiceWorkerResponseReader>
« no previous file with comments | « content/browser/service_worker/service_worker_write_to_cache_job.h ('k') | content/browser/streams/stream_url_request_job.h » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698