1489503 - nsDocShell::SetTitle shouldn't attempt to set the same title a second time

Status: RESOLVED FIXED

(Core :: DOM: Navigation, enhancement, P2)

Description

[Mark Banner (:standard8)]

Currently if nsDocShell::SetTitle is called twice in a row with the same title, it will attempt to re-set the title on the parent, in history and in the session history.

This causes unnecessary disk i/o, and maybe some other perf impact.

I can see this by adding a little bit of debug, and then browsing around sites, e.g.

- If I navigate somewhere, and then reload a page, the title will be set a second time.
- If I load https://www.reddit.com/, then the site seems to set the title a couple of times.
- Also navigating around reddit will hit this (e.g. load a sub-group, load the "all" page).

Comment 1

[Mark Banner (:standard8)]

I've a try push in progress for this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=806a5351278408092a69c3222d2269f6eb7a566d

My only minor concern is that the patch as-is currently will also ignore an initial setTitle where the title is the empty string. I think that is probably OK, but hopefully the test run will pull out any issues.

Comment 2

[Mark Banner (:standard8)]

So my basic change didn't quite work. It appears we need to check that both the URI and the title haven't changed (rather than just the title).

This is to protect against e.org -> e.com transitions where the pages have the same title, this was caught by browser_e10s_switchbrowser.js.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6a7f62baa6ef5f0ba5d8fecd4a32648f2be776a

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1489503 - In docShell, don't set the same title a second time.

This avoids setting the title a second time when the title and uri remain the same across setTitle calls. This means we can avoid unnecessary history updates which currently result in extra disk i/o.

Comment 4

[Mark Banner (:standard8)]

New try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06012778ac925590d0b7db83e4ca8c9446f9e404

Comment 5

[Mark Banner (:standard8)]

Boris: did you see the patch update here fixing your review comments?

Comment 6

[Boris Zbarsky [:bzbarsky]]

I did.  I was on unexpected PTO for a while and am now working through my backlog... ETA sometime today.

Comment 7

[Mark Banner (:standard8)]

No problem. Thank you.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9007785 [details]
Bug 1489503 - In docShell, don't set the same title a second time.

Boris Zbarsky [:bzbarsky, bz on IRC] has approved the revision.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3266c319db3b
In docShell, don't set the same title a second time. r=bzbarsky

Comment 10

[Marco Bonardo [:mak]]

Would it be possible, in a separate bug, to add a test for this?

I think it should be enough to make a mochitest-browser that, after the first visit and title were notified, sets location.hash  and checks we don't get a history notification for a title change.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3266c319db3b

Comment 12

[Mark Banner (:standard8)]

(In reply to Marco Bonardo [::mak] from comment #10)
> Would it be possible, in a separate bug, to add a test for this?
> 
> I think it should be enough to make a mochitest-browser that, after the
> first visit and title were notified, sets location.hash  and checks we don't
> get a history notification for a title change.

I debated that with myself previously, and I wasn't quite sure as it would mean that we were attempting to check for the existence of something that happens asynchronously. Replacing the history service seemed pretty hairy, and possibly only doable from c++ tests.

The other issue here is that the history code didn't used to send out two updates - it checks for the current title on the page before doing the write:

https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/toolkit/components/places/History.cpp#1409-1417

So we're saving ourselves some internal updates and a database read, but I don't see a way to test for that.