Closed Bug 1001137 Opened 10 years ago Closed 10 years ago

Enabling FifoWatcher via user pref does not work

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: erahm, Assigned: erahm)

References

Details

(Whiteboard: [MemShrink])

Attachments

(1 file)

We have two problems here:

1) Setting |memory_info_dumper.watch_fifo.enabled| to true via about:config has no effect. After tracing through it looks like the user pref doesn't get loaded until after nsDumpUtils is initialized. B2G seems to get around this by overriding the default value in it's build config (rather than using a user pref).

2) The default value libpref/src/init/all.js is actually set for |memory_info_dumper.watch_fifo|, it should be |memory_info_dumper.watch_fifo.enabled|
This patch renames the default pref to match what our code expects and registers an observer for the pref to handle the case where user prefs are loaded after initialization has taken place.
Comment on attachment 8412206 [details] [diff] [review]
Enabling FifoWatcher via user pref does not work

Nathan can you take a look at this?
Attachment #8412206 - Flags: review?(nfroyd)
Comment on attachment 8412206 [details] [diff] [review]
Enabling FifoWatcher via user pref does not work

Review of attachment 8412206 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/nsMemoryInfoDumper.cpp
@@ +169,5 @@
>  }
>  
> +bool SetupFifo()
> +{
> +  if (FifoWatcher::MaybeCreate()) {

Since this is now a separate function that people might call from other places, WDYT about:

- Inverting the logic so all the callback registration doesn't happen under the if, but in the main control flow of the function?

- Adding MOZ_ASSERT or similar to enforce that we only run the callback registrations once?

@@ +193,5 @@
> +{
> +  LOG("memory_info_dumper.watch_fifo.enabled changed");
> +  if (SetupFifo()) {
> +    Preferences::UnregisterCallback(OnFifoEnabledChange,
> +                                    "memory_info_dumper.watch_fifo.enabled",

Maybe pull this out into a static const char[] somewhere and use it where you register the callback too?
Attachment #8412206 - Flags: review?(nfroyd) → review+
Reordered logic, added assert on double-registering and made the pref name a static in FifoWatcher.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f628c9d48728
https://hg.mozilla.org/mozilla-central/rev/f628c9d48728
https://hg.mozilla.org/mozilla-central/rev/950fadd70f9e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Blocks: 1015497
You need to log in before you can comment on or make changes to this bug.