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

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: farre, Assigned: farre)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → afarre
(Assignee)

Updated

2 years ago
Blocks: 1377766
(Assignee)

Comment 1

2 years ago
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)
(Assignee)

Comment 4

2 years ago
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)
(Assignee)

Comment 5

2 years ago
(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.
(Assignee)

Comment 6

2 years ago
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+

Comment 8

2 years ago
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
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.