Closed Bug 1477761 Opened Last year Closed Last year

Sketchy TimeDuration::Forever computation in SliceBudget

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: sfink, Assigned: jonco)

Details

Attachments

(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]
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
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 jcoppeard@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/764e0cce4cd1
https://hg.mozilla.org/mozilla-central/rev/5e38f6929994
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.