1420526 - Improve SVG-as-image :visited reftests to be effective at testing for :visited styles not applying

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Daniel Holbert [:dholbert]]

+++ This bug was initially created as a clone of Bug #1420001 +++

Per bug 1420001 comment 2, apparently our reftests are ineffective right now, for :visited styles not applying in SVG-as-an-image.

We need to get some automated tests landed/fixed, to be sure that doesn't regress again.  Though we can't land anything until bug 1420001's fix has made it to all affected users and we can open up that bug.

Comment 1

[Daniel Holbert [:dholbert]]

I'm working on this -- taking.

Comment 2

[Daniel Holbert [:dholbert]]

I'm splitting this into two patches:
 Part 1: Update the existing reftests to do the following:
   - use the test_visited_reftests.html mochitest as the "harness"
   - include both an <img> and an <embed> in each testcase, using the same SVG file, with the <embed> *expecting to render the visited styles*.  This lets us ensure that the test doesn't trivially pass in buggy builds by virtue of taking an immediate snapshot which hasn't yet considered visitedness.
   - include an SVG animation inside the SVG images.  This ensures we won't trivially pass the test by pulling a prerasterized snapshot of the SVG from our surface cache (i.e. we'll bypass the imagelib surface cache via this early-return):
 https://dxr.mozilla.org/mozilla-central/rev/709f355a7a8c4ae426d1824841a71ffdb5ce0137/image/VectorImage.cpp#1076


 Part 2: For robustness, make new copies of the existing reftests, using visited-page.html as the "visited" href target, rather than the same page (href=""). visited-page.html is a target that test_visited_reftests.html visits first.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 1: Change svg-as-image :visited reftests to be more robust (and to run as part of test_visited_reftests.html)

See comment 2 for rough overview of the changes here.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 2: Make copies of svg-as-image :visited reftests, using canonical "visited-page.html" as link-target

Comment 5

[Daniel Holbert [:dholbert]]

Note that part 2 is mostly file-copies-with-modification.

(Splinter & bugzilla's diff-viewer are a bit confusing -- they look a lot like it's a modification of an existing file.  But if you like, you can verify that it's a modification to a copy by noticing the "copied from ..." text at the top-left of each file's view in splinter, and also in the text of the patch file itself.)

(I wanted to use mozreview, which represents this more clearly IMO, but mozreview doesn't work with sec bugs, sadly (and this bug is still technically hidden since it's part of bug 1420001 which is still hidden).

Comment 6

[Daniel Holbert [:dholbert]]

Try runs to demonstrate that these tests are effective (tl;dr: everything looks good)

* Try run with these test-patches: tests pass!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=77022928c238f9ba054b0773a641029377efbc90

* Try run with these test-patches, and with bug 1420001's fix reverted: tests fail, as expected!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b3c3c887d78633fd32a95f64487409a3055edb8
(Failures are mostly in M(5) and M(h5) (for "headless mochitest-5"), though the number varies depend on bucket count per platform)

Sample failure log:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b3c3c887d78633fd32a95f64487409a3055edb8&selectedJob=149925064

The reftest snapshots (which require a little hand-editing) show the testcase having two purple boxes (both <img> and <embed> honoring :visited, when only the latter one should), whereas the reference case shows lime/purple (trivially since it's just got two divs).

I also did a Try run with the *gecko* version of this fix reverted (reverting bug 641731, to make us incorrectly honor :visited in the Gecko style system).  This try run should cause failures in this bug's tests, in stylo-disabled mochitest runs, if everything goes as expected. Here's that Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=56e372e51feab81ae62d1b6bbfc4c2839d96a7df

(It hasn't completed yet, but I'm confident it'll show test failures because of some earlier Try results from other runs which I slightly bungled by disabling stylo *globally* for the run, which made unrelated stuff fail.)

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8934585 [details] [diff] [review]
part 1: Change svg-as-image :visited reftests to be more robust (and to run as part of test_visited_reftests.html)

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

::: layout/reftests/svg/as-image/svg-image-visited-1a-helper.svg
@@ +16,5 @@
>  
>    <!-- Note: the <a> element below links to _this document_, so it'll
>         normally be treated as visited. However, in an image context, we want to
>         ignore visitedness. -->
>    <a xlink:href="" id="foo">

Should it use visited-page.html and such? Other reftests in this directory do so, though arguably we could just remove those?

::: layout/reftests/svg/as-image/svg-image-visited-1b-helper.svg
@@ +24,5 @@
> +       imagelib's SurfaceCache optimization.  Specifically, we want to bypass
> +       the SurfaceCache so that we can ensure that repaints of this file (as an
> +       image) will *actually repaint the SVG content*, rather than painting a
> +       previously-rasterized snapshot (which may've been rasterized before we
> +       had any chance to consider :visited styles). -->

Huh, smart :)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8934586 [details] [diff] [review]
part 2: Make copies of svg-as-image :visited reftests, using canonical "visited-page.html" as link-target

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

I guess I should've read the second patch before...

This looks great, thanks Daniel! :)

Comment 9

[Daniel Holbert [:dholbert]]

Thanks for the review! FWIW, I pinged dveditz over on bug 1420001 to find out when I'm OK to land these tests (since typically we hold sec-bug tests until we're ready to open the bug, at least for bugs that have made it to release).

Comment 10

[Daniel Holbert [:dholbert]]

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af8a2386459610f21160d16932f1a99d11e9e7c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/791193343db3b10953fa0026cb76c28f0affba84

Comment 11

[Daniel Holbert [:dholbert]]

--> setting ni=me to uplift the tests (with a=testonly) after they've gotten a day or so to prove themselves as non-flaky.  This'll provide extra validation to be sure that things are working correctly in those releases.

(For now I'm just intending to uplift at least as far as beta, though really we could take these on all supported branches including release & esr and they should be good there too, as long as I'm allowed to uplift stuff to those branches with a=testonly)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af8a23864596
https://hg.mozilla.org/mozilla-central/rev/791193343db3

I don't think we need to worry about ESR52, but uplifting to m-r also would be a good idea.

Comment 13

[Daniel Holbert [:dholbert]]

I verified locally that the test_visited_reftests.html mochitest still passes with these patches applied on beta/release, and then I uplifted to those branches.

Beta:
 https://hg.mozilla.org/releases/mozilla-beta/rev/f31109c74b9adf0ac433c11fb9d6457eec56d076
 https://hg.mozilla.org/releases/mozilla-beta/rev/d73769804d8b2872ac11cfe2c7fd59b3fbfe21cd

Release:
 https://hg.mozilla.org/releases/mozilla-release/rev/55e79179b9c4701f1470e9387aaee34941501185
 https://hg.mozilla.org/releases/mozilla-release/rev/52e4b211f350b26dfde449b91f5360dadf7e29bb

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> I don't think we need to worry about ESR52, but uplifting to m-r also would
> be a good idea.

Sounds good. I tried esr52 just in case it was easy, but the patch doesn't apply cleanly there (in part because of some stylistic/convenience cleanup that happened in these tests as part of bug 1331081, in the time since current ESR52 diverged from central).