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

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P2
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

(Blocks 1 bug, {perf})

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [fxperf:p3])

Attachments

(1 attachment)

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).
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.
Status: NEW → ASSIGNED
Priority: -- → P2
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
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.
Whiteboard: [fxperf] → [fxperf:p3]
Boris: did you see the patch update here fixing your review comments?
Flags: needinfo?(bzbarsky)
I did.  I was on unexpected PTO for a while and am now working through my backlog... ETA sometime today.
Flags: needinfo?(bzbarsky)
No problem. Thank you.
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.
Attachment #9007785 - Flags: review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3266c319db3b
In docShell, don't set the same title a second time. r=bzbarsky
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.
Flags: needinfo?(standard8)
https://hg.mozilla.org/mozilla-central/rev/3266c319db3b
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
(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.
Flags: needinfo?(standard8)
You need to log in before you can comment on or make changes to this bug.