It would be helpful to report to the console what happened when a service worker registration fails due to a scope path issue: https://github.com/slightlyoff/ServiceWorker/issues/775#issuecomment-152916565 We basically need an nsContentUtils::ReportToConsole() here: https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#992 This will require a new dom.properties string, so will not make it into 44.

This will need infrastructure I am adding bug 1217909.

Thanks for filing this, Ben!

Doesn't look like Ben is actively working on this ... someone else could pick it up.

I would like to try on this bug.

(In reply to John Dai[:johnz][:jdai] from comment #4) > I would like to try on this bug. Please take a look at what Chrome spits out for this error condition. It's particularly helpful, so it would be great to replicate that error message.

I use |ServiceWorkerManager::ReportToAllClients| to report service worker registration fails and add comment 5 suggestion into my patch. I didn't pass filename, line and lineNumber when I call |ServiceWorkerManager::ReportToAllClients|, it is because ComparisonResult()[1] doesn't allow me to do that.(Please correct me if I misunderstand anything) I didn't find testcase test browser console message, so I left testcase blank. (Please correct me if I misunderstand anything) [1] https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#1395 Hi Ben, may I have your review? Thank you.

Comment on attachment 8736304 [details] [diff] [review] Bug 1220757 - Add report to console when service worker register fails due scope path mismatch. Review of attachment 8736304 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good, but a few issues to address. r=me with comments addressed. You are correct we don't have a test, but we really should. Would you be willing to write one? Either in this bug or a follow-up bug? There is a devtools console test you could probably crib from: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/test/test_console_serviceworker.html ::: dom/locales/en-US/chrome/dom/dom.properties @@ +187,5 @@ > 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". %S is a URL. > +ServiceWorkerScopePathMismatch=Failed to register a ServiceWorker: The path of the provided scope ('%S') is not under the max scope allowed ('%S'). Adjust the scope, move the Service Worker script, or use the Service-Worker-Allowed HTTP header to allow the scope. Can you you the %1$S and %2$S syntax here? I think its nicer to be explicit and it makes it easier for the people localizing the string into other languages. nit: Please remove the parens to match the style of the other messages. ::: dom/workers/ServiceWorkerManager.cpp @@ +1464,5 @@ > + > + rv = nsContentUtils::FormatLocalizedString(nsContentUtils::eDOM_PROPERTIES, > + "ServiceWorkerScopePathMismatch", > + params, message); > + swm->ReportToAllClients(mScope, I don't think we should call ReportToAllClients() if FormatLocalizedString() fails. Can you wrap this in an NS_WARN_IF_FALSE(NS_SUCCEEDED(rv)) check? @@ +1473,1 @@ > NS_WARNING("By default a service worker's scope is restricted to at or below it's script's location."); Can you remove this NS_WARNING now that we have a console message? @@ +2724,5 @@ > { > nsCOMPtr<nsIURI> uri; > nsresult rv = NS_NewURI(getter_AddRefs(uri), aFilename); > if (NS_WARN_IF(NS_FAILED(rv))) { > + uri = nullptr; This ends up ignoring malformed filenames which we probably want to treat as errors. Can you instead wrap the NS_NewURI() logic in an if (!aFilename.IsEmpty()) check?

(In reply to Ben Kelly [:bkelly] from comment #7) > Comment on attachment 8736304 [details] [diff] [review] > Bug 1220757 - Add report to console when service worker register fails due > scope path mismatch. > > Review of attachment 8736304 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall looks good, but a few issues to address. r=me with comments > addressed. > > You are correct we don't have a test, but we really should. Would you be > willing to write one? Either in this bug or a follow-up bug? > > There is a devtools console test you could probably crib from: > > > https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/ > test/test_console_serviceworker.html > Filed bug 1260980 for this. > ::: dom/locales/en-US/chrome/dom/dom.properties > @@ +187,5 @@ > > 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". %S is a URL. > > +ServiceWorkerScopePathMismatch=Failed to register a ServiceWorker: The path of the provided scope ('%S') is not under the max scope allowed ('%S'). Adjust the scope, move the Service Worker script, or use the Service-Worker-Allowed HTTP header to allow the scope. > > Can you you the %1$S and %2$S syntax here? I think its nicer to be explicit > and it makes it easier for the people localizing the string into other > languages. > > nit: Please remove the parens to match the style of the other messages. > > ::: dom/workers/ServiceWorkerManager.cpp > @@ +1464,5 @@ > > + > > + rv = nsContentUtils::FormatLocalizedString(nsContentUtils::eDOM_PROPERTIES, > > + "ServiceWorkerScopePathMismatch", > > + params, message); > > + swm->ReportToAllClients(mScope, > > I don't think we should call ReportToAllClients() if FormatLocalizedString() > fails. Can you wrap this in an NS_WARN_IF_FALSE(NS_SUCCEEDED(rv)) check? > > @@ +1473,1 @@ > > NS_WARNING("By default a service worker's scope is restricted to at or below it's script's location."); > > Can you remove this NS_WARNING now that we have a console message? > > @@ +2724,5 @@ > > { > > nsCOMPtr<nsIURI> uri; > > nsresult rv = NS_NewURI(getter_AddRefs(uri), aFilename); > > if (NS_WARN_IF(NS_FAILED(rv))) { > > + uri = nullptr; > > This ends up ignoring malformed filenames which we probably want to treat as > errors. > > Can you instead wrap the NS_NewURI() logic in an if (!aFilename.IsEmpty()) > check? I will address them in next patch. Thanks for the review.

Address comment 7 and carry on r+. Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46557dbd6999