Test XMLDocument.load() with fetch interception

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: noemi, Assigned: albert)

Tracking

(Depends on 1 bug)

Trunk
FxOS-S5 (21Aug)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected, firefox43 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Reporter

Description

4 years ago
No description provided.
Reporter

Updated

4 years ago
Target Milestone: --- → FxOS-S3 (24Jul)
Assignee

Comment 1

4 years ago
Posted patch Test (obsolete) — Splinter Review
mlDocument.load has same-origin restriction, test checks if sw is able to load xml from another domain.
Attachment #8633344 - Flags: review?(ehsan)

Updated

4 years ago
Attachment #8633344 - Flags: review?(ehsan) → review+
Assignee

Updated

4 years ago
Depends on: 1184855
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.
Reporter

Updated

4 years ago
Target Milestone: FxOS-S3 (24Jul) → FxOS-S4 (07Aug)
Assignee

Comment 3

4 years ago
Posted patch Test (obsolete) — Splinter Review
Check request.mode
Attachment #8633344 - Attachment is obsolete: true
Attachment #8643923 - Flags: review?(bkelly)
Assignee

Comment 4

4 years ago
Test not green because request.mode is 'no-cors' instead of 'same-origin'.
Depends on: 1189945
I will try to review this tomorrow.  Sorry for the delay.
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.
Attachment #8643923 - Flags: review?(bkelly) → review-
Reporter

Updated

4 years ago
Target Milestone: FxOS-S4 (07Aug) → FxOS-S5 (21Aug)
Assignee

Comment 7

4 years ago
Posted patch Test (obsolete) — Splinter Review
Changes from comment 6
Attachment #8643923 - Attachment is obsolete: true
Attachment #8646173 - Flags: review?(bkelly)
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?
Attachment #8646173 - Flags: review?(bkelly) → review+
Assignee

Comment 9

4 years ago
(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.
Assignee

Comment 10

4 years ago
Posted patch Fix Request.mode (obsolete) — Splinter Review
Attachment #8646487 - Flags: review?(bkelly)
Assignee

Comment 11

4 years ago
Posted patch Test (obsolete) — Splinter Review
Attachment #8646173 - Attachment is obsolete: true
Attachment #8646489 - Flags: review?(bkelly)
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.
Attachment #8646487 - Flags: review?(bkelly) → review+
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.
Attachment #8646489 - Flags: review?(bkelly) → review+
Assignee

Comment 15

4 years ago
Commit message updated
Attachment #8646487 - Attachment is obsolete: true
Attachment #8646752 - Flags: review+
Assignee

Comment 16

4 years ago
Posted patch TestSplinter Review
Changes from comment 14
Attachment #8646489 - Attachment is obsolete: true
Attachment #8646753 - Flags: review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/08385a5ad3d5
https://hg.mozilla.org/mozilla-central/rev/4aed3ba5bb41
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.