Last Comment Bug 660640 - Preferences spams lots of failing NS_ENSURE_TRUE at shutdown
: Preferences spams lots of failing NS_ENSURE_TRUE at shutdown
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Preferences: Backend (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla7
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
: Benjamin Smedberg [:bsmedberg]
Mentors:
: 661046 (view as bug list)
Depends on:
Blocks: 660121
  Show dependency treegraph
 
Reported: 2011-05-30 07:16 PDT by Josh Matthews [:jdm] (on vacation until Dec 5)
Modified: 2011-08-30 07:19 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reduce console noise during shutdown from Preferences. (6.77 KB, patch)
2011-05-30 07:30 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
roc: review+
Details | Diff | Splinter Review
Part.1 Delays to forget sPreferences and make only RemoveObserver(s) and UnregisterCallback are silent at shutting down (3.88 KB, patch)
2011-06-02 17:43 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part.2 Fix some wrong type results (1.52 KB, patch)
2011-06-02 17:45 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
Details | Diff | Splinter Review
Part.1 Delays to forget sPreferences and make only RemoveObserver(s) and UnregisterCallback are silent at shutting down (4.19 KB, patch)
2011-06-02 19:42 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (on vacation until Dec 5) 2011-05-30 07:16:16 PDT
It's unnecessary. Let's just fail silently instead.
Comment 1 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-05-30 07:30:31 PDT
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.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-30 16:03:26 PDT
Comment on attachment 536091 [details] [diff] [review]
Reduce console noise during shutdown from Preferences.

Review of attachment 536091 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-31 17:11:43 PDT
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.
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-31 17:55:11 PDT
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.
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-31 19:21:59 PDT
http://tbpl.mozilla.org/?tree=Try&rev=58e992f8bc11

I'm testing my idea.
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-01 04:57:03 PDT
(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.
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-02 17:43:46 PDT
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.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-02 17:45:13 PDT
Created attachment 537046 [details] [diff] [review]
Part.2 Fix some wrong type results

Some methods which return nsresult return PR_FALSE when InitStaticMembers() fails.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-02 18:15:27 PDT
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 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-02 18:16:35 PDT
Comment on attachment 537046 [details] [diff] [review]
Part.2 Fix some wrong type results

Review of attachment 537046 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 11 Hiroyuki Ikezoe (:hiro) 2011-06-02 18:24:45 PDT
(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.
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-02 18:40:57 PDT
(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
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-02 19:42:49 PDT
Created attachment 537062 [details] [diff] [review]
Part.1 Delays to forget sPreferences and make only RemoveObserver(s) and UnregisterCallback are silent at shutting down
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-02 19:52:17 PDT
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]:
-----------------------------------------------------------------
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-02 23:11:31 PDT
*** Bug 661046 has been marked as a duplicate of this bug. ***
Comment 17 Virgil Dicu [:virgil] [QA] 2011-08-24 02:34:09 PDT
Could anyone please provide a test case or some STR in order to verify this issue?
Comment 18 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-08-24 02:46:10 PDT
(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.
Comment 19 Virgil Dicu [:virgil] [QA] 2011-08-30 07:19:08 PDT
Thank you for the feedback.

Setting to verified fixed based on comment.

Note You need to log in before you can comment on or make changes to this bug.