Service Worker: Change last update check location and HTTP cache bypass rule (2/2)

content/browser/service_worker/service_worker_write_to_cache_job.cc

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 781a65257b61f1371eb8eaa703cdae73565c3dd0..a5cd2002fd0b6166ee0ba859a06630ce8b44f3d6 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
@@ -623,6 +623,13 @@ void ServiceWorkerWriteToCacheJob::OnBeforeNetworkStart(
 void ServiceWorkerWriteToCacheJob::OnResponseStarted(
     net::URLRequest* request) {
   DCHECK_EQ(net_request_, request);
+  if (!context_) {
+    AsyncNotifyDoneHelper(
+        net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_FAILED),
+        kFetchScriptError);
+    return;
+  }
+
   if (!request->status().is_success()) {
     AsyncNotifyDoneHelper(request->status(), kFetchScriptError);
     return;
@@ -637,6 +644,18 @@ void ServiceWorkerWriteToCacheJob::OnResponseStarted(
     // response to our consumer, just don't cache it?
     return;
   }
+
+  // Update |last_update_check| only when the request bypassed the browser
+  // cache.
+  if (request->response_info().network_accessed) {

[michaeln, 2015/09/11 18:59:26]

I'm not sure this matters, but...

I don't think network_accessed necessarily means the resource was retrieved from
the network. I think this flag will be true in cases where the resource in the
http cache gets validated (requiring a server resposne) but the resource is not
necessarily downloaded from the server.

[jungkees, 2015/09/22 01:23:40]

Sorry for getting back late.

You are right. What this line of code wanted to do is to check whether the
resource was served from the server not from the http cache. Having looked at
the code, I presume |was_cached| of HttpResponseInfo object can help here. How
about doing the following in ServiceWorkerWriteToCacheJob::OnResponseStarted()

if (net_request_->response_info().network_accessed &&
    !(net_request_->response_info().was_cached)) // i.e add this condition
    version_->embedded_worker()->OnNetworkAccessedForScriptLoad();

And check the EmbeddedWorkerInstance's |network_accessed_for_script_| in the
beginning of ServiceWorkerRegisterJob::OnStartWorkerFinished() like the
following?

void ServiceWorkerRegisterJob::OnStartWorkerFinished(
    ServiceWorkerStatusCode status) {
  if (new_version()->embedded_worker()->network_accessed_for_script() ||
      new_version()->force_bypass_cache_for_scripts()) {
    // set_last_update_check
  }
  if (status == SERVICE_WORKER_OK) {
  ...

For this change, the Update algorithm step 4.13 needs to move to 4.24. I think
that'd be fine if this change makes sense. WDYT?

+    ServiceWorkerRegistration* registration =
+        context_->GetLiveRegistration(version_->registration_id());
+    DCHECK(registration);  // We're registering or updating so must be there.
+
+    registration->set_last_update_check(base::Time::Now());

[michaeln, 2015/09/10 20:26:45]

I think this logic that applies to the register/update job as whole would
probably fit more naturally in the ServiceWorkerRegisterJob class. This class is
about downloading and writing a single resource to disk.

[jungkees, 2015/09/11 04:28:14]

I see. The reason I put this code here is to update the time stamp before
checking any errors such as the MIME type and the script location. This is
because the spec defines the last update check as purely the most recent time
the UA accessed the network to check the script resource.

If we'd want to move this to ServiceWorkerRegisterJob class, the spec may
correspondingly need to move the Update algorithm step 4.13 to step 4.24, which
would mean the MIME type error and the script location violation prevent it from
updating the time stamp. I'd like to hear some opinions on this. /cc michaeln@,
nhiroki@, falken@

+    context_->storage()->UpdateLastUpdateCheckTime(registration);

[michaeln, 2015/09/10 20:26:45]

I'm not sure i understand this change?

1) The writing of the resources hasn't yet succeeded at this time. If it
eventually fails we'll have a new update check time stored but no new script
will have been downloaded or completely written to disk or run or ...

2) A different ServiceWorkerWriteToCacheJob is used for the main script and all
imports. The check time will be updated multiple times as the main script and
each import is loaded.

3) This can occur prior to a registration being 'stored' for the very first
time. It'll be trying to updating something that does not yet exist in the db?
Looks like that won't cause any problem but we'll be reading when we don't have
to and getting errors.

  RegistrationData registration;
  status = ReadRegistrationData(registration_id, origin, &registration);
  if (status != STATUS_OK)
    return status;

[jungkees, 2015/09/11 04:28:14]

This is expected behavior as per the current spec text.
Yeah, this is a problem. I wanted to update it only for the main script.
This seems a bit different from the spec where the registration is stored for
the first time before getting to the update phase. If we decide to move the code
to ServiceWorkerRegisterJob class, I think we'd still have to put it in the
beginning of ServiceWorkerRegisterJob::OnStartWorkerFinished method. Then,
that'd be still before ServiceWorkerStorage::StoreRegistration has been executed
for the first registration attempt.

[michaeln, 2015/09/11 18:59:26]

Seems odd to update the time even if the job is aborted (browser is quit) prior
to finishing the script resource download, but ok.
you have to squint a little when directly comparing a spec with an impl

iirc, the spec didn't make a distinction between being 'stored' in the map of
scopes to registrations and being persisted to disk. In chrome, registrations
are 'stored' in the map when a job commences and if it get far enough along to
warrant being written to disk, that happens. I think updating the value of the
reg instance corresponds to 'storing' the new time in the language of the spec.
When that gets blasted to disk is an impl detail.

[jungkees, 2015/09/22 01:23:40]

Indeed.

+  }
+
   // OnSSLCertificateError is not called when the HTTPS connection is reused.
   // So we check cert_status here.
   if (net::IsCertStatusError(request->ssl_info().cert_status)) {