Shows error messages in the inspector when .respondWith() is called with wrong responses.

Source/modules/serviceworkers/RespondWithObserver.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 "config.h"
 #include "modules/serviceworkers/RespondWithObserver.h"
 
 #include "bindings/core/v8/ScriptFunction.h"
 #include "bindings/core/v8/ScriptPromise.h"
 #include "bindings/core/v8/ScriptValue.h"
 #include "bindings/core/v8/V8Binding.h"
 #include "bindings/modules/v8/V8Response.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/dom/ExecutionContext.h"
 #include "core/inspector/ConsoleMessage.h"
 #include "core/streams/Stream.h"
 #include "modules/fetch/BodyStreamBuffer.h"
 #include "modules/serviceworkers/ServiceWorkerGlobalScopeClient.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "public/platform/WebServiceWorkerResponse.h"
 #include "wtf/Assertions.h"
 #include "wtf/RefPtr.h"
 #include <v8.h>
 
 namespace blink {
+namespace {
+
+// Outputs the error message to let the developer know about the reason of the unusual failures.
+void maybeOutputErrorMessage(ExecutionContext* context, WebServiceWorkerResponseError error, const KURL& requestURL)
+{

[falken, 2015/07/10 11:20:44]

I suspect a more user-friendly way to present this to developers is something
like:

"The FetchEvent for \"<requestURL.string>\" resulted in a network error
response: "

and then each case outputs a reason.

[horo, 2015/07/10 12:13:26]

Done.

+    String errorMessage;
+    switch (error) {
+    case WebServiceWorkerResponseErrorUnknown:
+        errorMessage = "An unexpected error occured while handling the FetchEvent (" + requestURL.string() + ").";

[falken, 2015/07/10 11:20:44]

"an unexpected error occurred."
(this should probably also be done in the default case)

[horo, 2015/07/10 12:13:27]

Done.

+        break;
+    case WebServiceWorkerResponseErrorPromiseRejected:

[falken, 2015/07/10 11:20:44]

Can you comment why this doesn't have a message (I believe it's because the
rejected promise is already displayed to console).

[horo, 2015/07/10 12:13:27]

Humm.
It may better to show more detailed message from here.
Added new message for it.

+        return;
+        break;
+    case WebServiceWorkerResponseErrorDefaultPrevented:
+        errorMessage = "event.preventDefault() for the FetchEvent (" + requestURL.string() + ") without calling event.respondWith() is treated as a network error.";

[falken, 2015/07/10 11:20:44]

"preventDefault() was called without calling respondWith()."

[horo, 2015/07/10 12:13:27]

Done.

+        break;
+    case WebServiceWorkerResponseErrorNoV8Instance:
+        errorMessage = "The object with which respond to the request (" + requestURL.string() + ") must be a Response object.";

[falken, 2015/07/10 11:20:44]

"an object that was not a Response was passed to respondWith()"

[horo, 2015/07/10 12:13:27]

Done.

+        break;
+    case WebServiceWorkerResponseErrorResponseTypeError:

[falken, 2015/07/10 11:20:44]

ditto, comment

[horo, 2015/07/10 12:13:26]

Added.

+        return;
+        break;
+    case WebServiceWorkerResponseErrorResponseTypeOpaque:
+        errorMessage = "Opaque type response can't respond to the request (" + requestURL.string() + ") which type isn't no-cors.";

[falken, 2015/07/10 11:20:44]

"an opaque response was used for a request whose type is not no-cors" (yay
triple negatives)

[horo, 2015/07/10 12:13:26]

Done.

+        break;
+    case WebServiceWorkerResponseErrorResponseTypeNotBasicOrDefault:
+        errorMessage = "The type of the Response object with which respond to the client request (" + requestURL.string() + ") must be basic or default.";

[falken, 2015/07/10 11:20:44]

"the response for a client request must have type \"basic\" or \"default\"" (are
these types exposed to script? if not we shouldn't quote them)

[horo, 2015/07/10 12:13:27]

Done.

+        break;
+    case WebServiceWorkerResponseErrorBodyUsed:
+        errorMessage = "The body used response can't respond to the request (" + requestURL.string() + ").";

[falken, 2015/07/10 11:20:44]

"a Response whose \"bodyUsed\" is \"true\" cannot be used to respond to a
request"

[horo, 2015/07/10 12:13:26]

Done.

+        break;
+    }
+    context->addConsoleMessage(ConsoleMessage::create(JSMessageSource, ErrorMessageLevel, errorMessage));

[falken, 2015/07/10 11:20:44]

I suspect this should be a Warning or Info, it's conceivable the developer is
intentionally doing this. +mkwst

[horo, 2015/07/10 12:13:26]

Changed to Info.

[Mike West, 2015/07/10 13:01:09]

The "intentionally doing this" worries me a bit; we generally try to keep the
console clean unless there's a real error that someone needs to deal with. If
there are good reasons that a developer might be producing these kinds of
scenarios (feature-detecting something, etc), it would be better to wrap the
error up into something the developer could inspect. We might throw an
exception, for instance, which could be caught and suppressed.

It doesn't look like `respondWith` returns anything useful in the case of an
error (which means it's a great idea to throw some messages somewhere!). Have
you considered altering it to either throw in the case of an irrecoverable
error, or to return a promise which would be rejected with one of the messages
you've put together here in the case of error?

[falken, 2015/07/13 00:51:36]

+pfeldman FYI

I don't have a very compelling use case in mind. The only thing I could think
was using Service Worker as a kind of network simulation/HTTP proxy server that
intentionally creates network errors. I mean the spec does allow the developer
to use "preventDefault()" to create a network error.

What might be better long term is to expose detailed SW network activity in
DevTools Network tab. I'd say for now these console messages are OK, as
currently these errors are pretty much impossible to debug.

I'll also bring up changing the return value of respondWith() in a spec issue.

[falken, 2015/07/13 00:58:10]

Ah.. just realized respondWith()'s return value won't help in the case of
preventDefault(), since it's never called. It was considered in a similar issue
https://github.com/slightlyoff/ServiceWorker/issues/150 that "we may want to
indicate something specific in devtools console / network panel".

+}
+
+} // namespace
 
 class RespondWithObserver::ThenFunction final : public ScriptFunction {
 public:
     enum ResolveType {
         Fulfilled,
         Rejected,
     };
 
     static v8::Local<v8::Function> createFunction(ScriptState* scriptState, RespondWithObserver* observer, ResolveType type)
     {
@@ skipping 26 matching lines @@
             m_observer->responseWasFulfilled(value);
         }
         m_observer = nullptr;
         return value;
     }
 
     Member<RespondWithObserver> m_observer;
     ResolveType m_resolveType;
 };
 
-RespondWithObserver* RespondWithObserver::create(ExecutionContext* context, int eventID, WebURLRequest::FetchRequestMode requestMode, WebURLRequest::FrameType frameType)
+RespondWithObserver* RespondWithObserver::create(ExecutionContext* context, int eventID, const KURL& requestURL, WebURLRequest::FetchRequestMode requestMode, WebURLRequest::FrameType frameType)
 {
-    return new RespondWithObserver(context, eventID, requestMode, frameType);
+    return new RespondWithObserver(context, eventID, requestURL, requestMode, frameType);
 }
 
 void RespondWithObserver::contextDestroyed()
 {
     ContextLifecycleObserver::contextDestroyed();
     m_state = Done;
 }
 
 void RespondWithObserver::didDispatchEvent(bool defaultPrevented)
 {
@@ skipping 19 matching lines @@
 
     m_state = Pending;
     ScriptPromise::cast(scriptState, value).then(
         ThenFunction::createFunction(scriptState, this, ThenFunction::Fulfilled),
         ThenFunction::createFunction(scriptState, this, ThenFunction::Rejected));
 }
 
 void RespondWithObserver::responseWasRejected(WebServiceWorkerResponseError error)
 {
     ASSERT(executionContext());
+    maybeOutputErrorMessage(executionContext(), error, m_requestURL);
     // The default value of WebServiceWorkerResponse's status is 0, which maps
     // to a network error.
     WebServiceWorkerResponse webResponse;
     webResponse.setError(error);
     ServiceWorkerGlobalScopeClient::from(executionContext())->didHandleFetchEvent(m_eventID, webResponse);
     m_state = Done;
 }
 
 void RespondWithObserver::responseWasFulfilled(const ScriptValue& value)
 {
@@ skipping 43 matching lines @@
         buffer->startLoading(loader, nullptr);
         m_state = Done;
         return;
     }
     WebServiceWorkerResponse webResponse;
     response->populateWebServiceWorkerResponse(webResponse);
     ServiceWorkerGlobalScopeClient::from(executionContext())->didHandleFetchEvent(m_eventID, webResponse);
     m_state = Done;
 }
 
-RespondWithObserver::RespondWithObserver(ExecutionContext* context, int eventID, WebURLRequest::FetchRequestMode requestMode, WebURLRequest::FrameType frameType)
+RespondWithObserver::RespondWithObserver(ExecutionContext* context, int eventID, const KURL& requestURL, WebURLRequest::FetchRequestMode requestMode, WebURLRequest::FrameType frameType)
     : ContextLifecycleObserver(context)
     , m_eventID(eventID)
+    , m_requestURL(requestURL)
     , m_requestMode(requestMode)
     , m_frameType(frameType)
     , m_state(Initial)
 {
 }
 
 DEFINE_TRACE(RespondWithObserver)
 {
     ContextLifecycleObserver::trace(visitor);
 }
 
 } // namespace blink