If the network load of the service worker script fails we currently just reject register() with: "Service worker failed to register: TypeError: ServiceWorker script at http://ubuntu:5000/does-not-exist.js for scope http://ubuntu:5000/ encountered an error during installation." We should probably log what went wrong. What was the status code, etc? Since this can happen during update as well its probably better to report to the console instead of piping it to the rejection value.

This is the base patch for the error logging improvements generally discussed on bug 1229156 with just support for the 404 right now. I plan to implement the other errors as distinct patches that add more tests to this file. Although in https://bugzilla.mozilla.org/show_bug.cgi?id=1229156#c3 :bkelly discussed building on the logging tests in https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/test/test_console_serviceworker.html which uses the webconsole debugger actor, I felt using the existing mochitest support for matching console service messages sufficient and simpler/less brittle. My rationale is that although it makes sense to have tests of ServiceWorkerManager::ReportToAllClients' mechanism for reporting to multiple consoles somewhere, that's arguably orthogonal and for these errors all that matters is that they are being reported to ReportToAllClients. The 404 message added looks like so in the error console: """ Failed to register/update a ServiceWorker: Load failed with status 404 for script `http://mochi.test:8888/tests/dom/workers/test/serviceworkers/404.js`. """

Comment on attachment 8768193 [details] [diff] [review] sw-error-logging-404-v1.diff Review of attachment 8768193 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! r=me with comments addressed. ::: dom/locales/en-US/chrome/dom/dom.properties @@ +208,5 @@ > 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 stringified numeric HTTP status code like "404" and %2$S is a URL. > +ServiceWorkerRegisterNetworkError=Failed to register/update a ServiceWorker: Load failed with status %1$S for script `%2$S`. Can we include the scope string here? Maybe something like: Failed to register/update a ServiceWorker for scope `$1%S`. Load Failed with status %2$S for script `%3$S`. ::: dom/workers/ServiceWorkerScriptCache.cpp @@ +741,5 @@ > + // Get the stringified numeric status code, not statusText which could be > + // something misleading like OK for a 404. > + uint32_t status; > + rv = httpChannel->GetResponseStatus(&status); > + NS_ENSURE_SUCCESS(rv, rv); I don't think we want to return if this fails. We will eat the error message. Instead, can we initialize the status to 0 and just NS_WARN_IF_FALSE() like you do below? @@ +751,5 @@ > + PromiseFlatString(mManager->URL()).get() }; > + rv = nsContentUtils::FormatLocalizedString(nsContentUtils::eDOM_PROPERTIES, > + "ServiceWorkerRegisterNetworkError", > + params, message); > + NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed to format localized string"); It might be nice to roll the formatting into a SWM method. Something like swm->LocalizeAndReportToAllClients(...). That could be a follow-up patch, though. ::: dom/workers/test/serviceworkers/test_error_reporting.html @@ +46,5 @@ > + */ > +function expect_console_message(msgId, args) { > + let expectedString = localizer.formatStringFromName(msgId, args, args.length); > + return new Promise(resolve => { > + SimpleTest.monitorConsole(resolve, [{ errorMessage: expectedString }]) Wow! I didn't know about this function. Very nice. @@ +72,5 @@ > +} > + > +add_task(function setupPrefs() { > + return SpecialPowers.pushPrefEnv({"set": [ > + ["dom.serviceWorkers.exemptFromPerDomainMax", true], I think this pref was recently removed.

I believe I've made all requested changes. Re-requesting review since the reusable localizing error function with improved safety is not trivial (but not complex!) and maybe even a bad call on my part from a design perspective. The error message now looks like: 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’.test_error_reporting.html from the current commit message: 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.

Comment on attachment 8768941 [details] [diff] [review] sw-error-logging-404-v2.diff Review of attachment 8768941 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsContentUtils.cpp @@ +3473,5 @@ > + const char* aKey, > + const nsTArray<nsString>& aParamArray, > + nsXPIDLString& aResult) > +{ > + const char16_t **rawParams = new const char16_t*[aParamArray.Length()]; It looks like you ran into a variant of this problem in bug 1215140 and solved it more cleanly there. ConsoleReportCollector has idiomatic C++ logic that ensures proper cleanup via destructors. I'll plan to copy that over unless you declare raw new/delete handling a refreshing change (and deluxe ensurer of continued employment): https://dxr.mozilla.org/mozilla-central/source/dom/console/ConsoleReportCollector.cpp#62 Note: that code cites bug 1219762 for that logic block, but that bug seems to apply to the nsIURI/spec hunk before it. I'm not sure if you filed a different bug for this scenario too?

Comment on attachment 8768941 [details] [diff] [review] sw-error-logging-404-v2.diff Review of attachment 8768941 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. Thanks! ::: dom/base/nsContentUtils.cpp @@ +3473,5 @@ > + const char* aKey, > + const nsTArray<nsString>& aParamArray, > + nsXPIDLString& aResult) > +{ > + const char16_t **rawParams = new const char16_t*[aParamArray.Length()]; Yea, I think we should use UniquePtr<> here to clean up the rawParams array. And yes, I put the bug 1219762 reference in the wrong comment block. Oops! Also, please add a main thread assertion here. Its kind of disturbing to me we don't have that already in FormatLocalizedString() since I'm fairly certain the string bundle code is not thread safe. ::: dom/workers/ServiceWorkerManager.cpp @@ +1509,5 @@ > + return; > + } > + > + nsTArray<nsString> fullParams; > + fullParams.AppendElement(NS_ConvertUTF8toUTF16(aScope)); Yea, I don't think we should automatically do this since the caller already has the scope. Also, if we were keeping this code you probably want to use AutoTArray here. ::: dom/workers/ServiceWorkerManager.h @@ +182,5 @@ > + * Given a scope and an error message that you hopefully already localized, > + * report the error message to: all documents controlled by the scope, all > + * documents that have called register() for the scope, and all active > + * navigation interceptions for the scope. And if we weren't able to report > + * the error to at least one of those, report it to the Browser Console. This heuristic might change over time. I think it would be better just to declare that this reports to any window we think might be interested in the service worker and a heuristic is used for finding those windows. @@ +204,5 @@ > + * to report the error to at least one of those, report it to the Browser > + * Console. > + * > + * Your localized string can contain any number of arguments, but you must > + * use $1%S for the scope, which we provide for you. This seems a bit surprising. Why have this restriction when the caller already has to have the scope to call this method? It seems they could put it in their argument list themselves. @@ +225,5 @@ > + * we're logging exceptional and/or obvious breakage. > + * @param [aFilename] > + * @param [aLine] > + * @param [aLineNumber] > + * @param [aColumnNumber] I'm not a huge fan of doxygen style comments, but infinitely better to have some documentation here. Thanks! ::: dom/workers/ServiceWorkerScriptCache.cpp @@ +289,5 @@ > > return NS_OK; > } > > + const nsString& Why do we need to do this? @@ +747,5 @@ > + > + RefPtr<ServiceWorkerRegistrationInfo> registration = mManager->GetRegistration(); > + ServiceWorkerManager::LocalizeAndReportToAllClients( > + registration->mScope, "ServiceWorkerRegisterNetworkError", > + nsTArray<nsString> { statusAsText, mManager->URL() }); Can you use PromiseFlatString() around the URL() call instead of requiring an exact nsString return value?

NI is just for the bottom follow-up Q. Thanks for the review! (In reply to Ben Kelly [:bkelly] from comment #5) > Comment on attachment 8768941 [details] [diff] [review] > ::: dom/workers/ServiceWorkerManager.cpp > > + nsTArray<nsString> fullParams; > > + fullParams.AppendElement(NS_ConvertUTF8toUTF16(aScope)); > > Yea, I don't think we should automatically do this since the caller already > has the scope. Yeah, I think I was pushing too hard to shrink the size of the call-sites. It's too awkward/unexpected an idiom. Will fix (as with all other requests). > Also, if we were keeping this code you probably want to use AutoTArray here. Thank you for this and similar comments! They are invaluable to me. > @@ +225,5 @@ > > + * we're logging exceptional and/or obvious breakage. > > + * @param [aFilename] > > + * @param [aLine] > > + * @param [aLineNumber] > > + * @param [aColumnNumber] > > I'm not a huge fan of doxygen style comments, but infinitely better to have > some documentation here. Thanks! To be clear, I'm not a fan of randomly strewing @param arguments all over the place sans actual documentation. The type signatures of the actual argument list are sufficient/superior if there are no docs. In this case, I put these in mainly as placeholders because I am/was expecting to provide comments with some useful context, like why on earth there's a string aLine plus the numeric aLineNumber and aColumnNumber. (I am expecting bug 1222720 to use this info and code path because there are specific exceptions involved, although maybe it will want/merit receiving a proper JS stack?) If there's an alternate machine parse-able-ish syntax you prefer, I can use that too. My main goal is to give tooling a fighting change and to provide some structure so that humans skimming the documentation can find what they want more quickly. (There is a balance between large prose blocks that have to be read in their entirety and uselessly splitting out the params into distinct @param clauses that have to be puzzled back together, and I'll probably take some time to get that right for dom/ too.) In JS code I've done documentation that's basically just markdown without trying to do semantic @param markup, and I'm fine with that too. Especially with our naming convention involving prefixes, any tooling can certainly figure out how to bold/infer argument names if it slaps it up on MDN, etc. > ::: dom/workers/ServiceWorkerScriptCache.cpp > > + const nsString& > > Why do we need to do this? > > @@ +747,5 @@ > > + > > + RefPtr<ServiceWorkerRegistrationInfo> registration = mManager->GetRegistration(); > > + ServiceWorkerManager::LocalizeAndReportToAllClients( > > + registration->mScope, "ServiceWorkerRegisterNetworkError", > > + nsTArray<nsString> { statusAsText, mManager->URL() }); > > Can you use PromiseFlatString() around the URL() call instead of requiring > an exact nsString return value? Yes, that works. But, is there a reason to favor returning the superclass (nsSubstring/nsAString)? I did this because: - Under the hood, mURL is already an nsString. - The related class CompareCache already returns an nsString&. (I'm modifying CompareManager) - I understand why there are benefits to a function call taking the more generic nsSubstring type as an argument, but not why returning the more generic type would be beneficial. (String-wise, that is. For general interfaces you obviously want to maintain an abstraction boundary and/or just shield callers from needing the type information of all the concrete implementation classes, etc.)

(In reply to Andrew Sutherland [:asuth] from comment #6) > > Can you use PromiseFlatString() around the URL() call instead of requiring > > an exact nsString return value? > > Yes, that works. > > But, is there a reason to favor returning the superclass > (nsSubstring/nsAString)? I did this because: > - Under the hood, mURL is already an nsString. > - The related class CompareCache already returns an nsString&. (I'm > modifying CompareManager) > - I understand why there are benefits to a function call taking the more > generic nsSubstring type as an argument, but not why returning the more > generic type would be beneficial. (String-wise, that is. For general > interfaces you obviously want to maintain an abstraction boundary and/or > just shield callers from needing the type information of all the concrete > implementation classes, etc.) I guess its fine here. I just have a stylist preference for using the abstract types in all public APIs. But since we can always change the callers if we want to change the type its not that big of a deal.

Pushed by bugmail@asutherland.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad2cbcabbc57 Report to console if service worker script 404s. r=bkelly

try job for this was: https://treeherder.mozilla.org/#/jobs?repo=try&revision=86fbfe09ca7b