Internal timers fail, when TIMER0.SYS is in use by other apps

VERIFIED FIXED in 4.6

Status

NSPR
NSPR
VERIFIED FIXED
13 years ago
12 years ago

People

(Reporter: Rüdiger Ihle, Assigned: mkaply)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

13 years ago
The use of the OS/2 API function DosTmrQueryTime() in os2inrval.c has a side
effect. Since this API will not work correctly while another application is
using the TIMER0.SYS (the hi-res timer device driver), several timer triggered
operations (bookmark loading, automatic submenu opening) will function properly.

It was suggested to disable the use of the mentioned API by mean of an
environment variable like MOZ_NO_HIRES_TIMER=1
(Assignee)

Comment 1

13 years ago
Created attachment 170819 [details] [diff] [review]
Possible fix

Something like this should work.

Comment 2

13 years ago
Michael, your patch is missing the outermost pair of
parentheses for the if statement :-)

I would check the environment variable before calling
DosTmrQueryFreq, and declare envp with const.
Assignee: wtchang → mkaply
(Reporter)

Comment 3

13 years ago
Is it common practice to just check for the existence of the variable and not
for it's value ?

Comment 4

13 years ago
most code, e.g. the bits in nsdebugimpl checks the value.
(Assignee)

Comment 5

13 years ago
Actually, I was copying other NSPR code:

http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/malloc/prmem.c#175
(Reporter)

Comment 6

13 years ago
Hmm, they do check the value in the very next line of code. I'll vote for doing
it the same way.

(Assignee)

Comment 7

13 years ago
(In reply to comment #6)
> Hmm, they do check the value in the very next line of code. I'll vote for doing
> it the same way.
> 
> 

duh. Guess I should have read on :)
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 8

13 years ago
Created attachment 170946 [details] [diff] [review]
The real fix

I put a little more thought into this one... and I actually built it :)
Attachment #170819 - Attachment is obsolete: true

Comment 9

13 years ago
Comment on attachment 170946 [details] [diff] [review]
The real fix

> void
> _PR_MD_INTERVAL_INIT()
> {
>+    char *envp;
>+    if ((envp = getenv("NSPR_OS2_NO_HIRES_TIMER")) != NULL) {
>+        if (atoi(envp) == 1)
>+           return;
>+    }
>+
>     ULONG timerFreq = 0; /* OS/2 high-resolution timer frequency in Hz */
>     APIRET rc = DosTmrQueryFreq(&timerFreq);

Your C compiler allows the C++ practice of
declaring variables anywhere in the function.
I would still try to declare all variables
at the beginning of the function.
(Assignee)

Comment 10

13 years ago
Created attachment 171031 [details] [diff] [review]
Final fix

wtc's comments addressed
Attachment #170946 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #171031 - Flags: superreview?(wtchang)
Attachment #171031 - Flags: review?(mozilla)

Comment 11

13 years ago
Comment on attachment 171031 [details] [diff] [review]
Final fix

r=wtc.	By the way, you can still initialize 'timerFreq'
to 0.  It is 'rc' that we don't want to initialize (to
avoid the possibly unnecessary DosTmrQueryFreq() call).
Attachment #171031 - Flags: superreview?(wtchang) → superreview+
(Assignee)

Comment 12

13 years ago
(In reply to comment #11)
> (From update of attachment 171031 [details] [diff] [review] [edit])
> r=wtc.	By the way, you can still initialize 'timerFreq'
> to 0.  It is 'rc' that we don't want to initialize (to
> avoid the possibly unnecessary DosTmrQueryFreq() call).
> 

Yeah. I thought it looked cleaner to initialize timerFreq separately.

Comment 13

13 years ago
Comment on attachment 171031 [details] [diff] [review]
Final fix

Yes, looks fine to me.
Attachment #171031 - Flags: review?(mozilla) → review+
(Assignee)

Comment 14

13 years ago
wtc: Can you check this in trunk? I can do client branch.

Comment 15

13 years ago
Patch checked in on the NSPR trunk (NSPR 4.6) and
the NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.8 Beta).
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.6
(Reporter)

Comment 16

13 years ago
(In reply to comment #15)
> Patch checked in on the NSPR trunk (NSPR 4.6) and
> the NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.8 Beta).

Fine. I have tested that the patch actually works ;-).

Comment 17

13 years ago
Marked the bug verified per comment 16.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 18

12 years ago
*** Bug 342021 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.