Closed Bug 1220757 Opened 4 years ago Closed 4 years ago

report to console when service worker register fails due scope path issues

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

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.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
This will need infrastructure I am adding bug 1217909.
Depends on: 1217909
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]
I would like to try on this bug.
Assignee: bkelly → jdai
(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.
Attachment #8736304 - Flags: review?(bkelly)
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+
(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.
Whiteboard: [tw-dom] → [tw-dom] btpp-active
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1d2d0e61c4d1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.