Closed Bug 1144353 Opened 5 years ago Closed 3 years ago

Replace stack rewriting in Task.jsm with async stacks

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: Paolo, Unassigned)

References

Details

Attachments

(1 file)

Now that we have async stacks, we can use those and remove the rewriting in Task.jsm.
Blocks: 856878
Summary: Remove stack rewriting from Task.jsm → Replace stack rewriting in Task.jsm with async stacks
Attached patch Work in progressSplinter Review
Attachment #8578962 - Flags: feedback?(dteller)
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?
Attachment #8578962 - Flags: feedback?(dteller) → feedback+
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.  :)
(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!
No longer blocks: 1038238
Depends on: 1038238
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.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.