Michal Novotny [:michal] Reporter
	Description • 14 years ago

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

	Shawn Wilsher :sdwilsh Assignee
	Comment 1 • 14 years ago

I regressed this back in 1.9.2 I think...

	Johnny Stenback (:jst)
	Comment 2 • 14 years ago

Marking a betaN blocker since this blocks betaN blocker bug 513008.

Shawn, got an ETA for this?

	Shawn Wilsher :sdwilsh 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.

	Shawn Wilsher :sdwilsh 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.

	Shawn Wilsher :sdwilsh 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

	Shawn Wilsher :sdwilsh Assignee
	Comment 7 • 14 years ago

In fact, I'll do just that.  Pushed to the try server (changeset 1205a4afde7e).

	Andrew Sutherland [:asuth] (he/him)
	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".

	Shawn Wilsher :sdwilsh 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.

	Andrew Sutherland [:asuth] (he/him)
	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).

	Shawn Wilsher :sdwilsh Assignee
	Comment 11 • 14 years ago

Try hated that one and did not build a thing.  I pushed it again in changeset
b74c2c7188ff

	Shawn Wilsher :sdwilsh 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.

	Shawn Wilsher :sdwilsh 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.

	Andrew Sutherland [:asuth] (he/him)
	Comment 14 • 14 years ago

Comment on attachment 459845 [details] [diff] [review]
v1.2

I like.

	Shawn Wilsher :sdwilsh Assignee
	Comment 15 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/f9e77a6f0adc