Ben Kelly [:bkelly, not reviewing] Reporter
	Description • 7 years ago

Currently we have a disabled implementation of the old Request.context:

https://searchfox.org/mozilla-central/source/dom/webidl/Request.webidl#21

We should eventually implement Request.destination.

https://fetch.spec.whatwg.org/#dom-request-destination

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 1 • 7 years ago

Can we add this to our planning for first half next year?  Developers have begun to ask for it, like in:

https://github.com/whatwg/fetch/issues/634

(Or should I just clear the priority to get it re-triaged?  I don't know how this works...)

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 2 • 7 years ago

(In reply to Ben Kelly [:bkelly] from comment #1)
> Can we add this to our planning for first half next year?  Developers have
> begun to ask for it, like in:
> 
> https://github.com/whatwg/fetch/issues/634

Noted. Let me take that into 2018 planning consideration, thanks Ben.

> 
> (Or should I just clear the priority to get it re-triaged?  I don't know how
> this works...)

Needinfo works very well, though either way should do. :)

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 3 • 6 years ago

Eden is planning to put this into his working queue. Thanks!

	Andrew Sutherland [:asuth] (he/him)
	Comment 9 • 6 years ago

:ckerschb, I could use some guidance on this bug.  The situation as I understand it is:

- CSP defines a certain set of policy types, one of which is "media".
- We define a set of additional types of INTERNAL types that originate from within Gecko, such as when <audio> or <video> tags are encountered.  These get down-mapped to CSP types when dealt with "externally".  For example, we map TYPE_INTERNAL_{AUDIO,VIDEO,TRACK} to TYPE_MEDIA.
- We use the same type "nsContentPolicyType" for both internal and external types.  nsILoadInfo distinguishes between the two types.  It only stores mInternalContentPolicyType, but its `externalContentPolicyType` getter uses nsContentUtils::InternalContentPolicyTypeToExternal to return an external value which is guaranteed to not be one of the INTERNAL constants.  Unfortunately, most variables of type nsContentPolicy do not indicate whether they are expected to be internal/external.

- fetch defines a set of "destination" types at https://fetch.spec.whatwg.org/#concept-request-destination that are more specific than those CSP understands.  Specifically, there are distinct "audio", "video", and "track" destinations.
- It's possible to map these destinations to/from our nsContentPolicyType values, but things get awkward because INTERNAL no longer makes a lot of sense.
- Preload added a <link as="fetch-destination"> thing.
  - Link::ParseAsValue maps from a subset of these strings to Link::ASDestination enums.
  - Link::AsValueToContentPolicy maps from these DESTIONATION_* enum values to nsContentPolicyType values, flattening all of DESTINATION_{AUDIO,TRACK,VIDEO} to nsIContentPolicy::TYPE_MEDIA.

In order to implement Request.destination and make us conformant with existing WPT tests, these patches make Link::AsValueToContentPolicy map to internal types as the path of least resistance.  Unfortunately, it appears that there are areas in the code where TYPE_MEDIA is used, TYPE_INTERNAL_{AUDIO,VIDEO,TRACK} are not, but the INTERNAL script types are.  My first instinct would be to expand the cases to include the internal media types as well, but I'm worried that might be making the situation worse.

My question is:
- What should we be doing about the destination types versus the CSP types?  Should the internal nsContentPolicyTypes become a completely distinct type?  Practically speaking, I think this means nsInternalContentType would be introduced and all its values would all have an INTERNAL prefix.
- If not that, should we just ensure that every switch-statement-like thing that's not dealing with nsILoadInfo and guaranteed to have a notion of internal/external therefore have complete coverage of all of the nsContentPolicyType cases?

	Andrew Sutherland [:asuth] (he/him)
	Comment 10 • 6 years ago

Comment on attachment 8959121 [details] [diff] [review]
Implement Request.destination.

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

:baku, can you sign off on the WebIDL changes (and I guess notionally delegate review to me for these patches)?  I confirm they're consistent with what's currently in https://fetch.spec.whatwg.org/#idl-index

:catalinb, please also remove the pref from the default prefs file at https://searchfox.org/mozilla-central/rev/de5c4376b89c3603341d226a65acea12f8851ec5/modules/libpref/init/all.js#5877.

And the tests where it gets set:
- https://searchfox.org/mozilla-central/rev/de5c4376b89c3603341d226a65acea12f8851ec5/dom/serviceworkers/test/test_not_intercept_plugin.html#68
- https://searchfox.org/mozilla-central/rev/de5c4376b89c3603341d226a65acea12f8851ec5/dom/serviceworkers/test/test_request_context.js#67
- https://searchfox.org/mozilla-central/rev/de5c4376b89c3603341d226a65acea12f8851ec5/dom/tests/mochitest/fetch/fetch_test_framework.js#17

::: dom/base/Link.cpp
@@ -944,2 @@
>    case DESTINATION_VIDEO:
> -    return nsIContentPolicy::TYPE_MEDIA;

I've needinfo'ed :ckerschb about the meta-issue here, although I think I'm now okay with this change.  It's all just very complicated and confusing because the nsContentPolicyType seems to conflate three sets of semantics into a single type.

::: dom/fetch/InternalRequest.cpp
@@ +230,5 @@
>    SetContentPolicyType(aContentPolicyType);
>    mContentPolicyTypeOverridden = true;
>  }
>  
> +/* static */ RequestDestination

It would be great to add an explanation of the relationship between this method, its return type, and the private Link::ASDestination enum type at https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/dom/base/Link.h#200

Based on my understanding, perhaps something like this:

"""
Map the content policy type to the associated fetch destination, as defined by the spec at https://fetch.spec.whatwg.org/#concept-request-destination.  Note that while the HTML spec for the "Link" element and its "as" attribute (https://html.spec.whatwg.org/#attr-link-as) reuse fetch's definition of destination, and the Link class has an internal Link::ASDestination enum type, the latter is only a support type to map the string values via Link::ParseAsValue and Link::AsValueToContentPolicy to our canonical nsContentPolicyType.
"""

::: dom/fetch/InternalRequest.h
@@ +44,2 @@
>   * font              | TYPE_FONT
> + * image             | TYPE_INTERNAL_IMAGE, TYPE_INTERNAL_IMAGE_PRELOAD,

image is missing TYPE_IMAGE

@@ +47,5 @@
>   * manifest          | TYPE_WEB_MANIFEST
> + * object            | TYPE_INTERNAL_OBJECT, TYPE_OBJECT
> + * "paintworklet"    | TODO
> + * report"           | TODO
> + * script            | TYPE_INTERNAL_SCRIPT, TYPE_INTERNAL_SCRIPT_PRELOAD,

script is missing TYPE_SCRIPT

@@ +53,1 @@
>   * sharedworker      | TYPE_INTERNAL_SHARED_WORKER

There should probably be a "serviceworker" entry here with a TODO or "impossible" or something like that, given the presence of the entry in the fetch spec.

::: testing/web-platform/tests/fetch/api/request/destination/resources/fetch-destination-worker-no-load-event.js
@@ +6,4 @@
>              if (event.request.destination == destination) {
> +              result = "PASS";
> +            }
> +            let cl = await clients.matchAll({includeUncontrolled: true});

It's not clear why the changes to this file and fetch-destination-no-load-event.https.html were made.  Was this a temporary hack fix that snuck in the patch but is no longer required?

Restating my understanding of the test without the changes:
- The test pages (fetch-destination-no-load-event.https.html and also fetch-destination-worker.https.html) register this SW with the scope of an iframe it will create.
- The test pages wait for the SW to activate.
- The iframe is created and therefore should be controlled.  (And since the test works at all, this seems to be sound.)
- All test cases manipulate the iframe, so the SW should intercept the requests.
- The (unmodified) SW sends the results back to the client that triggered the fetch.
- Matching that, the tests add their message listener inside the iframe, so they will receive the message in the client.

Aside: Since the SW is dealing with a fetch event and its clientId field, bug 1446225 does come into play, but as noted there, although our id's are inconsistent in this case, clients.get() should make things work out.

	Christoph Kerschbaumer [:ckerschb]
	Comment 11 • 6 years ago

(In reply to Andrew Sutherland [:asuth] from comment #9)

Andrew, let me first provide some background information. Legacy extensions were able to hook into nsIContentPolicy API which allowed them to do all kinds of content blocking. Among others, that API exposed the content policy type to addons. Since legacy extensions were able to hook into that API we basically haven't touched it for years but needed some workarounds for dealing with the various types that we needed to distinguish within Firefox. E.g. CSP (which implements nsIContentPolicy) needed to handle script preloads differently, hence we started to add INTERNAL_SCRIPT_PRELOAD. To implementations within addons however, we only passed the external type. Now that legacy extensions are gone we updated the API and pass a loadinfo instead of the various arguments (see Bug 965637) which improves things drastically. Having that freedom allows use some other bit within the loadInfo to indicate a preload and not having to rely on the INTERNAL_SCRIPT_PRELOAD type if needed.

> My question is:
> - What should we be doing about the destination types versus the CSP types? 
CSP uses it's own mapping internally [1] which should be fairly easy to update depending on what we decide.
[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPUtils.cpp#207

> Should the internal nsContentPolicyTypes become a completely distinct type? 
> Practically speaking, I think this means nsInternalContentType would be
> introduced and all its values would all have an INTERNAL prefix.

I don't think that anything 'extern' can still access any of our content policy types. So there is no need to prefix 'INTERNAL'. In other words we have all the freedom we need as long as we update all of our own consumers of nsIContentPolicy to deal with the new types correctly.

> - If not that, should we just ensure that every switch-statement-like thing
> that's not dealing with nsILoadInfo and guaranteed to have a notion of
> internal/external therefore have complete coverage of all of the
> nsContentPolicyType cases?

Given what you are trying to accomplish within this bug, this second solution of fixing all the switch statements sounds more error prone to me. But one thing I don't understand. All channels should have a loadinfo by now and all desitinations are somehow based on a requests/channel right? What are the contests where we would need such switch statements? Would it make sense to expose a method on the Loadinfo like
  readonly attribute DestinationType destinationPolicyType;
or am I missing something?

	Cătălin Badea (:catalinb) Assignee
	Comment 12 • 6 years ago

> It's not clear why the changes to this file and
> fetch-destination-no-load-event.https.html were made.  Was this a temporary
> hack fix that snuck in the patch but is no longer required?

The client creates a worker that loads importer.js
https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/testing/web-platform/tests/fetch/api/request/destination/resources/importer.js

This worker is the client which shows up in the intercepted request and gets the PASS/FAIL message from the service worker. I think this is related to bug 1451124.

	Cătălin Badea (:catalinb) Assignee
	Comment 13 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46d724ea6978767a6e73c5b42e32d7a8ac4bcc1f

	Cătălin Badea (:catalinb) Assignee
	Comment 16 • 6 years ago

This patch removes mochitests that were testing only request.context and
updates a couple others to use request.destination instead.

Carrying r+, :asuth was ok (on vidyo) with removing request.context specific
mochitests.

	Pulsebot
	Comment 17 • 6 years ago

Pushed by catalin.badea392@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34e219813b64
Enable fetch Request.destination webplatform tests. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fce477733f0
Implement Request.destination. r=asuth,baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/7218641c9b12
Update mochitests related to request.context. r=asuth

	Cristina Coroiu [:ccoroiu]
	Comment 18 • 6 years ago

Backed out 3 changesets (bug 1402892) for wpt failures in /fetch/api/request/request-idl.html on a CLOSED TREE

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7218641c9b12c94c01a6dbd10cb2e96e5692b9ef&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=172694565&repo=mozilla-inbound&lineNumber=2482

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/30a4eaf2f075b32d9e60d04af9361169bf168eac

	Cătălin Badea (:catalinb) Assignee
	Comment 19 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f17b9aa15460a213c855c5c54f86eba4273277c

	Pulsebot
	Comment 21 • 6 years ago

Pushed by catalin.badea392@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d761483af02
Enable fetch Request.destination webplatform tests. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/284b35d145ff
Implement Request.destination. r=asuth,baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/30dd7a04d53a
Update mochitests related to request.context. r=asuth

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 22 • 6 years ago

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10425 for changes under testing/web-platform/tests

	Pulsebot
	Comment 23 • 6 years ago

Pushed by catalin.badea392@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaafa0277f81
Update wpt expectations for request.destination. a=test-only CLOSED TREE

	Stefan Hindli [:stefan_hindli]
	Comment 24 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2d761483af02
https://hg.mozilla.org/mozilla-central/rev/284b35d145ff
https://hg.mozilla.org/mozilla-central/rev/30dd7a04d53a
https://hg.mozilla.org/mozilla-central/rev/aaafa0277f81

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 25 • 6 years ago

Upstream PR merged

	Eric Shepherd [:sheppy]
	Comment 26 • 6 years ago

New pages:

https://developer.mozilla.org/en-US/docs/Web/API/RequestDestination

https://developer.mozilla.org/en-US/docs/Web/API/Request.destination

Updated page:

https://developer.mozilla.org/en-US/Firefox/Releases/61

I'm certain there's more that could be done here in terms of integrating the concept into the main body of the docs, but there are clarifications that need to be gathered, so this will do for now.

	Eric Shepherd [:sheppy]
	Comment 27 • 6 years ago

Oh - also, BCD and other database submissions have PRs filed awaiting review.

	Marion Daly [:mdaly]
	Comment 28 • 6 years ago

removing old need-info