Closed
Bug 114858
Opened 23 years ago
Closed 23 years ago
[Xlib] Xlib toolkit broken because timer gets created before nsAppShell is initalised
Categories
(Core :: XUL, defect)
Tracking
()
RESOLVED
FIXED
mozilla0.9.7
People
(Reporter: roland.mainz, Assigned: roland.mainz)
References
Details
(Keywords: regression)
Attachments
(3 files)
3.52 KB,
patch
|
timeless
:
review+
blizzard
:
superreview+
blizzard
:
approval+
|
Details | Diff | Splinter Review |
1.90 KB,
patch
|
Details | Diff | Splinter Review | |
1.73 KB,
patch
|
darin.moz
:
review+
|
Details | Diff | Splinter Review |
Xlib toolkit is broken since 2001-12-09-08-trunk because someone creates a timer
during XPCOM registration.
Assignee | ||
Updated•23 years ago
|
Assignee | ||
Comment 1•23 years ago
|
||
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+
Assignee | ||
Comment 2•23 years ago
|
||
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 --
Assignee | ||
Comment 3•23 years ago
|
||
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 ...
Keywords: regression
shaver of course responded that timers shouldn't be created at registration
time.
Comment 5•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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
Attachment #61582 -
Flags: needs-work+
Comment 10•23 years ago
|
||
incorporating darins comments
Assignee | ||
Comment 11•23 years ago
|
||
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•23 years ago
|
||
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+
Updated•23 years ago
|
Keywords: mozilla0.9.7+
Comment 13•23 years ago
|
||
Comment on attachment 61717 [details] [diff] [review]
incorporating darins comments
r=darin
Attachment #61717 -
Flags: review+
Comment 14•23 years ago
|
||
unless you want this for 0.9.7, i suggest marking it invalid now that 78611 has
landed.
Assignee | ||
Comment 15•23 years ago
|
||
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•23 years ago
|
||
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
Attachment #61437 -
Attachment description: Patch for 2001-12-09-08-trunk → Patch for 0.9.7 Branch
Updated•23 years ago
|
Keywords: fixed0.9.7
Comment 17•23 years ago
|
||
a=asa (on behalf of drivers) for checkin to 0.9.7
Assignee | ||
Comment 18•23 years ago
|
||
badami/darin:
Can I mark this bug as FIXED ?
Assignee | ||
Comment 19•23 years ago
|
||
Marking FIXED (asked darin via IRC) ...
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•