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

RESOLVED FIXED in Firefox 40

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

unspecified
mozilla40
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(19 attachments)

15.17 KB, patch
nsm
: review+
Details | Diff | Splinter Review
3.11 KB, patch
nsm
: review+
Details | Diff | Splinter Review
6.89 KB, patch
nsm
: review+
Details | Diff | Splinter Review
5.36 KB, patch
nsm
: review+
Details | Diff | Splinter Review
9.75 KB, patch
nsm
: review+
Details | Diff | Splinter Review
2.54 KB, patch
nsm
: review+
Details | Diff | Splinter Review
5.42 KB, patch
nsm
: review+
Details | Diff | Splinter Review
5.06 KB, patch
nsm
: review+
Details | Diff | Splinter Review
4.01 KB, patch
nsm
: review+
Details | Diff | Splinter Review
2.78 KB, patch
nsm
: review+
Details | Diff | Splinter Review
3.38 KB, patch
nsm
: review+
Details | Diff | Splinter Review
3.05 KB, patch
nsm
: review+
Details | Diff | Splinter Review
5.42 KB, patch
nsm
: review+
Details | Diff | Splinter Review
5.20 KB, patch
nsm
: review+
Details | Diff | Splinter Review
2.74 KB, patch
nsm
: review+
Details | Diff | Splinter Review
2.82 KB, patch
nsm
: review+
Details | Diff | Splinter Review
2.91 KB, patch
nsm
: review+
Details | Diff | Splinter Review
2.67 KB, patch
nsm
: review+
Details | Diff | Splinter Review
4.75 KB, patch
nsm
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8583517 [details] [diff] [review]
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.
(Assignee)

Comment 2

3 years ago
Created attachment 8583518 [details] [diff] [review]
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.
(Assignee)

Comment 3

3 years ago
Created attachment 8583519 [details] [diff] [review]
Part 3: Add a test for FetchEvent.request.context when intercepting an image load; r=nsm
(Assignee)

Comment 4

3 years ago
Created attachment 8583520 [details] [diff] [review]
Part 4: Add a test for FetchEvent.request.context when intercepting a responsive image load; r=nsm
(Assignee)

Comment 5

3 years ago
Created attachment 8583521 [details] [diff] [review]
Part 5: Add a test for FetchEvent.request.context when intercepting an audio element load; r=nsm
(Assignee)

Comment 6

3 years ago
Created attachment 8583522 [details] [diff] [review]
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.
(Assignee)

Comment 7

3 years ago
Created attachment 8583523 [details] [diff] [review]
Part 7: Add a test for FetchEvent.request.context when intercepting a beacon load; r=nsm
(Assignee)

Comment 8

3 years ago
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!)
Attachment #8583517 - Flags: review?(nsm.nikhil)
(Assignee)

Updated

3 years ago
Attachment #8583518 - Flags: review?(nsm.nikhil)
(Assignee)

Updated

3 years ago
Attachment #8583519 - Flags: review?(nsm.nikhil)
(Assignee)

Updated

3 years ago
Attachment #8583520 - Flags: review?(nsm.nikhil)
(Assignee)

Updated

3 years ago
Attachment #8583521 - Flags: review?(nsm.nikhil)
(Assignee)

Updated

3 years ago
Attachment #8583522 - Flags: review?(nsm.nikhil)
(Assignee)

Updated

3 years ago
Attachment #8583523 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 9

3 years ago
Created attachment 8584131 [details] [diff] [review]
Part 8: Add a test for FetchEvent.request.context when intercepting a CSP report
Attachment #8584131 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 10

3 years ago
Created attachment 8584132 [details] [diff] [review]
Part 9: Add tests for FetchEvent.request.context when intercepting embeds and objects
Attachment #8584132 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 11

3 years ago
Created attachment 8584133 [details] [diff] [review]
Part 10: Add a test for FetchEvent.request.context when intercepting font loads
Attachment #8584133 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 12

3 years ago
Created attachment 8584134 [details] [diff] [review]
Part 11: Add tests for FetchEvent.request.context when intercepting iframes and frames
Attachment #8584134 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 13

3 years ago
Created attachment 8584135 [details] [diff] [review]
Part 12: Add a test for FetchEvent.request.context when intercepting new window loads
Attachment #8584135 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 14

3 years ago
Created attachment 8584136 [details] [diff] [review]
Part 13: Add a test for FetchEvent.request.context when intercepting ping loads
Attachment #8584136 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 15

3 years ago
Created attachment 8584137 [details] [diff] [review]
Part 14: Add a test for FetchEvent.request.context when intercepting loads coming from plugins
Attachment #8584137 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 16

3 years ago
Created attachment 8584138 [details] [diff] [review]
Part 15: Add a test for FetchEvent.request.context when intercepting script loads
Attachment #8584138 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 17

3 years ago
Created attachment 8584139 [details] [diff] [review]
Part 16: Add a test for FetchEvent.request.context when intercepting style loads
Attachment #8584139 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 18

3 years ago
Created attachment 8584140 [details] [diff] [review]
Part 17: Add a test for FetchEvent.request.context when intercepting media track loads
Attachment #8584140 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 19

3 years ago
Created attachment 8584141 [details] [diff] [review]
Part 18: Add a test for FetchEvent.request.context when intercepting XHR loads
Attachment #8584141 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 20

3 years ago
Created attachment 8584142 [details] [diff] [review]
Part 19: Add a test for FetchEvent.request.context when intercepting XSLT loads
Attachment #8584142 - Flags: review?(nsm.nikhil)
(Assignee)

Updated

3 years ago
Depends on: 1126820
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.
Attachment #8583517 - Flags: review?(nsm.nikhil) → review+
Attachment #8583518 - Flags: review?(nsm.nikhil) → review+
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.
Attachment #8583519 - Flags: review?(nsm.nikhil) → review+
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.
Attachment #8583520 - Flags: review?(nsm.nikhil) → review+
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.
Attachment #8583521 - Flags: review?(nsm.nikhil) → review+
Attachment #8583522 - Flags: review?(nsm.nikhil) → review+
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.
Attachment #8583523 - Flags: review?(nsm.nikhil) → review+
Attachment #8584131 - Flags: review?(nsm.nikhil) → review+
Attachment #8584132 - Flags: review?(nsm.nikhil) → review+
Attachment #8584133 - Flags: review?(nsm.nikhil) → review+
Attachment #8584134 - Flags: review?(nsm.nikhil) → review+
Attachment #8584135 - Flags: review?(nsm.nikhil) → review+
Attachment #8584136 - Flags: review?(nsm.nikhil) → review+
Attachment #8584137 - Flags: review?(nsm.nikhil) → review+
Attachment #8584138 - Flags: review?(nsm.nikhil) → review+
Attachment #8584139 - Flags: review?(nsm.nikhil) → review+
Attachment #8584140 - Flags: review?(nsm.nikhil) → review+
Attachment #8584141 - Flags: review?(nsm.nikhil) → review+
Attachment #8584142 - Flags: review?(nsm.nikhil) → review+
Phew! :)
(Assignee)

Comment 27

3 years ago
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.
Attachment #8583520 - Flags: review?(bzbarsky)
(Assignee)

Comment 28

3 years ago
Thanks for doing all of these reviews, Nikhil!
> 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 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
Attachment #8583520 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 31

3 years ago
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
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.