Bug 1267473 - Report to console if service worker script 404s. r=bkelly
authorAndrew Sutherland <asutherland@asutherland.org>
Tue, 05 Jul 2016 16:47:10 -0400
changeset 344441 ad2cbcabbc571104bb0656668bdef187d0a1b122
parent 344440 60fdac16b45828f1d2d979f3ff28c0783e8a2edc
child 344442 b8ca65757e7c645d759afe80d0fb8fd3f15d80d6
push id6389
push userraliiev@mozilla.com
push dateMon, 19 Sep 2016 13:38:22 +0000
treeherdermozilla-beta@01d67bfe6c81 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbkelly
bugs1267473, 1228641
milestone50.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1267473 - Report to console if service worker script 404s. r=bkelly Add an error message of the following form for when a register/update job fails for network reasons: Failed to register/update a ServiceWorker for scope ‘http://mochi.test:8888/tests/dom/workers/test/serviceworkers/network_error/’: Load failed with status 404 for script ‘http://mochi.test:8888/tests/dom/workers/test/serviceworkers/404.js’. A mochitest is added that verifies this. To simplify the process of logging error messages, ServiceWorkerManager gains a new LocalizeAndReportToAllClients method that always provides the SW scope as the first argument to the localized string since all good error messages should include it. Its argument list takes an nsTArray<nsString> in order to reduce the potential for use-after-free scenarios from the char16_t** signature that unfortunately has rippled outwards from the nsIStringBundle interface. This potentially results in more memory allocation and byte shuffling than is strictly necessary, but we're also talking about rare error logging where it's better to optimize for easily adding the messages without needing to get hung up on the life-cycle of temporaries. nsTArray gained a std::initializer_list in bug 1228641. It is explicit, so inline argument usages may take a form along the lines of: `nsTArray<nsString> { string1, string2, ... }` This change did necessitate a change to nsContentUtils to add an nsTArray variant of FormatLocalizedString since the existing public function was slightly too clever. It used a template function to statically acquire the number of arguments at compile time, which is not compatible with the dynamic nsTArray usage. Since nsTArray may be useful to other consumers as well, I placed the conversion logic in nsContentUtils.
dom/base/nsContentUtils.cpp
dom/base/nsContentUtils.h
dom/locales/en-US/chrome/dom/dom.properties
dom/workers/ServiceWorkerManager.cpp
dom/workers/ServiceWorkerManager.h
dom/workers/ServiceWorkerScriptCache.cpp
dom/workers/test/serviceworkers/mochitest.ini
dom/workers/test/serviceworkers/test_error_reporting.html
--- a/dom/base/nsContentUtils.cpp
+++ b/dom/base/nsContentUtils.cpp
@@ -3462,16 +3462,38 @@ nsresult nsContentUtils::FormatLocalized
   NS_ENSURE_SUCCESS(rv, rv);
   nsIStringBundle *bundle = sStringBundles[aFile];
 
   return bundle->FormatStringFromName(NS_ConvertASCIItoUTF16(aKey).get(),
                                       aParams, aParamsLength,
                                       getter_Copies(aResult));
 }
 
+/* static */
+nsresult nsContentUtils::FormatLocalizedString(
+                                          PropertiesFile aFile,
+                                          const char* aKey,
+                                          const nsTArray<nsString>& aParamArray,
+                                          nsXPIDLString& aResult)
+{
+  MOZ_ASSERT(NS_IsMainThread());
+
+  UniquePtr<const char16_t*[]> params;
+  uint32_t paramsLength = aParamArray.Length();
+  if (paramsLength > 0) {
+    params = MakeUnique<const char16_t*[]>(paramsLength);
+    for (uint32_t i = 0; i < paramsLength; ++i) {
+      params[i] = aParamArray[i].get();
+    }
+  }
+  return FormatLocalizedString(aFile, aKey, params.get(), paramsLength,
+                               aResult);
+}
+
+
 /* static */ void
 nsContentUtils::LogSimpleConsoleError(const nsAString& aErrorText,
                                       const char * classification)
 {
   nsCOMPtr<nsIScriptError> scriptError =
     do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
   if (scriptError) {
     nsCOMPtr<nsIConsoleService> console =
@@ -6401,17 +6423,17 @@ nsContentUtils::WidgetForDocument(const 
 }
 
 static already_AddRefed<LayerManager>
 LayerManagerForDocumentInternal(const nsIDocument *aDoc, bool aRequirePersistent)
 {
   nsIWidget *widget = nsContentUtils::WidgetForDocument(aDoc);
   if (widget) {
     RefPtr<LayerManager> manager =
-      widget->GetLayerManager(aRequirePersistent ? nsIWidget::LAYER_MANAGER_PERSISTENT : 
+      widget->GetLayerManager(aRequirePersistent ? nsIWidget::LAYER_MANAGER_PERSISTENT :
                               nsIWidget::LAYER_MANAGER_CURRENT);
     return manager.forget();
   }
 
   return nullptr;
 }
 
 already_AddRefed<LayerManager>
@@ -7632,17 +7654,17 @@ nsContentUtils::TransferableToIPCTransfe
               data = parent;
             }
 
             IPCDataTransferItem* item = aIPCDataTransfer->items().AppendElement();
             item->flavor() = flavorStr;
             item->data() = data;
           } else {
             // This is a hack to support kFilePromiseMime.
-            // On Windows there just needs to be an entry for it, 
+            // On Windows there just needs to be an entry for it,
             // and for OSX we need to create
             // nsContentAreaDragDropDataProvider as nsIFlavorDataProvider.
             if (flavorStr.EqualsLiteral(kFilePromiseMime)) {
               IPCDataTransferItem* item = aIPCDataTransfer->items().AppendElement();
               item->flavor() = flavorStr;
               item->data() = NS_ConvertUTF8toUTF16(flavorStr);
             } else if (!data) {
               // Empty element, transfer only the flavor
--- a/dom/base/nsContentUtils.h
+++ b/dom/base/nsContentUtils.h
@@ -969,16 +969,27 @@ public:
                                         const char* aKey,
                                         const char16_t* (&aParams)[N],
                                         nsXPIDLString& aResult)
   {
     return FormatLocalizedString(aFile, aKey, aParams, N, aResult);
   }
 
   /**
+   * Fill (with the parameters given) the localized string named |aKey| in
+   * properties file |aFile| consuming an nsTArray of nsString parameters rather
+   * than a char16_t** for the sake of avoiding use-after-free errors involving
+   * temporaries.
+   */
+  static nsresult FormatLocalizedString(PropertiesFile aFile,
+                                        const char* aKey,
+                                        const nsTArray<nsString>& aParamArray,
+                                        nsXPIDLString& aResult);
+
+  /**
    * Returns true if aDocument is a chrome document
    */
   static bool IsChromeDoc(nsIDocument *aDocument);
 
   /**
    * Returns true if aDocument is in a docshell whose parent is the same type
    */
   static bool IsChildOfSameType(nsIDocument* aDoc);
--- a/dom/locales/en-US/chrome/dom/dom.properties
+++ b/dom/locales/en-US/chrome/dom/dom.properties
@@ -203,16 +203,18 @@ BadRedirectModeInterceptionWithURL=Failed to load ‘%S’. A ServiceWorker passed a redirected Response to FetchEvent.respondWith() while RedirectMode is not ‘follow’.
 # LOCALIZATION NOTE: Do not translate "ServiceWorker" or "FetchEvent.preventDefault()". %S is a URL.
 InterceptionCanceledWithURL=Failed to load ‘%S’. A ServiceWorker canceled the load by calling FetchEvent.preventDefault().
 # LOCALIZATION NOTE: Do not translate "ServiceWorker", "promise", or "FetchEvent.respondWith()". %1$S is a URL. %2$S is an error string.
 InterceptionRejectedResponseWithURL=Failed to load ‘%1$S’. A ServiceWorker passed a promise to FetchEvent.respondWith() that rejected with ‘%2$S’.
 # LOCALIZATION NOTE: Do not translate "ServiceWorker", "promise", "FetchEvent.respondWith()", or "Response". %1$S is a URL. %2$S is an error string.
 InterceptedNonResponseWithURL=Failed to load ‘%1$S’. A ServiceWorker passed a promise to FetchEvent.respondWith() that resolved with non-Response value ‘%2$S’.
 # LOCALIZATION NOTE: Do not translate "ServiceWorker", "Service-Worker-Allowed" or "HTTP". %1$S and %2$S are URLs.
 ServiceWorkerScopePathMismatch=Failed to register a ServiceWorker: The path of the provided scope ‘%1$S’ is not under the max scope allowed ‘%2$S’. Adjust the scope, move the Service Worker script, or use the Service-Worker-Allowed HTTP header to allow the scope.
+# LOCALIZATION NOTE: Do not translate "ServiceWorker". %1$S is a URL representing the scope of the ServiceWorker, %2$S is a stringified numeric HTTP status code like "404" and %3$S is a URL.
+ServiceWorkerRegisterNetworkError=Failed to register/update a ServiceWorker for scope ‘%1$S’: Load failed with status %2$S for script ‘%3$S’.
 ExecCommandCutCopyDeniedNotInputDriven=document.execCommand(‘cut’/‘copy’) was denied because it was not called from inside a short running user-generated event handler.
 ManifestShouldBeObject=Manifest should be an object.
 ManifestScopeURLInvalid=The scope URL is invalid.
 ManifestScopeNotSameOrigin=The scope URL must be same origin as document.
 ManifestStartURLOutsideScope=The start URL is outside the scope, so the scope is invalid.
 ManifestStartURLInvalid=The start URL is invalid.
 ManifestStartURLShouldBeSameOrigin=The start URL must be same origin as document.
 # LOCALIZATION NOTE: %1$S is the name of the object whose property is invalid. %2$S is the name of the invalid property. %3$S is the expected type of the property value. E.g. "Expected the manifest's start_url member to be a string."
--- a/dom/workers/ServiceWorkerManager.cpp
+++ b/dom/workers/ServiceWorkerManager.cpp
@@ -1487,16 +1487,46 @@ ServiceWorkerManager::ReportToAllClients
                                                 aLine,
                                                 aLineNumber,
                                                 aColumnNumber,
                                                 nsContentUtils::eOMIT_LOCATION);
     return;
   }
 }
 
+/* static */
+void
+ServiceWorkerManager::LocalizeAndReportToAllClients(
+                                          const nsCString& aScope,
+                                          const char* aStringKey,
+                                          const nsTArray<nsString>& aParamArray,
+                                          uint32_t aFlags,
+                                          const nsString& aFilename,
+                                          const nsString& aLine,
+                                          uint32_t aLineNumber,
+                                          uint32_t aColumnNumber)
+{
+  RefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
+  if (!swm) {
+    return;
+  }
+
+  nsresult rv;
+  nsXPIDLString message;
+  rv = nsContentUtils::FormatLocalizedString(nsContentUtils::eDOM_PROPERTIES,
+                                             aStringKey, aParamArray, message);
+  if (NS_SUCCEEDED(rv)) {
+    swm->ReportToAllClients(aScope, message,
+                            aFilename, aLine, aLineNumber, aColumnNumber,
+                            aFlags);
+  } else {
+    NS_WARNING("Failed to format and therefore report localized error.");
+  }
+}
+
 void
 ServiceWorkerManager::HandleError(JSContext* aCx,
                                   nsIPrincipal* aPrincipal,
                                   const nsCString& aScope,
                                   const nsString& aWorkerURL,
                                   const nsString& aMessage,
                                   const nsString& aFilename,
                                   const nsString& aLine,
--- a/dom/workers/ServiceWorkerManager.h
+++ b/dom/workers/ServiceWorkerManager.h
@@ -173,25 +173,60 @@ public:
   RemoveRegistration(ServiceWorkerRegistrationInfo* aRegistration);
 
   void StoreRegistration(nsIPrincipal* aPrincipal,
                          ServiceWorkerRegistrationInfo* aRegistration);
 
   void
   FinishFetch(ServiceWorkerRegistrationInfo* aRegistration);
 
+  /**
+   * Report an error for the given scope to any window we think might be
+   * interested, failing over to the Browser Console if we couldn't find any.
+   *
+   * Error messages should be localized, so you probably want to call
+   * LocalizeAndReportToAllClients instead, which in turn calls us after
+   * localizing the error.
+   */
   void
   ReportToAllClients(const nsCString& aScope,
                      const nsString& aMessage,
                      const nsString& aFilename,
                      const nsString& aLine,
                      uint32_t aLineNumber,
                      uint32_t aColumnNumber,
                      uint32_t aFlags);
 
+  /**
+   * Report a localized error for the given scope to any window we think might
+   * be interested.
+   *
+   * Note that this method takes an nsTArray<nsString> for the parameters, not
+   * bare chart16_t*[].  You can use a std::initializer_list constructor inline
+   * so that argument might look like: nsTArray<nsString> { some_nsString,
+   * PromiseFlatString(some_nsSubString_aka_nsAString),
+   * NS_ConvertUTF8toUTF16(some_nsCString_or_nsCSubString),
+   * NS_LITERAL_STRING("some literal") }.  If you have anything else, like a
+   * number, you can use an nsAutoString with AppendInt/friends.
+   *
+   * @param [aFlags]
+   *   The nsIScriptError flag, one of errorFlag (0x0), warningFlag (0x1),
+   *   infoFlag (0x8).  We default to error if omitted because usually we're
+   *   logging exceptional and/or obvious breakage.
+   */
+  static void
+  LocalizeAndReportToAllClients(const nsCString& aScope,
+                                const char* aStringKey,
+                                const nsTArray<nsString>& aParamArray,
+                                uint32_t aFlags = 0x0,
+                                const nsString& aFilename = EmptyString(),
+                                const nsString& aLine = EmptyString(),
+                                uint32_t aLineNumber = 0,
+                                uint32_t aColumnNumber = 0);
+
   // Always consumes the error by reporting to consoles of all controlled
   // documents.
   void
   HandleError(JSContext* aCx,
               nsIPrincipal* aPrincipal,
               const nsCString& aScope,
               const nsString& aWorkerURL,
               const nsString& aMessage,
--- a/dom/workers/ServiceWorkerScriptCache.cpp
+++ b/dom/workers/ServiceWorkerScriptCache.cpp
@@ -14,16 +14,17 @@
 #include "mozilla/ipc/BackgroundUtils.h"
 #include "mozilla/ipc/PBackgroundSharedTypes.h"
 #include "nsICacheInfoChannel.h"
 #include "nsIHttpChannelInternal.h"
 #include "nsIStreamLoader.h"
 #include "nsIThreadRetargetableRequest.h"
 
 #include "nsIPrincipal.h"
+#include "nsIScriptError.h"
 #include "nsContentUtils.h"
 #include "nsNetUtil.h"
 #include "nsScriptLoader.h"
 #include "ServiceWorkerManager.h"
 #include "Workers.h"
 #include "nsStringStream.h"
 
 using mozilla::dom::cache::Cache;
@@ -284,17 +285,17 @@ public:
         Cleanup();
         return rv;
       }
     }
 
     return NS_OK;
   }
 
-  const nsAString&
+  const nsString&
   URL() const
   {
     AssertIsOnMainThread();
     return mURL;
   }
 
   void
   SetMaxScope(const nsACString& aMaxScope)
@@ -732,16 +733,28 @@ CompareNetwork::OnStreamComplete(nsIStre
   bool requestSucceeded;
   rv = httpChannel->GetRequestSucceeded(&requestSucceeded);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     mManager->NetworkFinished(rv);
     return NS_OK;
   }
 
   if (NS_WARN_IF(!requestSucceeded)) {
+    // Get the stringified numeric status code, not statusText which could be
+    // something misleading like OK for a 404.
+    uint32_t status = 0;
+    httpChannel->GetResponseStatus(&status); // don't care if this fails, use 0.
+    nsAutoString statusAsText;
+    statusAsText.AppendInt(status);
+
+    RefPtr<ServiceWorkerRegistrationInfo> registration = mManager->GetRegistration();
+    ServiceWorkerManager::LocalizeAndReportToAllClients(
+      registration->mScope, "ServiceWorkerRegisterNetworkError",
+      nsTArray<nsString> { NS_ConvertUTF8toUTF16(registration->mScope),
+        statusAsText, mManager->URL() });
     mManager->NetworkFinished(NS_ERROR_FAILURE);
     return NS_OK;
   }
 
   nsAutoCString maxScope;
   // Note: we explicitly don't check for the return value here, because the
   // absence of the header is not an error condition.
   Unused << httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("Service-Worker-Allowed"),
--- a/dom/workers/test/serviceworkers/mochitest.ini
+++ b/dom/workers/test/serviceworkers/mochitest.ini
@@ -212,16 +212,17 @@ support-files =
 [test_claim.html]
 [test_claim_fetch.html]
 [test_claim_oninstall.html]
 [test_close.html]
 [test_controller.html]
 [test_cross_origin_url_after_redirect.html]
 [test_csp_upgrade-insecure_intercept.html]
 [test_empty_serviceworker.html]
+[test_error_reporting.html]
 [test_escapedSlashes.html]
 [test_eval_allowed.html]
 [test_eventsource_intercept.html]
 [test_fetch_event.html]
 skip-if = (debug && e10s) # Bug 1262224
 [test_file_blob_response.html]
 [test_file_blob_upload.html]
 [test_force_refresh.html]
new file mode 100644
--- /dev/null
+++ b/dom/workers/test/serviceworkers/test_error_reporting.html
@@ -0,0 +1,104 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <title>Test Error Reporting of Service Worker Failures</title>
+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script src="/tests/SimpleTest/SpawnTask.js"></script>
+  <link rel="stylesheet" href="/tests/SimpleTest/test.css"/>
+  <meta http-equiv="Content-type" content="text/html;charset=UTF-8">
+</head>
+<body>
+
+<script type="text/javascript">
+"use strict";
+/**
+ * Test that a bunch of service worker coding errors and failure modes that
+ * might otherwise be hard to diagnose are surfaced as console error messages.
+ * The driving use-case is minimizing cursing from a developer looking at a
+ * document in Firefox testing a page that involves service workers.
+ *
+ * This test assumes that errors will be reported via
+ * ServiceWorkerManager::ReportToAllClients and that that method is reliable and
+ * tested via some other file.  Because this generates nsIScriptError instances
+ * with flattened strings (the interpolated arguments aren't kept around), we
+ * load the string bundle and use it to derive the exact string message we
+ * expect for the given payload.
+ **/
+
+let stringBundleService =
+  SpecialPowers.Cc["@mozilla.org/intl/stringbundle;1"]
+               .getService(SpecialPowers.Ci.nsIStringBundleService);
+let localizer =
+  stringBundleService.createBundle("chrome://global/locale/dom/dom.properties");
+
+/**
+ * Start monitoring the console for the given localized error message string
+ * with the given arguments to be logged.  Call before running code that will
+ * generate the console message.  Pair with a call to
+ * `wait_for_expected_message` invoked after the message should have been
+ * generated.
+ *
+ * @param {String} msgId
+ *   The localization message identifier used in the properties file.
+ * @param {String[]} args
+ *   The list of formatting arguments we expect the error to be generated with.
+ * @return {Object} Promise/handle to pass to wait_for_expected_message.
+ */
+function expect_console_message(msgId, args) {
+  let expectedString = localizer.formatStringFromName(msgId, args, args.length);
+  return new Promise(resolve => {
+    SimpleTest.monitorConsole(resolve, [{ errorMessage: expectedString }])
+  });
+}
+
+/**
+ * Stop monitoring the console, returning a Promise that will be resolved when
+ * the sentinel console message sent through the async data path has been
+ * received.  The Promise will not reject on failure; instead a mochitest
+ * failure will have been generated by ok(false)/equivalent by the time it is
+ * resolved.
+ */
+function wait_for_expected_message(expectedPromise) {
+  SimpleTest.endMonitorConsole();
+  return expectedPromise;
+}
+
+/**
+ * Derive an absolute URL string from a relative URL to simplify error message
+ * argument generation.
+ */
+function make_absolute_url(relUrl) {
+  return new URL(relUrl, window.location).href;
+}
+
+add_task(function setupPrefs() {
+  return SpecialPowers.pushPrefEnv({"set": [
+    ["dom.serviceWorkers.enabled", true],
+    ["dom.serviceWorkers.testing.enabled", true],
+    ["dom.caches.testing.enabled", true],
+  ]});
+});
+
+/**
+ * Ensure an error is logged during the initial registration of a SW when a 404
+ * is received.
+ */
+add_task(function* register_404() {
+  // Start monitoring for the error
+  let expectedMessage = expect_console_message(
+    "ServiceWorkerRegisterNetworkError",
+    [make_absolute_url("network_error/"), "404", make_absolute_url("404.js")]);
+
+  // Register, generating the 404 error.  This will reject with a TypeError
+  // which we need to consume so it doesn't get thrown at our generator.
+  yield navigator.serviceWorker.register("404.js", { scope: "network_error/" })
+    .then(
+      () => { ok(false, "should have rejected"); },
+      (e) => { ok(e.name === "TypeError", "404 failed as expected"); });
+
+  yield wait_for_expected_message(expectedMessage);
+});
+</script>
+
+</body>
+</html>