1023787 - Make Assert.jsm and Task.jsm error rewriting play nicely together

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

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

Bug 1010518 introduces stack rewriting for Task. Unfortunately, Assert.jsm captures stacks too early for it to work nicely. We need to get them to play nicely together.

Comment 1

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

Mike, why can't we have Assert.jsm throw an error instead of capturing the stack itself?

Comment 2

[:Paolo Amadini]

(In reply to David Rajchenbach Teller [:Yoric] from comment #1)
> Mike, why can't we have Assert.jsm throw an error instead of capturing the
> stack itself?

I believe all the testing assertion functions capture the stack at the time they're called, using Components.stack.caller. Especially in asynchronous tests called without Task the error must be reported immediately because any thrown exception might be swallowed (for example, by a DOM event handler).

For this to work correctly, I believe we should keep track of the active task, and all the assertion functions should be modified to call something like Task.Debugging.augmentStack which will then do the right thing if a task is currently active.

Comment 3

[Mike de Boer [:mikedeboer]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #1)
> Mike, why can't we have Assert.jsm throw an error instead of capturing the
> stack itself?

Yeah, what Paolo said. I don't think Assert.jsm introduced new behavior here.

Comment 4

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

I assume that we don't want Assert.jsm do depend on Task.jsm, do we?

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> I assume that we don't want Assert.jsm do depend on Task.jsm, do we?

No, but all the error reporting logic is implemented in testing/xpcshell/head.js, at the top. A custom reporting function is passed to the Assert constructor.

Comment 6

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

Attachment: Making xpcshell and mochitests display human-readable stack traces for Task

Let's give it a go.
I haven't found a mochitest self-test, so no unit tests for mochi.

Comment 7

[:Paolo Amadini]

Comment on attachment 8438485 [details] [diff] [review]
Making xpcshell and mochitests display human-readable stack traces for Task

This looks good, I'd appreciate a comment describing the effect of the stack line rewriting done with Components.stack, it is not exactly clear to me.

I think I'll do a more thorough review tomorrow, but I didn't see anything major. This is great work :-)

Comment 8

[Mike de Boer [:mikedeboer]]

My eyes are not deceiving me, right? This _is_ generating a uniform stack trace for XPCShell-test and Mochitest?

<3 <3 <3

Comment 9

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

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> My eyes are not deceiving me, right? This _is_ generating a uniform stack
> trace for XPCShell-test and Mochitest?
> 
> <3 <3 <3

Might be :)

Comment 10

[Mike de Boer [:mikedeboer]]

Much bitrot, this patch has. David, could you upload an updated one? I'd really like to see it in action.

Comment 11

[:Paolo Amadini]

Comment on attachment 8438485 [details] [diff] [review]
Making xpcshell and mochitests display human-readable stack traces for Task

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

I have only a few suggestions, you can request the next review to Mike.

::: toolkit/modules/Task.jsm
@@ +279,5 @@
>     * @param aSendValue
>     *        Resolution result or rejection exception, if any.
>     */
>    _run: function TaskImpl_run(aSendResolved, aSendValue) {
> +    gCurrentTask = this;

I believe the various assignments of gCurrentTask can be simplified. We only need the assignment here, and one reset to null at the end of this function.

To be on the safe side, you can put the reset to null in a "finally" section of the existing catch blocks, in case of exceptions within _handleException.

@@ +317,5 @@
>        }
>      }
>    },
>  
> +  _handleFinalResult: function TaskImpl_handleFinalResult(aValue) {

So, you don't need this helper or all the other changes handling gCurrentTask.

We'll need a test for the non-star-generator case. A modification of the "test_throw_complex_stack" function from the other patch might actually suffice.

@@ +420,5 @@
> + * The Task currently being executed
> + */
> +let gCurrentTask = null;
> +
> +function* linesOf(string) {

Globals should be placed at the beginning of the file.

@@ +451,5 @@
> +   * a Task.
> +   *
> +   * @param {string} topStack The stack provided by the error.
> +   */
> +  generateReadableStack: function(topStack) {

Is the behavior of this function sensible when maintaining the stack is disabled globally?

::: toolkit/modules/tests/xpcshell/test_task.js
@@ +548,5 @@
> +    "task_1",
> +    "function_3",
> +    "function_2",
> +    "function_1",
> +    "test_throw_complex_stack"];

I think these changes belonged to the other patch.

Comment 12

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

(In reply to :Paolo Amadini from comment #11)
> Comment on attachment 8438485 [details] [diff] [review]
> Making xpcshell and mochitests display human-readable stack traces for Task
> 
> Review of attachment 8438485 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have only a few suggestions, you can request the next review to Mike.
> 
> ::: toolkit/modules/Task.jsm
> @@ +279,5 @@
> >     * @param aSendValue
> >     *        Resolution result or rejection exception, if any.
> >     */
> >    _run: function TaskImpl_run(aSendResolved, aSendValue) {
> > +    gCurrentTask = this;
> 
> I believe the various assignments of gCurrentTask can be simplified. We only
> need the assignment here, and one reset to null at the end of this function.
> 
> To be on the safe side, you can put the reset to null in a "finally" section
> of the existing catch blocks, in case of exceptions within _handleException.

I don't think this is sufficient. If you have nested Tasks, you need to return to the parent task when you pop out of the child task, and I don't think your strategy will do that.

> We'll need a test for the non-star-generator case. A modification of the
> "test_throw_complex_stack" function from the other patch might actually
> suffice.

Note that we can't test the feature from xpcshell itself, so I have added a second test in selftest.py. 

> @@ +451,5 @@
> > +   * a Task.
> > +   *
> > +   * @param {string} topStack The stack provided by the error.
> > +   */
> > +  generateReadableStack: function(topStack) {
> 
> Is the behavior of this function sensible when maintaining the stack is
> disabled globally?

It should degrade gracefully.

Comment 13

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

Attachment: Making xpcshell and mochitests display human-readable stack traces for Task, v2

Here we go.

Comment 14

[:Paolo Amadini]

(In reply to David Rajchenbach Teller [:Yoric] from comment #12)
> > I believe the various assignments of gCurrentTask can be simplified. We only
> > need the assignment here, and one reset to null at the end of this function.
> 
> I don't think this is sufficient. If you have nested Tasks, you need to
> return to the parent task when you pop out of the child task, and I don't
> think your strategy will do that.

Have you verified? We enter _run through "then" for each piece of the task:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Task.jsm#333

There is no loop inside _run itself.

> > Is the behavior of this function sensible when maintaining the stack is
> > disabled globally?
> 
> It should degrade gracefully.

No errors or warnings in the Console?

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8442732 [details] [diff] [review]
Making xpcshell and mochitests display human-readable stack traces for Task, v2

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

r=me! Self test is running a-ok, can you push a try run before landing to make sure no tools break that depend on the 'old' stacktrace format?

I trust that Paolo reviewed the Task.jsm bits thoroughly. They look good and sane to me, but I'm not what you'd call a "Task.jsm peer".

Thanks!

::: testing/xpcshell/selftest.py
@@ +626,5 @@
> +        self.assertInLog("run_next_test")
> +        self.assertInLog("run_test")
> +        self.assertNotInLog("Task.jsm")
> +
> +    3def testAddTaskStackTraceWithoutStar(self):

well, this obviously doesn't work :P

Comment 16

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

Attachment: Making xpcshell and mochitests display human-readable stack traces for Task, v3

Applied feedback.
I took the opportunity to add one more test.
Try: https://tbpl.mozilla.org/?tree=Try&rev=40431d672402

Comment 17

[:Paolo Amadini]

Comment on attachment 8442843 [details] [diff] [review]
Making xpcshell and mochitests display human-readable stack traces for Task, v3

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

Hi David, let's sort out how to clarify the logic before landing this patch. It should be pretty easy to do.

So, apparently the _run function can re-enter when it's called directly by Task.spawn:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Task.jsm#250

I think the best thing to do is to change _run to be called as a resolution callback:

Promise.resolve().then(() => this._run(true));

This may, however, cause some differences in timing that affect calling code, so we may want to do this simple one-line change in a separate bug.

For the moment you can may set "gCurrentTask = this" just after the call into the iterator ("send" or "throw") returns. This is, however, just a workaround and is subject to a bug that probably exists in the current implementation as well: if you call Task.spawn within a task but don't yield the returned promise immediately, gCurrentTask will be set to the wrong value (probably null) until the outer task yields, returns, or throws. You may want a test case for this.

In both cases you can apply the following simplifications.

::: toolkit/modules/Task.jsm
@@ +313,5 @@
>                                     : this._iterator.throw(aSendValue);
>  
>          if (result.done) {
>            // The generator function returned.
> +          this._handleFinalResult(result.value);

The _handleFinalResult helper is unneeded, you can reset gCurrentTask to null in a finally block _after_ you call this.deferred.resolve directly here. The promise resolution handler is guaranteed not to be called before "resolve" returns.

@@ +325,1 @@
>          this._handleException(ex);

This addition is unneeded (in fact you set gCurrentTask again in _handleException).

@@ +331,5 @@
>          this._handleResultValue(yielded);
>        } catch (ex if ex instanceof Task.Result) {
>          // The generator function threw the special exception that allows it to
>          // return a specific value on resolution.
> +        this._handleFinalResult(ex.value);

ditto with regard to _handleFinalResult

@@ +336,3 @@
>        } catch (ex if ex instanceof StopIteration) {
>          // The generator function terminated with no specific result.
> +        this._handleFinalResult(undefined);

ditto

@@ +345,5 @@
>  
> +  _handleFinalResult: function TaskImpl_handleFinalResult(aValue) {
> +    gCurrentTask = null;
> +    this.deferred.resolve(aValue);
> +  },

Remove the helper.

@@ +382,5 @@
>     *        The uncaught exception to handle.
>     */
>    _handleException: function TaskImpl_handleException(aException) {
> +
> +    gCurrentTask = this;

This is unneeded here if you remove the reset above. You can move this assignment right after the "let yielded =" above, where it logically belongs (unless you go for running the first chunk of the task in a resolution callback from the start, in which case that is also unneeded).

Comment 18

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

(In reply to :Paolo Amadini from comment #17)
> @@ +382,5 @@
> >     *        The uncaught exception to handle.
> >     */
> >    _handleException: function TaskImpl_handleException(aException) {
> > +
> > +    gCurrentTask = this;
> 
> This is unneeded here if you remove the reset above. You can move this
> assignment right after the "let yielded =" above, where it logically belongs
> (unless you go for running the first chunk of the task in a resolution
> callback from the start, in which case that is also unneeded).

I may be missing something, but if we move the assignment right after the "let yielded=", it will not be executed in case of exception.

Comment 19

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

Attachment: Making xpcshell and mochitests display human-readable stack traces for Task, v4

Applied feedback, added a test.
I also took the opportunity to add a few whitespaces at the start of lines in the xpcshell and mochitest stacks. This is what mochitests already do and it improves readability.

Comment 20

[:Paolo Amadini]

Comment on attachment 8443730 [details] [diff] [review]
Making xpcshell and mochitests display human-readable stack traces for Task, v4

(In reply to David Rajchenbach Teller [:Yoric] from comment #18)
> I may be missing something, but if we move the assignment right after the
> "let yielded=", it will not be executed in case of exception.

Ah, sure :-)

This version looks good! As a follow-up bug, I still think we should move the first "run" call in a Promise, as that will totally simplify the interleaving logic.

Comment 21

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=4ba02191e66e
As far as I can tell, all oranges are unrelated bugs.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8adcd39585a

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/c8adcd39585a