service worker: Fix the type of an update promise reject value

third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp

 // Copyright 2014 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 "modules/serviceworkers/ServiceWorkerRegistration.h"
 
 #include "bindings/core/v8/CallbackPromiseAdapter.h"
 #include "bindings/core/v8/ScriptPromise.h"
 #include "bindings/core/v8/ScriptState.h"
 #include "core/dom/DOMException.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/dom/ExecutionContext.h"
 #include "core/events/Event.h"
 #include "modules/EventTargetModules.h"
 #include "modules/serviceworkers/ServiceWorkerContainerClient.h"
 #include "modules/serviceworkers/ServiceWorkerError.h"
 #include "public/platform/modules/serviceworker/WebServiceWorkerProvider.h"
 
 namespace blink {
 
+namespace {
+
+class UpdateCallbacks : public WebServiceWorkerRegistration::WebServiceWorkerUpdateCallbacks {
+public:
+    explicit UpdateCallbacks(ScriptPromiseResolver* resolver)
+        : m_resolver(resolver) {}
+
+    ~UpdateCallbacks() override {}
+
+    void onSuccess() override
+    {
+        if (!m_resolver->getExecutionContext() || m_resolver->getExecutionContext()->activeDOMObjectsAreStopped())
+            return;
+        m_resolver->resolve();
+    }
+
+    void onError(const WebServiceWorkerError& error) override
+    {
+        if (!m_resolver->getExecutionContext() || m_resolver->getExecutionContext()->activeDOMObjectsAreStopped())
+            return;
+        switch (error.errorType) {
+        case WebServiceWorkerError::ErrorTypeNetwork:
+        case WebServiceWorkerError::ErrorTypeScriptEvaluateFailed:
+        case WebServiceWorkerError::ErrorTypeType: {

[falken, 2016/06/14 02:27:53]

Can you explain why are network, script evaluate, and type the ones here that
become TypeError, and the others do the default ServiceWorkerError::take?

[e_hakkinen, 2016/06/16 20:55:14]

ServiceWorkerError::take returns a DOMException which cannot be represent
TypeError but can represent all other errors such as SecurityError.

According to the SW spec Update algorithm step 5, If the fetch algorithm
asynchronously completes with null, then invoke Reject Job Promise with job and
a TypeError. That is the reason for the ErrorTypeNetwork case.

According to the SW spec Update algorithm step 7.5, if an uncaught runtime
script error occurs during the above step, then invoke Reject Job Promise with
job and a TypeError. That is the reason for the ErrorTypeScriptEvaluateFailed
case.

That list is not complete but it is a start.

The case that error.errorType is equal to ErrorTypeType actually never happens,
because ServiceWorkerMsg_ServiceWorkerUpdateError IPC cannot carry ErrorTypeType
as that is greater than ErrorTypeLast.

It might make sense to move this logic to ServiceWorkerError, for instance, so
that it could be reused and that default error messages used by
ServiceWorkerError could be reused.

[falken, 2016/06/17 02:12:14]

I see. Looks like that changed here:
https://github.com/slightlyoff/ServiceWorker/commit/e9fabe32d10ed8f2fbf1bdea4...

We have LayoutTests tests for this in
http/tests/serviceworker/resources/registration-tests.js that will need to be
changed.

It's a bit weird that the browser is sending a network error code to the
renderer, which just becomes a TypeError. I guess the advantage would be that
the renderer can use a default error message.
I see. One slight worry is that ScriptEvaluateFailed is not always indicative of
a runtime script error. If the thread gets terminated for some reason during
startup and happens to be in the evaluate state, it sends a
OnScriptEvaluteFailed to the browser, which now would come back as a
ScriptEvaluateFailed. But that's probably not so important.

Generally our error handling is not so elegant.
Yikes, I think that's an oversight. We should fix that.
Thanks for the explanation. I think we should have a comment above these cases
that the spec says network errors and runtime script errors during registration
result in a TypeError.

[e_hakkinen, 2016/06/17 12:29:18]

Yes, but that tests the register code path and not this update code path.
Therefore, that change is handled by CL https://crrev.com/2059953003.
Done.

+            ScriptState* scriptState = m_resolver->getScriptState();
+            ScriptState::Scope scope(scriptState);
+            m_resolver->reject(V8ThrowException::createTypeError(scriptState->isolate(), error.message));
+            break;
+        }
+        default:
+            m_resolver->reject(ServiceWorkerError::take(m_resolver.get(), error));
+        }
+    }
+
+private:
+    Persistent<ScriptPromiseResolver> m_resolver;
+    WTF_MAKE_NONCOPYABLE(UpdateCallbacks);
+};
+
+} // namespace
+
 const AtomicString& ServiceWorkerRegistration::interfaceName() const
 {
     return EventTargetNames::ServiceWorkerRegistration;
 }
 
 void ServiceWorkerRegistration::dispatchUpdateFoundEvent()
 {
     dispatchEvent(Event::create(EventTypeNames::updatefound));
 }
 
@@ skipping 39 matching lines @@
 }
 
 ScriptPromise ServiceWorkerRegistration::update(ScriptState* scriptState)
 {
     ServiceWorkerContainerClient* client = ServiceWorkerContainerClient::from(getExecutionContext());
     if (!client || !client->provider())
         return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "Failed to update a ServiceWorkerRegistration: No associated provider is available."));
 
     ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
     ScriptPromise promise = resolver->promise();
-    m_handle->registration()->update(client->provider(), new CallbackPromiseAdapter<void, ServiceWorkerError>(resolver));
+    m_handle->registration()->update(client->provider(), new UpdateCallbacks(resolver));
     return promise;
 }
 
 ScriptPromise ServiceWorkerRegistration::unregister(ScriptState* scriptState)
 {
     ServiceWorkerContainerClient* client = ServiceWorkerContainerClient::from(getExecutionContext());
     if (!client || !client->provider())
         return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "Failed to unregister a ServiceWorkerRegistration: No associated provider is available."));
 
     ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
@@ skipping 45 matching lines @@
 
 void ServiceWorkerRegistration::stop()
 {
     if (m_stopped)
         return;
     m_stopped = true;
     m_handle->registration()->proxyStopped();
 }
 
 } // namespace blink