each nsDocShell should not register as a pref observer

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dbaron, Assigned: nzr)

Tracking

({perf})

Trunk
mozilla27
Points:
---

Firefox Tracking Flags

(blocking-b2g:-)

Details

(Whiteboard: [mentor=bz][lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

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).
Yes to this bug, but we should also just fix the pref observers so they aren't horribly slow.
I'd think a pref var cache would do the job here (created when the first docshell is created).
Whiteboard: [mentor=bz][lang=c++]

Updated

5 years ago
blocking-b2g: --- → leo?
triage: not blocking leo, please renominate with justification if you feel this should block release.
blocking-b2g: leo? → -
(Assignee)

Comment 4

5 years ago
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

5 years ago
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
(Assignee)

Comment 6

5 years ago
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

5 years ago
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.
(Assignee)

Comment 8

5 years ago
@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

5 years ago
Yep. Kill the inheritance.

Comment 10

5 years ago
Created attachment 813769 [details] [diff] [review]
patch_906282.patch
Attachment #813769 - Flags: review?(josh)
(Assignee)

Comment 11

5 years ago
Created attachment 813827 [details] [diff] [review]
p906282_nzr.patch

What's done (in summary):
1. Replaced pref observer with var cache.
2. Removed nsIObserver inheritance from docshell.
Attachment #813827 - Flags: review?(josh)
Attachment #813827 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #813827 - Attachment is obsolete: true
Attachment #813827 - Flags: review?(josh)
Attachment #813827 - Flags: review?(bzbarsky)
(Assignee)

Comment 12

5 years ago
Created attachment 813832 [details] [diff] [review]
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.
Attachment #813832 - Flags: review?(josh)
Attachment #813832 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #813832 - Attachment description: p906282_nzr (v2).patch → p906282_nzr(v2).patch
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.
Attachment #813832 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 14

5 years ago
Created attachment 813962 [details] [diff] [review]
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().
Attachment #813962 - Flags: review?(bzbarsky)
Comment on attachment 813962 [details] [diff] [review]
p906282_nzr(v2-revised).patch

r=me
Attachment #813962 - Flags: review?(bzbarsky) → review+

Updated

5 years ago
Assignee: nobody → david.christiandy
Status: NEW → ASSIGNED
(Assignee)

Comment 16

5 years ago
Feels good to solve my first bug!
Many thanks for all the help, guys. Sorry if it took so long for me to fix.
Attachment #813769 - Flags: review?(josh)
Attachment #813832 - Flags: review?(josh)
Attachment #813769 - Attachment is obsolete: true
Attachment #813832 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0e2144372236
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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.
You need to log in before you can comment on or make changes to this bug.