1477761 - Sketchy TimeDuration::Forever computation in SliceBudget

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Steve Fink [:sfink] [:s:]]

As bhackett points out in bug 1477566, there's a very sketchy computation in js/public/SliceBudget.h:

    static const mozilla::TimeStamp unlimitedDeadline =
        mozilla::TimeStamp::Now() + mozilla::TimeDuration::Forever();

I don't really see anything in the documentation that claims this won't be an overflow. In fact, looking at the code:

  TimeStamp& operator+=(const TimeDuration& aOther)
  {
    MOZ_ASSERT(!IsNull(), "Cannot compute with a null value");
    TimeStampValue value = mValue + aOther.mValue;
    // Check for underflow.
    // (We don't check for overflow because it's not obvious what the error
    //  behavior should be in that case.)
    if (aOther.mValue < 0 && value > mValue) {
      value = 0;
    }
    mValue = value;
    return *this;
  }

...that doesn't exactly give me a lot of confidence either.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1477761-sketchy-deadline

Looking at this again I can't see how this can work.

Here's a patch to initialise the 'unlimited' timestamp explicitly and set it to 100 years in the future.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1477761-using-time-types

Add a using directive for mozilla::TimeDuration and strip the namespace from mozilla::TimeStamp in GC.cpp.

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8995192 [details] [diff] [review]
bug1477761-sketchy-deadline

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

I'm still unclear on what it was doing before, but this seems vastly preferable. Thanks!

::: js/src/gc/GC.cpp
@@ +3238,5 @@
> +SliceBudget::Init()
> +{
> +    MOZ_ASSERT(!unlimitedDeadline);
> +    uint64_t oneYearsInSeconds = 365 * 24 * 60 * 60;
> +    unlimitedDeadline = TimeStamp::Now() + mozilla::TimeDuration::FromSeconds(100 * oneYearsInSeconds);

*oneYearInSeconds

Comment 4

[Steve Fink [:sfink] [:s:]]

I wonder if TimeStamp could provide a constexpr EpochTime or something (it doesn't matter which Epoch it uses), so we can add 1000 years to it and eliminate the initialization. But that starts sounding complicated; this is simple and good.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/764e0cce4cd1
Fix sketchy timestamp computation for unlimited slice budget r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e38f6929994
Use using declaration to make time related classes available in GC.cpp r=sfink

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/764e0cce4cd1
https://hg.mozilla.org/mozilla-central/rev/5e38f6929994