Closed Bug 114858 Opened 22 years ago Closed 22 years ago

[Xlib] Xlib toolkit broken because timer gets created before nsAppShell is initalised


(Core :: XUL, defect)

Not set





(Reporter: roland.mainz, Assigned: roland.mainz)



(Keywords: regression)


(3 files)

Xlib toolkit is broken since 2001-12-09-08-trunk because someone creates a timer
during XPCOM registration.
Blocks: 79119
Target Milestone: --- → mozilla0.9.7
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.
Attachment #61437 - Flags: review+
Blocks: 114818
Per timeless's request:
Output of regxpcom failure:
-- snip --
% ./ ./regxpcom 
Type Manifest File: /home/mozilla/xlib/objdir/dist/bin/components/xpti.dat
nsNativeComponentLoader: autoregistering begins.
*** Registering nsSampleModule components (all right -- a generic module!)
*** 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
###!!! Break: at file
../../../../../../src/mozilla/widget/timer/src/unix/xlib/nsTimerXlib.cpp, line
Error: Couldn't find per display information
-- snip --

Stack trace of the call to nsTimerXlib::Init() (I placed an abort() in that
-- 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
dbx: warning: can't find file
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
  [10] nsGenericFactory::CreateInstance(0x146928, 0x0, 0xfdbd1210, 0xffbee618,
0xff345348, 0xff25b650), at 0xff25b674
dbx: warning: can't find file
  [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
  [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
  [31] mozJSComponentLoader::AttemptRegistration(this = ???, component = ???,
deferred = ???) (optimized), at 0xfe0e8b94 (line ~828) in
  [32] mozJSComponentLoader::AutoRegisterComponent(this = ???, when = ???,
component = ???, registered = ???) (optimized), at 0xfe0e88bc (line ~758) in
  [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
  [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
-- 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(";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, 
-- 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 ...
Keywords: regression
shaver of course responded that timers shouldn't be created at registration 
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(";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:
Attachment #61582 - Flags: needs-work+
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

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
Attachment #61437 - Flags: superreview+
Attachment #61437 - Flags: approval+
Comment on attachment 61717 [details] [diff] [review]
incorporating darins comments

Attachment #61717 - Flags: review+
unless you want this for 0.9.7, i suggest marking it invalid now that 78611 has
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  <-- 
new revision:; previous revision: 1.17
Checking in nsTimerXlib.h;
/cvsroot/mozilla/widget/timer/src/unix/xlib/Attic/nsTimerXlib.h,v  <-- 
new revision:; previous revision: 1.9
Attachment #61437 - Attachment description: Patch for 2001-12-09-08-trunk → Patch for 0.9.7 Branch
a=asa (on behalf of drivers) for checkin to 0.9.7
No longer blocks: 114455
Can I mark this bug as FIXED ?
Marking FIXED (asked darin via IRC) ...
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.