Closed Bug 1393764 Opened 3 years ago Closed 3 years ago

TimeoutManager loops when going from BudgetThrottlingEnabled returning true to it returning false

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: farre, Assigned: farre)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → afarre
Blocks: 1377766
Attachment #8901181 - Flags: review?(bkelly)
Can you explain how this loops?  It seems to me:

1. If totalTimeLimit is zero then the mExecutionBudget must be <= 0.
2. In this case you the nextDeadline is set to the next timer.
3. We call MaybeSchedule(nextDeadline) which calls MinSchedulingDelay().
4. This should schedule us to run with some delay since we're behind schedule.

I guess the root cause here is when we disable budget throttling we can leave the mExecutionBudget negative and we stop updating it?

Would it make more sense to reset mExecutionBudget in BudgetThrottlingEnabled() if we return false?
Flags: needinfo?(afarre)
Comment on attachment 8901181 [details] [diff] [review]
0001-Bug-1393764-Run-at-least-one-timeout.-r-bkelly.patch

Dropping flag for NI for now.
Attachment #8901181 - Flags: review?(bkelly)
We loop because numTimersToRun is 0, which makes us return early without running any timeouts. We still call MaybeSchedule, but since BudgetThrottlingEnabled returns false we won't delay the runner, but call RunTimeout again.

So in some sense the problem is that we use mExecutionBudget for total time limit without checking if throttling is enabled. But we really don't want to call BudgetThrottlingEnabled too much either. Fixing Bug 1378125 would fix this as well. 

With this change we'd never loop, but RunTimeout would always run at least one timeout. With Bug 1378125, total time limit would always be correct.
Flags: needinfo?(afarre)
(In reply to Ben Kelly [:bkelly] from comment #2)
> Can you explain how this loops?  It seems to me:
> 
> 1. If totalTimeLimit is zero then the mExecutionBudget must be <= 0.
> 2. In this case you the nextDeadline is set to the next timer.
> 3. We call MaybeSchedule(nextDeadline) which calls MinSchedulingDelay().
> 4. This should schedule us to run with some delay since we're behind
> schedule.
> 
> I guess the root cause here is when we disable budget throttling we can
> leave the mExecutionBudget negative and we stop updating it?
> 
> Would it make more sense to reset mExecutionBudget in
> BudgetThrottlingEnabled() if we return false?

Yes, this is what I'll be doing.
Attachment #8901181 - Attachment is obsolete: true
Attachment #8901798 - Flags: review?(bkelly)
Comment on attachment 8901798 [details] [diff] [review]
0001-Bug-1393764-Reset-execution-budget-if-BudgetThrottli.patch

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

Thanks!
Attachment #8901798 - Flags: review?(bkelly) → review+
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12d69143d17f
Reset execution budget if BudgetThrottlingEnabled returns false. r=bkelly
https://hg.mozilla.org/mozilla-central/rev/12d69143d17f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.