Closed Bug 1371895 Opened 3 years ago Closed 2 years ago

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

Categories

(Toolkit :: Async Tooling, defect, minor)

55 Branch
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: arai, Assigned: Paolo)

References

Details

(Keywords: addon-compat)

Attachments

(1 file)

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;
> }
Severity: normal → minor
Summary: aTaskFn parameter description doesn't match the actual behavior → aTaskFn parameter description doesn't match the actual behavior in DeferredTask.jsm
Version: 53 Branch → 55 Branch
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.
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.
Flags: needinfo?(florian)
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.
Flags: needinfo?(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
Attachment #8876522 - Flags: review?(florian) → review-
(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 on attachment 8876522 [details]
Bug 1371895 - Support legacy Task.jsm generators in DeferredTask.jsm.

https://reviewboard.mozilla.org/r/147852/#review152342
Attachment #8876522 - Flags: review?(florian) → review+
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/170baf9783f6
Support legacy Task.jsm generators in DeferredTask.jsm. r=florian
https://hg.mozilla.org/mozilla-central/rev/170baf9783f6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee: nobody → paolo.mozmail
You want to uplift this to 55, right?
Flags: needinfo?(paolo.mozmail)
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
Flags: needinfo?(paolo.mozmail)
Attachment #8876522 - Flags: approval-mozilla-beta?
Comment on attachment 8876522 [details]
Bug 1371895 - Support legacy Task.jsm generators in DeferredTask.jsm.

add-on compat fix, beta55+
Attachment #8876522 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: checkin-needed
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.