661590 - Repeated calls to location.hash = "#foo" cause large amounts of disk activity

Status: NEW

(Toolkit :: Places, defect, P3)

Description

[Scott Baker]

Attachment: Test Case

I had a application where I was calling location.hash in JavaScript repeatedly. Looking at some system monitoring at the time I see significant increases in disk activity at the time. Since we're not reloading the page, there should be no reason to hit the disk at all?

I've tested in Opera and Chrome an didn't see any increased disk activity. I've attached a test case that makes this easily repeatable.

Comment 1

[Boris Zbarsky [:bzbarsky]]

There seems to be a bunch of fcntl() and write() and read() traffic on Mac when I click the buttons on that testcase... all of it under  mozilla::places::(anonymous namespace)::InsertVisitedURIs::Run to a first approximation.

And that happens because nsDocShell::OnNewURI (which is fired for ref changes, even if the ref did not actually change) calls nsDocShell::AddURIVisit which calls mozilla::places::History::VisitURI.

Comment 2

[Justin Lebar (not reading bugmail)]

Are we OK if repeated calls to |location.hash = random()| causes large amounts of disk activity?

Comment 3

[Boris Zbarsky [:bzbarsky]]

As in, if we optimize out OnNewURI when the ref did not change?

"Maybe".  I wonder what values Scott's application actually set location.hash to....

Comment 4

[Justin Lebar (not reading bugmail)]

I guess I mean: The problem here seems to be that a page can easily cause us to thrash the disk.  So do you think it's worth trying not to hit disk every time location.hash is set, regardless of whether it changes?

Not hitting the disk only in the case when location.hash doesn't change doesn't seem to address the larger issue...

Comment 5

[Boris Zbarsky [:bzbarsky]]

> So do you think it's worth trying not to hit disk every time location.hash is
> set

Yes.

> Not hitting the disk only in the case when location.hash doesn't change doesn't > seem to address the larger issue...

Indeed.

Comment 6

[Scott Baker]

My application had the user using up/down to navigate a table of data. When a row would get highlighted I called location.hash = $appropriate_hashtag to make sure the focus of the viewport was what the user was navigating to. Probably not the ideal use of this, but I certainly wasn't expecting it to thrash the disk.

The way I noticed it was that navigation was MUCH slower on Firefox than it was on Chrome/Opera. Some digging showed the increased disk usage.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So, I'm looking into bug 1478417, that is an actual case of this happening on what we could name a popular website.

We keep adding visits through onNewURI even if the ref didn't change, and we also call UpdateGlobalHistoryTitle even if the title didn't change. These 2 optimizations look harmless in general.

Clearly, it wouldn't address the case of the page continuously setting a different hash, but that's also less common, and for that we probably just need an anti flooding protection in history.
Note, we currently already have a flooding protection in Places, but it relies on the aPreviousURI value passed to visitURI by the docshell. It looks like in this case there's no aPreviousURI, because we pass a null channel (shortcircuitload) and ExtractLastVisit returns early in that case. Though, this is the only case where there isn't a channel, and I wonder if we could just make ExtractLastVisit default to aPreviousURI = aURI when no channel is provided, that would trigger the history flooding protection (can store 1 visit to the same url every 6 minutes). Or we'll have to pass some new flag to onNewURI.

We could also make VisitURI return a flag stating whether the addition was skipped due to flooding, and use that flag to skip the next UpdateGlobalHistoryTitle call. We currently don't use the VisitURI return value.

To sum up:
1. the docshell should probably not try to add to history if the hash is the same, nor store a title if it is the same
2. the docshell should somehow pass aPreviousURI to history (or implement an anti flooding system itself)
3. history should ignore fragments in its anti flooding protection. Note this means both android History and Places have to implement their own protection, while if this would be handled by the docshell, it would be centralized

Thoughts?
Who is taking care of the docshell in modern days?

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

One problem, is that if previousURI and URI are the same, History assumes it's a reload and give the visit a TRANSITION_RELOAD. That wouldn't be ok for this case. Maybe we should just pass an additional flag to VisitURI and have a TRANSITION_HASH or make it TRANSITION_EMBED (stored in memory, not on disk)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Docshell shouldn't have anti-flooding stuff, to API users should have, IMO.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I think I agree with Olli.  We can skip UpdateGlobalHistoryTitle and pass false for the aAddToGlobalHistory arg of OnNewURI if the hash did not change.  But past that, I don't think docshell has the context to do anti-flooding stuff.  That said, if we can somehow pass more context to Places code we should certainly do that.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Thank you, then I'd say:
1. the docshell should not try to add to history if the hash is the same, nor store a title if it is the same
2. the docshell should pass a new flag to History for shortcircuitload
3. history should ignore fragments and use the new flags in its anti flooding protection

Comment 13

[Ajedi32]

FYI, I just encountered a site exploiting this (or perhaps a similar bug, I couldn't really tell because it froze the browser so I couldn't open Dev Tools to check) in the wild for phishing: https://i.imgur.com/jPjvCAV.png

It took me to that screen, then froze the browser so I couldn't close the site without using task manager.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

(not startup related: fxperf:p2 -> fxperf:p3)

Comment 16

[Francis Diller]

https://abudhabi.peugeot.com

Comment 17

[:Gijs (he/him)]

Paul, isn't this covered by bug 1314912 now?

Comment 18

[Paul Zühlcke [:pbz]]

I think so!
It goes through SetURI so it should be covered here: https://searchfox.org/mozilla-central/rev/169bf38e150667afac81ab73ef8b5ace8f1dfa8d/dom/base/LocationBase.cpp#125
SetURI is called from the setter here: https://searchfox.org/mozilla-central/source/dom/base/Location.cpp#152,175

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Is the rate limit no more than 200 calls in 10s? That would still be high for Global History and to reduce I/O, thus we may still want to add additional limits to global history.

Comment 20

[James Teow [:jteow]]

(In reply to Marco Bonardo [:mak] from Bug 1866978 comment #0)

The site at https://agregen.gitlab.io/cyoa-viewer/v2.html# does excessive use of history. APIs (from the minified source it looks like history.replaceState or history.pushState) on mousemove events.
We should check what can be done to reduce I/O in this case.

(In reply to Marco Bonardo [:mak] from Bug 1866978 comment #1)
Bug 1314912 has introduced a rate limit, that this site is indeed hitting, but that's not sufficient to avoid history poisoning.