Last Comment Bug 580790 - Connection::initialize can access preferences off of the main thread
: Connection::initialize can access preferences off of the main thread
Status: RESOLVED FIXED
[softblocker]
: assertion
Product: Toolkit
Classification: Components
Component: Storage (show other bugs)
: Trunk
: x86 Mac OS X
-- normal (vote)
: mozilla2.0b12
Assigned To: Shawn Wilsher :sdwilsh
:
: Marco Bonardo [::mak]
Mentors:
: 624300 (view as bug list)
Depends on: 578939 593046
Blocks: 616019
  Show dependency treegraph
 
Reported: 2010-07-21 15:09 PDT by Jesse Ruderman
Modified: 2011-02-04 13:21 PST (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+


Attachments
Prefs v1.0 (10.98 KB, patch)
2010-07-28 11:13 PDT, Shawn Wilsher :sdwilsh
dwitte: review+
benjamin: superreview+
Details | Diff | Splinter Review
Storage v1.0 (5.05 KB, patch)
2010-07-28 11:39 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
Storage v1.0 (5.55 KB, patch)
2010-07-28 11:41 PDT, Shawn Wilsher :sdwilsh
bugmail: review+
shaver: superreview+
Details | Diff | Splinter Review
Prefs v1.1 (11.14 KB, patch)
2010-08-05 12:19 PDT, Shawn Wilsher :sdwilsh
dwitte: review+
Details | Diff | Splinter Review
Storage v1.1 (5.60 KB, patch)
2011-02-04 10:17 PST, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review

Description User image Jesse Ruderman 2010-07-21 15:09:10 PDT
Hit these assertions while fuzzing, using a Mac32 Tinderbox build, built from http://hg.mozilla.org/mozilla-central/rev/1ac07fe5f6c9.  Both happened in the same session, but not together.  I wasn't able to reproduce.

Related to bug 380315?  Is this a bug in Storage, Prefs, or pldhash?

###!!! ASSERTION: RECURSION_LEVEL(table_) > 0: 'RECURSION_LEVEL(table) > 0', file pldhash.c, line 707
PL_DHashTableOperate [pldhash.c:707]
pref_HashTableLookup [modules/libpref/src/prefapi.cpp:676]
PREF_GetIntPref [modules/libpref/src/prefapi.cpp:459]
nsPrefBranch::GetIntPref [modules/libpref/src/nsPrefBranch.cpp:256]
nsPrefService::GetIntPref [modules/libpref/src/nsPrefService.h:60]
mozilla::storage::Connection::initialize [storage/src/mozStorageConnection.cpp:446]
mozilla::storage::Service::OpenDatabase [storage/src/mozStorageService.cpp:318]
nsUrlClassifierDBServiceWorker::OpenDb [toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp:3388]
nsUrlClassifierDBServiceWorker::BeginUpdate [toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp:2921]
NS_InvokeByIndex_P [xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179]
nsProxyObjectCallInfo::Run [xpcom/proxy/src/nsProxyEvent.cpp:181]
nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:547]
NS_ProcessNextEvent_P [nsThreadUtils.cpp:250]
nsThread::ThreadFunc [xpcom/threads/nsThread.cpp:262]
_pt_root [nsprpub/pr/src/pthreads/ptthread.c:231]
libSystem.B.dylib + 0x2e81d
libSystem.B.dylib + 0x2e6a2

###!!! ASSERTION: op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0: 'op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0', file pldhash.c, line 612
PL_DHashTableOperate [pldhash.c:613]
pref_HashPref [modules/libpref/src/prefapi.cpp:689]
PREF_SetIntPref [modules/libpref/src/prefapi.cpp:303]
nsPrefBranch::SetIntPref [modules/libpref/src/nsPrefBranch.cpp:269]
nsPrefService::SetIntPref [modules/libpref/src/nsPrefService.h:60]
NS_InvokeByIndex_P [xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179]
CallMethodHelper::Invoke [js/src/xpconnect/src/xpcwrappednative.cpp:3061]
CallMethodHelper::Call [js/src/xpconnect/src/xpcwrappednative.cpp:2340]
XPCWrappedNative::CallMethod [js/src/xpconnect/src/xpcwrappednative.cpp:2304]
XPC_WN_CallMethod [js/src/xpconnect/src/xpcwrappednativejsops.cpp:1796]
js::callJSNative [js/src/jscntxtinlines.h:339]
Invoke<JSBool (*)(JSContext*, JSObject*, uintN, jsval*, jsval*)> [js/src/jsinterp.cpp:591]
js_Invoke [js/src/jsinterp.cpp:693]
js_Interpret [js/src/jsops.cpp:2155]
Invoke<JSBool (*)(JSContext*, JSObject*, uintN, jsval*, jsval*)> [js/src/jsinterp.cpp:602]
js_Invoke [js/src/jsinterp.cpp:693]
nsXPCWrappedJSClass::CallMethod [js/src/xpconnect/src/xpcwrappedjsclass.cpp:1689]
nsXPCWrappedJS::CallMethod [js/src/xpconnect/src/xpcwrappedjs.cpp:570]
PrepareAndDispatch [xpcom/reflect/xptcall/src/md/unix/xptcstubs_unixish_x86.cpp:93]
nsXPTCStubBase::Stub3 [xptcstubsdef.inc:1]
NS_InvokeByIndex_P [xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179]
CallMethodHelper::Invoke [js/src/xpconnect/src/xpcwrappednative.cpp:3061]
CallMethodHelper::Call [js/src/xpconnect/src/xpcwrappednative.cpp:2340]
XPCWrappedNative::CallMethod [js/src/xpconnect/src/xpcwrappednative.cpp:2304]
XPC_WN_CallMethod [js/src/xpconnect/src/xpcwrappednativejsops.cpp:1796]
js::callJSNative [js/src/jscntxtinlines.h:339]
Invoke<JSBool (*)(JSContext*, JSObject*, uintN, jsval*, jsval*)> [js/src/jsinterp.cpp:591]
js_Invoke [js/src/jsinterp.cpp:693]
js_Interpret [js/src/jsops.cpp:2155]
Invoke<JSBool (*)(JSContext*, JSObject*, uintN, jsval*, jsval*)> [js/src/jsinterp.cpp:602]
js_Invoke [js/src/jsinterp.cpp:693]
js_InternalInvoke [js/src/jsinterp.cpp:739]
js_InternalGetOrSet [js/src/jsinterp.cpp:767]
JSScopeProperty::get [js/src/jsscopeinlines.h:274]
js_NativeGet [js/src/jsobj.cpp:4752]
js_GetPropertyHelper [js/src/jsobj.cpp:4922]
js_GetProperty [js/src/jsobj.cpp:4933]
JSObject::getProperty [js/src/jsobj.h:660]
JS_GetPropertyById [js/src/jsapi.cpp:3655]
JS_GetProperty [js/src/jsapi.cpp:3668]
mozJSComponentLoader::ImportInto [js/src/xpconnect/loader/mozJSComponentLoader.cpp:1779]
mozJSComponentLoader::Import [js/src/xpconnect/loader/mozJSComponentLoader.cpp:1624]
nsXPCComponents_Utils::Import [js/src/xpconnect/src/xpccomponents.cpp:3754]
NS_InvokeByIndex_P [xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179]
CallMethodHelper::Invoke [js/src/xpconnect/src/xpcwrappednative.cpp:3061]
CallMethodHelper::Call [js/src/xpconnect/src/xpcwrappednative.cpp:2340]
XPCWrappedNative::CallMethod [js/src/xpconnect/src/xpcwrappednative.cpp:2304]
XPC_WN_CallMethod [js/src/xpconnect/src/xpcwrappednativejsops.cpp:1796]
js::callJSNative [js/src/jscntxtinlines.h:339]
Invoke<JSBool (*)(JSContext*, JSObject*, uintN, jsval*, jsval*)> [js/src/jsinterp.cpp:591]
js_Invoke [js/src/jsinterp.cpp:693]
js_Interpret [js/src/jsops.cpp:2155]
Invoke<JSBool (*)(JSContext*, JSObject*, uintN, jsval*, jsval*)> [js/src/jsinterp.cpp:602]
js_Invoke [js/src/jsinterp.cpp:693]
js_InternalInvoke [js/src/jsinterp.cpp:739]
JS_CallFunctionValue [js/src/jsapi.cpp:4850]
nsJSContext::CallEventHandler [dom/base/nsJSEnvironment.cpp:2204]
nsGlobalWindow::RunTimeout [dom/base/nsGlobalWindow.cpp:8502]
nsGlobalWindow::TimerCallback [dom/base/nsGlobalWindow.cpp:8849]
nsTimerImpl::Fire [xpcom/threads/nsTimerImpl.cpp:428]
nsTimerEvent::Run [xpcom/threads/nsTimerImpl.cpp:521]
nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:547]
NS_ProcessPendingEvents_P [nsThreadUtils.cpp:200]
nsBaseAppShell::NativeEventCallback [widget/src/xpwidgets/nsBaseAppShell.cpp:127]
nsAppShell::ProcessGeckoEvents [widget/src/cocoa/nsAppShell.mm:395]
CoreFoundation + 0x3f0fb
CoreFoundation + 0x3cbbf
CoreFoundation + 0x3c094
CoreFoundation + 0x3bec1
HIToolbox + 0x34f9c
HIToolbox + 0x34d51
HIToolbox + 0x34bd6
AppKit + 0x48a89
-AppKit + 0x482ca
-AppKit + 0xa55b
nsAppShell::Run [widget/src/cocoa/nsAppShell.mm:747]
nsAppStartup::Run [toolkit/components/startup/src/nsAppStartup.cpp:191]
XRE_main [toolkit/xre/nsAppRunner.cpp:3630]
main [browser/app/nsBrowserApp.cpp:158]
firefox-bin + 0x1486
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2010-07-21 20:40:47 PDT
Sounds like a storage bug to me; neither prefs nor pldhash claimed to be threadsafe when I last checked.
Comment 2 User image Benjamin Smedberg [:bsmedberg] 2010-07-22 06:11:22 PDT
Sure looks like it, unless calling mozilla::storage::Service::OpenDatabase is supposed to be a main-thread-only thing.
Comment 3 User image Shawn Wilsher :sdwilsh 2010-07-22 06:30:53 PDT
It...is not.  I'm not sure if I just want to drop the preference, or get stuff off the main thread...

This is totally me.  Should this be blocking?
Comment 4 User image Benjamin Smedberg [:bsmedberg] 2010-07-22 06:34:23 PDT
Seems like it could cause pref crashes that won't show up obviously in crashreporter signatures, so yes.
Comment 5 User image Benjamin Smedberg [:bsmedberg] 2010-07-22 06:34:42 PDT
Sounds like we should add mainthread assertions to the prefservice too.
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2010-07-22 06:46:38 PDT
Would it make sense to not make it use threadsafe refcounting?  Just so people get those asserts?  Or is that more likely to just get us in trouble in release builds?
Comment 7 User image Benjamin Smedberg [:bsmedberg] 2010-07-22 06:53:41 PDT
I think that there are people who use XPCOM proxies with it still, so probably not for this cycle.
Comment 8 User image Shawn Wilsher :sdwilsh 2010-07-22 09:12:44 PDT
And need bug 578939 (already a blocker) to do this sanely...
Comment 9 User image Shawn Wilsher :sdwilsh 2010-07-28 11:13:15 PDT
Created attachment 460929 [details] [diff] [review]
Prefs v1.0

This makes callers of the pref service go BOOM (NS_ERROR_UNEXPECTED) when they try to do anything with preferences off of the main thread.  This makes the storage tests that open a database connection off of the main thread fail.
Comment 10 User image Shawn Wilsher :sdwilsh 2010-07-28 11:39:36 PDT
Created attachment 460942 [details] [diff] [review]
Storage v1.0

And now the storage bits.  Pretty self explanatory.
Comment 11 User image Shawn Wilsher :sdwilsh 2010-07-28 11:41:59 PDT
Created attachment 460943 [details] [diff] [review]
Storage v1.0

It helps when one saves before qrefreshing...
Comment 12 User image Andrew Sutherland [:asuth] 2010-07-28 12:15:52 PDT
Comment on attachment 460943 [details] [diff] [review]
Storage v1.0

So, the code seems reasonable, but this will potentially result in all databases initialized off the main thread not being initialized with the value of the preference.  Since I presume the preference was added in the first place because enough vocal people were concerned about fsync load on their system or database corruption on crashes, this might be concerning to those same vocal people.  Of note is that apparently fedora firefox builds set this value to 0 by default.

As such this is sorta a de facto API change, or at the very least something that I can shirk the blame to a super-reviewer on.  I'm setting sr? because I don't know who to direct it at, please pick somebody.

I do think correctness trumps the preference which is why I'm r+'ing.  If you know the set of currently off-main-thread ininitialized db's and that they are unlikely to cause problems, it might be good to mention that here or in a comment in the code.
Comment 13 User image Shawn Wilsher :sdwilsh 2010-07-28 12:22:25 PDT
(In reply to comment #12)
> Comment on attachment 460943 [details] [diff] [review]
> Storage v1.0
> 
> So, the code seems reasonable, but this will potentially result in all
> databases initialized off the main thread not being initialized with the value
> of the preference.  Since I presume the preference was added in the first place
> because enough vocal people were concerned about fsync load on their system or
> database corruption on crashes, this might be concerning to those same vocal
> people.  Of note is that apparently fedora firefox builds set this value to 0
> by default.
We'll still use the default value, which isn't FULL at least.  Also, fedora should not be doing that if they are still calling it Firefox, so we may need to open a bug up for that...

> I do think correctness trumps the preference which is why I'm r+'ing.  If you
> know the set of currently off-main-thread ininitialized db's and that they are
> unlikely to cause problems, it might be good to mention that here or in a
> comment in the code.
Code comment is likely to be out of date.  Right now the only two databases would be indexedDB and urlclassifier.  Although, I don't see why someone would initialize a database on one thread (background thread) and then use it on another.  I would expect all of these to happen on a thread where possibly more fsyncs would matter.  The only thing this actually sucks for is people who set it to 2.
Comment 14 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-07-28 13:26:18 PDT
If we're going to ignore the pref, we should remove it everywhere.  If not, then we should honour it.  We don't need to support changes to this without a restart, so I think we should just load the pref's value into an int somewhere early on, and then the different threads can read that int.  Easy, fast, safe.
Comment 15 User image Shawn Wilsher :sdwilsh 2010-07-28 13:29:34 PDT
(In reply to comment #14)
> If we're going to ignore the pref, we should remove it everywhere.  If not,
> then we should honour it.  We don't need to support changes to this without a
> restart, so I think we should just load the pref's value into an int somewhere
> early on, and then the different threads can read that int.  Easy, fast, safe.
And that's what this patch does, except when the storage service gets initialized off of the main thread.  We can't get the pref value there, so we dump an event to the main thread to get it for us.  I suppose we could proxy it, but in the rare case that we get initialized off of the main thread, I don't think this will kill us.
Comment 16 User image Shawn Wilsher :sdwilsh 2010-07-28 13:30:07 PDT
(fwiw, I don't think, because of places, that the service will ever be initialized off of the main thread in the browser)
Comment 17 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-07-28 13:42:34 PDT
Ah, sorry, yes!  Can we warn if storage initialized off-thread, or perhaps even error out?  Fine for a follow-up, this is strictly improvement now.
Comment 18 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-07-28 13:43:02 PDT
Comment on attachment 460943 [details] [diff] [review]
Storage v1.0

sr=shaver, thanks.
Comment 19 User image Shawn Wilsher :sdwilsh 2010-07-28 13:47:09 PDT
(In reply to comment #17)
> Ah, sorry, yes!  Can we warn if storage initialized off-thread, or perhaps even
> error out?  Fine for a follow-up, this is strictly improvement now.
Well, it is something we explicitly support (see dependent bug).  Up until now, it would not have mattered if we were initialized on or off of the main thread - stuff that would work on a background thread would have worked still.  The preference is the only thing that breaks.  Not sure if it is disallowing initializing the service off of the main thread (or even warning) as a result of this.  Database connections can still set PRAGMA synchronous on their own (which we do at least for places and cookies) if they really care what it gets set to.
Comment 20 User image dwitte@gmail.com 2010-08-03 15:09:02 PDT
Comment on attachment 460929 [details] [diff] [review]
Prefs v1.0

Gah! Do we have to do this in release builds? Make it an assert, like it is in NS_IMPL_ADDREF: http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#342

Also I'd rather see a macro for all this copy/pasted code, or at least fold the NS_IsMainThread() check and LogBackgroundThreadUse() into a single inline function.

r=dwitte with that.
Comment 21 User image Shawn Wilsher :sdwilsh 2010-08-03 15:15:06 PDT
(In reply to comment #20)
> Gah! Do we have to do this in release builds? Make it an assert, like it is in
> NS_IMPL_ADDREF:
> http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#342
The whole point was to do this in release builds because add-ons don't usually test in debug builds.

> Also I'd rather see a macro for all this copy/pasted code, or at least fold the
> NS_IsMainThread() check and LogBackgroundThreadUse() into a single inline
> function.
bsmedberg doesn't like things that return to be inside macros, which is why I didn't do that.
Comment 22 User image dwitte@gmail.com 2010-08-03 16:18:31 PDT
(In reply to comment #21)
> The whole point was to do this in release builds because add-ons don't usually
> test in debug builds.

What about every other service that's not threadsafe that could be used by addons? Granted, prefservice is a bigger target, but adding release checks to each interface method and accessor on a bunch of other services is a non-starter.

Is there a way to have the servicemgr throw if you access a non-threadsafe service off main thread? I don't *think* there is. We could potentially add something to nsIClassInfo.flags for that purpose. Or alter the signature of _InstanceClass##Constructor (http://mxr.mozilla.org/mozilla-central/source/xpcom/components/ModuleUtils.h#105) to return a 'threadsafe' outparam, which the servicemgr could stash in its flags.

Not sure if any of this is worth it... bsmedberg?

> > Also I'd rather see a macro for all this copy/pasted code, or at least fold the
> > NS_IsMainThread() check and LogBackgroundThreadUse() into a single inline
> > function.
> bsmedberg doesn't like things that return to be inside macros, which is why I
> didn't do that.

Fair enough -- roll them into a CheckMainThread() function?
Comment 23 User image Shawn Wilsher :sdwilsh 2010-08-03 16:26:46 PDT
(In reply to comment #22)
> What about every other service that's not threadsafe that could be used by
> addons? Granted, prefservice is a bigger target, but adding release checks to
> each interface method and accessor on a bunch of other services is a
> non-starter.
We added them to Places in Firefox 3.5.  the NS_IsMainThread checks are supposed to be fast now, and PGO should make it not main thread branch be the uncommon case.

> Fair enough -- roll them into a CheckMainThread() function?
Isn't this really what LogBackgroundThreadUse is essentially?  I mean, we'd go from:
  if (!NS_IsMainThread()) {
    nsPrefService::LogBackgroundThreadUse();
    return NS_ERROR_UNEXPECTED;
  }
to:
  if (!nsPrefService::CheckAndLogBackgroundThreadUse()) {
    return NS_ERROR_UNEXPECTED;
  }
which I can do, but seems like marginal gain.
Comment 24 User image dwitte@gmail.com 2010-08-03 16:35:28 PDT
(In reply to comment #22)
> Is there a way to have the servicemgr throw if you access a non-threadsafe
> service off main thread? I don't *think* there is. We could potentially add
> something to nsIClassInfo.flags

Nevermind, we have one! http://mxr.mozilla.org/mozilla-central/source/xpcom/components/nsIClassInfo.idl#113

We could check it here-ish: http://mxr.mozilla.org/mozilla-central/source/xpcom/components/nsComponentManager.cpp#1391

I'd like to get bsmedberg's input on this. I'm OK with the checks as you have them if he is; I just don't like specialcasing the prefservice and paying the cost on every call.

(In reply to comment #23)
> We added them to Places in Firefox 3.5.  the NS_IsMainThread checks are
> supposed to be fast now, and PGO should make it not main thread branch be the
> uncommon case.

Please wrap it in NS_LIKELY.

> which I can do, but seems like marginal gain.

Please do. If you're copy-pasting code, make it as short as possible!
Comment 25 User image Benjamin Smedberg [:bsmedberg] 2010-08-03 17:01:38 PDT
I think we should pay the cost on every call: it's a trivially small cost on all the platforms we care about.

The component-manager flag doesn't help much now, and my post-FF4 thoughts will make it irrelevant, so let's not worry about it now.
Comment 26 User image dwitte@gmail.com 2010-08-03 18:29:55 PDT
Ok, r=me then!
Comment 27 User image Shawn Wilsher :sdwilsh 2010-08-05 12:19:23 PDT
(In reply to comment #24)
> Please wrap it in NS_LIKELY.
Presume you mean NS_UNLIKELY.  New patch RSN, please take a look.
Comment 28 User image Shawn Wilsher :sdwilsh 2010-08-05 12:19:51 PDT
Created attachment 463256 [details] [diff] [review]
Prefs v1.1
Comment 29 User image Shawn Wilsher :sdwilsh 2010-08-06 11:16:31 PDT
The prefs changed landed, but I had a compiler issue with the storage part:
http://hg.mozilla.org/mozilla-central/rev/1302220623fd
Comment 30 User image Shawn Wilsher :sdwilsh 2010-08-06 11:22:40 PDT
Except that I just backed that out too since I realized the pref changes by themselves will make Bd go orange.  *sigh*
Comment 31 User image Shawn Wilsher :sdwilsh 2010-09-02 08:16:36 PDT
Based on try server results, bug 585706 and bug 593046 are the only two things blocking this from landing.
Comment 32 User image Shawn Wilsher :sdwilsh 2010-09-23 17:15:34 PDT
Changing this to beta N since it does change the behavior of the prefs API (admittedly to throw on something people shouldn't have been doing anyway, but still a change).
Comment 33 User image Benjamin Smedberg [:bsmedberg] 2010-12-15 11:41:10 PST
Not a blocker any more because we added thread checks to prefservice which prevents the crashes/corruption. We should still fix this because you'll get asserts (and you won't get the prefs you expect anyway).
Comment 34 User image Shawn Wilsher :sdwilsh 2010-12-15 14:10:44 PST
Preferences code spun off to bug 619487.
Comment 35 User image Andrew Sutherland [:asuth] 2011-01-06 00:19:08 PST
sdwilsh, can we land the storage patch since the preferences logic has been spun off onto its own bug (and fixed)?  It looks like the whiteboard annotation about needing bug 585706 was for the preferences part of this bug (when it was more explodey).
Comment 36 User image Shawn Wilsher :sdwilsh 2011-01-06 06:29:05 PST
(In reply to comment #35)
> sdwilsh, can we land the storage patch since the preferences logic has been
> spun off onto its own bug (and fixed)?  It looks like the whiteboard annotation
> about needing bug 585706 was for the preferences part of this bug (when it was
> more explodey).
probably, although it may have bitrotted.  I need to see if tests pass now.
Comment 37 User image John J. Barton 2011-01-09 11:54:01 PST
This patch causes messages in Error Console:

Invalid use of the preferences on a background thread!

These message have no value: there is no call stack, no filename, no line number, and you have to search through the Firefox source code to realize that it is caused by the platform code. 

Please remove this message or change it to be useful.
Comment 38 User image dwitte@gmail.com 2011-01-09 13:47:08 PST
Try setting a breakpoint in a debug build?
Comment 39 User image Ben Bucksch (:BenB) 2011-01-09 15:19:23 PST
> Try setting a breakpoint in a debug build?

You're asking JS developers to do that??

Are JS modules affected? I.e. can we use prefs in JS modules at least? I am not sure where they run, but they sure need prefs.

Please make preferences thread-safe instead of asking all apps and extensions to adapt.
Comment 40 User image Shawn Wilsher :sdwilsh 2011-01-09 15:21:22 PST
The patch that everyone is complaining about is actually bug 619487.  Please take the discussion there and stop polluting this bug.
Comment 41 User image Shawn Wilsher :sdwilsh 2011-01-10 08:20:52 PST
*** Bug 624300 has been marked as a duplicate of this bug. ***
Comment 42 User image Ben Bucksch (:BenB) 2011-01-10 09:24:39 PST
FWIW, this bug is now causing a Warning with the following stack. It should be solved by the Storage 1.0 patch.

Breakpoint 1, nsPrefService::CheckAndLogBackgroundThreadUse ()
    at /home/laga/work/firefox/source/modules/libpref/src/nsPrefService.cpp:983
983       NS_WARNING("Cannot be used on a background thread!");
(gdb) bt
#0  nsPrefService::CheckAndLogBackgroundThreadUse ()
    at /home/laga/work/firefox/source/modules/libpref/src/nsPrefService.cpp:983
#1  0x00007ffff4cd5ce5 in nsPrefBranch::GetIntPref (this=0x7fffebbee400,
    aPrefName=0x7ffff6cf4f83 "toolkit.storage.synchronous", _retval=0x7fffd23fe64c)
    at /home/laga/work/firefox/source/modules/libpref/src/nsPrefBranch.cpp:235
#2  0x00007ffff4cde999 in nsPrefService::GetIntPref (this=0x7fffe6861920,
    aPrefName=0x7ffff6cf4f83 "toolkit.storage.synchronous", _retval=0x7fffd23fe64c)
    at /home/laga/work/firefox/source/modules/libpref/src/nsPrefService.h:62
#3  0x00007ffff5d9081f in mozilla::storage::Connection::initialize (this=0x7fffd26ede40,
    aDatabaseFile=0x7fffd26eda80, aVFSName=0x0)
    at /home/laga/work/firefox/source/storage/src/mozStorageConnection.cpp:474
#4  0x00007ffff5d8df2c in mozilla::storage::Service::OpenDatabase (this=0x7fffe3283e20,
    aDatabaseFile=0x7fffd26eda80, _connection=0x7fffd23fe800)
    at /home/laga/work/firefox/source/storage/src/mozStorageService.cpp:454
#5  0x00007ffff5c246c6 in nsUrlClassifierDBServiceWorker::OpenDb (this=0x7fffd354a000)
    at /home/laga/work/firefox/source/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp:3391
#6  0x00007ffff5c22b02 in nsUrlClassifierDBServiceWorker::BeginUpdate (this=0x7fffd354a000,
    observer=0x7fffd26e17c0, tables=..., clientKey=...)
    at /home/laga/work/firefox/source/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp:2921
#7  0x00007ffff62660a6 in NS_InvokeByIndex_P (that=0x7fffd354a000, methodIndex=6,
    paramCount=3, params=0x7fffd2446470)
    at /home/laga/work/firefox/source/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:208
#8  0x00007ffff625152b in nsProxyObjectCallInfo::Run (this=0x7fffd24464c0)
    at /home/laga/work/firefox/source/xpcom/proxy/src/nsProxyEvent.cpp:181
#9  0x00007ffff62466f2 in nsThread::ProcessNextEvent (this=0x7fffd26ed9c0, mayWait=1,
    result=0x7fffd23fed7c) at /home/laga/work/firefox/source/xpcom/threads/nsThread.cpp:633
#10 0x00007ffff61d0178 in NS_ProcessNextEvent_P (thread=0x7fffd26ed9c0, mayWait=1)
    at nsThreadUtils.cpp:250
#11 0x00007ffff62456bd in nsThread::ThreadFunc (arg=0x7fffd26ed9c0)
    at /home/laga/work/firefox/source/xpcom/threads/nsThread.cpp:278
#12 0x00007ffff397998f in _pt_root (arg=0x7fffd3aef9b0)
    at /home/laga/work/firefox/source/nsprpub/pr/src/pthreads/ptthread.c:187
#13 0x00007ffff7bc8cb0 in start_thread () from /lib/libpthread.so.0
#14 0x00007fffefe2b9dd in clone () from /lib/libc.so.6
#15 0x0000000000000000 in ?? ()




#0  nsPrefService::CheckAndLogBackgroundThreadUse ()
    at /home/laga/work/firefox/source/modules/libpref/src/nsPrefService.cpp:983
#1  0x00007ffff4cd5ce5 in nsPrefBranch::GetIntPref (this=0x7fffebbee400,
    aPrefName=0x7ffff6cf4f83 "toolkit.storage.synchronous", _retval=0x7fffd23fe62c)
    at /home/laga/work/firefox/source/modules/libpref/src/nsPrefBranch.cpp:235
#2  0x00007ffff4cde999 in nsPrefService::GetIntPref (this=0x7fffe6861920,
    aPrefName=0x7ffff6cf4f83 "toolkit.storage.synchronous", _retval=0x7fffd23fe62c)
    at /home/laga/work/firefox/source/modules/libpref/src/nsPrefService.h:62
#3  0x00007ffff5d9081f in mozilla::storage::Connection::initialize (this=0x7fffd26ede40,
    aDatabaseFile=0x7fffd26eda80, aVFSName=0x0)
    at /home/laga/work/firefox/source/storage/src/mozStorageConnection.cpp:474
#4  0x00007ffff5d8df2c in mozilla::storage::Service::OpenDatabase (this=0x7fffe3283e20,
    aDatabaseFile=0x7fffd26eda80, _connection=0x7fffd23fe7e0)
    at /home/laga/work/firefox/source/storage/src/mozStorageService.cpp:454
#5  0x00007ffff5c246c6 in nsUrlClassifierDBServiceWorker::OpenDb (this=0x7fffd354a000)
    at /home/laga/work/firefox/source/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp:3391
#6  0x00007ffff5c23bb6 in nsUrlClassifierDBServiceWorker::ApplyUpdate (this=0x7fffd354a000)
    at /home/laga/work/firefox/source/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp:3210
#7  0x00007ffff5c23cf5 in nsUrlClassifierDBServiceWorker::FinishUpdate (this=0x7fffd354a000)
    at /home/laga/work/firefox/source/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp:3234
#8  0x00007ffff62660a6 in NS_InvokeByIndex_P (that=0x7fffd354a000, methodIndex=10,
    paramCount=0, params=0x0)
    at /home/laga/work/firefox/source/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:208
#9  0x00007ffff625152b in nsProxyObjectCallInfo::Run (this=0x7fffd24466f0)
    at /home/laga/work/firefox/source/xpcom/proxy/src/nsProxyEvent.cpp:181
#10 0x00007ffff62466f2 in nsThread::ProcessNextEvent (this=0x7fffd26ed9c0, mayWait=1,
    result=0x7fffd23fed7c) at /home/laga/work/firefox/source/xpcom/threads/nsThread.cpp:633
#11 0x00007ffff61d0178 in NS_ProcessNextEvent_P (thread=0x7fffd26ed9c0, mayWait=1)
    at nsThreadUtils.cpp:250
#12 0x00007ffff62456bd in nsThread::ThreadFunc (arg=0x7fffd26ed9c0)
    at /home/laga/work/firefox/source/xpcom/threads/nsThread.cpp:278
#13 0x00007ffff397998f in _pt_root (arg=0x7fffd3aef9b0)
    at /home/laga/work/firefox/source/nsprpub/pr/src/pthreads/ptthread.c:187
#14 0x00007ffff7bc8cb0 in start_thread () from /lib/libpthread.so.0
#15 0x00007fffefe2b9dd in clone () from /lib/libc.so.6
#16 0x0000000000000000 in ?? ()
Comment 43 User image Shawn Wilsher :sdwilsh 2011-01-10 10:12:13 PST
(In reply to comment #42)
> FWIW, this bug is now causing a Warning with the following stack. It should be
> solved by the Storage 1.0 patch.
That's exactly why this bug was filed...
Comment 44 User image Shawn Wilsher :sdwilsh 2011-02-04 10:16:31 PST
(In reply to comment #33)
> Not a blocker any more because we added thread checks to prefservice which
> prevents the crashes/corruption. We should still fix this because you'll get
> asserts (and you won't get the prefs you expect anyway).
Except that that didn't actually stick, and was spun off into bug 619487.  I still think we should softblock on this.
Comment 45 User image Shawn Wilsher :sdwilsh 2011-02-04 10:17:54 PST
Created attachment 509804 [details] [diff] [review]
Storage v1.1

Unbitrotting attachment 460943 [details] [diff] [review].  This now passes tests locally.
Comment 46 User image Shawn Wilsher :sdwilsh 2011-02-04 10:25:32 PST
Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=9b053ab2d5dd
Comment 47 User image Shawn Wilsher :sdwilsh 2011-02-04 13:10:39 PST
http://hg.mozilla.org/mozilla-central/rev/16734797ece9

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