1182120 - Test XMLDocument.load() with fetch interception

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Comment 1

[Albert [:albert]]

Attachment: Test

mlDocument.load has same-origin restriction, test checks if sw is able to load xml from another domain.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

It would be nice if our tests also checked things like:

  - event.request.type is the expected type.  In theory this should be 'same-origin' here, right?  I doubt we set that properly, though.
  - CORS and synthetic responses.  I think our current thought is to allow these for same-origin interceptions.

Comment 3

[Albert [:albert]]

Attachment: Test

Check request.mode

Comment 4

[Albert [:albert]]

Test not green because request.mode is 'no-cors' instead of 'same-origin'.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I will try to review this tomorrow.  Sorry for the delay.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8643923 [details] [diff] [review]
Test

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

Sorry it took me so long to look at this.  It was much shorter than I remembered.

I see you are now checking for the synthetic response as I requested, but unfortunately you removed the opaque response test case.

Can you please do three different cases?  One for synthetic, one for opaque, and one for a CORS response?  You can use different\ request URLs to separate out the desired response type in the service worker.

Also, just go ahead and remove the check for RequestMode right now.  Instead, put a TODO comment to check request mode for same-origin once bug 1189945 lands.

Comment 7

[Albert [:albert]]

Attachment: Test

Changes from comment 6

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8646173 [details] [diff] [review]
Test

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

Looks good.  Thanks!

Does the opaque interception test pass right now given that the RequestMode is set wrong?

Comment 9

[Albert [:albert]]

(In reply to Ben Kelly [:bkelly] from comment #8)
> Comment on attachment 8646173 [details] [diff] [review]
> Test
> 
> Review of attachment 8646173 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.  Thanks!
> 
> Does the opaque interception test pass right now given that the RequestMode
> is set wrong?

As talked in IRC I created a patch to fix the Request.mode until 1189945 lands.

Comment 10

[Albert [:albert]]

Attachment: Fix Request.mode

Comment 11

[Albert [:albert]]

Attachment: Test

Comment 12

[Albert [:albert]]

Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ac17f4d47e4

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8646487 [details] [diff] [review]
Fix Request.mode

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

Looks good.  Thanks!  If you could mention XMLDocument.load() in the commit message that would be great.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8646489 [details] [diff] [review]
Test

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

r=me with comment addressed.  Thanks!

::: dom/workers/test/serviceworkers/fetch_event_worker.js
@@ +220,5 @@
> +  else if (ev.request.url.includes('load_cross_origin_xml_document_synthetic.xml')) {
> +    if (ev.request.mode != 'same-origin') {
> +      ev.respondWith(Promise.reject());
> +      return;
> +    }

Please perform this check in the other two tests as well.

Comment 15

[Albert [:albert]]

Attachment: Fix Request.mode

Commit message updated

Comment 16

[Albert [:albert]]

Attachment: Test

Changes from comment 14

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08385a5ad3d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4aed3ba5bb41

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/08385a5ad3d5
https://hg.mozilla.org/mozilla-central/rev/4aed3ba5bb41