ServiceWorker: Implement ServiceWorkerRegistration.update() (2)

content/browser/service_worker/service_worker_dispatcher_host.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/service_worker/service_worker_dispatcher_host.h"
 
 #include "base/logging.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/trace_event/trace_event.h"
@@ skipping 140 matching lines @@
 void ServiceWorkerDispatcherHost::OnDestruct() const {
   BrowserThread::DeleteOnIOThread::Destruct(this);
 }
 
 bool ServiceWorkerDispatcherHost::OnMessageReceived(
     const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(ServiceWorkerDispatcherHost, message)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_RegisterServiceWorker,
                         OnRegisterServiceWorker)
+    IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_UpdateServiceWorker,
+                        OnUpdateServiceWorker)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_UnregisterServiceWorker,
                         OnUnregisterServiceWorker)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_GetRegistration,
                         OnGetRegistration)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_GetRegistrationForReady,
                         OnGetRegistrationForReady)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_ProviderCreated,
                         OnProviderCreated)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_ProviderDestroyed,
                         OnProviderDestroyed)
@@ skipping 180 matching lines @@
       pattern,
       script_url,
       provider_host,
       base::Bind(&ServiceWorkerDispatcherHost::RegistrationComplete,
                  this,
                  thread_id,
                  provider_id,
                  request_id));
 }
 
+void ServiceWorkerDispatcherHost::OnUpdateServiceWorker(int provider_id,
+                                                        int64 registration_id) {
+  TRACE_EVENT0("ServiceWorker",
+               "ServiceWorkerDispatcherHost::OnUpdateServiceWorker");
+  if (!GetContext())
+    return;
+
+  ServiceWorkerProviderHost* provider_host =
+      GetContext()->GetProviderHost(render_process_id_, provider_id);
+  if (!provider_host) {
+    bad_message::ReceivedBadMessage(this, bad_message::SWDH_UPDATE_NO_HOST);
+    return;
+  }
+  if (!provider_host->IsContextAlive())
+    return;
+
+  // TODO(ksakamoto): This check can be removed once crbug.com/439697 is fixed.
+  if (provider_host->document_url().is_empty())
+    return;
+
+  if (!OriginCanAccessServiceWorkers(provider_host->document_url())) {

[kinuko, 2015/06/11 03:50:36]

This check's not necessary now?

[nhiroki, 2015/06/11 05:48:33]

Removed.

+    bad_message::ReceivedBadMessage(this, bad_message::SWDH_UPDATE_CANNOT);
+    return;
+  }
+
+  if (!GetContentClient()->browser()->AllowServiceWorker(
+          provider_host->document_url(), provider_host->topmost_frame_url(),
+          resource_context_, render_process_id_, provider_host->frame_id())) {
+    return;
+  }

[kinuko, 2015/06/11 03:50:36]

This check's not necessary now?

[nhiroki, 2015/06/11 05:48:33]

Hmm... we may have to respect Cookie setting changed during a session...??

[kinuko, 2015/06/11 06:33:05]

Um, wasn't really reading the code. Let's keep it. I'd stick to origin-based
check though.

+
+  GetContext()->storage()->FindRegistrationForId(
+      registration_id, provider_host->document_url().GetOrigin(),
+      base::Bind(&ServiceWorkerDispatcherHost::DidFindRegistrationForUpdate,
+                 this));
+}
+
 void ServiceWorkerDispatcherHost::OnUnregisterServiceWorker(
     int thread_id,
     int request_id,
     int provider_id,
     const GURL& pattern) {
   TRACE_EVENT0("ServiceWorker",
                "ServiceWorkerDispatcherHost::OnUnregisterServiceWorker");
   if (!GetContext()) {
     Send(new ServiceWorkerMsg_ServiceWorkerUnregistrationError(
         thread_id,
@@ skipping 105 matching lines @@
     return;
   }
 
   if (!CanGetRegistration(provider_host->document_url(), document_url)) {
     bad_message::ReceivedBadMessage(this,
                                     bad_message::SWDH_GET_REGISTRATION_CANNOT);
     return;
   }
 
   if (!GetContentClient()->browser()->AllowServiceWorker(
           provider_host->document_url(), provider_host->topmost_frame_url(),

[nhiroki, 2015/06/11 05:48:33]

Question (maybe falken@ knows an answer?): According to a header comment on
AllowServiceWorker(), the first argument should be 'scope', but this is
'document_url'. Is this correct?

[falken, 2015/06/11 06:26:55]

Mmm... good question. In practice, in the Chrome browser, they are equivalent,
Chrome's content setting policies are at the origin-level not path, if you trace
through AllowServiceWorker -> IsSettingCookieAllowed -> ... ->
ContentSettingsPattern::Match. But there could be embedders other than Chrome
that use the path. I don't know if it makes sense to worry about those as the
general web security/privacy model is considered per-origin.

I guess the question is, if SW is blocked for //a/foo*, should //a/foobar be
able to get/register/unregister at scope //a/fo? Probably not. But currently we
allow register/unregister and block get. That's kinda inconsistent.

On the other hand, if SW is blocked for //a/foo* should //a/f be able to
get/register/unregister at scope //a/foobar. I'd say definitely not. 

So maybe the right thing to do is AllowServiceWorker(scope, topmost_frame) &&
AllowServiceWorker(doc_url, topmost_frame) for get/register/unregister.

Everyone, WDYT? Maybe better to split this to another bug/codereview...

[nhiroki, 2015/06/12 08:50:57]

Thank you for your reply. Probably I got the point of this... in my opinion, the
per-origin model seems sufficient and we might have to explicitly mention the
model on a header comment. I'll open another bug/codereview later.

           resource_context_, render_process_id_, provider_host->frame_id())) {
     Send(new ServiceWorkerMsg_ServiceWorkerGetRegistrationError(
         thread_id, request_id, WebServiceWorkerError::ErrorTypeUnknown,
         base::ASCIIToUTF16(kServiceWorkerGetRegistrationErrorPrefix) +
             base::ASCIIToUTF16(kUserDeniedPermissionMessage)));
     return;
   }
 
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (GetContext()->storage()->IsDisabled()) {
@@ skipping 144 matching lines @@
 
   ServiceWorkerRegistrationObjectInfo info;
   ServiceWorkerVersionAttributes attrs;
   GetRegistrationObjectInfoAndVersionAttributes(
       provider_host->AsWeakPtr(), registration, &info, &attrs);
 
   Send(new ServiceWorkerMsg_AssociateRegistrationWithServiceWorker(
       kDocumentMainThreadId, provider_id, info, attrs));
 }
 
+void ServiceWorkerDispatcherHost::DidFindRegistrationForUpdate(
+    ServiceWorkerStatusCode status,
+    const scoped_refptr<ServiceWorkerRegistration>& registration) {
+  if (status != SERVICE_WORKER_OK)
+    return;
+  if (!GetContext())
+    return;
+  if (!registration->GetNewestVersion()) {
+    // This can happen if update() is called during initial script evaluation.
+    // Abort the following steps according to the spec.
+    return;
+  }

[kinuko, 2015/06/11 03:50:36]

It looks we don't check if the registration's for the given origin when we
return from LiveRegistrations in FindRegistrationForId, is that right?  Can we
check the origin in that case too?

[nhiroki, 2015/06/11 05:48:33]

Good point. Added an origin check here.

+  // The spec says, "update() pings the server for an updated version of this
+  // script without consulting caches", so set |force_bypass_cache| to true.
+  GetContext()->UpdateServiceWorker(registration.get(),
+                                    true /* force_bypass_cache */);
+}
+
 ServiceWorkerRegistrationHandle*
 ServiceWorkerDispatcherHost::FindRegistrationHandle(int provider_id,
                                                     int64 registration_id) {
   for (IDMap<ServiceWorkerRegistrationHandle, IDMapOwnPointer>::iterator
            iter(&registration_handles_);
        !iter.IsAtEnd();
        iter.Advance()) {
     ServiceWorkerRegistrationHandle* handle = iter.GetCurrentValue();
     DCHECK(handle);
     DCHECK(handle->registration());
@@ skipping 387 matching lines @@
   if (!handle) {
     bad_message::ReceivedBadMessage(this,
                                     bad_message::SWDH_TERMINATE_BAD_HANDLE);
     return;
   }
   handle->version()->StopWorker(
       base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
 }
 
 }  // namespace content