1316780 - browser.test.* stacks are no longer useful

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Kris Maglione [:kmag]]

They report the stack in the parent process, from the point where the message was received, rather than from where the assertion function was called.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1316780: Part 1 - Proxy extension events between the parent and child process.

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

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1316780: Part 2 - Remove ext-test.js.

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

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1316780: Part 3 - Capture the current unprivileged stack in browser.test.* assertion functions.

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

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8809685 [details]
Bug 1316780: Part 1 - Proxy extension events between the parent and child process.

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

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8809686 [details]
Bug 1316780: Part 2 - Remove ext-test.js.

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

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8809687 [details]
Bug 1316780: Part 3 - Capture the current unprivileged stack in browser.test.* assertion functions.

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

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8809685 [details]
Bug 1316780: Part 1 - Proxy extension events between the parent and child process.

https://reviewboard.mozilla.org/r/92220/#review92416

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8809686 [details]
Bug 1316780: Part 2 - Remove ext-test.js.

https://reviewboard.mozilla.org/r/92222/#review92418

::: toolkit/components/extensions/ext-c-test.js:87
(Diff revision 2)
> +    extension.emit("test-result", Boolean(value), String(msg));
>    }
>  
>    return {
>      test: {
> -      // These functions accept arbitrary values. Convert the parameters to
> +      sendMessage: function(...args) {

nit: why this syntax instead of just `sendMessage(...args) {`

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8809686 [details]
Bug 1316780: Part 2 - Remove ext-test.js.

https://reviewboard.mozilla.org/r/92222/#review92418

> nit: why this syntax instead of just `sendMessage(...args) {`

Just because I moved this code from ext-test.js and didn't change it. I was thinking about it, though, so I guess I will now.

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 8809687 [details]
Bug 1316780: Part 3 - Capture the current unprivileged stack in browser.test.* assertion functions.

https://reviewboard.mozilla.org/r/92224/#review92420

Looks good to me and the SimpleTest change seems like a good one, but should probably be approved by an owner or peer for that code.

::: testing/mochitest/tests/SimpleTest/ExtensionTestUtils.js:57
(Diff revision 2)
>    }
>  
>    function testHandler(kind, pass, msg, ...args) {
>      if (kind == "test-eq") {
>        var [expected, actual] = args;
> -      SimpleTest.ok(pass, `${msg} - Expected: ${expected}, Actual: ${actual}`);
> +      SimpleTest.ok(pass, `${msg} - Expected: ${expected}, Actual: ${actual}`, undefined, args[2]);

can you just pull this out of args on the line above (and maybe switch var -> let at the same time)

::: toolkit/components/extensions/ext-c-test.js:104
(Diff revision 2)
>        notifyFail: function(msg) {
> -        extension.emit("test-done", false, msg);
> +        extension.emit("test-done", false, msg, getStack());
>        },
>  
>        log: function(msg) {
> -        extension.emit("test-log", true, msg);
> +        extension.emit("test-log", true, msg, getStack());

No need for a stack here...

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8809687 [details]
Bug 1316780: Part 3 - Capture the current unprivileged stack in browser.test.* assertion functions.

https://reviewboard.mozilla.org/r/92224/#review92420

> No need for a stack here...

Yeah, I guess, but I figured it might be useful at some point so I may as well include it.

Comment 12

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9867579cf623cd1396fee24d18179a149399e92
Bug 1316780: Part 1 - Proxy extension events between the parent and child process. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/c93fbd9ced3c64a4e48bb45ee90187be46f65212
Bug 1316780: Part 2 - Remove ext-test.js. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee3edae1e4e0ac07b3032a30b2685d2cd8c85658
Bug 1316780: Part 3 - Capture the current unprivileged stack in browser.test.* assertion functions. r=aswan

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b9867579cf62
https://hg.mozilla.org/mozilla-central/rev/c93fbd9ced3c
https://hg.mozilla.org/mozilla-central/rev/ee3edae1e4e0