Generates a dbus roundtrip with every HTTP request

[Chris Coulson]

Firefox 11 has the ability to load proxy settings from gconf. However, it generates a dbus roundtrip to dconf with every HTTP request, because we call g_settings_new each time. I've submitted a patch upstream for this, and this bug is just to remind me that we need to backport it to the beta so that we don't ship the release like this

[Chris Coulson (chrisccoulson)]

I've noticed for the last few days that whilst running Thunderbird Daily, dbus-daemon will use a lot of CPU for a few minutes every now and again and power consumption goes through the roof. When I look at traffic on the session bus, I see thousands of these messages from Thunderbird:

method call sender=:1.527 -> dest=org.freedesktop.DBus serial=34074 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=AddMatch
   string "type='signal',interface='ca.desrt.dconf.Writer',path='/ca/desrt/dconf/Writer/user',arg0path='/system/proxy/'"

What is happening is that each time we look for the system proxy settings, we are calling g_settings_new(). When doing this, the dconf backend sets up a watch on these settings to be notified of change events.

Of course, there might actually be a Thunderbird bug here, but we can probably improve this situation in nsUnixSystemProxySettings by not creating a new GSettings instance (and not setting up a new watch) each time we look up the proxy settings.

[Chris Coulson (chrisccoulson)]

Created attachment 586912
Don't call g_settings_new each time we look up system proxy settings

[Karlt (karlt)]

Comment on attachment 586912
Don't call g_settings_new each time we look up system proxy settings

Thanks for this. I was surprised to see how often GetProxyForURI is called,
and it is disturbing that this causes so much CPU use.
The approach in this patch makes sense.

Would mProxySettings be a better name than mGSettingsKeys?

Those who have "mode" set to "manual" are still going to see the same systems,
but fixing that is more involved, and that is a smaller portion of users. If you fix that also, then it might be a good idea to make nsGSettingsCollection hold a strong reference to mSettings, so that nsUnixSystemProxySettings doesn't need to keep its reference to mGettings.

[Karlt (karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #2)
> it might be a good idea to make
> nsGSettingsCollection hold a strong reference to mSettings, so that
> nsUnixSystemProxySettings doesn't need to keep its reference to mGettings.

I meant the nsGSettingsService not mSettings, but keeping a reference to that would be unnecessary anyway I assume.

[Karlt (karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #2)
> Those who have "mode" set to "manual" are still going to see the same
> systems,

I should just go to bed, but to clarify, I mean "the same symptoms".

[Nicholas Miell (nmiell-deactivatedaccount)]

This bug results in four unnecessary D-Bus method calls per HTTP request instead of two during the entire lifetime of the application (one at startup, one at shutdown).

[Chris Coulson (chrisccoulson)]

Created attachment 599787
Don't call g_settings_new each time we look up system proxy settings (v2)

Hi,

I decided to fix the case with manual proxy settings too when addressing your other comment. I'm not sure if this is the best approach though.

[Karlt (karlt)]

Comment on attachment 599787
Don't call g_settings_new each time we look up system proxy settings (v2)

Looks fine to me, thanks.

[Chris Coulson (chrisccoulson)]

Note, this affects Thunderbird much more than Firefox, and Thunderbird causes a measurable amount of extra CPU usage in dbus-daemon

[Ryanvm (ryanvm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/62c8bf0d48bb

[Mak77 (mak77)]

https://hg.mozilla.org/mozilla-central/rev/62c8bf0d48bb

[Launchpad Janitor (janitor)]

This bug was fixed in the package firefox - 11.0~b4+build1-0ubuntu1

---------------
firefox (11.0~b4+build1-0ubuntu1) precise; urgency=low

  * New upstream release from the beta channel (FIREFOX_11_0b4_BUILD1)

  * Update globalmenu-extension to 2.0.4
    - Add support for Firefox 12
    - Ensure we correctly hide dummy menu items
  * Fix LP: #915895 - Just set autoDisableScopes to 0. Other distributions
    are already doing this, and we already made this feature pretty much
    useless by allowing extensions in the application directory, so that our
    language packs aren't disabled by default
    - update debian/vendor.js
  * Fix LP: #926495 - Add patch based on one from bmo: #691898 to enable
    building on ppc again
    - add debian/patches/fix-build-failure-without-yarr-jit.patch
    - update debian/patches/series
  * Fix LP: #939657 - Don't call g_settings_new each time we create a HTTP
    channel. Doing this causes a dbus roundtrip, and results in us spamming
    the session bus unnecessarily
    - add debian/patches/avoid-dbus-roundtrip-for-httpchannel.patch
    - update debian/patches/series
  * Fix LP: #894166 - Make Firefox work with our system hyphenation patterns,
    and stop including our own
    - update debian/vendor.js
    - add debian/patches/dont-include-hyphenation-patterns.patch
    - update debian/patches/series

  [ Jamie Strandboge <email address hidden> ]
  * debian/usr.bin.firefox.apparmor.12.04: include p11-kit abstraction (only
    needed on 12.04). LP: #918973
 -- Chris Coulson <email address hidden> Fri, 24 Feb 2012 14:55:30 +0000

[Launchpad Janitor (janitor)]

This bug was fixed in the package thunderbird - 11.0~b3+build1-0ubuntu1

---------------
thunderbird (11.0~b3+build1-0ubuntu1) precise; urgency=low

  * New upstream release from the beta channel (THUNDERBIRD_11_0b3_BUILD1)

  * Update globalmenu-extension to 2.0.4
    - Add support for Thunderbird 12
    - Ensure we correctly hide dummy menu items
  * Fix LP: #915895 - Just set autoDisableScopes to 0. Other distributions
    are already doing this, and we already made this feature pretty much
    useless by allowing extensions in the application directory, so that our
    language packs aren't disabled by default
    - update debian/vendor.js
  * Fix LP: #939657 - Don't call g_settings_new each time we create a HTTP
    channel. Doing this causes a dbus roundtrip, and results in us spamming
    the session bus and dbus-daemon using lots of CPU when updating mailboxes
    - add debian/patches/avoid-dbus-roundtrip-for-httpchannel.patch
    - update debian/patches/series
  * Fix LP: #926495 - Add patch based on one from bmo: #691898 to enable
    building on ppc again
    - add debian/patches/fix-build-failure-without-yarr-jit.patch
    - update debian/patches/series
  * Fix LP: #894166 - Make Thunderbird work with our system hyphenation
    patterns, and stop including our own
    - update debian/vendor.js
    - add debian/patches/dont-include-hyphenation-patterns.patch
    - update debian/patches/series
 -- Chris Coulson <email address hidden> Fri, 24 Feb 2012 14:46:13 +0000

[Akeybl (akeybl)]

Tracking for Firefox 11 and 12 since this is a regression in 11. Based upon the perf symptoms detailed here and the lack of dupes, we likely will not want to uplift this fix to Beta prior to release based upon the risk/reward. Please send email to <email address hidden> if you disagree. We'd be willing to take this fix for Aurora 12 in the near future, however. Please nominate when ready.

[Launchpad Janitor (janitor)]

This bug was fixed in the package firefox - 11.0+build1-0ubuntu0.11.10.1

---------------
firefox (11.0+build1-0ubuntu0.11.10.1) oneiric-security; urgency=low

  * New upstream stable release (FIREFOX_11_0_BUILD1)
    - Fix LP: #875266 - Firefox ignores GNOME3 proxy settings
    - Fix LP: #857153 - Needs to get accessibility settings from GSettings
    - see LP: #951250 for USN information

  * Update globalmenu-extension to 2.0.3
  * Ensure that the crash reporter is disabled if rebuilt by Ubuntu
    derivatives, as there will be no crash symbols for those
    - update debian/rules
  * Only add "Ubuntu" to the UA string when being built for Ubuntu
    - update debian/rules
  * Temporarily disable ipdl tests due to build failures. These aren't
    enabled upstream, anyway
    - update debian/config/mozconfig.in
  * Always set the update channel - not setting it at build-time on release
    builds breaks the extensions.checkCompatibility pref. The only things
    using it at runtime are nsBlocklistService, Test Pilot (beta + aurora)
    and the about dialog (where the channel is hidden anyway)
    - update debian/rules
    - update debian/firefox.install.in
  * Fix LP: #898883 - IPC xpcshell tests hang the buildd's. Give all
    xpcshell tests an X display, as plugin-container won't work without one
    - update debian/build/testsuite.mk
  * Turn on all IPC xpcshell tests again
    - update debian/build/testsute.mk
  * Ship Test Pilot as a distribution addon on aurora + beta, like upstream.
    This means that the addon manager can update it. It does also mean that it
    will remain installed in users profiles if they try the beta or aurora
    builds, but the Feedback button is disabled on release builds
    - update debian/firefox.install.in
    - fixes LP: #913357
  * Drop patches fixed upstream
    - remove debian/patches/fix-cursor-handling.patch
    - update debian/patches/series
  * Call xvfb-run with "-a" in case there are other servers running on the
    builder
    - update debian/build/testsuite.mk
  * Really fix LP: #898883 - IPC xpcshell tests hang the build. What was
    actually happening is plugin-container would fail to start because all
    available X connections had been used up by many instances of dbus-launch,
    spawned each time an xpcshell tried to talk to the session bus. Because
    we run all of the xpcshell tests with one Xvfb instance, the buses
    accumulate until the available X connections all run out. To fix this, run
    all tests requiring a display inside dbus-launch, so we create just a
    single bus for all xpcshell tests
    - update debian/build/testsuite.mk
    - update debian/control{,.in}
  * Add Ligurian to locale blacklist, as we don't support this in Ubuntu
    - update debian/config/locales.blacklist
  * Refresh patches
    - update debian/patches/ubuntu-ua-string-changes.patch
    - update debian/patches/mozilla-kde.patch
    - update debian/patches/firefox-kde.patch
  * Fix LP: #915895 - Just set autoDisableScopes to 0. Other distributions
    are already doing this, and we already made this feature pretty much
    useless by allowing extensions in the application directory, so that our
   ...

[Akeybl (akeybl)]

Including Mark to see if we've gotten any feedback that would push us to uplift this fix to Beta 12.

[Chris Coulson (chrisccoulson)]

Note, this actually only affects builds that are built with --enable-gio, so it doesn't affect mozilla.org builds. It only affects Linux distributions who are building with gio support and running on GNOME 3, and we're carrying this as a distro patch in Ubuntu already.

[Launchpad Janitor (janitor)]

This bug was fixed in the package thunderbird - 11.0+build1-0ubuntu0.11.10.1

---------------
thunderbird (11.0+build1-0ubuntu0.11.10.1) oneiric-security; urgency=low

  * New upstream stable release (THUNDERBIRD_11_0_BUILD1)
    - see LP: #951262 for USN information

  * Update globalmenu-extension to 2.0.3
  * Drop patches fixed upstream:
    - remove debian/patches/fix-cursor-handling.patch
    - update debian/patches/series
  * Refresh patches:
    - update debian/patches/theme-refresh-messenger-toolbar-icons.patch
  * Ensure we include locales in the tarball if they are in shipped-locales
    but not in all-locales
    - update debian/build/create-tarball.py
  * Always set the update channel - not setting it at build-time on release
    builds breaks the extensions.checkCompatibility pref. The only things
    using it at runtime are nsBlocklistService, Test Pilot (beta + aurora)
    and the about dialog (where the channel is hidden anyway)
    - update debian/rules
    - update debian/thunderbird.install.in
  * Fix LP: #898883 - IPC xpcshell tests hang the buildd's. Give all
    xpcshell tests an X display, as plugin-container won't work without one
    - update debian/build/testsuite.mk
  * Turn on all IPC xpcshell tests again (only applicable when the testsuite
    is enabled in the future)
    - update debian/build/testsute.mk
  * Update theme-refresh-messenger-toolbar-icons.patch to work with
    tabs-on-top, where the toolbar isn't styled like the menubar
    - update debian/patches/theme-refresh-messenger-toolbar-icons.patch
  * Refresh shipped locales for beta (addition of Armenian and Croatian)
    - refresh debian/config/locales.shipped
    - refresh debian/config/locales.all
    - refresh debian/control
  * Fix LP: #915895 - Just set autoDisableScopes to 0. Other distributions
    are already doing this, and we already made this feature pretty much
    useless by allowing extensions in the application directory, so that our
    language packs aren't disabled by default
    - update debian/vendor.js
  * Fix LP: #939657 - Don't call g_settings_new each time we create a HTTP
    channel. Doing this causes a dbus roundtrip, and results in us spamming
    the session bus and dbus-daemon using lots of CPU when updating mailboxes
    - add debian/patches/avoid-dbus-roundtrip-for-httpchannel.patch
    - update debian/patches/series
  * Fix LP: #926495 - Add patch based on one from bmo: #691898 to enable
    building on ppc again
    - add debian/patches/fix-build-failure-without-yarr-jit.patch
    - update debian/patches/series
  * Fix LP: #926495 - Disable the SPS profiler on unsupported architectures
    - add debian/patches/no-sps-profiler-on-unsupported-archs.patch
    - update debian/patches/series
 -- Chris Coulson <email address hidden> Sat, 10 Mar 2012 01:00:14 +0000

[Akeybl (akeybl)]

(In reply to Chris Coulson from comment #12)
> Note, this actually only affects builds that are built with --enable-gio, so
> it doesn't affect mozilla.org builds. It only affects Linux distributions
> who are building with gio support and running on GNOME 3, and we're carrying
> this as a distro patch in Ubuntu already.

Thanks Chris. No need to track on our end in that case.