Closed Bug 558306 Opened 10 years ago Closed 9 years ago

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

Categories

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

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: joseph.crowe, Assigned: bzbarsky)

References

()

Details

Attachments

(5 files, 6 obsolete files)

19.84 KB, patch
cjones
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
15.87 KB, patch
jst
: review+
Details | Diff | Splinter Review
919 bytes, patch
cjones
: review+
Details | Diff | Splinter Review
6.38 KB, patch
cjones
: review+
Details | Diff | Splinter Review
42.23 KB, patch
Details | Diff | Splinter Review
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.
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....
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Non-working patch (obsolete) — Splinter Review
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....
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?
In any case, perhaps we should use TimeStamp in the timer thread too?  The posix version, at least, might give us the right results...
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 :/.
Assignee: nobody → bzbarsky
Attachment #455815 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #455834 - Flags: review?(jst)
Attachment #455833 - Attachment description: Use TimeStamp/TimeDuration for xpcom timers → Part 2. Use TimeStamp/TimeDuration for xpcom timers
Attachment #455834 - Attachment is obsolete: true
Attachment #455839 - Flags: review?(jst)
Attachment #455834 - Flags: review?(jst)
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 on attachment 455833 [details] [diff] [review]
Part 2.  Use TimeStamp/TimeDuration for xpcom timers

>From: Boris Zbarsky <bzbarsky@mit.edu>
>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.
Attachment #455833 - Flags: review?(jones.chris.g) → review+
> and implement inline helper(s) for the canonical converter.

OK, done.
Attachment #455832 - Attachment is obsolete: true
Attachment #456317 - Flags: review?(jones.chris.g)
Attachment #455832 - Flags: review?(jones.chris.g)
Comment on attachment 456317 [details] [diff] [review]
Part 1 updated to comments

Thanks.
Attachment #456317 - Flags: review?(jones.chris.g) → review+
Attachment #455839 - Flags: review?(jst) → review+
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.
Attachment #455833 - Flags: superreview?(brendan) → superreview+
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.
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?
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.
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...
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.
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.
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.
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).
Attachment #457887 - Flags: superreview?(roc)
Attachment #457887 - Flags: review?(jones.chris.g)
Attachment #457887 - Attachment is obsolete: true
Attachment #457972 - Flags: superreview?(roc)
Attachment #457972 - Flags: review?(jones.chris.g)
Attachment #457887 - Flags: superreview?(roc)
Attachment #457887 - Flags: review?(jones.chris.g)
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.
Attachment #457972 - Flags: review?(jones.chris.g) → review+
(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.
> 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.
Attached patch Part 1.5 updated to comments (obsolete) — Splinter Review
Attachment #457972 - Attachment is obsolete: true
Attachment #458051 - Flags: superreview?(roc)
Attachment #457972 - Flags: superreview?(roc)
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.
(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.
+ 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?
(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.
Depends on: 579812
> 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.
(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.
> 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....
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 >.
although you could just avoid doing the conversion when ticks == LL_MAXINT. e.g.
  if (ticks > double(LL_MAXINT)) {
    intTicks = LL_MAXINT;
  } else ...
er
  if (ticks >= double(LL_MAXINT)) {
    intTicks = LL_MAXINT;
  } else if (ticks <= double(LL_MININT)) {
    intTicks = LL_MININT;
  } else {
    intTicks = PRInt64(ticks);
  }
Yeah, that last is what I basically ended up with independently.  I _think_ it should work...
And another overflow that this uncovered fixed.
Attachment #458051 - Attachment is obsolete: true
Attachment #458258 - Flags: superreview?(roc)
Attachment #458258 - Flags: review?(jones.chris.g)
Attachment #458051 - Flags: superreview?(roc)
Attachment #458258 - Flags: superreview?(roc) → superreview+
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.
Attachment #458258 - Flags: review?(jones.chris.g) → review+
> 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.
Attached patch Roll-up patchSplinter Review
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.
Attachment #458943 - Flags: approval2.0?
Attachment #458943 - Flags: approval2.0? → approval2.0+
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
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.
Yep, indeed.  Thanks for reporting that!  I filed bug 581072.
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
You need to log in before you can comment on or make changes to this bug.