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

VERIFIED FIXED in 4.6

Status

defect
VERIFIED FIXED
15 years ago
13 years ago

People

(Reporter: r.ihle, Assigned: mkaply)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

15 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

15 years ago
Posted patch Possible fix (obsolete) — Splinter Review
Something like this should work.

Comment 2

15 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

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

Comment 4

15 years ago
most code, e.g. the bits in nsdebugimpl checks the value.
Assignee

Comment 5

15 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

15 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

15 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

15 years ago
Posted patch The real fix (obsolete) — Splinter Review
I put a little more thought into this one... and I actually built it :)
Attachment #170819 - Attachment is obsolete: true

Comment 9

15 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

15 years ago
Posted patch Final fixSplinter Review
wtc's comments addressed
Attachment #170946 - Attachment is obsolete: true
Assignee

Updated

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

Comment 11

15 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

15 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

15 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

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

Comment 15

15 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
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.6
Reporter

Comment 16

15 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

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

Comment 18

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