463562 - unnecessary dependency on mmtimer.lib

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

we're linking in mmtimer.lib simply to use timeGetTime() for _PR_MD_GET_INTERVAL(). It appears that GetTickCount() will do the same thing without the additional dependency.

Additionally, the dependancy on mmtimer is causing nss to fail to load softokn3.dll on wince, resulting in a crash when trying to start an https connection.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: uses GetTickCount instead of timeGetTime

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Please elaborate on the statement: "the dependancy on mmtimer is causing nss 
to fail to load softokn3.dll on wince".

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

softokn3.dll was failing to load with error code 0x7e, which a missing dependency.  I looked at the dependency tree with depends.exe and saw that nspr was depending on mmtimer.dll, which I didn't see in my system path.  Rebuilding nspr and relinking nss with this patch fixed the problem of softokn3.dll's load failure.

Comment 4

[Stuart Parmenter]

Comment on attachment 346842 [details] [diff] [review]
uses GetTickCount instead of timeGetTime

why do we want to do this?
timeGetTime has 1ms precision vs GetTickCount which has 10ms.

Comment 5

[timeless]

please say you filed a bug w/ a real stack for the actual crash.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #4)
> (From update of attachment 346842 [details] [diff] [review])
> why do we want to do this?
> timeGetTime has 1ms precision vs GetTickCount which has 10ms.

According to the MSDN documentation, GetTickCount returns a number
whose units are milliseconds since startup (mod 2^32), but its 
resolution is platform dependent, and is said to be 60hz on many 
WinXXX platforms, meaning that it updates in multiples of 16.66... :) 

If that's good enough for WinCE, then I think this patch is OK, otherwise...
This patch seems to include at least one unrelated file modification.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

yes, it does.  Please ignore the change to map.cpp

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: does a little proc address magic

This will use timeGetTime if mmtimer exists, if not it'll fall back on GetTickCount

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 347331 [details] [diff] [review]
does a little proc address magic

We should seperate this out into two patches -- one for our wince tools, one for nspr?

what about the vs8 tools?  http://mxr.mozilla.org/mozilla-central/search?string=mmtimer

should we put timeGetTime() in the shunt?  it seems to also be used in:

/js/tamarin/platform/win32/OSDepWin.cpp

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 347331 [details] [diff] [review]
does a little proc address magic

This code dynamically loads a system library under some circumstances.
Where's the code to do the corresponding unload when NSPR is shut down?

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

according to msdn:

> The system maintains a per-process reference count on all loaded modules.
> Calling LoadLibrary increments the reference count. Calling the FreeLibrary or
> FreeLibraryAndExitThread function decrements the reference count. The system 
> unloads a module when its reference count reaches zero or when the process 
> terminates (regardless of the reference count).

I believe the unload on process termination is the behavior we want

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Fennec may desire this behavior, but I don't believe that means that all 
users of NSPR on WinCE will.  

In general, it is not the practice of NSPR to leak dynamically loaded 
libraries.  Shutting down NSPR should leave the process very much in the
same state as before NSPR is initialized.

Comment 13

[Wan-Teh Chang]

Comment on attachment 347331 [details] [diff] [review]
does a little proc address magic

You should initialize 'intervalFunc' in _PR_MD_INTERVAL_INIT(),
which is called once during NSPR initialization.

The way you initialize 'intervalFunc' lazily in _PR_MD_GET_INTERVAL()
may not be thread-safe.

You can also consider just using GetTickCount.  It's usually bad
to behave differently on different machines unless the feature
is highly desirable.

Comment 14

[Wan-Teh Chang]

Attachment: NSPR patch v2

Brad, please review and test this NSPR patch.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Wan-Teh, I've got your patch in my tree but can't test it right now because of a js crash.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 347689 [details] [diff] [review]
NSPR patch v2

looks good and works for me

Comment 17

[Wan-Teh Chang]

Attachment: NSPR patch v2 (as checked in)

I added an XXX comment to note that the mmtimer.dll library handle
is leaked.  I checked in the patch on the NSPR trunk (NSPR 4.7.4).

Checking in ntinrval.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/ntinrval.c,v  <--  ntinrval.c
new revision: 3.14; previous revision: 3.13
done

This will eventually make it into mozilla-central.  Brad, you need to
check in your change to build/wince/tools/vs9ppc2003arm/arm-wince-link.c
yourself.

Comment 18

[Wan-Teh Chang]

I'm targetting Windows CE support at NSPR 4.8 now.  So the
NSPR patch v2 (attachment 349479 [details] [diff] [review]) won't be in NSPR 4.7.4.
(This is just a version number change.  Don't worry about
it.)