1147699 - Enable FetchEvent.request.context to reflect the existing contexts supported by nsContentPolicyType

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[(no longer active)]

Attachment: Part 1: Move Request::mContext to InternalRequest, and determine the mapping to nsContentPolicyType; r=nsm

As the documentation in InternalRequest.h in this patch shows, the
mapping between nsContentPolicyType and RequestContext is not complete
yet.  Because the InternalRequest object needs to know the actual
nsContentPolicyType in order for FetchDriver to be able to use that
information, we can't just store the RequestContext.  Therefore, this
patch adds both of these to InternalRequest.  Once we get to a stage
where we have a complete mapping of these values, we can store only one
of them and compute the other from it.  That requires addressing all of
the TODO comments in the InternalRequest.h documentation.

Comment 2

[(no longer active)]

Attachment: Part 2: Set the content policy type on FetchEvent.request based on the content policy type of the channel; r=nsm

Note that InternalRequest::SetContentPolicyType takes care of updating the
RequestContext value stored in InternalRequest too.

Comment 3

[(no longer active)]

Attachment: Part 3: Add a test for FetchEvent.request.context when intercepting an image load; r=nsm

Comment 4

[(no longer active)]

Attachment: Part 4: Add a test for FetchEvent.request.context when intercepting a responsive image load; r=nsm

Comment 5

[(no longer active)]

Attachment: Part 5: Add a test for FetchEvent.request.context when intercepting an audio element load; r=nsm

Comment 6

[(no longer active)]

Attachment: Part 6: Add a test for FetchEvent.request.context when intercepting a video element load; r=nsm

Note that currently our implementation incorrectly reflects this as
'audio'.  This will be fixed later.

Comment 7

[(no longer active)]

Attachment: Part 7: Add a test for FetchEvent.request.context when intercepting a beacon load; r=nsm

Comment 8

[(no longer active)]

Comment on attachment 8583517 [details] [diff] [review]
Part 1: Move Request::mContext to InternalRequest, and determine the mapping to nsContentPolicyType; r=nsm

(More patches to come tomorrow to add tests for the rest of the RequestContext types!)

Comment 9

[(no longer active)]

Attachment: Part 8: Add a test for FetchEvent.request.context when intercepting a CSP report

Comment 10

[(no longer active)]

Attachment: Part 9: Add tests for FetchEvent.request.context when intercepting embeds and objects

Comment 11

[(no longer active)]

Attachment: Part 10: Add a test for FetchEvent.request.context when intercepting font loads

Comment 12

[(no longer active)]

Attachment: Part 11: Add tests for FetchEvent.request.context when intercepting iframes and frames

Comment 13

[(no longer active)]

Attachment: Part 12: Add a test for FetchEvent.request.context when intercepting new window loads

Comment 14

[(no longer active)]

Attachment: Part 13: Add a test for FetchEvent.request.context when intercepting ping loads

Comment 15

[(no longer active)]

Attachment: Part 14: Add a test for FetchEvent.request.context when intercepting loads coming from plugins

Comment 16

[(no longer active)]

Attachment: Part 15: Add a test for FetchEvent.request.context when intercepting script loads

Comment 17

[(no longer active)]

Attachment: Part 16: Add a test for FetchEvent.request.context when intercepting style loads

Comment 18

[(no longer active)]

Attachment: Part 17: Add a test for FetchEvent.request.context when intercepting media track loads

Comment 19

[(no longer active)]

Attachment: Part 18: Add a test for FetchEvent.request.context when intercepting XHR loads

Comment 20

[(no longer active)]

Attachment: Part 19: Add a test for FetchEvent.request.context when intercepting XSLT loads

Comment 21

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8583517 [details] [diff] [review]
Part 1: Move Request::mContext to InternalRequest, and determine the mapping to nsContentPolicyType; r=nsm

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

::: dom/fetch/InternalRequest.h
@@ +24,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +/*

This comment has some trailing whitespace.

::: dom/workers/test/serviceworkers/fetch/context/index.html
@@ +23,5 @@
>    testFetch()
>    .then(function() {
>      finish();
> +  }, function(e) {
> +    ok(false, "A promise was rejected: " + e);

call finish() in this branch too.

Comment 22

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8583519 [details] [diff] [review]
Part 3: Add a test for FetchEvent.request.context when intercepting an image load; r=nsm

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

image file is missing!

::: dom/workers/test/serviceworkers/fetch/context/context_test.js
@@ +12,5 @@
>      event.respondWith(new Response(body));
> +  } else if (event.request.url.indexOf("img.jpg") >= 0) {
> +    if (event.request.context == "image") {
> +      event.respondWith(fetch("realimg.jpg"));
> +    }

else {
  event.respondWith(Promise.reject());
}

::: dom/workers/test/serviceworkers/fetch/context/index.html
@@ +39,4 @@
>    .then(function() {
>      finish();
>    }, function(e) {
>      ok(false, "A promise was rejected: " + e);

Why not just call finish() here instead of waiting for the test to time out? The test above will fail and we will know something went wrong. Update comment accordingly.

Comment 23

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8583520 [details] [diff] [review]
Part 4: Add a test for FetchEvent.request.context when intercepting a responsive image load; r=nsm

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

Please get review from someone else on the imgLoader changes.

::: dom/workers/test/serviceworkers/fetch/context/context_test.js
@@ +21,1 @@
>    } else {

If you remove the else and always call respondWith, you can get rejection by default for when you want to fail the test (context does not match). Just like Promises, respondWith() does not do anything when called a second time, so it won't always fail.

Comment 24

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8583521 [details] [diff] [review]
Part 5: Add a test for FetchEvent.request.context when intercepting an audio element load; r=nsm

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

ignore earlier review about image file being missing, i see that it shows in the splinter comment.

Comment 25

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8583523 [details] [diff] [review]
Part 7: Add a test for FetchEvent.request.context when intercepting a beacon load; r=nsm

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

::: dom/workers/test/serviceworkers/fetch/context/context_test.js
@@ +34,3 @@
>    } else {
>      // Fail any request that we don't know about.
>      event.respondWith(Promise.reject());

also, to add to earlier patch about making this the default, you can reject with event.request.url so we know exactly which test failed.

Comment 26

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Phew! :)

Comment 27

[(no longer active)]

Comment on attachment 8583520 [details] [diff] [review]
Part 4: Add a test for FetchEvent.request.context when intercepting a responsive image load; r=nsm

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

::: image/src/imgLoader.cpp
@@ +784,5 @@
>      rv = NS_NewChannel(aResult,
>                         aURI,
>                         triggeringPrincipal,
>                         securityFlags,
> +                       aPolicyType,

This was done in <http://hg.mozilla.org/mozilla-central/rev/b292b91c6755#l7.44> which Boris reviewed, and it seems wrong to me, so asking for his review on this code again.

Comment 28

[(no longer active)]

Thanks for doing all of these reviews, Nikhil!

Comment 29

[Boris Zbarsky [:bzbarsky]]

> This was done in <http://hg.mozilla.org/mozilla-central/rev/b292b91c6755#l7.44> which
> Boris reviewed

Ugh.  yes, that's clearly wrong; looks like a merge conflict partway through the patches in bug 1083422 which I didn't catch.  :(

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8583520 [details] [diff] [review]
Part 4: Add a test for FetchEvent.request.context when intercepting a responsive image load; r=nsm

That said, it's a bit disturbing that we had no testcases for this....  I guess now we will!

r=me

Comment 31

[(no longer active)]

Landed with the tests for ping disabled for now until bug 1148064 gets fixed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/65285daacef2
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f6ec0b83379
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b347e46f2f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fb48d7a6ccd
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d1ae408b104
https://hg.mozilla.org/integration/mozilla-inbound/rev/36091401d965
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f7d471a067d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea101f965a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba9a1f1309d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3a2f7b309f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/3be619182773
https://hg.mozilla.org/integration/mozilla-inbound/rev/04ebf0e2d155
https://hg.mozilla.org/integration/mozilla-inbound/rev/a34afcef2bd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7e532d69293

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/65285daacef2
https://hg.mozilla.org/mozilla-central/rev/1f6ec0b83379
https://hg.mozilla.org/mozilla-central/rev/9b347e46f2f0
https://hg.mozilla.org/mozilla-central/rev/8fb48d7a6ccd
https://hg.mozilla.org/mozilla-central/rev/9d1ae408b104
https://hg.mozilla.org/mozilla-central/rev/36091401d965
https://hg.mozilla.org/mozilla-central/rev/9f7d471a067d
https://hg.mozilla.org/mozilla-central/rev/3ea101f965a8
https://hg.mozilla.org/mozilla-central/rev/ba9a1f1309d7
https://hg.mozilla.org/mozilla-central/rev/b3a2f7b309f5
https://hg.mozilla.org/mozilla-central/rev/3be619182773
https://hg.mozilla.org/mozilla-central/rev/04ebf0e2d155
https://hg.mozilla.org/mozilla-central/rev/a34afcef2bd5
https://hg.mozilla.org/mozilla-central/rev/e7e532d69293
https://hg.mozilla.org/mozilla-central/rev/368af8026ced