Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw NetworkError for cross-origin importScripts() exceptions #166

Closed
wants to merge 1 commit into from

Conversation

annevk
Copy link
Member

@annevk annevk commented Sep 18, 2015

This fixes the following issues from #164:

  • importScripts() can fetch “no-cors” cross-origin resources. If those
    fail to parse or throw an exception we shouldn’t simply rethrow that
    without muting it as that would leak data that <script> does not leak.
    This makes them throw a NetworkError instead.
  • This also clarifies that a NetworkError is thrown if the response is
    not an ok status using the Fetch terminology.

@annevk
Copy link
Member Author

annevk commented Sep 18, 2015

It does seem the wording here could be further improved since "create a script" already has "report the error" which presumably is used for parsing purposes. So it's unclear how that error could suddenly find itself outside that algorithm. Does it end up being reported twice?

@Hixie any thoughts?

@bzbarsky
Copy link
Contributor

@annevk What exactly does it mean that I'm the assignee here?

@annevk
Copy link
Member Author

annevk commented Sep 18, 2015

@bzbarsky I was hoping you'd review it since it provides a fix (of sorts) for https://www.w3.org/Bugs/Public/show_bug.cgi?id=28961. If you have suggestions for "create a script" already reporting the exception that would be most welcome too.

@Hixie
Copy link
Member

Hixie commented Sep 18, 2015

I couldn't get github to show me the diff for #95, so I'm not really sure what any of these terms mean any more.

@jdm
Copy link
Member

jdm commented Sep 18, 2015

#95 isn't a PR; did you mean #144? https://patch-diff.githubusercontent.com/raw/whatwg/html/pull/144.diff will show the raw diff.

@Hixie
Copy link
Member

Hixie commented Sep 18, 2015

@jdm ah, thanks.

@annevk
Copy link
Member Author

annevk commented Sep 18, 2015

Note that the "create a script" setup has remained the same. So that reporting errors to the global while importScripts() is running synchronously (and wants to rethrow exceptions itself) is a problem either way.

@@ -96196,7 +96196,8 @@ interface <dfn>SharedWorker</dfn> : <span>EventTarget</span> {

<li>

<p>To <span>get a fetch result</span>, the user agent must run these steps:
<p>To <span>get a fetch result</span>, given a <var>url</var> and <var>settings object</var> the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: comma after settings object

@domenic
Copy link
Member

domenic commented Sep 23, 2015

This LGTM. Will leave it up to you to decide if further review is needed.

@bzbarsky
Copy link
Contributor

LGTM too, assuming something already sets the CORS-cross-origin flag for these requests correctly.

This fixes the following issues from #164:

* importScripts() can fetch “no-cors” cross-origin resources. If those
  fail to parse or throw an exception we shouldn’t simply rethrow that
  without muting it as that would leak data that <script> does not
  leak. This makes them throw a NetworkError instead.

* This also clarifies that a NetworkError is thrown if the response is
  not an ok status using the Fetch terminology.
@annevk
Copy link
Member Author

annevk commented Sep 24, 2015

1e865ed is the commit to master. Noticed an error in a prior commit to master just before push.

@annevk annevk closed this Sep 24, 2015
@annevk annevk deleted the importscripts branch September 24, 2015 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants