906282 - each nsDocShell should not register as a pref observer

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Every nsDocShell object (one per frame) registers itself as a preference observer, and then unregisters on destruction.  This unregistration uses a search through a linked list (something that we should perhaps also fix).  This can add up to a noticeable amount of time when a lot of tabs are open; see the end of bug 906269 comment 0.

This observer does not manipulate any state that needs to be per-docshell; the current value of the single preference being observed could easily be in a global variable, although it would require slight refactoring of the mUseErrorPages (to reflect three states of use-preference, false, and true, possibly doing that with two booleans).

This should be converted into a single preference observer rather than having one observer for every nsDocShell (which is also bad for performance in the case of the preference actually being changed).

Comment 1

[Justin Lebar (not reading bugmail)]

Yes to this bug, but we should also just fix the pref observers so they aren't horribly slow.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'd think a pref var cache would do the job here (created when the first docshell is created).

Comment 3

[Dylan Oliver [:doliver]]

triage: not blocking leo, please renominate with justification if you feel this should block release.

Comment 4

[David Christiandy [:nzr]]

Hi, i'm interested in doing this for my first bug, if it's okay :)
I've read the getting started guide, and have built mozilla-central (browser).

Currently I'm reading about how docshell works and the properties around it, and I would love to know where to start on this. Thanks!

Comment 5

[Josh Matthews [:jdm]]

nsDocShell::Create calls Preferences::AddStrongObserver: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4917
Instead, it would be better to use the preference cache API: http://mxr.mozilla.org/mozilla-central/source/modules/libpref/public/Preferences.h#265

Comment 6

[David Christiandy [:nzr]]

Hi, Josh. Thanks for replying! :D
Sorry for late reply; I've had quite a struggle with my ISP.

So, what I understand at the moment is I need to add mUseErrorPages to pref var cache *only* when the first docshell is created. For this to work (for all docshells, the variable mUseErrorPages should be changed to a static variable. Then, I need to clean up nsDocShell.cpp (and .h) for usages of Preferences::AddStrongObserver and Preferences::RemoveObserver (and possibly nsDocShell::Observe ?)

In summary, this is what I have to do currently:
- Make the mUseErrorPages attribute static.
- Replace Preferences::AddStrongObserver with Preferences::AddBoolVarCache in nsDocShell::Create,
  http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4943
- Remove Preferences::RemoveObserver in nsDocShell::SetUseErrorPages and nsDocShell::Destroy,
  http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2585 and #4975

I'm still reading here and there, though. Hopefully I'm in the right direction!

Comment 7

[Josh Matthews [:jdm]]

That doesn't sound quite right - mUseErrorPages should remain a non-static member, because SetUseErrorPages can be called on some docshells, causing them to fall out of sync (intentionally) with the pref value. Just add another static member for the cached pref value instead of changing mUseErrorPages.

Comment 8

[David Christiandy [:nzr]]

@Josh

Noted. Alright, I'll start working on it.
And one more thing, What should I do with nsDocShell::Observe?
Since the observer methods will be removed, I don't see the use of implementing observer on Docshell.

Comment 9

[Josh Matthews [:jdm]]

Yep. Kill the inheritance.

Comment 10

[Aroon]

Attachment: patch_906282.patch

Comment 11

[David Christiandy [:nzr]]

Attachment: p906282_nzr.patch

What's done (in summary):
1. Replaced pref observer with var cache.
2. Removed nsIObserver inheritance from docshell.

Comment 12

[David Christiandy [:nzr]]

Attachment: p906282_nzr(v2).patch

What's done (in summary):
1. Replaced pref observer with var cache.
2. Removed nsIObserver inheritance from docshell.
3. (v2) added aDefault parameter for AddBoolVarCache usage.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 813832 [details] [diff] [review]
p906282_nzr(v2).patch

>+    bool tUseErrorPages = mObserveErrorPages ? sUseErrorPages : mUseErrorPages;
>+    *aUseErrorPages = tUseErrorPages;

How about just:

  *aUseErrorPages = mObserveErrorPages ? sUseErrorPages : mUseErrorPages;

?

>+    bool tUseErrorPages = mObserveErrorPages ? sUseErrorPages : mUseErrorPages;

Or better yet, add an inline method on nsDocShell called UseErrorPages() that will return the right boolean, instead of duplicating the ?: bit?

>+    // This boolean replaces docshell's pref observer, as filed in bug
> #908262.

The blame will have the bug number.  How about:

  // Cached value of the "browser.xul.error_pages.enabled" preference.

for the comment here?

r=me with those nits.

Comment 14

[David Christiandy [:nzr]]

Attachment: p906282_nzr(v2-revised).patch

Hi, bz. Thanks for reviewing!
I revised the patch by following your review notes:
1. Modified comments.
2. Added inline method UseErrorPages().

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 813962 [details] [diff] [review]
p906282_nzr(v2-revised).patch

r=me

Comment 16

[David Christiandy [:nzr]]

Feels good to solve my first bug!
Many thanks for all the help, guys. Sorry if it took so long for me to fix.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e2144372236

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0e2144372236

Comment 19

[[e-mail]]

Comment on attachment 813962 [details] [diff] [review]
p906282_nzr(v2-revised).patch

[Just some style nits, don't bother fixing them unless a module owner or peer thinks that it's necessary.]

>     if (mObserveErrorPages) {
>         mObserveErrorPages = false;
>     }
I wonder whether it's worth removing the conditional here in SetUseErrorPages.

>     mUseErrorPages =
>         Preferences::GetBool("browser.xul.error_pages.enabled", mUseErrorPages);
Technically we never use this value; UseErrorPages doesn't read mUseErrorPages unless you call SetUseErrorPages which writes it, while Preferences::AddBoolVarCache will read the pref for you anyway.

>+    if(!gAddedPreferencesVarCache) {
Mozilla style is to have a space between keywords such as if and the (.

>     // Remove our pref observers
>     if (mObserveErrorPages) {
>-        Preferences::RemoveObserver(this, "browser.xul.error_pages.enabled");
>         mObserveErrorPages = false;
>     }
And in Destroy I wonder whether it's worth resetting mObserveErrorPages.