report to console when service worker register fails due to mime-type issues

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bkelly, Assigned: asuth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [tw-dom])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
I just helped someone diagnose a bug in their node server where they could not register a service worker.  It turns out they were not setting the mime-type for the script.  This could have been resolved a lot faster if we simply logged a message for this failure.
(Reporter)

Updated

2 years ago
Whiteboard: [tw-dom]
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1262677
(Reporter)

Updated

2 years ago
Blocks: 1262699
(Assignee)

Updated

a year ago
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
(Assignee)

Comment 2

a year ago
Created attachment 8768268 [details] [diff] [review]
MIME type error message and test v1, layers on top of bug 1267473 fix.

Build on bug 1267473 patch adding logging for the missing/bad MIME type case.  If I've got any major idiomatic issues, feel free to just comment on that one and I'll propagate through.

Error looks like:
"""
Failed to register/update a ServiceWorker: Non-JS Content-Type of ‘text/plain’ received for script ‘http://mochi.test:8888/tests/dom/workers/test/serviceworkers/sw_bad_mime_type.js’.
"""
Attachment #8768268 - Flags: review?(bkelly)
(Reporter)

Comment 3

a year ago
Comment on attachment 8768268 [details] [diff] [review]
MIME type error message and test v1, layers on top of bug 1267473 fix.

Review of attachment 8768268 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +210,5 @@
>  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’.
> +# LOCALIZATION NOTE: Do not translate "ServiceWorker". %1$S is a MIME Media Type like "text/plain" and %2$S is a URL.
> +ServiceWorkerRegisterMimeTypeError=Failed to register/update a ServiceWorker: Non-JS Content-Type of ‘%1$S’ received for script ‘%2$S’.

Similar to the other bug, can we include the scope?  Also, I think we should specify exactly what content-types are allowed.  Maybe something like:

Failed to register/update a ServiceWorker for scope `%1$S`. Bad Content-Type of `%2$S` received for script `%3$S`. Must be `text/javascript`, `application/x-javascript`, or `application/javascript`.

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +803,5 @@
>        !mimeType.LowerCaseEqualsLiteral("application/x-javascript") &&
>        !mimeType.LowerCaseEqualsLiteral("application/javascript")) {
> +    nsXPIDLString message;
> +    const char16_t* params[] = { NS_ConvertUTF8toUTF16(mimeType).get(),
> +                                 PromiseFlatString(mManager->URL()).get() };

Is this usage of temporary return values valid?  Since they are not declared as variables on the stack, do they live long enough to pass into FormatLocalizedString()?

@@ +807,5 @@
> +                                 PromiseFlatString(mManager->URL()).get() };
> +    rv = nsContentUtils::FormatLocalizedString(nsContentUtils::eDOM_PROPERTIES,
> +                                               "ServiceWorkerRegisterMimeTypeError",
> +                                               params, message);
> +    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed to format localized string");

Again, might be nice to roll the localization into a shared method in SWM.
Attachment #8768268 - Flags: review?(bkelly) → review+
(Assignee)

Comment 4

a year ago
(In reply to Ben Kelly [:bkelly] from comment #3)
> Is this usage of temporary return values valid?  Since they are not declared
> as variables on the stack, do they live long enough to pass into
> FormatLocalizedString()?

Yeah, the temporaries are almost certainly invalid by the time of use... possibly doubly so.  I was lining up the types and stopped waaay too early.  I'll see if I can make the SWM logging helper function reduce the boilerplate string management required without getting into a template nightmare.

Comment 5

a year ago
Pushed by bugmail@asutherland.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8ca65757e7c
report to console when service worker register fails due to mime-type issues. r=bkelly
(Assignee)

Comment 6

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

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b8ca65757e7c
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.