1182103 - Test EventSource scenarios with fetch interception

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v1

Apparently EventSource requests get intercepted by the service worker and opaques responses are handled correctly. The onfetch event handler responds with a opaque response (cross origin request previously stored in the cache) and the onerror event is fired on the EventSource object. If I'm not wrong this is how this should work.

Ben, may I have your feedback here please? Do not pay attention to the bunch of printfs and dumps I left here and there. Thanks!

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Sorry, this review might take me till tomorrow.  I need to learn about EventSource.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8634185 [details] [diff] [review]
v1

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

Actually, before I do the full review can you expand the test to do the following:

1) Verify that event.request.type is "cors".  It seems that EventSource spec calls for it to make a CORS request.
2) Verify that a CORS response can be used to intercept.
3) Verify that a synthetic response can be used to intercept.

Also, I think we correctly block the opaque interception due to this check for event.request.type === 'no-cors':

  http://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#279

Can you verify that is what we are hitting?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Here is the spec I was looking at:

  https://html.spec.whatwg.org/multipage/comms.html#dom-eventsource

I also thought we should check the credentials CORS mode stuff being done there, but Anne suggests its ok if the SW passed credentials when EventSource is not expecting it.

Comment 5

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v2

Thanks for your review. Actually I wanted that to be a feedback request.

(In reply to Ben Kelly [:bkelly] from comment #3)
> Comment on attachment 8634185 [details] [diff] [review]
> v1
> 
> Review of attachment 8634185 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually, before I do the full review can you expand the test to do the
> following:
> 
> 1) Verify that event.request.type is "cors".  It seems that EventSource spec
> calls for it to make a CORS request.
> 2) Verify that a CORS response can be used to intercept.
> 3) Verify that a synthetic response can be used to intercept.
> 
> Also, I think we correctly block the opaque interception due to this check
> for event.request.type === 'no-cors':
> 
>  
> http://mxr.mozilla.org/mozilla-central/source/dom/workers/
> ServiceWorkerEvents.cpp#279
>
> Can you verify that is what we are hitting?

Sure, yes I can. That's what we are hitting.

(In reply to Ben Kelly [:bkelly] from comment #4)
> Here is the spec I was looking at:
> 
>   https://html.spec.whatwg.org/multipage/comms.html#dom-eventsource
> 
> I also thought we should check the credentials CORS mode stuff being done
> there, but Anne suggests its ok if the SW passed credentials when
> EventSource is not expecting it.

Hmm, I do not understand this well, could you elaborate this a bit more please? thanks!

Another round and some feedback would be really appreciate.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8636554 [details] [diff] [review]
v2

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

I think this is a good first start.

Unfortunately, though, I think there is even more to test:

In addition to synthetic, cors, and opaque responses, I think we also need to test mixed-content cors responses.  For this I think you might need to put the sw-client/ iframe on https.  Most of your current cross-origin requests would then be https as well.  Then you can add a new cors Response that is pulled from http.

Finally, you can ignore my comments about credentials.  I was just complaining that it wasn't clearly spec'd, but the result of asking Anne was that we don't need to worry about it.

::: dom/workers/test/serviceworkers/eventsource_cors_response_intercept_worker.js
@@ +22,5 @@
> +  event.waitUntil(
> +    Promise.all([caches.open(name), fetch(request)]).then(function(results) {
> +      var cache = results[0];
> +      var response = results[1];
> +      return cache.put('./sw_clients/eventsource.resource', response);

Can we just do this fetch() in the fetch event handler?  Is there a reason to store it in the Cache?  This works, but seems to add some complexity to the tests.

::: dom/workers/test/serviceworkers/eventsource_opaque_response_intercept_worker.js
@@ +51,5 @@
> +        return fetch(fetchRequest).then((fetchResponse) => {
> +          dump('### ### - onfetch(), is going to respond with a response from the network. URL is ' + fetchResponse.url + '. Response type is ' + fetchResponse.type + '. Response mode is ' + fetchResponse.mode + '\n');
> +          return fetchResponse;
> +        });
> +      }  

nit: trailing white-space

::: dom/workers/test/serviceworkers/eventsource_synthetic_response_intercept_worker.js
@@ +18,5 @@
> +
> +  var headerList = {
> +    'Content-Type': 'text/event-stream',
> +    'Cache-Control': 'no-cache, must-revalidate',
> +    'Access-Control-Allow-Origin': '*'

Is there a particular reason to use these headers for this test?

@@ +23,5 @@
> +  };
> +  var headers = new Headers(headerList);
> +  var init = {
> +    headers: headers,
> +    mode: 'cors'    

nit: trailing white-space

@@ +33,5 @@
> +  var response = new Response(body, init);
> +
> +  event.waitUntil(
> +    caches.open(name).then(function(cache) {
> +      return cache.put('./sw_clients/eventsource.resource', response);

Is there a reason to put this in Cache?  It seems it adds a bit of complexity to the test that isn't necessary.  Why not just create the synthetic response inline in the fetch event?

@@ +60,5 @@
> +        return fetch(fetchRequest).then((fetchResponse) => {
> +          dump('### ### - onfetch(), is going to respond with a response from the network. URL is ' + fetchResponse.url + '. Response type is ' + fetchResponse.type + '. Response mode is ' + fetchResponse.mode + '\n');
> +          return fetchResponse;
> +        });
> +      }  

nit: trailing white-space

::: dom/workers/test/serviceworkers/test_eventsource_intercept.html
@@ +132,5 @@
> +
> +  function runTest() {
> +      Promise.resolve()
> +      .then(() => {
> +        return register("eventsource_opaque_response_intercept_worker.js");

nit: You could incorporate these register() calls in to the test*ResponseIntercept() functions to be a little clearer.

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v3

(In reply to Ben Kelly [:bkelly] from comment #6)
> Comment on attachment 8636554 [details] [diff] [review]
> v2

Thanks for the feedback! Last round I think.

I took me a bit since the whole test needed a refactor to accommodate the mixed content cors case. I tried to keep it as much as simple I could. Hope you like it.
 
> Review of attachment 8636554 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this is a good first start.
> 
> Unfortunately, though, I think there is even more to test:
> 
> In addition to synthetic, cors, and opaque responses, I think we also need
> to test mixed-content cors responses.  For this I think you might need to
> put the sw-client/ iframe on https.  Most of your current cross-origin
> requests would then be https as well.  Then you can add a new cors Response
> that is pulled from http.

I think everything is done.

> Finally, you can ignore my comments about credentials.  I was just
> complaining that it wasn't clearly spec'd, but the result of asking Anne was
> that we don't need to worry about it.

Gotcha!

Please ignore the bunch of dumps I put everywhere. I'll remove then once the work is complete and ready for the review.

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v4

New version, no dump()'s, no printf_stderr(),s. Ben, would you mind to review it? Thanks!

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8638516 [details] [diff] [review]
v4

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

Thanks Jose Antonio!

I'm actually quite surprised we perform the mixed content check in the platform.  Can you try to determine why we are correctly failing the request in that case?  Is it for the mixed content check (good) or because we are accidentally failing some other check (bad)?

Also, various clean up and simplification suggestions.

::: dom/workers/test/serviceworkers/eventsource.resource
@@ +1,1 @@
> +:this file must be enconded in utf8

Are serviceworkers/eventsource.resource and serviceworkers/eventsource.resource^headers^ used any more?  Seems these were copied (or moved?) into the eventsource directory.

::: dom/workers/test/serviceworkers/eventsource/eventsource_cors_response_intercept_worker.js
@@ +1,4 @@
> +// Cross origin request
> +var prefix = 'http://example.com/tests/dom/workers/test/serviceworkers/';
> +
> +function ok(aCondition, aMessage) {

Can you put the ok() function definition in a shared script and then use importScript()?

@@ +17,5 @@
> +self.addEventListener('fetch', function (event) {
> +  var request = event.request;
> +  var url = new URL(request.url);
> +
> +  var fetchRequest = request.clone();

A lot of the tests use clone().  Is there a reason why?  I think you can remove it.

@@ +19,5 @@
> +  var url = new URL(request.url);
> +
> +  var fetchRequest = request.clone();
> +  if (url.pathname === '/tests/dom/workers/test/serviceworkers/eventsource/eventsource.resource') {
> +    ok(request.mode === 'cors', 'EventSource should make a CORS request');

Please verify here and in other tests that require.context === 'eventsource'.

@@ +20,5 @@
> +
> +  var fetchRequest = request.clone();
> +  if (url.pathname === '/tests/dom/workers/test/serviceworkers/eventsource/eventsource.resource') {
> +    ok(request.mode === 'cors', 'EventSource should make a CORS request');
> +    fetchRequest = new Request(prefix + 'eventsource.resource', { mode: 'cors'});

I think its simpler if you just do a event.respondWith() here and nothing if its a different URL.  Seems normal browser processing is fine for other URLs.

::: dom/workers/test/serviceworkers/eventsource/eventsource_mixed_content_cors_response.html
@@ +34,5 @@
> +    };
> +    source.onerror = function(error) {
> +      source.onmessage = null;
> +      source.close();
> +      ok(true, "EventSource should not work with mixed content cors responses");

I'm actually surprised we do this already.  Can you determine why we are failing for this test?  I just want to know if we're accidentally hitting another problem instead of a mixed content check.

::: dom/workers/test/serviceworkers/eventsource/eventsource_opaque_response_intercept_worker.js
@@ +38,5 @@
> +  }
> +
> +  event.respondWith(
> +    caches.open(name).then(function(cache) {
> +      return cache.match(event.request);

Seems the opaque response test is still using Cache.

@@ +49,5 @@
> +        var fetchRequest = request.clone();
> +        return fetch(fetchRequest).then((fetchResponse) => {
> +          return fetchResponse;
> +        });
> +      }  

nit: trailing white space

::: dom/workers/test/serviceworkers/eventsource/eventsource_synthetic_response_intercept_worker.js
@@ +19,5 @@
> +    ok(request.mode === 'cors', 'EventSource should make a CORS request');
> +    var headerList = {
> +      'Content-Type': 'text/event-stream',
> +      'Cache-Control': 'no-cache, must-revalidate',
> +      'Access-Control-Allow-Origin': '*'

Are these headers actually needed for the EventSource to work with the synthetic response?  Is it trying to do CORS checking again after interception or something?

@@ +35,5 @@
> +    event.respondWith(response);
> +
> +  } else {
> +    var fetchRequest = request.clone();
> +    event.respondWith(fetch(fetchRequest).then((fetchResponse) => {

I think you can just remove the entire else clause here.  Not calling respondWith() just lets the browser do its normal default processing.

Comment 10

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v5

Sorry for the late response here. I've been on vacation these three weeks ;)

New version with latest review comments addressed.

(In reply to Ben Kelly [:bkelly] from comment #9)
> Comment on attachment 8638516 [details] [diff] [review]
> v4
> 
> Review of attachment 8638516 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the review!

> :::
> dom/workers/test/serviceworkers/eventsource/
> eventsource_cors_response_intercept_worker.js

> @@ +19,5 @@
> > +  var url = new URL(request.url);
> > +
> > +  var fetchRequest = request.clone();
> > +  if (url.pathname === '/tests/dom/workers/test/serviceworkers/eventsource/eventsource.resource') {
> > +    ok(request.mode === 'cors', 'EventSource should make a CORS request');
> 
> Please verify here and in other tests that require.context === 'eventsource'.

Request.context property is no longer part of the spec. I double-checked this with Anne.

> :::
> dom/workers/test/serviceworkers/eventsource/
> eventsource_mixed_content_cors_response.html
> @@ +34,5 @@
> > +    };
> > +    source.onerror = function(error) {
> > +      source.onmessage = null;
> > +      source.close();
> > +      ok(true, "EventSource should not work with mixed content cors responses");
> 
> I'm actually surprised we do this already.  Can you determine why we are
> failing for this test?  I just want to know if we're accidentally hitting
> another problem instead of a mixed content check.

At first glance I saw the platform does some checks about it around [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#111

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8648718 [details] [diff] [review]
v5

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

Thanks.  Sorry for all the delays with reviewing!

Comment 12

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v6

Ready to land. Carrying over r=bkelly

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

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a0a47e0545e

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0a0a47e0545e