Preferences spams lots of failing NS_ENSURE_TRUE at shutdown

VERIFIED FIXED in mozilla7

Status

()

Core
Preferences: Backend
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: jdm, Assigned: masayuki)

Tracking

Trunk
mozilla7
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
It's unnecessary. Let's just fail silently instead.
(Reporter)

Comment 1

6 years ago
Created attachment 536091 [details] [diff] [review]
Reduce console noise during shutdown from Preferences.

This also fixes some incorrect returns of PR_FALSE in error cases.
(Reporter)

Updated

6 years ago
Attachment #536091 - Flags: review?(roc)
Comment on attachment 536091 [details] [diff] [review]
Reduce console noise during shutdown from Preferences.

Review of attachment 536091 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #536091 - Flags: review?(roc) → review+
(Assignee)

Comment 3

6 years ago
I don't think this is good fix because if caller try to get/set prefs after shutdown of preferences module, that is a bug. This patch kills the notification.
(Assignee)

Comment 4

6 years ago
I have an idea.

1. The Preferences::sPreferences should be cleared when the singleton instance is destroyed.
2. Preferenses::Add(Strong|Weak)Observer() adds refcount.
3. Preferences::RemoveObserver() release refcount.

The observer users must remove them by this, but they can access prefs until they don't need prefs if they grabs the singleton instance.
(Assignee)

Comment 5

6 years ago
http://tbpl.mozilla.org/?tree=Try&rev=58e992f8bc11

I'm testing my idea.
(Assignee)

Comment 6

6 years ago
(In reply to comment #4)
> 3. Preferences::RemoveObserver() release refcount.

Sigh, this is impossible with current design. nsPrefBranch releases owning observers at XPCOM shutdown. Then, we cannot know whether the RemoveObserver() should decrease the refcount.

I'll post a better patch than attachment 536091 [details] [diff] [review] for disabling the warnings at shutting down.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
(Assignee)

Comment 7

6 years ago
Created attachment 537044 [details] [diff] [review]
Part.1 Delays to forget sPreferences and make only RemoveObserver(s) and UnregisterCallback are silent at shutting down

This patch makes only RemoveObserver(), RemoveObservers() and UnregisterCallback() silent only when it has shutdown because it's buggy if other methods are called after shutdown.

However, if somebody grabs the singleton instance, the static methods of Preferences should be still available. Therefore, this patch moves the clearing sPreferences to the destructor (just releases it at Shutdown()).

Furthermore, PREF_Cleanup() is also called from the destructor. So, we don't need to call it in UnloadPrefsModule().

On the other hand, GetService() and GetRootBranch() should call InitStaticMembers() because it might be called before nobody access the service and use other utility methods.
Attachment #536091 - Attachment is obsolete: true
Attachment #537044 - Flags: review?(roc)
(Assignee)

Comment 8

6 years ago
Created attachment 537046 [details] [diff] [review]
Part.2 Fix some wrong type results

Some methods which return nsresult return PR_FALSE when InitStaticMembers() fails.
Attachment #537046 - Flags: review?(roc)
Comment on attachment 537044 [details] [diff] [review]
Part.1 Delays to forget sPreferences and make only RemoveObserver(s) and UnregisterCallback are silent at shutting down

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

::: modules/libpref/src/Preferences.cpp
@@ +1336,5 @@
>  {
> +  if (!InitStaticMembers()) {
> +    // If tried to remove the observer after shutdown, it's OK but if not so,
> +    // it's really a trouble.
> +    NS_ENSURE_TRUE(sShutdown, NS_ERROR_NOT_AVAILABLE);

why are we calling InitStaticMembers here? how could a RemoveObserver call be the first call to require static member initialization? Can't we just assert that they've been initialized?
Comment on attachment 537046 [details] [diff] [review]
Part.2 Fix some wrong type results

Review of attachment 537046 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #537046 - Flags: review?(roc) → review+
(In reply to comment #3)
> I don't think this is good fix because if caller try to get/set prefs after
> shutdown of preferences module, that is a bug. This patch kills the
> notification.

Can't we create a module which changes its behavior depends on preference value at exit time? For example, cleanup cache data at exit time something like cookie.
(Assignee)

Comment 12

6 years ago
(In reply to comment #9)
> Comment on attachment 537044 [details] [diff] [review] [review]
> Part.1 Delays to forget sPreferences and make only RemoveObserver(s) and
> UnregisterCallback are silent at shutting down
> 
> Review of attachment 537044 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libpref/src/Preferences.cpp
> @@ +1336,5 @@
> >  {
> > +  if (!InitStaticMembers()) {
> > +    // If tried to remove the observer after shutdown, it's OK but if not so,
> > +    // it's really a trouble.
> > +    NS_ENSURE_TRUE(sShutdown, NS_ERROR_NOT_AVAILABLE);
> 
> why are we calling InitStaticMembers here? how could a RemoveObserver call
> be the first call to require static member initialization? Can't we just
> assert that they've been initialized?

Ah, it makes sense.

(In reply to comment #11)
> (In reply to comment #3)
> > I don't think this is good fix because if caller try to get/set prefs after
> > shutdown of preferences module, that is a bug. This patch kills the
> > notification.
> 
> Can't we create a module which changes its behavior depends on preference
> value at exit time? For example, cleanup cache data at exit time something
> like cookie.

No, you cannot. It's safe to store necessary pref values before unloading. But callers *can* prevent unloading by grabbing the nsIPrefServer pointer if part.1 is landed. However, note that all observers are released forcibly by nsPrefBranch at shutting down.
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefBranch.cpp#646
(Assignee)

Comment 13

6 years ago
Created attachment 537062 [details] [diff] [review]
Part.1 Delays to forget sPreferences and make only RemoveObserver(s) and UnregisterCallback are silent at shutting down
Attachment #537044 - Attachment is obsolete: true
Attachment #537044 - Flags: review?(roc)
Attachment #537062 - Flags: review?(roc)
Comment on attachment 537062 [details] [diff] [review]
Part.1 Delays to forget sPreferences and make only RemoveObserver(s) and UnregisterCallback are silent at shutting down

Review of attachment 537062 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #537062 - Flags: review?(roc) → review+
(Assignee)

Comment 15

6 years ago
http://hg.mozilla.org/mozilla-central/rev/c2d3110ead01
http://hg.mozilla.org/mozilla-central/rev/e05fed21ea35
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Assignee)

Updated

6 years ago
Duplicate of this bug: 661046
(Assignee)

Updated

6 years ago
Blocks: 660121
Could anyone please provide a test case or some STR in order to verify this issue?
(Assignee)

Comment 18

6 years ago
(In reply to Virgil Dicu from comment #17)
> Could anyone please provide a test case or some STR in order to verify this
> issue?

You cannot see the warning in console at shutdown of a debug build, it's verified. There were a lot of warnings in console. If you saw a couple of warnings, it could be the caller's bug.
Thank you for the feedback.

Setting to verified fixed based on comment.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.