Bug 184345 – importScripts() should throw exception with a reason when it fails to load a script from the network

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

importScripts() should throw exception with a reason when it fails to load a script from the network.

[Daniel Bates]

Created attachment 337315 [details]
Patch

[EWS Watchlist]

Comment on attachment 337315 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/worker-blob-inherits-csp-importScripts-redirect-cross-origin-blocked.html
http/tests/security/contentSecurityPolicy/worker-csp-importScripts-redirect-cross-origin-blocked.html

[EWS Watchlist]

Created attachment 337319 [details]
Archive of layout-test-results from ews100 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 337315 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/worker-blob-inherits-csp-importScripts-redirect-cross-origin-blocked.html
http/tests/security/contentSecurityPolicy/worker-csp-importScripts-redirect-cross-origin-blocked.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 337315 [details]
Patch

Attachment 337315 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7220926

New failing tests:
http/tests/security/contentSecurityPolicy/worker-blob-inherits-csp-importScripts-redirect-cross-origin-blocked.html
http/tests/security/contentSecurityPolicy/worker-csp-importScripts-redirect-cross-origin-blocked.html

[EWS Watchlist]

Created attachment 337324 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 337315 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/worker-blob-inherits-csp-importScripts-redirect-cross-origin-blocked.html
http/tests/security/contentSecurityPolicy/worker-csp-importScripts-redirect-cross-origin-blocked.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 337315 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/worker-blob-inherits-csp-importScripts-redirect-cross-origin-blocked.html
http/tests/security/contentSecurityPolicy/worker-csp-importScripts-redirect-cross-origin-blocked.html

[EWS Watchlist]

Created attachment 337329 [details]
Archive of layout-test-results from ews115 for mac-sierra

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

[Daniel Bates]

Created attachment 337368 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 337368 [details]
Patch

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

> Source/WebCore/workers/WorkerGlobalScope.cpp:292
> +            return Exception { NetworkError, String { scriptLoader->error().localizedDescription() } };

I thought that the policy was that developer facing error messages were not localized. Is that incorrect?

Also, why is constructing a String necessary here?

[Daniel Bates]

(In reply to Alexey Proskuryakov from comment #13)
> Comment on attachment 337368 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337368&action=review
> 
> > Source/WebCore/workers/WorkerGlobalScope.cpp:292
> > +            return Exception { NetworkError, String { scriptLoader->error().localizedDescription() } };
> 
> I thought that the policy was that developer facing error messages were not
> localized. Is that incorrect?
> 

As I wrote on IRC today (04/06):

[09:40am] dydx: ap: ResourceError extends ResourceErrorBase and ResourceErrorBase owns a single string ResourceErrorBase::m_localizedDescription

As far as I can tell ResourceErrorBase::m_localizedDescription is a misnomer. It always represents a non-localized string. We should consider renaming it.


> Also, why is constructing a String necessary here?

[09:41am] dydx: ap: Exception takes an optional message as its second argument by rvalue reference; => we need to copy the String 
[09:41am] dydx: ap: We should change Exception to take a String then it will support both moving and copying semantics based on how the caller passes the String

[Alexey Proskuryakov]

Comment on attachment 337368 [details]
Patch

> As far as I can tell ResourceErrorBase::m_localizedDescription is a misnomer. It always represents a non-localized string. We should consider renaming it.

How did you come to this conclusion? On Mac at least, it's calling an SPI which performs localization work.

However, there is a lot of precedent for using this particular ResourceError function. I think that it is unfortunate that we are mixing localized platform error strings with non-localized WebCore strings. As mentioned before, my understanding is that we want both non-localized, but I don't really know.

[Daniel Bates]

(In reply to Alexey Proskuryakov from comment #15)
> Comment on attachment 337368 [details]
> Patch
> 
> > As far as I can tell ResourceErrorBase::m_localizedDescription is a misnomer. It always represents a non-localized string. We should consider renaming it.
> 
> How did you come to this conclusion? On Mac at least, it's calling an SPI
> which performs localization work.
> 

I'm blind.

[Daniel Bates]

Committed r230345: <https://trac.webkit.org/changeset/230345>

[Daniel Bates]

(In reply to Daniel Bates from comment #14)
> > Also, why is constructing a String necessary here?
> 
> [09:41am] dydx: ap: Exception takes an optional message as its second
> argument by rvalue reference; => we need to copy the String 
> [09:41am] dydx: ap: We should change Exception to take a String then it will
> support both moving and copying semantics based on how the caller passes the
> String

For completeness, I chose to teach Exception to take a String by value in the patch for bug #184360 and updated this patch to remove the explicit call to the String constructor.

[Radar WebKit Bug Importer]

<rdar://problem/39245413>