Migrate native messaging tests to xpcshell

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

a year ago
Created attachment 8776157 [details]
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/
Attachment #8776157 - Flags: review?(aswan)
Attachment #8776158 - Flags: review?(aswan)
Attachment #8776159 - Flags: review?(aswan)
(Assignee)

Comment 2

a year ago
Created attachment 8776158 [details]
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/
(Assignee)

Comment 3

a year ago
Created attachment 8776159 [details]
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/
(Assignee)

Comment 4

a year ago
Created attachment 8776310 [details]
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/
Attachment #8776310 - Flags: review?(mhowell)
(Assignee)

Comment 5

a year ago
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/
(Assignee)

Comment 6

a year ago
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/
(Assignee)

Comment 7

a year ago
N.B. If this doesn't land before the merge, we're probably going to have to try to get it uplifted.
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.
Attachment #8776310 - Flags: review?(mhowell) → review-
(Assignee)

Comment 9

a year ago
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 on attachment 8776157 [details]
Bug 1290598: Refactor native messaging test setup code into separate head file.

https://reviewboard.mozilla.org/r/68052/#review65488
Attachment #8776157 - Flags: review?(aswan) → review+
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)
Attachment #8776158 - Flags: review?(aswan) → review+
Comment on attachment 8776159 [details]
Bug 1290598: Move slower native messaging tests to a separate unit.

https://reviewboard.mozilla.org/r/68056/#review65492
Attachment #8776159 - Flags: review?(aswan) → review+
(Assignee)

Updated

a year ago
Assignee: nobody → kmaglione+bmo
(Assignee)

Updated

a year ago
Attachment #8776310 - Flags: review- → review?(mhowell)
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
Attachment #8776310 - Flags: review?(mhowell) → review+
https://reviewboard.mozilla.org/r/68154/#review65360

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

Comment 15

a year ago
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
backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=33184510&repo=mozilla-inbound
Flags: needinfo?(kmaglione+bmo)

Comment 17

a year ago
Backout by cbook@mozilla.com:
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
(Assignee)

Comment 18

a year ago
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

a year ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
this set of changes had a slight performance improvement for windows 7 ts_paint!

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

Updated

a year ago
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.