Closed
Bug 578939
Opened 14 years ago
Closed 14 years ago
Storage Service can't be initialized off the main thread
Categories
(Core :: SQLite and Embedded Database Bindings, defect)
Core
SQLite and Embedded Database Bindings
Tracking
()
RESOLVED
FIXED
mozilla2.0b3
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: michal, Assigned: sdwilsh)
References
Details
Attachments
(1 file, 3 obsolete files)
9.47 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
backtrace of the assertion: #3 0x0021a014 in mozalloc_abort (msg= 0xf5dfe5ec "###!!! ASSERTION: Using observer service off the main thread!: 'Error', file /opt/moz/hg/src/xpcom/ds/nsObserverService.cpp, line 128") at /opt/moz/hg/src/memory/mozalloc/mozalloc_abort.cpp:75 #4 0x0229aaee in Abort (aMsg=0xf5dfe5ec "###!!! ASSERTION: Using observer service off the main thread!: 'Error', file /opt/moz/hg/src/xpcom/ds/nsObserverService.cpp, line 128") at /opt/moz/hg/src/xpcom/base/nsDebugImpl.cpp:379 #5 0x0229aab1 in NS_DebugBreak_P (aSeverity=1, aStr=0x2929290 "Using observer service off the main thread!", aExpr=0x2929287 "Error", aFile= 0x2929188 "/opt/moz/hg/src/xpcom/ds/nsObserverService.cpp", aLine=128) at /opt/moz/hg/src/xpcom/base/nsDebugImpl.cpp:366 #6 0x0223ee7c in nsObserverService::AddObserver (this=0x8a30ce8, anObserver=0xf5200be4, aTopic=0x286de48 "xpcom-shutdown", ownsWeak=0) at /opt/moz/hg/src/xpcom/ds/nsObserverService.cpp:128 #7 0x01f19143 in mozilla::storage::Service::initialize (this=0xf5200be0) at /opt/moz/hg/src/storage/src/mozStorageService.cpp:189 #8 0x01f18cec in mozilla::storage::Service::getSingleton () at /opt/moz/hg/src/storage/src/mozStorageService.cpp:114 #9 0x01f18512 in mozilla::storage::ServiceConstructor (aOuter=0x0, aIID=..., aResult=0xf5dfecc0) at /opt/moz/hg/src/storage/build/mozStorageModule.cpp:53 #10 0x0222b110 in nsGenericFactory::CreateInstance (this=0xf5200bc8, aOuter=0x0, aIID=..., aResult=0xf5dfecc0) at nsGenericFactory.cpp:80 #11 0x022805d8 in nsComponentManagerImpl::CreateInstanceByContractID (this=0x893ef88, aContractID=0x257bde0 "@mozilla.org/storage/service;1", aDelegate=0x0, aIID=..., aResult= 0xf5dfecc0) at /opt/moz/hg/src/xpcom/components/nsComponentManager.cpp:1725 #12 0x02281961 in nsComponentManagerImpl::GetServiceByContractID (this=0x893ef88, aContractID=0x257bde0 "@mozilla.org/storage/service;1", aIID=..., result=0xf5dfedac) at /opt/moz/hg/src/xpcom/components/nsComponentManager.cpp:2310 #13 0x0221e536 in CallGetService (aContractID=0x257bde0 "@mozilla.org/storage/service;1", aIID=..., aResult=0xf5dfedac) at nsComponentManagerUtils.cpp:94 #14 0x0221ea6a in nsGetServiceByContractIDWithError::operator() (this=0xf5dfeedc, aIID=..., aInstancePtr=0xf5dfedac) at nsComponentManagerUtils.cpp:288 #15 0x0103b726 in nsCOMPtr<mozIStorageService>::assign_from_gs_contractid_with_error (this=0xf5dfeec0, gs=..., aIID=...) at ../../dist/include/nsCOMPtr.h:1239 #16 0x0103a759 in nsCOMPtr<mozIStorageService>::nsCOMPtr (this=0xf5dfeec0, gs=...) at ../../dist/include/nsCOMPtr.h:612 #17 0x01031837 in nsOfflineCacheDevice::Init (this=0xf5200780) at /opt/moz/hg/src/netwerk/cache/nsDiskCacheDeviceSQL.cpp:980 #18 0x0101a73a in nsCacheService::CreateOfflineDevice (this=0x8fd51f8) at /opt/moz/hg/src/netwerk/cache/nsCacheService.cpp:1067
Assignee | ||
Comment 1•14 years ago
|
||
I regressed this back in 1.9.2 I think...
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Flags: in-testsuite?
Flags: in-litmus-
Comment 2•14 years ago
|
||
Marking a betaN blocker since this blocks betaN blocker bug 513008. Shawn, got an ETA for this?
blocking2.0: --- → betaN+
Assignee | ||
Comment 3•14 years ago
|
||
(In reply to comment #2) > Shawn, got an ETA for this? I was working on this on the plane on tuesday. I have a patch, but I'm hitting an assertion that I added that I don't expect to be. Need to debug it still.
Assignee | ||
Comment 4•14 years ago
|
||
Assignee | ||
Comment 5•14 years ago
|
||
Now that I have a proper debugger, I can see that it was just a test harness issue. This passes storage tests, and I've pushed this to the try server (changeset a6334ac133bc). I've also added a test that ensures that initialization works off of the main thread.
Attachment #459462 -
Attachment is obsolete: true
Attachment #459471 -
Flags: review?(bugmail)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review asuth]
Target Milestone: --- → mozilla2.0b3
Assignee | ||
Comment 6•14 years ago
|
||
So, we may want to just call Run() on the helper class if we are already on the main thread. Getting lots of test failures on the try server because we are not... http://tinderbox.mozilla.org/showlog.cgi?tree=MozillaTry&errorparser=unittest&logfile=1279820584.1279821753.21675.gz&buildtime=1279820584&buildname=Rev3%20Fedora%2012%20tryserver%20debug%20test%20xpcshell&fulltext=1
Assignee | ||
Comment 7•14 years ago
|
||
In fact, I'll do just that. Pushed to the try server (changeset 1205a4afde7e).
Attachment #459471 -
Attachment is obsolete: true
Attachment #459516 -
Flags: review?(bugmail)
Attachment #459471 -
Flags: review?(bugmail)
Comment 8•14 years ago
|
||
This looks safe but dangerous to the addition of new code. Specifically, code requesting access to the service can have a reference to the service before the initialization code has run to completion. This goes for both code off the main thread and on the main thread. (For code on the main thread, the scenario is that code off the main thread caused the service to be initialized but the event got put in the queue after some code that also wants a copy of the service.) This is fine because the things that happen in the initialization are pretty boring and not essential to the correct operation of storage. Specifically, sXPConnect is allowed to be uninitialized because getXPConnect will just fetch an (uncached) reference. But if someone adds new code... potential blammo! So, please add clear comments to the ::initialize and its helper that all new code needs to be compatible with those assumptions. It would also be friendly to explain in a comment why each piece of code needs to be run on the main thread. I think your assertion might still be dangerous to badly written tests. Namely, ShutdownXPCOM sends "xpcom-shutdown" before it ever spins the shutdown thread. A test in which a thread is spun up that does stuff with mozStorage but not sufficiently joined on/synchronized with could conceivably experience this. I'm not sure there's much you can do about this unless you can detect that XPCOM shutdown has already begun and not initialize oneself (or will the nsIXPConnect fetch have failed?), but a slightly more "shame on you" assertion phrasing might shield you from having to debug other people's oranges. For example "Storage service initialized off the main-thread without proper synchronization".
Updated•14 years ago
|
Attachment #459516 -
Flags: review?(bugmail) → review+
Updated•14 years ago
|
Whiteboard: [needs review asuth] → [needs more comments but then can land]
Assignee | ||
Comment 9•14 years ago
|
||
(In reply to comment #8) > This looks safe but dangerous to the addition of new code. Specifically, code > requesting access to the service can have a reference to the service before the > initialization code has run to completion. This goes for both code off the > main thread and on the main thread. (For code on the main thread, the scenario > is that code off the main thread caused the service to be initialized but the > event got put in the queue after some code that also wants a copy of the > service.) Yeah, I don't really know of a way to deal with this without adding some synchronization primitives for initialization (which sucks). Off main thread stuff will not be impacted by this at all since they can't use anything we have on the main thread anyway. I can do this with thread synchronization stuff if you think we need to. I'm presently still on the fence. The other alternative, of course, is to say that this must be initialized on the main thread. > This is fine because the things that happen in the initialization are pretty > boring and not essential to the correct operation of storage. Specifically, > sXPConnect is allowed to be uninitialized because getXPConnect will just fetch > an (uncached) reference. But if someone adds new code... potential blammo! off main thread stuff also should not be touching the language helpers anyway because xpconnect cannot be there. > I think your assertion might still be dangerous to badly written tests. > Namely, ShutdownXPCOM sends "xpcom-shutdown" before it ever spins the shutdown > thread. A test in which a thread is spun up that does stuff with mozStorage > but not sufficiently joined on/synchronized with could conceivably experience > this. I'm not sure there's much you can do about this unless you can detect > that XPCOM shutdown has already begun and not initialize oneself (or will the > nsIXPConnect fetch have failed?), but a slightly more "shame on you" assertion > phrasing might shield you from having to debug other people's oranges. For > example "Storage service initialized off the main-thread without proper > synchronization". You are correct. According to https://wiki.mozilla.org/XPCOM_Shutdown, xpcom-shutdown-threads happens after xpcom-shutdown, so threads could still hold a reference if they are not manually killed. But really, any test that is written is going to have to be correct because it will turn tinderbox orange.
Comment 10•14 years ago
|
||
(In reply to comment #9) > I can do this with thread synchronization stuff if > you think we need to. I'm presently still on the fence. I think it's fine as it is in terms of logic, I just like comments. > The other alternative, of course, is to say that this must be initialized on > the main thread. This also works for me; I have no real opinion myself as I use JS for everything and so have no choice (for now).
Assignee | ||
Comment 11•14 years ago
|
||
Try hated that one and did not build a thing. I pushed it again in changeset b74c2c7188ff
Assignee | ||
Comment 12•14 years ago
|
||
Try server checks out, although it only ran unit tests on win2003... I'm going to assume that that is OK. Will update the patch based on comments in a bit.
Assignee | ||
Comment 13•14 years ago
|
||
Per review comments, but I want you to take a look at the comments and make sure they are to your liking.
Attachment #459516 -
Attachment is obsolete: true
Attachment #459845 -
Flags: review?(bugmail)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs more comments but then can land] → [needs review asuth]
Comment 14•14 years ago
|
||
Comment on attachment 459845 [details] [diff] [review] v1.2 I like.
Attachment #459845 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 15•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/f9e77a6f0adc
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review asuth]
Updated•20 days ago
|
Product: Toolkit → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•