Last Comment Bug 751727 - Remove nspr TimeStamp implementation
: Remove nspr TimeStamp implementation
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Jeff Muizelaar [:jrmuizel]
:
:
Mentors:
Depends on: 752280
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-03 15:21 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2012-05-14 12:34 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove nspr TimeStamp implementation (4.30 KB, patch)
2012-05-03 15:21 PDT, Jeff Muizelaar [:jrmuizel]
roc: review+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-05-03 15:21:59 PDT
Created attachment 620865 [details] [diff] [review]
Remove nspr TimeStamp implementation

PR_IntervalNow() is not monotonic and so we shouldn't be using it for implementing TimeStamp, especially now that it is exposed to web content (Bug 539095).

Furthermore, it seems that the nspr implementation is being used unintentionally on some platforms. For example, we used to use it on Android and just now I see that it is being used in FF12 on Ubuntu.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-03 15:52:47 PDT
Comment on attachment 620865 [details] [diff] [review]
Remove nspr TimeStamp implementation

Review of attachment 620865 [details] [diff] [review]:
-----------------------------------------------------------------

Fine, as long as this doesn't break any builds.
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-05-03 22:20:21 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c694e6169d5
Comment 4 Landry Breuil (:gaston) 2012-05-05 02:20:39 PDT
It seems this broke the build on OpenBSD where HAVE_CLOCK_MONOTONIC is not defined.
Comment 5 Landry Breuil (:gaston) 2012-05-05 02:24:11 PDT
After a quick look configure fails to find CLOCK_MONOTONIC #define because it's in sys/time.h and not time.h. I'll file a followup bug to fix configure.in.
Comment 6 Jeff Muizelaar [:jrmuizel] 2012-05-05 17:00:26 PDT
(In reply to Landry Breuil from comment #5)
> After a quick look configure fails to find CLOCK_MONOTONIC #define because
> it's in sys/time.h and not time.h. I'll file a followup bug to fix
> configure.in.

Great. This is exactly the kind of thing this patch was supposed to catch.
Comment 7 Landry Breuil (:gaston) 2012-05-06 10:13:40 PDT
Hrm apparently POSIS says that it should be in time.h, so i'll see if i can get OpenBSD fixed instead. Fwiw, i've sent a sys/time.h fix to try here : https://tbpl.mozilla.org/?tree=Try&rev=15aea6efea09
Apparently there wasnt related breakage..
Comment 8 Landry Breuil (:gaston) 2012-05-08 00:25:42 PDT
OpenBSD got fixed in http://marc.info/?l=openbsd-cvs&m=133643021430806&w=2, and the HAVE_CLOCK_MONOTONIC test passes fine now.
Comment 9 Landry Breuil (:gaston) 2012-05-12 05:38:30 PDT
Thinking more about it.. maybe the configure check for clock_gettime(CLOCK_MONOTONIC) should fail configure itself now, so that linking doesnt fail later with missing syms.
Comment 10 Jeff Muizelaar [:jrmuizel] 2012-05-12 10:50:33 PDT
(In reply to Landry Breuil (:gaston) from comment #9)
> Thinking more about it.. maybe the configure check for
> clock_gettime(CLOCK_MONOTONIC) should fail configure itself now, so that
> linking doesnt fail later with missing syms.

That would definitely be an improvement.
Comment 11 Landry Breuil (:gaston) 2012-05-12 12:27:48 PDT
Ok, then two questions :
- would "[Your system doesn't support clock_gettime(CLOCK_MONOTONIC) or its detection failed]" be a warning clear enough ?
- should i go all the way and also remove all references to HAVE_CLOCK_MONOTONIC which is now mandatory true ?
Comment 12 Jeff Muizelaar [:jrmuizel] 2012-05-13 18:23:29 PDT
(In reply to Landry Breuil (:gaston) from comment #11)
> Ok, then two questions :
> - would "[Your system doesn't support clock_gettime(CLOCK_MONOTONIC) or its
> detection failed]" be a warning clear enough ?
> - should i go all the way and also remove all references to
> HAVE_CLOCK_MONOTONIC which is now mandatory true ?

I would prefer getting rid of HAVE_CLOCK_MONOTONIC all together.
Comment 13 Landry Breuil (:gaston) 2012-05-14 12:34:49 PDT
I've tried that approach in https://hg.mozilla.org/try/rev/8f0371a839db  (see https://tbpl.mozilla.org/?tree=Try&rev=a3e9ca6b8c0e ) but that broke mac os x..

Something is still fishy because the test succeeds on linux/bsd but fails on macos, Previously xpcom/ds/Makefile.in was reyling on that since _posix.cpp was used if the test succeeded and then macos implem was used otherwise, so making it a failure doesnt seem to work. Interestingly, from try logs clock_gettime(CLOCK_MONOTONIC) test is not run at all on windows. Of course i dont have a mac to test the clock_gettime() configure test failure..

Note You need to log in before you can comment on or make changes to this bug.