Adapt to Fetch's data URL changes #1782

annevk · 2016-09-14T14:11:25Z

The change to Fetch discussed in
whatwg/fetch#381 made it obsolete.

Closes #1243, closes #1778, and closes #1779 as these are all treated
as same-origin now per the change to Fetch.

domenic · 2016-09-14T14:34:58Z

LGTM (after #1782 lands). Would be nice to either write some WPTs or file a bug with suggestions on what sort of tests to write.

mikewest

LGTM with a few tiny nits.

mikewest · 2016-09-15T12:53:19Z

source

+   <li><p>If <var>is shared</var> is true and <var>url</var>'s <span
+   data-x="concept-url-scheme">scheme</span> is "<code>data</code>", then <span>queue a task</span>
+   to <span>fire a simple event</span> named <code data-x="event-error">error</code> at
+   <var>worker</var>, and abort these steps.</p></li>

   <li id="worker-processing-model-top">


Nit: Probably move this ID up to the new first item (or the ol?)

I guess that's fine, I wonder who uses that ID...

mikewest · 2016-09-15T12:54:31Z

source

@@ -97025,6 +97029,11 @@ interface <dfn>AbstractWorker</dfn> {
   <li><p>Let <var>worker global scope</var> be <var>realm</var>'s <span
   data-x="concept-realm-global">global object</span>.</p></li>

+   <li><p>Let <var>origin</var> be an <span data-x="concept-origin-opaque">opaque origin</span> if


In https://html.spec.whatwg.org/#concept-origin-opaque, Document objects are assigned "a unique opaque origin". Probably best to remain consistent with that "unique" usage.

mikewest · 2016-09-15T12:55:32Z

source

-    data-x="blob protocol">blob:</code> URLs.</p>
+    <p class="note">Any <span data-x="same origin">same-origin</span> URL, including <code
+    data-x="blob protocol">blob:</code> URLs, and any <code data-x="data protocol">data:</code> URL
+    works.</p>


Nit: s/works/can be used/?

mikewest · 2016-09-15T12:55:41Z

source

-    data-x="blob protocol">blob:</code> URLs.</p>
+    <p class="note">Any <span data-x="same origin">same-origin</span> URL, including <code
+    data-x="blob protocol">blob:</code> URLs, works. <code data-x="data protocol">data:</code> URLs
+    do not work.</p>


By-and-large browsers treat data URLs as same-origin, though there are some inconsistencies. This change will treat all data URLs, regardless of origin, as same-origin from the perspective of Fetch. HTML already assigns a unique opague origin to documents created from a data URL and the plan of record is to do so for dedicated workers too. HTML will likely also forbid shared workers to be created from data URLs. See whatwg/html#1782 for the proposed changes to HTML. (This has not landed yet, if that PR is tweaked further the note added here might need some tweaks.) Service workers already prevent anything but HTTP(S) URLs from creating them. Fixes #381.

annevk · 2016-09-16T10:32:04Z

It seems that given #1243 (comment) we might want to also make this work for shared workers after all. It would require an additional origin slot I think, or something better I have not thought of. All that is pending approval from Chrome so marking this red.

annevk · 2016-09-30T14:46:43Z

I discussed this with @mikewest again and he's fine with the Firefox model as that allows introducing opaque origin workers going forward even without the use of data URLs.

(We could maybe say that for module workers, data URLs are same-origin, but I'm not sure if we want to go there. But technically the concern that we have with classic workers doesn't apply there.)

The main remaining problem with this patch is that cf0355d badly regressed setting state on SharedWorkerGlobalScope. There's basically no place right now where we set name and constructor url on it. @domenic any suggestions for how to fix that?

Basically wherever we decide setting that state, we should also set constructor origin. That would make everything work and it's not even that much additional complexity.

cf0355d regressed setting up a SharedWorkerGlobalScope by removing the steps that set its constructor url and name. This was noticed in #1782 (comment).

domenic · 2016-10-04T17:30:30Z

Think I fixed that in #1858; unassigning from me. Reassign if you want me to actually review this PR :)

annevk · 2016-10-05T08:21:13Z

Yeah, I'd like review from you both I think. I realize there's a little bit of rebasing that needs to be done, but I'd rather wait with that until #1858 lands and I think this can be reviewed in quite some detail already.

cf0355d regressed setting up a SharedWorkerGlobalScope by removing the steps that set its constructor url and name. This was noticed in #1782 (comment). This also changes the constructor url from a string to a URL record, for consistency with other URLs stored on objects throughout the spec.

domenic · 2016-10-05T15:50:38Z

source

@@ -97073,6 +97070,12 @@ interface <dfn>AbstractWorker</dfn> {
   <li><p>Let <var>worker global scope</var> be <var>realm</var>'s <span
   data-x="concept-realm-global">global object</span>.</p></li>

+   <li><p>Let <var>origin</var> be a unique <span


It wouldn't impact anything normative, but I think you could inline this algorithm into the origin "getter" below instead of creating a variable for it.

domenic · 2016-10-05T15:53:11Z

source

-    data-x="blob protocol">blob:</code> URLs.</p>
+    <p class="note">Any <span data-x="same origin">same-origin</span> URL, including <code
+    data-x="blob protocol">blob:</code> URLs, and any <code data-x="data protocol">data:</code> URL
+    can be used.</p>


Phrasing is awkward, hmm.

"Any same-origin URL (including blob: URLs) can be used; data: URLs will also work, despite not being same-origin"?

(Here and below, whatever we end up on.)

domenic · 2016-10-05T15:57:43Z

source

+     data-x="concept-SharedWorkerGlobalScope-constructor-url">constructor url</span> is
+     <var>urlString</var>, and <span data-x="concept-SharedWorkerGlobalScope-name">name</span> is
+     <var>name</var>, then set <var>worker global scope</var> to that
+     <code>SharedWorkerGlobalScope</code> object.</p></li>


I think that we should add a note here that explains how it's possible for constructor url to be equal but constructor origin to be different. Something like: "The reason we compare both constructor url and constructor origin is that data: URLs cause the created worker settings object to have a unique opaque origin, instead of one derived from the constructor url. This ensures that using the same data: URL multiple times creates separate SharedWorkerGlobalScope instances, as desired." (Hope I got that right...)

That's not quite right. The same data URL can be used to get the same instance, but we don't want to allow that across documents of different origins. Therefore we store the origin of the constructor. So only if the constructor origin is same-origin can the URL be equal.

cf0355d regressed setting up a SharedWorkerGlobalScope by removing the steps that set its constructor url and name. This was noticed in #1782 (comment). This also changes the constructor url from a string to a URL record, for consistency with other URLs stored on objects throughout the spec.

annevk · 2016-10-07T09:41:51Z

@zcorpan do you know if data URLs are already tested? Is the top-level workers/ directory the only place where we have tests? (I guess it's not part of html/ due to forks?)

I also found stuff is wrong, e.g., https://github.com/w3c/web-platform-tests/blob/master/workers/constructors/SharedWorker/URLMismatchError.htm tests for a "URLMismatchError" which if I recall correctly we removed some time ago.

domenic · 2016-10-07T19:15:27Z

source

-    can be used.</p>
+    <p class="note">Any <span data-x="same origin">same-origin</span> URL (including <code
+    data-x="blob protocol">blob:</code> URLs) can be used. <code data-x="data protocol">data:</code>
+    URLs can also be used, but create a worker with an <span data-x="concept-opaque-origin">opaque


Nit: "but they create" instead of "but create"; both are correct but the former reads a bit easier. Here and below.

Provide basic tests for whatwg/html#1782.

The change to Fetch discussed in whatwg/fetch#381 made it obsolete. Closes #1778, and closes #1779 as these are all treated as same-origin now per the change to Fetch.

Fixes #1243. Basically, data URLs create (shared) workers that have a unique opaque origin.

Basic test for whatwg/html#1782.

See whatwg/html#1782 for pointers to more context.

annevk · 2016-10-10T13:14:14Z

I added some tests. This can be landed through "rebase and merge" imo.

domenic · 2016-10-12T03:43:30Z

For posterity and tracking purposes, the tests

Do @bzbarsky or @mikewest want to help review those? I hope we can get them merged sooner rather than later; letting tests languish is a bad failure mode for this more-tests push. (/cc @foolip)

Since #387 landed HTML’s change in whatwg/html#1782 was adjusted a bit for shared workers.

foolip · 2016-10-12T09:46:39Z

I've taken a look at the tests, but would really like @mikewest to do so as well.

Basic test for whatwg/html#1782.

See whatwg/html#1782 for pointers to more context.

Provide basic tests for whatwg/html#1782.

foolip · 2016-10-12T11:47:23Z

All three tests reviewed and merged by me, but further review wouldn't hurt.

bzbarsky · 2016-10-14T17:51:59Z

All three tests look good to me.

cf0355d regressed setting up a SharedWorkerGlobalScope by removing the steps that set its constructor url and name. This was noticed in whatwg#1782 (comment). This also changes the constructor url from a string to a URL record, for consistency with other URLs stored on objects throughout the spec.

annevk added topic: fetch compat Standard is not web compatible or proprietary feature needs standardizing labels Sep 14, 2016

annevk mentioned this pull request Sep 15, 2016

What is the interaction between data URLs and workers (and some other data URL discussion) #1243

Closed

annevk changed the title ~~Remove Fetch's same-origin data URL flag~~ Adapt to Fetch's data URL changes Sep 15, 2016

mikewest approved these changes Sep 15, 2016

View reviewed changes

annevk added the do not merge yet Pull request must not be merged per rationale in comment label Sep 16, 2016

annevk force-pushed the remove-same-origin-data-url-flag branch 2 times, most recently from 14d1e7a to 7434119 Compare September 30, 2016 14:22

annevk assigned mikewest and domenic Oct 4, 2016

domenic added a commit that referenced this pull request Oct 4, 2016

Fix regression in setting up SharedWorkerGlobalScope

8445883

cf0355d regressed setting up a SharedWorkerGlobalScope by removing the steps that set its constructor url and name. This was noticed in #1782 (comment).

domenic mentioned this pull request Oct 4, 2016

Fix regression in setting up SharedWorkerGlobalScope #1858

Merged

domenic removed their assignment Oct 4, 2016

annevk assigned domenic Oct 5, 2016

domenic reviewed Oct 5, 2016

View reviewed changes

annevk force-pushed the remove-same-origin-data-url-flag branch from dde4386 to 07b2fc7 Compare October 7, 2016 09:12

domenic approved these changes Oct 7, 2016

View reviewed changes

domenic added needs tests Moving the issue forward requires someone to write tests and removed do not merge yet Pull request must not be merged per rationale in comment labels Oct 7, 2016

annevk added a commit to web-platform-tests/wpt that referenced this pull request Oct 10, 2016

Workers and data URLs

318ab97

Provide basic tests for whatwg/html#1782.

annevk mentioned this pull request Oct 10, 2016

Workers and data URLs web-platform-tests/wpt#3943

Merged

annevk added 2 commits October 10, 2016 12:42

Remove Fetch's same-origin data URL flag

a61e36d

The change to Fetch discussed in whatwg/fetch#381 made it obsolete. Closes #1778, and closes #1779 as these are all treated as same-origin now per the change to Fetch.

Define how data URLs affect workers

d1088a2

Fixes #1243. Basically, data URLs create (shared) workers that have a unique opaque origin.

annevk force-pushed the remove-same-origin-data-url-flag branch from 19272bd to d1088a2 Compare October 10, 2016 10:44

annevk added a commit to web-platform-tests/wpt that referenced this pull request Oct 10, 2016

<img> and a data URL

89d5ac3

Basic test for whatwg/html#1782.

annevk mentioned this pull request Oct 10, 2016

<img> and a data URL web-platform-tests/wpt#3944

Merged

annevk added a commit to web-platform-tests/wpt that referenced this pull request Oct 10, 2016

Basic <script> and data URL test

743df7b

See whatwg/html#1782 for pointers to more context.

annevk mentioned this pull request Oct 10, 2016

Basic <script> and data URL test web-platform-tests/wpt#3945

Merged

domenic removed the needs tests Moving the issue forward requires someone to write tests label Oct 10, 2016

domenic merged commit c592f62 into master Oct 10, 2016

domenic deleted the remove-same-origin-data-url-flag branch October 10, 2016 17:30

annevk added a commit to whatwg/fetch that referenced this pull request Oct 12, 2016

Editorial: data URL shared workers are a thing now

8da8e2d

Since #387 landed HTML’s change in whatwg/html#1782 was adjusted a bit for shared workers.

foolip pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 12, 2016

<img> and a data URL (#3944)

e7faed0

Basic test for whatwg/html#1782.

foolip pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 12, 2016

Basic <script> and data URL test (#3945)

dd603c3

See whatwg/html#1782 for pointers to more context.

foolip pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 12, 2016

Workers and data URLs (#3943)

21a3253

Provide basic tests for whatwg/html#1782.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt to Fetch's data URL changes #1782

Adapt to Fetch's data URL changes #1782

annevk commented Sep 14, 2016

domenic commented Sep 14, 2016

mikewest left a comment

mikewest Sep 15, 2016

annevk Sep 15, 2016

mikewest Sep 15, 2016

mikewest Sep 15, 2016

mikewest Sep 15, 2016

annevk commented Sep 16, 2016

annevk commented Sep 30, 2016

domenic commented Oct 4, 2016

annevk commented Oct 5, 2016

domenic Oct 5, 2016

domenic Oct 5, 2016

domenic Oct 5, 2016

annevk Oct 7, 2016

annevk commented Oct 7, 2016

domenic Oct 7, 2016 •

edited

Loading

annevk commented Oct 10, 2016

domenic commented Oct 12, 2016

foolip commented Oct 12, 2016

foolip commented Oct 12, 2016

bzbarsky commented Oct 14, 2016

Adapt to Fetch's data URL changes #1782

Adapt to Fetch's data URL changes #1782

Conversation

annevk commented Sep 14, 2016

domenic commented Sep 14, 2016

mikewest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Sep 16, 2016

annevk commented Sep 30, 2016

domenic commented Oct 4, 2016

annevk commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Oct 7, 2016

domenic Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

annevk commented Oct 10, 2016

domenic commented Oct 12, 2016

foolip commented Oct 12, 2016

foolip commented Oct 12, 2016

bzbarsky commented Oct 14, 2016

domenic Oct 7, 2016 •

edited

Loading