558306 - Timers created using setInterval stop working when system date is decreased.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Joseph Crowe]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 (.NET CLR 3.5.30729)

The problem was observed on HTML pages loaded in Firefox, on Windows XP SP3.

Reproducible: Always

Steps to Reproduce:
1. Load a page which uses setInterval to create a timer, e.g. the attached URL.
2. While the timer is running, set the system date to some time which is before the current system date.
Actual Results:  
The timer executes once more and after that ceases to execute.

Expected Results:  
The timer should continue to run at the specified interval regarless of the system time configuration.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This happens because of the use of PR_Now to schedule timeouts (which is needed because timeouts may well NOT fire at their scheduled time, so that firing them involves figuring out the current time).

I wonder whether just moving this stuff over to nsTimeStamp would work....

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds like a plan.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Non-working patch

I thought this would work, but it doesn't, at least on Mac.  And specifically, if I set my date back by a day then I never get the nsITimer callbacks that might drive any of this stuff.  That is, nsGlobalWindow::TimerCallback stops getting called, so none of these changes matter.

nsTimerThread seems to use PRIntervalTime, so I have no idea why it's losing here....

Comment 4

[Boris Zbarsky [:bzbarsky]]

And what I see happening in TimerThread::Run is PR_IntervalNow() returning values like so around when I change the date (by one day back into the future):

  2515985736
  2515985738
  2429581108

then output stops.  That makes sense; that value is less than the timeout value in the first timer, so we don't fire it, and then set up a sleep until that timer's firing time (so for a day or so).

I thought PR_IntervalNow wasn't supposed to go backwards... Given that it does, it looks to me like TimeStamp::Now() is incorrect (will detect this as rollover).  Or is that ok?

Comment 5

[Boris Zbarsky [:bzbarsky]]

In any case, perhaps we should use TimeStamp in the timer thread too?  The posix version, at least, might give us the right results...

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Eww ... looks like an NSPR bug :(.  TimeStamp will detect an overflow, and that should be OK here; depending on how the poll timeout checks are done, the overflow might just result in a bunch of timers being fired ahead of schedule. 

TimeStamp_posix will do the right thing.  High-resolution timers for other Tier Is has been a low-priority TODO for a while (bug 539093), but if we need them for non-broken interval timing, maybe the priority should go up :/.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1. Add a ToMilliseconds() method on TimeDuration

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2. Use TimeStamp/TimeDuration for xpcom timers

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3. Use TimeStamp/TimeDuration in DOM timeout code

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 with the dummy timeout stuff fixed too

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 455832 [details] [diff] [review]
Part 1.  Add a ToMilliseconds() method on TimeDuration

>diff --git a/xpcom/ds/TimeStamp.h b/xpcom/ds/TimeStamp.h
>--- a/xpcom/ds/TimeStamp.h
>+++ b/xpcom/ds/TimeStamp.h
>@@ -69,16 +69,17 @@ public:
>   }
>   // Default copy-constructor and assignment are OK
> 
>   double ToSeconds() const;
>   // Return a duration value that includes digits of time we think to
>   // be significant.  This method should be used when displaying a
>   // time to humans.
>   double ToSecondsSigDigits() const;
>+  double ToMilliseconds() const;

I think we should make either ToSeconds() or ToMilliseconds() "canonical" and implement inline helper(s) for the canonical converter.  Keeping ToSeconds() canonical is fine for both existing implementations.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 455833 [details] [diff] [review]
Part 2.  Use TimeStamp/TimeDuration for xpcom timers

>From: Boris Zbarsky <[e-mail]>
>Bug 558306 part 2.  Switch XPCOM timers to TimeDuration/TimeStamp.  r=cjones, sr=brendan
>
>diff --git a/xpcom/threads/TimerThread.cpp b/xpcom/threads/TimerThread.cpp
>--- a/xpcom/threads/TimerThread.cpp
>+++ b/xpcom/threads/TimerThread.cpp
>@@ -274,18 +275,18 @@ NS_IMETHODIMP TimerThread::Run()
>           // We release mLock around the Fire call to avoid deadlock.
>           lock.unlock();
> 
> #ifdef DEBUG_TIMERS
>           if (PR_LOG_TEST(gTimerLog, PR_LOG_DEBUG)) {
>             PR_LOG(gTimerLog, PR_LOG_DEBUG,
>                    ("Timer thread woke up %dms from when it was supposed to\n",
>                     (now >= timer->mTimeout)
>-                    ? PR_IntervalToMilliseconds(now - timer->mTimeout)
>-                    : -(PRInt32)PR_IntervalToMilliseconds(timer->mTimeout-now))
>+                    ? PRInt32((now - timer->mTimeout).ToMilliseconds())
>+                    : -PRInt32((timer->mTimeout-now).ToMilliseconds()))

|PRInt32(fabs((now - timer->mTimeout).ToMilliseconds()))|.

>diff --git a/xpcom/threads/nsTimerImpl.cpp b/xpcom/threads/nsTimerImpl.cpp
>--- a/xpcom/threads/nsTimerImpl.cpp
>+++ b/xpcom/threads/nsTimerImpl.cpp
>@@ -41,16 +41,19 @@
> #include "nsTimerImpl.h"
> #include "TimerThread.h"
> #include "nsAutoLock.h"
> #include "nsAutoPtr.h"
> #include "nsThreadManager.h"
> #include "nsThreadUtils.h"
> #include "prmem.h"
> 
>+using mozilla::TimeDuration;
>+using mozilla::TimeStamp;
>+
> static PRInt32          gGenerator = 0;
> static TimerThread*     gThread = nsnull;
> 
> #ifdef DEBUG_TIMERS
> #include <math.h>
> 
> double nsTimerImpl::sDeltaSumSquared = 0;
> double nsTimerImpl::sDeltaSum = 0;
>@@ -137,27 +140,26 @@ NS_IMETHODIMP_(nsrefcnt) nsTimerImpl::Re
> 
> nsTimerImpl::nsTimerImpl() :
>   mClosure(nsnull),
>   mCallbackType(CALLBACK_TYPE_UNKNOWN),
>   mFiring(PR_FALSE),
>   mArmed(PR_FALSE),
>   mCanceled(PR_FALSE),
>   mGeneration(0),
>-  mDelay(0),
>-  mTimeout(0)
>+  mDelay(0)
> {
>   // XXXbsmedberg: shouldn't this be in Init()?
>   mEventTarget = static_cast<nsIEventTarget*>(NS_GetCurrentThread());
> 
>   mCallback.c = nsnull;
> 
> #ifdef DEBUG_TIMERS
>-  mStart = 0;
>-  mStart2 = 0;
>+  mStart = TimeStamp::TimeStamp();
>+  mStart2 = TimeStamp::TimeStamp();

These initializers are unnecessary.

With bug 507718 fixed, the API and implementation here could be a
lot prettier.  TODO I guess.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 updated to comments

> and implement inline helper(s) for the canonical converter.

OK, done.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 456317 [details] [diff] [review]
Part 1 updated to comments

Thanks.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 455833 [details] [diff] [review]
Part 2.  Use TimeStamp/TimeDuration for xpcom timers

Brendan says sr=him on irc, with a separate bug to consider nuking the filter.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Checked in part 1 as http://hg.mozilla.org/mozilla-central/rev/014d2f87bd51

I checked in the other two as well, but they made the reftest for bug 503531 fail due to the caret randomly not drawing.  I have no idea _why_ yet.  I can reproduce the fail locally only if I change the timeout for the test to 0ms and the one for the reference to 500ms.  Then it fails even without these patches.  With the patches it _sometimes_ fails even with both set to 50ms.  ccing some folks who might know about this test or caret drawing.

Comment 17

[(no longer active)]

Like I said on IRC, I think that we should get carets drawn by the drawWindow call used in reftests, no matter what the internal timer that the caret uses is.  But that doesn't explain why changing the timers even without this patch fiddles with the results.

Boris, do you get a consistent caret behavior with 0ms or 500ms timeouts?  Is it on a local build containing your changes?

Comment 18

[Boris Zbarsky [:bzbarsky]]

With the test set to 0ms and the reference set to 500ms I get consistent behavior: the test has no caret, while the reference has a caret.  This does not depend on whether my patches from this bug are applied.

Comment 19

[(no longer active)]

Hmm, that's really weird.  Unfortunately I don't think that I have any idea on why that happens, besides attaching a debugger and breaking on nsCaret::DrawCaret...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Aha!  roc pointed me to the code that's supposed to make caret not blink in reftest.  What that code does is set the caret blink interval pref to -1.  This becomes PRUint32(-1) for nsCaret::mBlinkRate, and gets passed to the timer init.  Timer init takes the delay as PRUint32 milliseconds.  Before my patch we used to call PR_MillisecondsToInterval on this, which returned 4294967295 (at least on Mac).  Then we compared this to DELAY_INTERVAL_MAX, which is 2147483647 and since the latter is smaller set the delay to 2147483647.  We then proceeded to use that as the delay.

With the new code, we just pass the input to TimeDuration::FromMilliseconds. But FromMilliseconds takes a _signed_ integer.  So we actually got a TimeDuration(-1), and the timer actually fired.  At least I'm guessing so; debugging this stuff is sort of a pain because it depends on the window focus to trigger the caret to start with.

I think reftest should be setting the blink rate to 0, not -1, since that would disable caret blinking altogether.  But I should also fix the above issue and see whether that helps.

Obvious fixes include adding a PRUint32 overload of FromMilliseconds, or just making it take a PRInt64.

Comment 21

[Boris Zbarsky [:bzbarsky]]

None of which explains what happens when I fiddle with the timeouts without my patches applied, but I bet that has something to do with the "don't show the caret until some random focus event happens" business I ran into while debugging.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Another note to self: some of the debug logging is confused with the second patch in this bug due to me passing doubles to printf when it uses a %d format.

Comment 23

[Boris Zbarsky [:bzbarsky]]

And other parts of the logging are broken because the pass unsigned int as %d (so lie about the delay time in exactly the way I got it wrong in the patch, in fact).

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1.5: Make FromMilliseconds take doubles

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1.5 even passing tests, I think

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 457972 [details] [diff] [review]
Part 1.5 even passing tests, I think

>diff --git a/xpcom/ds/TimeStamp.cpp b/xpcom/ds/TimeStamp.cpp
>--- a/xpcom/ds/TimeStamp.cpp
>+++ b/xpcom/ds/TimeStamp.cpp
>+static const PRInt64 maxDurationMs = LL_MAXINT /  PR_TicksPerSecond() * 1000;
>+static const PRInt64 minDurationMs =
>+  LL_MININT / PRInt64(PR_TicksPerSecond()) * PRInt64(1000);
>+

This code needs to document overflow avoidance.  I walked through it
myself to check this, so I conveniently have one ready for you if you
want it ;).

  We need to avoid overflowing the "tick" var in FromMilliseconds() or
  weird and bad things will happen.  NSPR gives us /u/ ticks per
  second, so the invariant we need to preserve is

    FromMilliseconds(x ms):
      (x / 1000) * u <= INT64_MAX and (x / 1000) * u >= INT64_MIN

  From that we derive the limits above, not INT64_MAX/MIN.

Couple of nits: first, style guide wants |kMax*/kMin|. Second, for
overflow-sensitive arithmetic like this, I prefer explicit parentheses
to emphasize the fact that the order of operations is significant.

I don't like MAXINT when we mean INT64_MAX, but that's of course
not your fault.  I filed bug 579517 on fixing that.

> TimeDuration
>-TimeDuration::FromSeconds(PRInt32 aSeconds)
>+TimeDuration::FromMilliseconds(double aMilliseconds)
> {
>-  // No overflow is possible here
>-  return TimeDuration::FromTicks(PRInt64(aSeconds)*PR_TicksPerSecond());
>-}
>-
>-TimeDuration
>-TimeDuration::FromMilliseconds(PRInt32 aMilliseconds)
>-{
>-  // No overflow is possible here
>-  return TimeDuration::FromTicks(PRInt64(aMilliseconds)*PR_TicksPerSecond()/1000);
>+  PRInt64 milliseconds;
>+  if (aMilliseconds > maxDurationMs)
>+    milliseconds = maxDurationMs;
>+  else if (aMilliseconds < minDurationMs)
>+    milliseconds = minDurationMs;
>+  else
>+    milliseconds = PRInt64(aMilliseconds);

These conditions can use >= and <=, but it's not a big deal.

I'd like to use a Clamp() helper for this, but I'm not sure of the
right way to parameterize it.  Maybe

 template<T, U, O=T> O Clamp(T in, U min, U max)

Feel up to taking a crack at that?  Just sugar either way, no worries
if "no".

>+  return TimeDuration::FromTicks(milliseconds*PR_TicksPerSecond()/1000);

I'd leave in the "// No overflow is possible here" comment, with a
"see above".

>diff --git a/xpcom/ds/TimeStamp.h b/xpcom/ds/TimeStamp.h
>--- a/xpcom/ds/TimeStamp.h
>+++ b/xpcom/ds/TimeStamp.h
>@@ -73,18 +73,23 @@ public:
>   // Return a duration value that includes digits of time we think to
>   // be significant.  This method should be used when displaying a
>   // time to humans.
>   double ToSecondsSigDigits() const;
>   double ToMilliseconds() const {
>     return ToSeconds() * 1000.0;
>   }
> 
>-  static TimeDuration FromSeconds(PRInt32 aSeconds);
>-  static TimeDuration FromMilliseconds(PRInt32 aMilliseconds);
>+  // Using a double here is safe enough; with 53 bits we can represent
>+  // durations up to over 280,000 years exactly.  That's probably more
>+  // than our internal storage can represent anyway.

I don't know what you mean by "internal storage" here.  This comment
should say that we clamp the params to the max/min representable
durations instead of overflowing.

>diff --git a/xpcom/ds/TimeStamp_posix.cpp b/xpcom/ds/TimeStamp_posix.cpp
>--- a/xpcom/ds/TimeStamp_posix.cpp
>+++ b/xpcom/ds/TimeStamp_posix.cpp
>@@ -44,25 +44,30 @@
> // doesn't mean that a nanosecond is the resolution of TimeDurations
> // obtained with this API; see TimeDuration::Resolution;
> //
> 
> #include <time.h>
> 
> #include "mozilla/TimeStamp.h"
> 
>+#include "prlong.h"
>+
> // Estimate of the smallest duration of time we can measure.
> static PRUint64 sResolution;
> static PRUint64 sResolutionSigDigs;
> 
> static const PRUint16 kNsPerUs   =       1000;
> static const PRUint64 kNsPerMs   =    1000000;
> static const PRUint64 kNsPerSec  = 1000000000; 
> static const double kNsPerSecd   = 1000000000.0;
> 
>+static const PRInt64 maxDurationMs = LL_MAXINT / kNsPerMs;
>+static const PRInt64 minDurationMs = LL_MININT / PRInt64(kNsPerMs);
>+

Same comments as for TimeStamp.cpp apply here.  The overflow-avoidance
comment can be much shorter since there's no ticks-per-second variable
obscuring the math.

> TimeDuration
>-TimeDuration::FromSeconds(PRInt32 aSeconds)
>+TimeDuration::FromMilliseconds(double aMilliseconds)
> {
>-  return TimeDuration::FromTicks((PRInt64(aSeconds) * PRInt64(kNsPerSec)));
>-}
>-
>-TimeDuration
>-TimeDuration::FromMilliseconds(PRInt32 aMilliseconds)
>-{
>-  return TimeDuration::FromTicks(PRInt64(aMilliseconds) * PRInt64(kNsPerMs));
>+  PRInt64 milliseconds;
>+  if (aMilliseconds > maxDurationMs)
>+    milliseconds = maxDurationMs;
>+  else if (aMilliseconds < minDurationMs)
>+    milliseconds = minDurationMs;
>+  else
>+    milliseconds = PRInt64(aMilliseconds);
>+  return TimeDuration::FromTicks(milliseconds * PRInt64(kNsPerMs));
> }
> 

Same as above.

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #26)
> Comment on attachment 457972 [details] [diff] [review]
> These conditions can use >= and <=, but it's not a big deal.
> 

Sorry, ignore that, I'm dumb.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> so I conveniently have one ready for you

Thanks!  Reading through it I realized that I in fact had overflow issues.  Fixed those, and adjusted the comment accordingly.  New code in FromMilliseconds:

  return
    TimeDuration::FromTicks(milliseconds*PRInt64(PR_TicksPerSecond()/1000));

> Couple of nits: first, style guide wants |kMax*/kMin|. Second, for
> overflow-sensitive arithmetic like this, I prefer explicit parentheses
> to emphasize the fact that the order of operations is significant.

Done for both.

> I'd like to use a Clamp() helper for this

Not doing this per irc discussion.

> I'd leave in the "// No overflow is possible here" comment

Done.

> I don't know what you mean by "internal storage" here.

New comment:

  // Using a double here is safe enough; with 53 bits we can represent
  // durations up to over 280,000 years exactly.  If the units of
  // mValue do not allow us to represent durations of that length,
  // long durations are clamped to the max/min representable value
  // instead of overflowing.

> Same comments as for TimeStamp.cpp apply here.

Yep, done.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1.5 updated to comments

Comment 30

[Boris Zbarsky [:bzbarsky]]

That last patch makes content/media/test/test_playback.html fail.

The reason it fails is that nsBuiltinDecoderStateMachine::Wait is called with aMs == 4294967295 == 0xffffffff.  The old duration code made this into a duration of -1ms, which made end < now, which made the whole call be a no-op.  But now we actually do what the caller asked for....

The bogus value comes from nsBuiltinDecoderStateMachine::AdvanceFrame, where frameDuration == -1 (which is then passed to the PRUint32 arg of Wait).  This happens because videoData looks like this:

$6 = {mOffset = 7106, mTime = 41, mEndTime = 40, mTimecode = 65, mImage = {
    mRawPtr = 0x0}, mDuplicate = 1 '\001', mKeyframe = 0 '\000'}

Should we just be skipping the Wait() call if frameDuration < 0?  Looks like this code was introduced in bug 568431.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #30)
> Should we just be skipping the Wait() call if frameDuration < 0?

Let's do that for now, and file a bug about fixing this code.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+ static const PRInt64 kTicksPerMs = PR_TicksPerSecond() / 1000;

We're supposed to avoid static initializers. Make these static members of FromMilliseconds?

+  NS_PRECONDITION(PR_TicksPerSecond() % 1000 == 0,
+                  "kTicksPerMs is incorrect");

This assertion can't be right, surely?

Wouldn't it be slightly simpler to convert to a 'double' tick count and then clamp the tick count before we convert to PRInt64 ticks?

Comment 33

[Matthew Gregan [:kinetik]]

(In reply to comment #31)
> (In reply to comment #30)
> > Should we just be skipping the Wait() call if frameDuration < 0?
> 
> Let's do that for now, and file a bug about fixing this code.

The frame duration should never be less than 0.  There are assertions in the VideoData constructor enforcing this, but it turns out I missed a chunk of ugly code in the Ogg decoder that rewrites the VideoData timestamps.  I've filed bug 579812 and attached a patch.

Comment 34

[Boris Zbarsky [:bzbarsky]]

> This assertion can't be right, surely?

Why not?  If that assertion is not right, then kTicksPerMs loses precision by being an integer....  and it so happens that right now that assertion is in fact correct at least on our main three platforms.

> I've filed bug 579812 and attached a patch.

Awesome.  Will land this once that lands.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #34)
> > This assertion can't be right, surely?
> 
> Why not?  If that assertion is not right, then kTicksPerMs loses precision by
> being an integer....  and it so happens that right now that assertion is in
> fact correct at least on our main three platforms.

Hmm ... OK.

Comment 36

[Boris Zbarsky [:bzbarsky]]

> Wouldn't it be slightly simpler to convert to a 'double' tick count and then
> clamp the tick count before we convert to PRInt64 ticks?

It might be, but it would reduce the range we can represent exactly in the posix case from 2^64 ticks to 2^53 ticks (since 2^53 ms is more than 2^64 ticks in this case, precision is limited only by the tick representation).  In human-sized units, that takes us from being abe to represent a duration range of 584 years to being able to represent a duration range of 104 days.  Are we ok with that?  Seems a bit chancy to me, but then again I doubt anyone would notice their 104-day durations being off by a few ns....

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Well, exactly...

Comment 38

[Boris Zbarsky [:bzbarsky]]

OK, so clamping the double tick count is actually sort of fun.

The obvious way to do it is to compute a |double ticks;| and then do:

  if (ticks > double(LL_MAXINT)) ticks = LL_MAXINT;
  else ...

but does anything guarantee that if ticks <= double(LL_MAXINT) then converting ticks to a PRInt64 won't overflow?  Note that LL_MAXINT cannot be represented exactly as a double.  I guess if I'm careful enough with my compares I should be ok.  In particular, have to use >= there, not >.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

although you could just avoid doing the conversion when ticks == LL_MAXINT. e.g.
  if (ticks > double(LL_MAXINT)) {
    intTicks = LL_MAXINT;
  } else ...

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

er
  if (ticks >= double(LL_MAXINT)) {
    intTicks = LL_MAXINT;
  } else if (ticks <= double(LL_MININT)) {
    intTicks = LL_MININT;
  } else {
    intTicks = PRInt64(ticks);
  }

Comment 42

[Boris Zbarsky [:bzbarsky]]

Yeah, that last is what I basically ended up with independently.  I _think_ it should work...

Comment 43

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1.5 with that change

And another overflow that this uncovered fixed.

Comment 44

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 458258 [details] [diff] [review]
Part 1.5 with that change

>diff --git a/xpcom/ds/TimeStamp.cpp b/xpcom/ds/TimeStamp.cpp
>--- a/xpcom/ds/TimeStamp.cpp
>+++ b/xpcom/ds/TimeStamp.cpp
>@@ -65,35 +65,28 @@ TimeDuration::ToSeconds() const
>-
>-TimeDuration
>-TimeDuration::FromMilliseconds(PRInt32 aMilliseconds)
>-{
>-  // No overflow is possible here
>-  return TimeDuration::FromTicks(PRInt64(aMilliseconds)*PR_TicksPerSecond()/1000);
>+  return TimeDuration::FromTicks(aMilliseconds *
>+                                 (double(PR_TicksPerSecond()) / 1000.0));
> }
> 

Any reason not to make |double(PR_TicksPerSecond()) / 1000.0| a static constant?

It's really ridiculous how hard this code is to get right.  Some tests for these new edge cases in xpcom/tests/TestTimeStamp.cpp would be nice, but that's not run on tinderbox so ah well.

Comment 45

[Boris Zbarsky [:bzbarsky]]

> Any reason not to make |double(PR_TicksPerSecond()) / 1000.0| a static
> constant?

Nope.  Done.

> It's really ridiculous how hard this code is to get right.

Yeah, magic type conversions will do that to you.  :(

The edge cases depend on the implementation (posix vs not), so adding tests might be ... difficult.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Attachment: Roll-up patch

This should be pretty safe (certainly passes all the tests, and we use timeouts a lot in them), and fixes some longstanding issues in our timer code while making it easier to maintain going forward.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Pushed:
  http://hg.mozilla.org/mozilla-central/rev/1a5e6a1cdea4
  http://hg.mozilla.org/mozilla-central/rev/3ab60fc0581d
  http://hg.mozilla.org/mozilla-central/rev/d225b78f6738

Comment 48

[Brian Toledo]

I think I am seeing a possible regression with this bug on the http://detroit.tigers.mlb.com/index.jsp?c_id=det site.

Mozilla/5.0 (Windows; Windows NT 5.1; rv:2.0b3pre) Gecko/20100722 Minefield/4.0b3pre ( .NET CLR 3.5.30729) ID:20100722041142

Comment 49

[Brian Toledo]

Sorry, More info: the site refreshes constantly.  I have also tried www.indians.com with the same results.  Does this in safe mode, as well.  Works fine in IE.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Yep, indeed.  Thanks for reporting that!  I filed bug 581072.

Comment 51

[Daniel Holbert [:dholbert]]

This bug's cset removed several comments of the form "Make sure to cast the unsigned PR_USEC_PER_MSEC to signed PRTime"[1] while removing usages of those keywords, but it left one straggling comment[2].  I just pushed a trivial followup to remove that straggling comment:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/a6a26c14f5e4


[1] https://hg.mozilla.org/mozilla-central/rev/d225b78f6738#l1.129
    https://hg.mozilla.org/mozilla-central/rev/d225b78f6738#l1.320

[2] https://hg.mozilla.org/mozilla-central/rev/d225b78f6738#l1.201

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a6a26c14f5e4