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

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Roland Mainz]

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

Comment 1

[Roland Mainz]

Attachment: Patch for 0.9.7 Branch

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.

Comment 2

[Roland Mainz]

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

Comment 3

[Roland Mainz]

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 ...

Comment 4

[timeless]

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

Comment 5

[Vinay Badami]

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.

Comment 6

[Vinay Badami]

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.

Comment 7

[Vinay Badami]

Attachment: to delay creation of the timer

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.

Comment 8

[Darin Fisher]

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 9

[Darin Fisher]

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

Comment 10

[Vinay Badami]

Attachment: incorporating darins comments

incorporating darins comments

Comment 11

[Roland Mainz]

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 12

[Christopher Blizzard (:blizzard)]

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 13

[Darin Fisher]

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

r=darin

Comment 14

[Stuart Parmenter]

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

Comment 15

[Roland Mainz]

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 16

[timeless]

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

Comment 17

[Asa Dotzler [:asa]]

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

Comment 18

[Roland Mainz]

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

Comment 19

[Roland Mainz]

Marking FIXED (asked darin via IRC) ...