1290598 - Migrate native messaging tests to xpcshell

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1290598: Refactor native messaging test setup code into separate head file.

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

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1290598: Migrate native messaging tests to xpcshell.

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

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1290598: Move slower native messaging tests to a separate unit.

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

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1290598: Manage and terminate Windows subprocess trees as a single job object.

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

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8776158 [details]
Bug 1290598: Migrate native messaging tests to xpcshell.

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

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8776159 [details]
Bug 1290598: Move slower native messaging tests to a separate unit.

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

Comment 7

[Kris Maglione [:kmag]]

N.B. If this doesn't land before the merge, we're probably going to have to try to get it uplifted.

Comment 8

[Molly Howell (she/her) (no longer active)]

Comment on attachment 8776310 [details]
Bug 1290598: Manage and terminate Windows subprocess trees as a single job object.

https://reviewboard.mozilla.org/r/68154/#review65360

I don't think this change actually achieves the stated goal of using a single job object for the subproces tree; it creates a new unnamed job object every time a process is spawned. To create a job object and then reuse it, the object has to be given a name.

Comment 9

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/68154/#review65360

Right. Each process we spawn needs to be in its own job object. But the processes that *they* spawn need to be in the same job object so we can kill them when we want to terminate the process. The problem we're running into is that our tests use a batch file to launch a python interpreter, but when we terminate the process, the python interpreter is not actually terminated.

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 8776157 [details]
Bug 1290598: Refactor native messaging test setup code into separate head file.

https://reviewboard.mozilla.org/r/68052/#review65488

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8776158 [details]
Bug 1290598: Migrate native messaging tests to xpcshell.

https://reviewboard.mozilla.org/r/68054/#review65484

::: toolkit/components/extensions/NativeMessaging.jsm:429
(Diff revision 2)
>      let result = this.startupPromise.then(() => {
>        this.send(msg);
>        return responsePromise;
>      });
>  
> +    result.catch(() => {

You could just add this to the second handler right below (or move the code from the second handler below up here if you prefer explicitly calling .catch)

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8776159 [details]
Bug 1290598: Move slower native messaging tests to a separate unit.

https://reviewboard.mozilla.org/r/68056/#review65492

Comment 13

[Molly Howell (she/her) (no longer active)]

Comment on attachment 8776310 [details]
Bug 1290598: Manage and terminate Windows subprocess trees as a single job object.

https://reviewboard.mozilla.org/r/68154/#review65502

Comment 14

[Molly Howell (she/her) (no longer active)]

https://reviewboard.mozilla.org/r/68154/#review65360

Oooooh, right. That's what I get for doing reviews early on Monday mornings. Thanks for clarifying.

Comment 15

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f6f0dc1663cb5cd175f8809c3d3312df1535823
Bug 1290598: Refactor native messaging test setup code into separate head file. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/3242411d3294521508f34d1b5beee52b1ae986ea
Bug 1290598: Manage and terminate Windows subprocess trees as a single job object. r=mhowell

https://hg.mozilla.org/integration/mozilla-inbound/rev/486639a0f3ca05afaf0250b514c3641805f1f497
Bug 1290598: Migrate native messaging tests to xpcshell. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/577158be08e8c1211ffa1198a21e3c059f98f477
Bug 1290598: Move slower native messaging tests to a separate unit. r=aswan

Comment 16

[Carsten Book [:Tomcat]]

backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=33184510&repo=mozilla-inbound

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d00c5d21cc
Backed out changeset 577158be08e8 
https://hg.mozilla.org/integration/mozilla-inbound/rev/34de0687c35a
Backed out changeset 486639a0f3ca 
https://hg.mozilla.org/integration/mozilla-inbound/rev/17aa766ced69
Backed out changeset 3242411d3294 
https://hg.mozilla.org/integration/mozilla-inbound/rev/0da61317235f
Backed out changeset 3f6f0dc1663c for xpcshell timeouts in test_ext_native_messaging.js

Comment 18

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccea65b170672288036c09e43baa418b8d36015f
Bug 1290598: Refactor native messaging test setup code into separate head file. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/89df3d72bcf795906bfeb45a7192019dc6ce7a44
Bug 1290598: Manage and terminate Windows subprocess trees as a single job object. r=mhowell

https://hg.mozilla.org/integration/mozilla-inbound/rev/6da2e5dd4ea28edd4257b315d7d4214788ac5dd7
Bug 1290598: Migrate native messaging tests to xpcshell. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/194d8684ef71133b12b7802c4b2628b21badf5e5
Bug 1290598: Move slower native messaging tests to a separate unit. r=aswan

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ccea65b17067
https://hg.mozilla.org/mozilla-central/rev/89df3d72bcf7
https://hg.mozilla.org/mozilla-central/rev/6da2e5dd4ea2
https://hg.mozilla.org/mozilla-central/rev/194d8684ef71

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

this set of changes had a slight performance improvement for windows 7 ts_paint!

https://treeherder.mozilla.org/perf.html#/alerts?id=2246