Closed Bug 1420526 Opened 3 years ago Closed 3 years ago

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


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

57 Branch



Tracking Status
firefox-esr52 --- wontfix
firefox57 --- fixed
firefox58 --- fixed
firefox59 --- fixed


(Reporter: dholbert, Assigned: dholbert)



(Keywords: regression, sec-other)


(2 files)

+++ 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.
Summary: History leakage through anchors in SVG in canvas tags → Improve SVG-as-image :visited reftests to be effective at testing for :visited styles not applying
Priority: -- → P3
Keywords: sec-other
I'm working on this -- taking.
Assignee: nobody → 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):

 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.
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).
Try runs to demonstrate that these tests are effective (tl;dr: everything looks good)

* Try run with these test-patches: tests pass!

* Try run with these test-patches, and with bug 1420001's fix reverted: tests fail, as expected!
(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:

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:

(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 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 :)
Attachment #8934585 - Flags: review?(emilio) → review+
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! :)
Attachment #8934586 - Flags: review?(emilio) → review+
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).
--> 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)
Flags: needinfo?(dholbert)

I don't think we need to worry about ESR52, but uplifting to m-r also would be a good idea.
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Group: layout-core-security → core-security-release
(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).
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.