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

Status: VERIFIED FIXED

(NSPR :: NSPR, defect)

Description

[Rüdiger Ihle]

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

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Possible fix

Something like this should work.

Comment 2

[Wan-Teh Chang]

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.

Comment 3

[Rüdiger Ihle]

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

Comment 4

[timeless]

most code, e.g. the bits in nsdebugimpl checks the value.

Comment 5

[Mike Kaply [:mkaply]]

Actually, I was copying other NSPR code:

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

Comment 6

[Rüdiger Ihle]

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

Comment 7

[Mike Kaply [:mkaply]]

(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 :)

Comment 8

[Mike Kaply [:mkaply]]

Attachment: The real fix

I put a little more thought into this one... and I actually built it :)

Comment 9

[Wan-Teh Chang]

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.

Comment 10

[Mike Kaply [:mkaply]]

Attachment: Final fix

wtc's comments addressed

Comment 11

[Wan-Teh Chang]

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

Comment 12

[Mike Kaply [:mkaply]]

(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

[Peter Weilbacher]

Comment on attachment 171031 [details] [diff] [review]
Final fix

Yes, looks fine to me.

Comment 14

[Mike Kaply [:mkaply]]

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

Comment 15

[Wan-Teh Chang]

Patch checked in on the NSPR trunk (NSPR 4.6) and
the NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.8 Beta).

Comment 16

[Rüdiger Ihle]

(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

[Wan-Teh Chang]

Marked the bug verified per comment 16.

Comment 18

[Mike Kaply [:mkaply]]

*** Bug 342021 has been marked as a duplicate of this bug. ***