Connection::initialize can access preferences off of the main thread

RESOLVED FIXED in mozilla2.0b12

Status

()

Toolkit
Storage
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: sdwilsh)

Tracking

({assertion})

Trunk
mozilla2.0b12
x86
Mac OS X
assertion
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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
Sounds like a storage bug to me; neither prefs nor pldhash claimed to be threadsafe when I last checked.
(Reporter)

Updated

7 years ago
Component: General → Storage
Product: Core → Toolkit
QA Contact: general → storage
Sure looks like it, unless calling mozilla::storage::Service::OpenDatabase is supposed to be a main-thread-only thing.
(Assignee)

Comment 3

7 years ago
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?
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Seems like it could cause pref crashes that won't show up obviously in crashreporter signatures, so yes.
blocking2.0: --- → final+
Sounds like we should add mainthread assertions to the prefservice too.
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?
I think that there are people who use XPCOM proxies with it still, so probably not for this cycle.
(Assignee)

Comment 8

7 years ago
And need bug 578939 (already a blocker) to do this sanely...
Depends on: 578939
(Assignee)

Updated

7 years ago
Summary: pldhash RECURSION_LEVEL assertion failures (due to Storage using Prefs on the wrong thread?) → Connection::initialize can access preferences off of the main thread
(Assignee)

Comment 9

7 years ago
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.
Attachment #460929 - Flags: superreview?(benjamin)
Attachment #460929 - Flags: review?(dwitte)
(Assignee)

Comment 10

7 years ago
Created attachment 460942 [details] [diff] [review]
Storage v1.0

And now the storage bits.  Pretty self explanatory.
Attachment #460942 - Flags: review?(bugmail)
(Assignee)

Updated

7 years ago
Whiteboard: [needs review dwitte][needs review asuth][needs sr bsmedberg]
(Assignee)

Comment 11

7 years ago
Created attachment 460943 [details] [diff] [review]
Storage v1.0

It helps when one saves before qrefreshing...
Attachment #460942 - Attachment is obsolete: true
Attachment #460943 - Flags: review?(bugmail)
Attachment #460942 - Flags: review?(bugmail)
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.
Attachment #460943 - Flags: superreview?
Attachment #460943 - Flags: review?(bugmail)
Attachment #460943 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #460943 - Flags: superreview? → superreview?(shaver)
(Assignee)

Comment 13

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

Updated

7 years ago
Whiteboard: [needs review dwitte][needs review asuth][needs sr bsmedberg] → [needs review dwitte][needs sr bsmedberg][needs sr shaver]
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.
(Assignee)

Comment 15

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

Comment 16

7 years ago
(fwiw, I don't think, because of places, that the service will ever be initialized off of the main thread in the browser)
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 on attachment 460943 [details] [diff] [review]
Storage v1.0

sr=shaver, thanks.
Attachment #460943 - Flags: superreview?(shaver) → superreview+
(Assignee)

Comment 19

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

Updated

7 years ago
Whiteboard: [needs review dwitte][needs sr bsmedberg][needs sr shaver] → [needs review dwitte][needs sr bsmedberg]

Comment 20

7 years ago
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.
Attachment #460929 - Flags: review?(dwitte) → review+
(Assignee)

Comment 21

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

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

Comment 23

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

7 years ago
(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!
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

7 years ago
Ok, r=me then!
Attachment #460929 - Flags: superreview?(benjamin) → superreview+
(Assignee)

Updated

7 years ago
Whiteboard: [needs review dwitte][needs sr bsmedberg] → [needs new patch]
(Assignee)

Comment 27

7 years ago
(In reply to comment #24)
> Please wrap it in NS_LIKELY.
Presume you mean NS_UNLIKELY.  New patch RSN, please take a look.
(Assignee)

Comment 28

7 years ago
Created attachment 463256 [details] [diff] [review]
Prefs v1.1
Attachment #460929 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Whiteboard: [needs new patch] → [can land]

Updated

7 years ago
Attachment #463256 - Flags: review+
(Assignee)

Comment 29

7 years ago
The prefs changed landed, but I had a compiler issue with the storage part:
http://hg.mozilla.org/mozilla-central/rev/1302220623fd
(Assignee)

Comment 30

7 years ago
Except that I just backed that out too since I realized the pref changes by themselves will make Bd go orange.  *sigh*
(Assignee)

Updated

7 years ago
Depends on: 585706
(Assignee)

Updated

7 years ago
Whiteboard: [can land] → [needs bug 585706]
(Assignee)

Updated

7 years ago
Depends on: 593046
(Assignee)

Updated

7 years ago
Whiteboard: [needs bug 585706] → [needs bug 585706, bug 593046]
(Assignee)

Comment 31

7 years ago
Based on try server results, bug 585706 and bug 593046 are the only two things blocking this from landing.
(Assignee)

Updated

7 years ago
Whiteboard: [needs bug 585706, bug 593046] → [needs bug 585706]
(Assignee)

Comment 32

7 years ago
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).
blocking2.0: final+ → betaN+
(Assignee)

Updated

7 years ago
Blocks: 616019
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).
blocking2.0: betaN+ → -
(Assignee)

Comment 34

7 years ago
Preferences code spun off to bug 619487.
No longer depends on: 585706
(Assignee)

Updated

7 years ago
Depends on: 585706
(Assignee)

Updated

7 years ago
No longer depends on: 585706
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).
(Assignee)

Comment 36

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

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

7 years ago
Try setting a breakpoint in a debug build?

Comment 39

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

Comment 40

7 years ago
The patch that everyone is complaining about is actually bug 619487.  Please take the discussion there and stop polluting this bug.

Updated

7 years ago
Depends on: 624315
(Assignee)

Updated

7 years ago
Duplicate of this bug: 624300
(Assignee)

Updated

7 years ago
No longer depends on: 624315

Comment 42

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

Comment 43

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

Updated

7 years ago
Blocks: 624490
(Assignee)

Updated

7 years ago
No longer blocks: 624490
(Assignee)

Comment 44

7 years ago
(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.
blocking2.0: - → ?
Whiteboard: [needs bug 585706]
(Assignee)

Comment 45

7 years ago
Created attachment 509804 [details] [diff] [review]
Storage v1.1

Unbitrotting attachment 460943 [details] [diff] [review].  This now passes tests locally.
Attachment #460943 - Attachment is obsolete: true
Attachment #463256 - Attachment is obsolete: true
blocking2.0: ? → final+
Whiteboard: [softblocker]
(Assignee)

Comment 46

7 years ago
Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=9b053ab2d5dd
(Assignee)

Comment 47

7 years ago
http://hg.mozilla.org/mozilla-central/rev/16734797ece9
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
You need to log in before you can comment on or make changes to this bug.