Closed Bug 1477761 Opened Last year Closed Last year

Sketchy TimeDuration::Forever computation in SliceBudget


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox63 --- fixed


(Reporter: sfink, Assigned: jonco)



(2 files)

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.
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.
Assignee: nobody → jcoppeard
Attachment #8995192 - Flags: review?(sphink)
Add a using directive for mozilla::TimeDuration and strip the namespace from mozilla::TimeStamp in GC.cpp.
Attachment #8995193 - Flags: review?(sphink)
Comment on attachment 8995192 [details] [diff] [review]

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);

Attachment #8995192 - Flags: review?(sphink) → review+
Attachment #8995193 - Flags: review?(sphink) → review+
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.
Pushed by
Fix sketchy timestamp computation for unlimited slice budget r=sfink
Use using declaration to make time related classes available in GC.cpp r=sfink
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.