1762387 - History removals are not reflected in link status

Status: NEW

(Toolkit :: Places, defect, P3)

Description

[Takanori MATSUURA]

Steps to reproduce:

1. Visit Bugzilla home (https:/bugzilla.mozilla.org/home)
2. Click "Bugzilla Etiquette" link (https://bugzilla.mozilla.org/page.cgi?id=etiquette.html) to visit the page
3. Back to Bugzilla home and make sure the color of "Bugzilla Etiquette" is changed to a:visited
4. Delete "Bugzilla Etiquette" from history
5. Reload Bugzilla home and check the color of "Bugzilla Etiquette"

Actual results:
The color of "Bugzilla Etiquette" is a:visited.

Expected results:
The color of "Bugzilla Etiquette" is a:link.

Regression range by mozregression:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=66531ac861bd5bd12e3c50f058fcc536e4f7c374&tochange=ad9c69e7ecf4d55e9bc649e09b7e4f1cc077c21c

Possibly regressed by bug 1760674.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Yeah, definitely bug 1760674. Somewhat hard to fix without regressing it tbh. We'd need to proactively globally notify of the unvisitedness like we do of the visitedness...

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Moving to the component of the regression bug.

Comment 5

[Daniel Holbert [:dholbert]]

FWIW, Ctrl+Shift+R (a "hard reload") doesn't work around this.

However, if I open the same URL in a new tab, then the link is styled as a regular un-visited link there.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Marco, do you know where in the history code do we remove stuff from the places database at runtime? We could NotifyVisited(VisitedState::Unvisited) or so from there.

Comment 7

[Daniel Holbert [:dholbert]]

(Side note, the link in comment 0's STR "step 1" is broken, due to what looks like a bug in bugzilla itself. I've filed bug 1764132 on that. As a workaround, you can just visit https://bugzilla.mozilla.org/ for that step [which is a URL that bugzilla does properly linkify, and which redirects to the same page mentioned in comment 0 here anyway.)

Comment 8

[Beth Rennie [:beth] (she/her)]

:emilio can you set a severity on this? Is this something okay to leave unfixed in 100?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Yes, I think this is relatively low-severity. This never worked "properly" (needed a refresh before my patch, now needs all links pointing to the URI to go away). The fix here would be in places, btw, so moving there. But happy to take this once comment 6 is answered.

Comment 10

[Marco Castelluccio [:marco]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

Yes, I think this is relatively low-severity. This never worked "properly" (needed a refresh before my patch, now needs all links pointing to the URI to go away). The fix here would be in places, btw, so moving there. But happy to take this once comment 6 is answered.

Doesn't this also have bad privacy implications? A user might think they have removed something from history, but then other users with access to the machine could figure out what pages the user has visited.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Well, iff the page was open and contained a link to the page the user had visited-and-then-removed. Which was always true pre-patch, the only thing my patch changes is whether after a refresh it keeps being displayed as visited.

Comment 12

[Marco Castelluccio [:marco]]

It also keeps showing the link as visited if you close the page and reopen it later. It doesn't show the page as visited if you close and reopen Firefox, which makes the problem less severe. Also, if you have the same link on a different page, it isn't shown as visited. (the last two things were my main concerns, but it seems they do not actually apply so we are good).

Comment 13

[Takanori MATSUURA]

5. Reload Bugzilla home

is important.

If not reloaded, the link status will not be updated before and after the patch.
If reloaded, the link status will be updated before the patch and not after the patch.

From experience, I know that Firefox behaves that way, but I'm not sure whether the pre-patch behavior is expected or not.

Comment 14

[Marco Bonardo [:mak] (away 22-26 Sept)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

Marco, do you know where in the history code do we remove stuff from the places database at runtime? We could NotifyVisited(VisitedState::Unvisited) or so from there.

History can be removed from many parts of Firefox, it may not just be the user, also automatic expiration or other background operations may remove an history entry. The best code path would be to observe the page-removed Places observer notification (and likely also history-cleared), though that comes with a cost as any other thing, so it's matter of figuring out if the problem is important enough. The cost is not extreme, especially if we have the list of links in memory, though it's also not free.

Are other browsers handling this problem?

Comment 15

[Mark Banner (:standard8)]

NI to James to try reproducing this on different browsers and seeing what the results are.

Comment 17

[Alice0775 White]

Bug 1760674 has been landed in ESR91, ESR91 is also affected.
https://treeherder.mozilla.org/jobs?repo=mozilla-esr91&searchStr=windows%2C2012%2Cx64%2Copt&revision=d18fef36c04a4b3069567dcc34ca3764d519a2cb

Comment 18

[James Teow [:jteow]]

On different browsers, here was my STR:

• Visit https://developer.mozilla.org/en-US/docs/Web/CSS/:visited
• Click on links within Pages that you might have visited:
• Observe colour change
• Clear recent history
• Check to see if the link state changed back to its original colour without reloading the page

Results:
In OSX, I checked Chrome 103, Safari 15.5, and on Windows I checked Chrome 103 and Edge 103. All those browsers reset the links back to the original colour when the history was cleared.

Comment 19

[Marco Bonardo [:mak] (away 22-26 Sept)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

Marco, do you know where in the history code do we remove stuff from the places database at runtime? We could NotifyVisited(VisitedState::Unvisited) or so from there.

The problem there may be with large removals. While handling history-cleared may be efficient (Every link should be marked as unvisited), if the user removes 100 or 200 urls, we'd end up with hundreds of IPC calls. We'd have to either batch removals into a single ipc (though then the list of urls may be quite large), or just send a message that "some" url has been removed, and requery all the urls at the next refresh. Or something in the middle (If there's a few urls removed do them one by one, otherwise refresh all).

Comment 20

[Kestrel]

(In reply to Marco Castelluccio [:marco] from comment #12)

It doesn't show the page as visited if you close and reopen Firefox

A workaround that does not require closing the entire browser is to close running content processes in the Firefox Task Manager ( about:processes).