Closed Bug 1477773 Opened 3 years ago Closed 3 years ago

layout.css.visited_links_enabled doesn't seem to completely disable :visited


(Core :: CSS Parsing and Computation, defect)

60 Branch
Not set



Tracking Status
firefox-esr60 64+ fixed
firefox63 --- fixed


(Reporter: mismith, Assigned: emilio)



(Whiteboard: [pixel-stealing])


(3 files)

In theory, turning off layout.css.visited_links_enabled should mitigate bug 1455898 - but it doesn't. Some sort of :visited-related processing still seems to be happening even with this pref off. Maybe something was broken in the switch to Stylo?
To clarify: turning off layout.css.visited_links_enabled does make all links *appear* unvisited, visually.
Component: Graphics → CSS Parsing and Computation
Flags: needinfo?(emilio)
Assignee: nobody → emilio
Flags: needinfo?(emilio)
We force a repaint from ContentStateChangedInternal if visited links are
disabled, and that's observable. Let's cut it off as early as we can to avoid
timing attacks even when :visited is disabled.
We match with AllLinksVisitedAndUnvisited for style invalidation, so all this
stuff is just not needed. The comment points to the attribute tests in
bug 1328509, but those still trivially pass with this change.

I think this was unneeded since I introduced AllLinksVisitedAndUnvisited, or
maybe since In any case it doesn't
really matter, and I already had done this cleanup in my WIP patches for
bug 1406622, but I guess this is a slightly more suitable place to land them :)
I don't think I have access to the test-case, but could you try a build from:

And confirm it does what you expect? Thanks!
Flags: needinfo?(lists)
Nvm, I do have access, and I can confirm that a build with these patches makes the test-case fail.
Flags: needinfo?(lists)
Comment on attachment 8999863 [details]
Bug 1477773: no-op visited changes earlier if visited links are disabled.

Xidorn Quan [:xidorn] UTC+10 has approved the revision.
Attachment #8999863 - Flags: review+
Comment on attachment 8999864 [details]
Bug 1477773: Simplify visited-related code in invalidation.

Xidorn Quan [:xidorn] UTC+10 has approved the revision.
Attachment #8999864 - Flags: review+
Pushed by
no-op visited changes earlier if visited links are disabled. r=xidorn
Pushed by
Simplify visited-related code in invalidation. r=xidorn
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1488155
Emilio, I presume this also affects esr60?  Would you be willing to rebase these there to bring them down for Tor?
Flags: needinfo?(emilio)
Attached patch Rebase for ESR.Splinter Review
Yeah, it does. Here's a patch with the first commit (second was just cleanup so didn't bother rebasing it), and the fix for bug 1488155 as well.
Flags: needinfo?(emilio)
Comment on attachment 9022267 [details] [diff] [review]
Rebase for ESR.

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Tor Browser would like to take these patches to mitigate history stealing attacks in the configuration where Tor Browser saves browsing history.

User impact if declined: Tor would have to apply it manually.

Fix Landed on Version: 63.0a1 / 20180816100035

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): I put it at Medium because while it's been tested for months, it's a medium-sized patch.

String or UUID changes made by this patch: None
Attachment #9022267 - Flags: approval-mozilla-esr60?
Comment on attachment 9022267 [details] [diff] [review]
Rebase for ESR.

Makes sense to keep ESR60.0.4 in line with 64 release to prevent this type of attack and protect user privacy.
Attachment #9022267 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Depends on: 1485655
You need to log in before you can comment on or make changes to this bug.