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

Status: RESOLVED FIXED

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

Comment 1

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1393764-Run-at-least-one-timeout.-r-bkelly.patch

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8901181 [details] [diff] [review]
0001-Bug-1393764-Run-at-least-one-timeout.-r-bkelly.patch

Dropping flag for NI for now.

Comment 4

[Andreas Farre [:farre]]

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.

Comment 5

[Andreas Farre [:farre]]

(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.

Comment 6

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1393764-Reset-execution-budget-if-BudgetThrottli.patch

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8901798 [details] [diff] [review]
0001-Bug-1393764-Reset-execution-budget-if-BudgetThrottli.patch

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

Thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12d69143d17f
Reset execution budget if BudgetThrottlingEnabled returns false. r=bkelly

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/12d69143d17f