Closed
Bug 1220757
Opened 9 years ago
Closed 8 years ago
report to console when service worker register fails due scope path issues
Categories
(Core :: DOM: Service Workers, defect)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: bkelly, Assigned: jdai, Mentored)
References
Details
(Whiteboard: [tw-dom] btpp-active)
Attachments
(1 file, 1 obsolete file)
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.
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Reporter | ||
Comment 1•9 years ago
|
||
This will need infrastructure I am adding bug 1217909.
Depends on: 1217909
Comment 2•9 years ago
|
||
Thanks for filing this, Ben!
Doesn't look like Ben is actively working on this ... someone else could pick it up.
Mentor: bkelly
Whiteboard: [tw-dom]
Comment 5•8 years ago
|
||
(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.
Assignee | ||
Comment 6•8 years ago
|
||
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.
Attachment #8736304 -
Flags: review?(bkelly)
Reporter | ||
Comment 7•8 years ago
|
||
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?
Attachment #8736304 -
Flags: review?(bkelly) → review+
Assignee | ||
Comment 8•8 years ago
|
||
(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.
Assignee | ||
Comment 9•8 years ago
|
||
Address comment 7 and carry on r+. Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46557dbd6999
Attachment #8736304 -
Attachment is obsolete: true
Attachment #8736603 -
Flags: review+
Updated•8 years ago
|
Whiteboard: [tw-dom] → [tw-dom] btpp-active
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d2d0e61c4d1
Keywords: checkin-needed
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1d2d0e61c4d1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•