1318526 - make reftests that use HTTP(..), etc., because of file: origin policy stop doing so

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Bug 846890 disabled the strict file origin policy for reftests, to make them faster.

This means we can stop using HTTP(..) (or HTTP(../..), etc.) on reftests where we were using it only because we needed to be same-origin with something in a different directory (most commonly, downloadable fonts).

Given that Xidorn says this would make things faster (bug 1258916 comment 79), we should probably do this.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

It seems there are 466 usage of "HTTP(.." in tree. Majority of them seem to be just for loading fonts from reftest/fonts directory. Removing those should make reftest run much faster.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

https://dxr.mozilla.org/mozilla-central/search?q=HTTP(..+path%3Areftest.list+-path%3Areceived&redirect=false

Comment 3

[Ryan VanderMeulen [:RyanVM]]

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

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Try push is green. I have concerns about how to detect any instances of both the test and ref being broken (and therefore ending up with identical screenshots). Any thoughts on that, dbaron?

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Attachment: remove unneeded instances of HTTP(..) from reftest manifests

Also, are there situations where removing plain HTTP annotations might also be appropriate?

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #4)
> Try push is green. I have concerns about how to detect any instances of both
> the test and ref being broken (and therefore ending up with identical
> screenshots). Any thoughts on that, dbaron?

The straightforward way is this:
 * generate a patch that touches every line you're changing, but also swaps != and == on those lines
 * generate a second patch that does the same but combines it with your patch (removal of HTTP(..))
 * push both patches to try
 * download the resulting logs, grep them for "REFTEST" and trim the timestamp-related gunk from the start of the lines
 * diff the logs, and see if you see any differences in the images

(Might need to do the run on more than one platform if there are platform-specific tests.)

This is probably substantially less human time than hacking the reftest harness to compare HTTP and non-HTTP.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> Also, are there situations where removing plain HTTP annotations might also
> be appropriate?

I suppose there could be some erroneous or no-longer-needed annotations for other reasons.  But it seems unlikely to yield anywhere near as large a number as the ones that were HTTP(..) for the same-origin policy for downloadable fonts.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Finally had a chance to rebase this patch and run the Try pushes recommended in the last comment. We'll see what turns up.
https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&group_state=expanded&fromchange=73f6d4a240607589c935a7731ec51c74a61056ac&tochange=a315b8e1c82471cbdb26aa4dd5c0c881f92095e5

Comment 8

[Jonathan Kew [:jfkthame]]

Ryan, are you hoping to get back to this any time soon? I was recently motivated to try and get rid of those HTTP(..)s, and started on a patch before realizing you'd already begun to look into it.... here's my try push, fwiw:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a4726cfc2840be473a591c5af449d36dd587492

It'd be great if we could get this finished...

Comment 9

[Ryan VanderMeulen [:RyanVM]]

All yours. I haven't had time to do the verification dbaron suggested, so it's stalled out (and I'm sure is horribly bitrotted at this point).

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: Remove HTTP(..) from reftest manifests where this was only needed to work around file-URI origin restrictions

Here's a patch to remove almost all these annotations, leaving just a handful where the use of HTTP(..) is actually relevant -- e.g. to test a font load via a URL with HTTP redirect, or where we're using an .sjs server to serve the file in a specific way (e.g. very slowly). I did a check as outlined in comment 6, and the failure images (resulting from swapping == and != in the manifest) looked exactly the same before and after the HTTP(..) removals.

Comment 11

[Jonathan Kew [:jfkthame]]

FTR, here are try runs I used:

(1) To collect failure images when swapping ==/!= on the affected lines:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84640aadf4f7b8d54156b096f4d2d5778dbb3949

(2) Base run with ==/!= swapped but HTTP(..) left in place:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=32d9d5a13e3954801f52127c3fc7624a963a0405

(3) To confirm reftests run green after this patch is applied:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a4726cfc2840be473a591c5af449d36dd587492

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8920781 [details] [diff] [review]
Remove HTTP(..) from reftest manifests where this was only needed to work around file-URI origin restrictions

Please put the comment that was in the reftest.list, saying that 621918-2.svg is using filters.svg in order to mask antialiasing issues where glyphs touch, into the test itself.

r=dbaron with that

Are there also some HTTP(../..) to clean up in a followup patch?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f1b4cb86b58
Remove HTTP(..) from reftest manifests where this was only needed to work around file-URI origin restrictions. r=dbaron

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #12)
> Comment on attachment 8920781 [details] [diff] [review]
> Remove HTTP(..) from reftest manifests where this was only needed to work
> around file-URI origin restrictions
> 
> Please put the comment that was in the reftest.list, saying that
> 621918-2.svg is using filters.svg in order to mask antialiasing issues where
> glyphs touch, into the test itself.
> 
> r=dbaron with that
> 
> Are there also some HTTP(../..) to clean up in a followup patch?

Yes, I believe so; I'll try to post a patch for that shortly, but wanted to get the bulk of this landed as quickly as possible while the tree is fairly quiet, as it seemed likely to bit-rot pretty fast.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - Also remove unnecessary HTTP(../..) from reftest manifests now that tests run with relaxed file-URI origin restrictions

Here's a followup to remove the HTTP(../..) occurrences. I've started a try run, to check that it runs without surprises... https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e9a821637b1a6d31a9773c3b88744d9987057f6.

Comment 16

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Did you run the same checks that the images didn't change?

Comment 17

[Jonathan Kew [:jfkthame]]

I haven't actually done that; I just looked at the tests and was comfortable that the HTTP(../..) usage was just about loading fonts, and not relevant to anything else. Would you strongly prefer that we do the additional image-comparison as well before landing this?

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1f1b4cb86b58

Comment 19

[Jonathan Kew [:jfkthame]]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #16)
> Did you run the same checks that the images didn't change?

OK, I ran additional try jobs with the comparisons on these lines flipped (== <-> !=), with and without the HTTP(../..) removed:

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

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

As expected, the failure images look the same in each case.

Comment 20

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8920854 [details] [diff] [review]
pt 2 - Also remove unnecessary HTTP(../..) from reftest manifests now that tests run with relaxed file-URI origin restrictions

r=dbaron; thanks for cleaning this up and for doing the extra test run

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98c5873f6cf0
pt 2 - Also remove unnecessary HTTP(../..) from reftest manifests now that tests run with relaxed file-URI origin restrictions. r=dbaron

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/98c5873f6cf0