corysanin/tubestation
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bug 1267473 - Report to console if service worker script 404s. r=bkelly

Browse Source 
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.
This commit is contained in:
Andrew Sutherland
2016-07-05 16:47:10 -04:00
parent c29f587041
commit 371cc6a534
8 changed files with 221 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
dom/base/nsContentUtils.cpp
View File

@@ -3467,6 +3467,28 @@ nsresult nsContentUtils::FormatLocalizedString(PropertiesFile aFile,
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)

11
dom/base/nsContentUtils.h
View File

@@ -973,6 +973,17 @@ public:
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
*/

2
dom/locales/en-US/chrome/dom/dom.properties
View File

@@ -208,6 +208,8 @@ InterceptionRejectedResponseWithURL=Failed to load ‘%1$S’. A ServiceWorker p
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.

30
dom/workers/ServiceWorkerManager.cpp
View File

@@ -1492,6 +1492,36 @@ ServiceWorkerManager::ReportToAllClients(const nsCString& aScope,
}
}
/* 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,

35
dom/workers/ServiceWorkerManager.h
View File

@@ -178,6 +178,14 @@ public:
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,
@@ -187,6 +195,33 @@ public:
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

15
dom/workers/ServiceWorkerScriptCache.cpp
View File

@@ -19,6 +19,7 @@
#include "nsIThreadRetargetableRequest.h"
#include "nsIPrincipal.h"
#include "nsIScriptError.h"
#include "nsContentUtils.h"
#include "nsNetUtil.h"
#include "nsScriptLoader.h"
@@ -289,7 +290,7 @@ public:
return NS_OK;
}
const nsAString&
const nsString&
URL() const
{
AssertIsOnMainThread();
@@ -737,6 +738,18 @@ CompareNetwork::OnStreamComplete(nsIStreamLoader* aLoader, nsISupports* aContext
}
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;
}

1
dom/workers/test/serviceworkers/mochitest.ini
View File

@@ -217,6 +217,7 @@ support-files =
[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]

104
dom/workers/test/serviceworkers/test_error_reporting.html Normal file
View File

@@ -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>