1371895 - aTaskFn parameter description doesn't match the actual behavior in DeferredTask.jsm

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[Tooru Fujisawa [:arai]]

https://dxr.mozilla.org/mozilla-central/rev/2d20b365eee19434657f6b365d310e8b70904d2b/toolkit/modules/DeferredTask.jsm#275,287
> this.DeferredTask.prototype = {
> ...
>   _timerCallback() {
> ...
>     runningDeferred.resolve((async () => {
> ...
>       await (this._taskFn() || Promise.resolve()).then(null, Cu.reportError);
> ...
>           await (this._taskFn() || Promise.resolve()).then(null, Cu.reportError);

it expects the task function returns promise, but it doesn't match the parameter description, as it says "Function or generator function", not "async function"

https://dxr.mozilla.org/mozilla-central/rev/2d20b365eee19434657f6b365d310e8b70904d2b/toolkit/modules/DeferredTask.jsm#102-105
> /**
>  * Sets up a task whose execution can be triggered after a delay.
>  *
>  * @param aTaskFn
>  *        Function or generator function to execute.  This argument is passed to
>  *        the "Task.spawn" method every time the task should be executed.  This
>  *        task is never re-entered while running.
>  * @param aDelayMs
>  *        Time between executions, in milliseconds.  Multiple attempts to run
>  *        the task before the delay has passed are coalesced.  This time of
>  *        inactivity is guaranteed to pass between multiple executions of the
>  *        task, except on finalization, when the task may restart immediately
>  *        after the previous execution finished.
>  */
> this.DeferredTask = function(aTaskFn, aDelayMs) {
>   this._taskFn = aTaskFn;
>   this._delayMs = aDelayMs;
> }

Comment 1

[Tooru Fujisawa [:arai]]

also it doesn't match the example :/

https://dxr.mozilla.org/comm-central/rev/f4262773c4331d4ae139be536ce278ea9aad3436/mozilla/toolkit/modules/DeferredTask.jsm#23
>  *   let saveDeferredTask = new DeferredTask(function* () {
>  *     yield OS.File.writeAtomic(...);
>  *     // Any uncaught exception will be reported.
>  *   }, 2000);

to be clear, this is breaking change and it will affect add-ons that uses DeferredTask.
since, the DeferredTask was previously used with function or generator function, but now the generator function case fails with error that calls undefined .then parameter for the returned value.

Comment 2

[Tooru Fujisawa [:arai]]

so, what's the intention here?
does DeferredTask no more support generator function?
in that case, the comments should be updated and the change should be mentioned in Add-on Compatibility.

Comment 3

[:Paolo Amadini]

Since it's an easy fix, I think it makes sense not to break add-on compatibility unnecessarily. When we remove Task.jsm from the tree, we can remove support for legacy generators here too.

Comment 4

[:Paolo Amadini]

Attachment: Bug 1371895 - Support legacy Task.jsm generators in DeferredTask.jsm.

This ensures legacy add-on compatibility by loading Task.jsm lazily when required. Once Task.jsm is removed, this code can be changed back to support only async functions.

Review commit: https://reviewboard.mozilla.org/r/147852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147852/

Comment 5

[:Paolo Amadini]

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

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8876522 [details]
Bug 1371895 - Support legacy Task.jsm generators in DeferredTask.jsm.

https://reviewboard.mozilla.org/r/147852/#review152254

Thanks for looking at this. I agree with the general idea but have comments about the implementation (would be f+ if this wasn't on reviewboard).

::: toolkit/modules/DeferredTask.jsm:277
(Diff revision 1)
>      this._armed = false;
>      this._runningPromise = runningDeferred.promise;
>  
>      runningDeferred.resolve((async () => {
>        // Execute the provided function asynchronously.
> -      await (this._taskFn() || Promise.resolve()).then(null, Cu.reportError);
> +      await this._runTask();

The || Promise.resolve() here is because sometimes DeferredTask is used with a normal function that returns nothing. We should handle that case without importing Task.jsm.

::: toolkit/modules/DeferredTask.jsm:305
(Diff revision 1)
> +   * Executes the associated task and catches exceptions.
> +   */
> +  async _runTask() {
> +    try {
> +      // Execute the provided function asynchronously, using Task.jsm only if
> +      // _taskFn is not already an async function.

I think we want to detect generators, rather than async functions.

await works for any function that returns a promise, not just async functions.

Also, detecting generators is what Task.jsm does internally, so I think we should re-use the same logic: http://searchfox.org/mozilla-central/rev/d840ebd5858a61dbc1622487c1fab74ecf235e03/toolkit/modules/Task.jsm#137

Comment 7

[:Paolo Amadini]

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> The || Promise.resolve() here is because sometimes DeferredTask is used with
> a normal function that returns nothing. We should handle that case without
> importing Task.jsm.

Ah yes, that's better. Even though there is no way to distinguish a legacy generator function from a normal function, we can actually invoke the function and check if the result is a generator. For any other result we can just use plain "await", which conveniently turns any non-Promise values into a resolved Promise.

I've added a new test case for legacy generators using some ESLint magic.

Comment 8

[:Paolo Amadini]

Comment on attachment 8876522 [details]
Bug 1371895 - Support legacy Task.jsm generators in DeferredTask.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147852/diff/1-2/

Comment 9

[:Paolo Amadini]

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

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8876522 [details]
Bug 1371895 - Support legacy Task.jsm generators in DeferredTask.jsm.

https://reviewboard.mozilla.org/r/147852/#review152342

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/170baf9783f6
Support legacy Task.jsm generators in DeferredTask.jsm. r=florian

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/170baf9783f6

Comment 13

[Florian Quèze [:florian]]

You want to uplift this to 55, right?

Comment 14

[:Paolo Amadini]

Comment on attachment 8876522 [details]
Bug 1371895 - Support legacy Task.jsm generators in DeferredTask.jsm.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1353542
[User impact if declined]: Add-on compatibility issues
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Covered by new regression tests
[String changes made/needed]: None

Comment 15

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8876522 [details]
Bug 1371895 - Support legacy Task.jsm generators in DeferredTask.jsm.

add-on compat fix, beta55+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eeabd79814f1