1524786 - Fix DeferredTask in new "about:config" and consider reverting the default not to use idleDispatchToMainThread

Status: RESOLVED INACTIVE

(Toolkit :: Async Tooling, defect, P1)

Description

[:Paolo Amadini]

Bug 1391707 changed DeferredTask to use idleDispatchToMainThread by default, which is not what the new code in "about:config" required. This new default also required a conspicuous number of follow-ups, see bug 1391707 comment 17 onwards.

The purpose of DeferredTask is to coalesce tasks so they don't happen too frequently, with built-in support for asynchronous functions so race conditions don't occur. It also happened to be used for a few operations that can wait until idle.

Here is another case I found with a quick DXR search where waiting indefinitely until an idle callback is likely unintended: https://searchfox.org/mozilla-central/source/mobile/android/modules/geckoview/GeckoViewAutoFill.jsm#61

Florian, what do you think of changing back to a safer default, possibly making the third an "options" argument that looks like:

{ [optional] idleDispatchTimeout: <0 or timeout> }

It's very easy to change current consumers, and it would make the idle timeout explicit preventing misuses in the future. Either way, the documentation in the module should be updated.

Comment 1

[:Paolo Amadini]

While looking into this, I noticed that the implementation was actually sub-optimal. Since the idle callback was part of the task, attempts to call "arm" on it during the idle wait (which can be potentially even longer than the coalesce delay) will cause an unnecessary second execution after the next task has completed. I have a patch to fix this and change the default.

Comment 2

[:Paolo Amadini]

Attachment: Bug 1524786 - Improve the DeferredTask idle dispatch mode. r=florian

Now the task is still considered armed during the idle wait, which prevents unnecessary executions if the "arm" method is called again.

The idle dispatch mode is now selected using a non-default option. The maximum idle timeout is also removed because there are no consumers using it.

Comment 3

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef1af1b00b318db3ac6539b7edf2f58abe522388

Comment 4

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fc57e199f917b6b14bc034ba5c81594c61fd71c

Comment 5

[:Paolo Amadini]

As noted in the review, while there is a 50%-50% split between the use cases, and the class naming could be improved to reflect those separately, I'm closing the bug since it's not relevant in the immediate future.