1144353 - Replace stack rewriting in Task.jsm with async stacks

Status: RESOLVED WONTFIX

(Toolkit :: General, defect)

Description

[:Paolo Amadini]

Now that we have async stacks, we can use those and remove the rewriting in Task.jsm.

Comment 1

[:Paolo Amadini]

Attachment: Work in progress

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8578962 [details] [diff] [review]
Work in progress

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

Nice clean-up, I like it.

::: toolkit/modules/Task.jsm
@@ +302,5 @@
> +    if (this._isStarGenerator) {
> +      try {
> +        let continueFn = aSendResolved ? this._iterator.next
> +                                       : this._iterator.throw;
> +        continueFn = continueFn.bind(this._iterator, aSendValue);

Nit: not a big fan of the naming convention and of reusing the same name twice.

@@ +307,3 @@
>  
> +        let result = this._stack ? Components.utils.callFunctionWithAsyncStack(
> +                                       continueFn, this._stack, "Task")

Note: looking at xpccomponents.idl, it's not clear to me what the `asyncCause` arg means.

@@ +430,5 @@
>     *
>     * @param {string} topStack The stack provided by the error.
>     * @param {string=} prefix Optionally, a prefix for each line.
>     */
>    generateReadableStack: function(topStack, prefix = "") {

Does this function still do anything useful?

Comment 3

[Alex Vincent [:WeirdAl]]

I'd like to ask for an automated test, please.  I plan on converting Task.jsm to use native Promises (bug 1095267), and I really don't want to regress this when the time comes.  :)

Comment 4

[:Paolo Amadini]

(In reply to Alex Vincent [:WeirdAl] from comment #3)
> I'd like to ask for an automated test, please.  I plan on converting
> Task.jsm to use native Promises (bug 1095267), and I really don't want to
> regress this when the time comes.  :)

Yeah, that's why the patch hadn't a review request yet. Unfortunately I won't be able to work on it this week, and since this isn't required to land bug 1038238, I don't think I'll be able to continue in the short term. If anyone feels like finishing the patch, they're more than welcome!

Comment 5

[:Paolo Amadini]

We've replaced most uses of Task.jsm with async functions, that now also report async stacks correctly. So, there is no need to modify the current Task.jsm async stack reporting.