Bug 184014 – MIMETypeRegistry::isSupportedJavaScriptMIMEType() does not need to be thread-safe

• Status: ASSIGNED
• Component: WebCore Misc.

[Daniel Bates]

MIMETypeRegistry::isSupportedJavaScriptMIMEType() is the only thread-safe function of MIMETypeRegistry. I do not see a reason to make MIMETypeRegistry thread-safe and it is error-prone to make a subset of its functions thread-safe as it can lead to confusion over what functions are thread-safe and what functions are not. The Service Worker thread is the only non-main thread that uses MIMETypeRegistry::isSupportedJavaScriptMIMEType() and it is unnecessary for it to do so. The Service Worker threaded code should make use of the existing DocumentThreadableLoader and ThreadableLoaderOptions abstractions to tell the DocumentThreadableLoader to validate the MIME type on the main thread on behalf of the Service Worker thread.

[Daniel Bates]

Created attachment 336525 [details]
Patch

[Daniel Bates]

Comment on attachment 336525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336525&action=review

> Source/WebCore/ChangeLog:17
> +        This change also fixes a bug where the Service Worker thread would reject the Job Promise
> +        twice: once if the MIME type of the service worker file was not a supported JavaScript MIME
> +        type and once if the scopeString (path portion of the registration scope URL) was not a prefix
> +        of the maxScopeString. For reading the code this bug is not observable (since nothing happens

Step 7 of step 9 of the update algorithm in <https://w3c.github.io/ServiceWorker/v1/#update-algorithm> reads:

[[
7. Extract a MIME type from the response’s header list. If this MIME type (ignoring parameters) is not a JavaScript MIME type, then:

    1. Invoke Reject Job Promise with job and "SecurityError" DOMException.
    2. Asynchronously complete these steps with a network error.
]]

I interpret "Asynchronously complete these steps with a network error" as queue a network error failure and stop the algorithm at this point.

[Ryosuke Niwa]

Comment on attachment 336525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336525&action=review

> Source/WebCore/platform/MIMETypeRegistry.cpp:486
> +    ASSERT(isMainThread());

We should use a release assert instead given we had a last minute P1 bug about this.

[Chris Dumez]

EWS seems unhappy.

[EWS Watchlist]

Comment on attachment 336525 [details]
Patch

Attachment 336525 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7105391

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/registration-iframe.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/registration-script.https.html

[EWS Watchlist]

Created attachment 336533 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 336525 [details]
Patch

Attachment 336525 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7105413

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/registration-iframe.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/registration-script.https.html

[EWS Watchlist]

Created attachment 336535 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Comment on attachment 336525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336525&action=review

>> Source/WebCore/ChangeLog:17
>> +        of the maxScopeString. For reading the code this bug is not observable (since nothing happens
> 
> Step 7 of step 9 of the update algorithm in <https://w3c.github.io/ServiceWorker/v1/#update-algorithm> reads:
> 
> [[
> 7. Extract a MIME type from the response’s header list. If this MIME type (ignoring parameters) is not a JavaScript MIME type, then:
> 
>     1. Invoke Reject Job Promise with job and "SecurityError" DOMException.
>     2. Asynchronously complete these steps with a network error.
> ]]
> 
> I interpret "Asynchronously complete these steps with a network error" as queue a network error failure and stop the algorithm at this point.

Regressed Web-Platform_tests:
--- /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/registration-iframe.https-expected.txt
+++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/registration-iframe.https-actual.txt
@@ -1,5 +1,6 @@
+
 
 PASS register method should use the "relevant global object" to parse its scriptURL and scope - normal case 
-PASS register method should use the "relevant global object" to parse its scriptURL and scope - error case 
+FAIL register method should use the "relevant global object" to parse its scriptURL and scope - error case assert_unreached: unexpected rejection: assert_equals: register method with scriptURL and scope parsed to nonexistent location should reject with TypeError expected "TypeError" but got "SecurityError" Reached unreachable code
 PASS A scope url should start with the given script url 
 
Note that the tests and other browsers do not always match the service worker spec unfortunately.

[EWS Watchlist]

Comment on attachment 336525 [details]
Patch

Attachment 336525 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7105487

New failing tests:
imported/w3c/web-platform-tests/workers/WorkerGlobalScope_importScripts_NosniffErr.htm

[EWS Watchlist]

Created attachment 336537 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6