Xlib toolkit is broken since 2001-12-09-08-trunk because someone creates a timer during XPCOM registration.

The patch simple splits up the Xlib timer initalisation into two parts: 1. Create timer queues 2. Register Xt timer callback Step 1 is always done, step 2 (which fails in current code due lack of a XtAppContext - which is only available after a nsAppShell object has been created) is deferred until we get a valid XtAppContext.

Per timeless's request: Output of regxpcom failure: -- snip -- % ./run-mozilla.sh ./regxpcom Type Manifest File: /home/mozilla/xlib/objdir/dist/bin/components/xpti.dat nsNativeComponentLoader: autoregistering begins. *** Registering nsSampleModule components (all right -- a generic module!) [snip] *** Registering MyService components (all right -- a generic module!) nsNativeComponentLoader: autoregistering succeeded xxlib_find_handle: 'xxlib-default' entry 0 ###!!! ASSERTION: xxlib_rgb_get_display() returned nsnull display. BAD.: 'mDisplay!=nsnull', file ../../../../../../src/mozilla/widget/timer/src/unix/xlib/nsTimerXlib.cpp, line 151 ###!!! Break: at file ../../../../../../src/mozilla/widget/timer/src/unix/xlib/nsTimerXlib.cpp, line 151 Error: Couldn't find per display information -- snip -- Stack trace of the call to nsTimerXlib::Init() (I placed an abort() in that function): -- snip -- [5] abort(0xfefb5d58, 0x0, 0x10, 0x0, 0x3c, 0x0), at 0xfef395b8 =>[6] nsTimerXlib::Init(this = ???, aDelay = ???, aPriority = ???) (optimized), at 0xfdfe1f74 (line ~203) in "nsTimerXlib.cpp" [7] nsTimerXlib::Init(this = ???, aFunc = ???, aClosure = ???, aDelay = ???, aPriority = ???, aType = ???) (optimized), at 0xfdfe1b00 (line ~109) in "nsTimerXlib.cpp" dbx: warning: can't find file "/home/mozilla/xlib/objdir/netwerk/build/nsHttpHandler.o" dbx: warning: see `help finding-files' [8] nsHttpHandler::Init(0x917d8, 0x0, 0xffbee470, 0x80070000, 0xfdba0108, 0x94700), at 0xfdae37d8 [9] nsHttpHandler::Create(0x80000000, 0xfdbd1210, 0xffbee618, 0xfdae3048, 0xe6490, 0x0), at 0xfdae30c4 dbx: warning: can't find file "/home/mozilla/xlib/objdir/xpcom/build/nsGenericFactory.o" [10] nsGenericFactory::CreateInstance(0x146928, 0x0, 0xfdbd1210, 0xffbee618, 0xff345348, 0xff25b650), at 0xff25b674 dbx: warning: can't find file "/home/mozilla/xlib/objdir/xpcom/build/nsComponentManager.o" [11] nsComponentManagerImpl::CreateInstanceByContractID(0x2a9b0, 0xffbee710, 0x0, 0xfdbd1210, 0xffbee618, 0xff3450fc), at 0xff25303c [12] nsComponentManagerImpl::GetServiceByContractID(0x2a9b0, 0xffbee710, 0xfdbd1210, 0xffbee7cc, 0x47c, 0xfdbc8768), at 0xff253d64 dbx: warning: can't find file "/home/mozilla/xlib/objdir/netwerk/build/nsIOService.o" [13] CallGetService<nsIProtocolHandler>(0xffbee710, 0xffbee7cc, 0xffbee7cc, 0x0, 0x0, 0x15d9e4), at 0xfda6e468 [14] nsIOService::GetProtocolHandler(0x9c0c8, 0x8fae8, 0xffbee7cc, 0x0, 0x3eb28, 0xfdbd1148), at 0xfda6b1f0 [15] nsIOService::NewURI(0x9c0c8, 0x81960, 0x0, 0xffbee8b4, 0xfdbd1148, 0xfda6b26c), at 0xfda6c34c [16] NS_NewURI(result = ???, spec = ???, baseURI = ???, ioService = ???) (optimized), at 0xfd85ee44 (line ~97) in "nsNetUtil.h" [17] nsCodebasePrincipal::InitFromPersistent(this = ???, aPrefName = ???, aURLStr = ???, aGrantedList = ???, aDeniedList = ???, aTrusted = ???) (optimized), at 0xfd85ec10 (line ~301) in "nsCodebasePrincipal.cpp" [18] nsScriptSecurityManager::InitPrincipals(this = ???, aPrefCount = ???, aPrefNames = ???, aSecurityPref = ???) (optimized), at 0xfd86b39c (line ~2476) in "nsScriptSecurityManager.cpp" [19] nsScriptSecurityManager::InitPrefs(this = ???) (optimized), at 0xfd86ba8c (line ~2538) in "nsScriptSecurityManager.cpp" [20] nsScriptSecurityManager::nsScriptSecurityManager(this = ???) (optimized), at 0xfd86a544 (line ~2116) in "nsScriptSecurityManager.cpp" [21] nsScriptSecurityManager::GetScriptSecurityManager() (optimized), at 0xfd86a894 (line ~2154) in "nsScriptSecurityManager.cpp" [22] Construct_nsIScriptSecurityManager(aOuter = ???, aIID = STRUCT, aResult = ???) (optimized), at 0xfd8731b4 (line ~337) in "nsSecurityManagerFactory.cpp" [23] nsGenericFactory::CreateInstance(0x7a580, 0x0, 0xfe1082d4, 0xffbeecc0, 0xff345348, 0xff25b650), at 0xff25b674 [24] nsComponentManagerImpl::CreateInstanceByContractID(0x2a9b0, 0xfe107e6b, 0x0, 0xfe1082d4, 0xffbeecc0, 0xff3450fc), at 0xff25303c [25] nsComponentManagerImpl::GetServiceByContractID(0x2a9b0, 0xfe107e6b, 0xfe1082d4, 0xffbeeda4, 0xff25a7c8, 0x770), at 0xff253d64 [26] nsGetServiceByContractID::operator()(0xffbeee70, 0xfe1082d4, 0xffbeeda4, 0xff24fcc0, 0x22a8, 0x7b8a0), at 0xff24fd60 dbx: warning: can't find file "/home/mozilla/xlib/objdir/xpcom/build/nsCOMPtr.o" [27] nsCOMPtr_base::assign_from_helper(0xffbeee80, 0xffbeee70, 0xfe1082d4, 0x0, 0x2278, 0x30a10), at 0xff2c7538 [28] nsCOMPtr<nsIScriptSecurityManager>::nsCOMPtr(this = ???, helper = CLASS) (optimized), at 0xfe0eb8a0 (line ~555) in "nsCOMPtr.h" [29] mozJSComponentLoader::ReallyInit(this = ???) (optimized), at 0xfe0e7ed8 (line ~512) in "mozJSComponentLoader.cpp" [30] mozJSComponentLoader::ModuleForLocation(this = ???, registryLocation = ???, component = ???) (optimized), at 0xfe0e9210 (line ~979) in "mozJSComponentLoader.cpp" [31] mozJSComponentLoader::AttemptRegistration(this = ???, component = ???, deferred = ???) (optimized), at 0xfe0e8b94 (line ~828) in "mozJSComponentLoader.cpp" [32] mozJSComponentLoader::AutoRegisterComponent(this = ???, when = ???, component = ???, registered = ???) (optimized), at 0xfe0e88bc (line ~758) in "mozJSComponentLoader.cpp" [33] mozJSComponentLoader::RegisterComponentsInDir(this = ???, when = ???, dir = ???) (optimized), at 0xfe0e8288 (line ~588) in "mozJSComponentLoader.cpp" [34] mozJSComponentLoader::AutoRegisterComponents(this = ???, when = ???, aDirectory = ???) (optimized), at 0xfe0e8104 (line ~544) in "mozJSComponentLoader.cpp" [35] nsComponentManagerImpl::AutoRegisterImpl(0x2a9b0, 0x0, 0xb3c28, 0x1, 0xffbef320, 0xffbef324), at 0xff255d28 [36] nsComponentManagerImpl::AutoRegister(0x2a9b0, 0x0, 0x0, 0x0, 0xffbef354, 0x80000000), at 0xff25568c [37] nsComponentManager::AutoRegister(0x0, 0x0, 0xff340a98, 0x2a9b0, 0xff338e8c, 0x80000000), at 0xff257478 [38] main(argc = ???, argv = ???) (optimized), at 0x11628 (line ~191) in "regxpcom.cpp" -- snip --

It looks that this is regression from bug 97833. Commenting out this part of attachment 60119 [details] [diff] [review] of bug 97833 fixed the issue, too: -- snip -- + mTimer = do_CreateInstance("@mozilla.org/timer;1"); + // If we were unable to create mTimer, we will still work. + // But connections are not aggressively reaped. + if (mTimer) { + // Reap once a minute + mTimer->Init(DeadConnectionCleanupCB, this, 60*1000, + NS_PRIORITY_NORMAL, NS_TYPE_REPEATING_SLACK); } -- snip -- timeless's question is valid here: "Why do we need to create a timer during regxpcom registration..." ?! Anyway, the fix attached to this bug ...

shaver of course responded that timers shouldn't be created at registration time.

Is this stuff documented somewhere ? If I'am not allowed to create a timer at registration then is it possible to fail my registration, abort or something to that effect to let me know that I'am doing something wrong ? The patch may just fix all issues, but I do not see how one is supposed to know about this. It certainly was not evident to me. Either way, sorry for the foo I caused.

The timer was being used later on to clean up http connections by registering a callback function. It can be created later too. It needs to be created at a time/place before the first http request is handled. I will start looking into it now.

If it is deemed necessary that the time creation be deferred, then it can be done under the lock while trying to process the first http request. We remember that we tried to create the timer once and do not try again if the first attempt failed for whatever reason.

so, we have two patches here. roland: do you want your patch? or should we just fix this by landing vinay's patch (which is good to have anyways)?

Comment on attachment 61582 [details] [diff] [review] to delay creation of the timer >Index: nsHttpHandler.cpp >+ , mTimer(nsnull) no reason to explicitly initialize a nsCOMPtr >+static PRBool timerInitTried = PR_FALSE; this variable should be declared within InitiateTransaction > PR_Lock(mConnectionLock); >+ >+ // Create timer once >+ if (!mTimer && (timerInitTried == PR_FALSE)) { >+ timerInitTried = PR_TRUE; >+ mTimer = do_CreateInstance("@mozilla.org/timer;1"); >+ // failure to create a timer is not a fatal error, but idle connections >+ // may not be cleaned up as aggressively. >+ if (mTimer) >+ mTimer->Init(DeadConnectionCleanupCB, this, 15*1000, // 15 seconds >+ NS_PRIORITY_NORMAL, >+ NS_TYPE_REPEATING_SLACK); >+ } indentation problems! otherwise, r/sr=darin ... vinay: you should really look for another sr to review your patch. this is important because you'll need 3 sr's to vouch for you before you get CVS access. check out this document for a list of sr's: http://mozilla.org/hacking/reviewers.html

incorporating darins comments

Darin Fisher wrote: > so, we have two patches here. roland: do you want your patch? or should we > just fix this by landing vinay's patch (which is good to have anyways)? It wise to landing my patch, too - otherwise the next checkin may bust Xlib toolkit again (however, this is only important for 0.9.7 as pavlov is landing his XP timer code soon which will make the toolkit-specific timer code obsolete). Requesting a= for attachment 61437 [details] [diff] [review], please ...

Comment on attachment 61437 [details] [diff] [review] Patch for 0.9.7 Branch sr/a=blizzard on behalf of drivers for 0.9.7

Comment on attachment 61717 [details] [diff] [review] incorporating darins comments r=darin

unless you want this for 0.9.7, i suggest marking it invalid now that 78611 has landed.

Stuart Parmenter wrote: > unless you want this for 0.9.7, i suggest marking it invalid now that 78611 > has landed. Note that we have two patches here (one for Xlib timer code which fails to initalise and a patch for the http transport which causes this issue) as we like to fix this bug on both ends. ---- CC:'ing mkaply for checkin attachment 61437 [details] [diff] [review] into the 0.9.7-branch...

Comment on attachment 61437 [details] [diff] [review] Patch for 0.9.7 Branch Checking in nsTimerXlib.cpp; /cvsroot/mozilla/widget/timer/src/unix/xlib/Attic/nsTimerXlib.cpp,v <-- nsTime rXlib.cpp new revision: 1.17.6.1; previous revision: 1.17 done Checking in nsTimerXlib.h; /cvsroot/mozilla/widget/timer/src/unix/xlib/Attic/nsTimerXlib.h,v <-- nsTimerX lib.h new revision: 1.9.6.1; previous revision: 1.9 done

a=asa (on behalf of drivers) for checkin to 0.9.7

badami/darin: Can I mark this bug as FIXED ?

Marking FIXED (asked darin via IRC) ...