extension will close native client and sub processes those native client created

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
WebExtensions: Untriaged
P1
blocker
RESOLVED FIXED
10 months ago
7 months ago

People

(Reporter: zhongyzh@cisco.com, Assigned: kmag, NeedInfo)

Tracking

({dev-doc-complete})

51 Branch
mozilla51
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox50 unaffected, firefox51+ fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

10 months ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160823121617

Steps to reproduce:

Extension call native client,  native client launch an application, when close Firefox or exit native client, the application will be closed.



Expected results:

The application that launched by native client should not be closed.
(Reporter)

Comment 1

10 months ago
It happened at Windows platform, and when we found the processes at same job, when the parent process exit, the sub process will terminated.
And we try to launch an application from CreateProcess and use CREATE_BREAKAWAY_FROM_JOB, it will return failed and the error code is 5. 
So, we strongly suggest sub process can CREATE_BREAKAWAY_FROM_JOB, it is very important for us.
(Reporter)

Updated

10 months ago
Severity: normal → blocker
Component: Untriaged → Extension Compatibility
Priority: -- → P1
Version: 48 Branch → 51 Branch
I think I need more details about what exactly you're doing. Let me try to write some steps to reproduce and you can correct/clarify:

1. Your Firefox addon is using native API to launch a process. We'll call this "process A"
2. That process launches another Windows process. We'll call this "process B"
2a. What exact API calls is process A making to launch process B?
3. When you quit Firefox, it terminates process A (this is expected and the correct behavior)
4. For some reason we don't yet understand, terminating process A causes process B to also terminate?

You filed this bug with a Mac user agent, but comment 1 says Windows. Is this bug specific to Windows?

On Windows we are creating a job for the native messaging process. I read through bug 1290598 where this was added, and I didn't really see a reason why this is the right behavior. Kris/Andrew, do you know if this was specced out somewhere? I think we do explicitly want to allow this use-case where a native messaging process can launch long-lived other processes. So if we need a job at all (I kinda don't think we do), then we should create it with either JOB_OBJECT_LIMIT_BREAKAWAY_OK or JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(aswan)
(Assignee)

Comment 3

10 months ago
(In reply to Benjamin Smedberg [:bsmedberg] from comment #2)
> On Windows we are creating a job for the native messaging process. I read
> through bug 1290598 where this was added, and I didn't really see a reason
> why this is the right behavior. Kris/Andrew, do you know if this was specced
> out somewhere? I think we do explicitly want to allow this use-case where a
> native messaging process can launch long-lived other processes. So if we
> need a job at all (I kinda don't think we do), then we should create it with
> either JOB_OBJECT_LIMIT_BREAKAWAY_OK or JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK.

It's necessary mainly to deal with processes launched by batch files, and the like, which need to be killed at shutdown. This isn't necessary on *nix, where signals propagate to child processes by default. I agree we should allow processes to explicitly break away, if they need to. In fact, I thought I'd added JOB_OBJECT_LIMIT_BREAKAWAY_OK in the original patch, but it looks like it didn't make it.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(aswan)
(Assignee)

Updated

10 months ago
Assignee: nobody → kmaglione+bmo
[Tracking Requested - why for this release]: Bug in a new feature, found by a partner who wants to use this feature. A blocker for that partner.
status-firefox50: --- → affected
status-firefox51: --- → affected
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
Component: Extension Compatibility → WebExtensions
Product: Firefox → Toolkit
Comment hidden (mozreview-request)
(Assignee)

Comment 6

10 months ago
(In reply to Kris Maglione [:kmag] from comment #5)
> Created attachment 8789570 [details]
> Bug 1301246: Allow processes to break away from their job.
> 
> Review commit: https://reviewboard.mozilla.org/r/77736/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/77736/

I tested this manually on Windows 7. It would be nice to have automated tests, but it's going to be extremely difficult to test reliably, and actually breaking processes away from the test harness's job is going to leave us with rogue processes if something goes wrong.
(Assignee)

Updated

10 months ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 7

10 months ago
mozreview-review
Comment on attachment 8789570 [details]
Bug 1301246: Allow processes to break away from their job.

https://reviewboard.mozilla.org/r/77736/#review76022

::: toolkit/modules/subprocess/subprocess_shared_win.js:26
(Diff revision 1)
>  
>    BYTE: ctypes.uint8_t,
>    WORD: ctypes.uint16_t,
>    DWORD: ctypes.uint32_t,
>    LONG: ctypes.long,
> +  LONGLONG: ctypes.int64_t,

nit: signed LONGLONG isn't used in any of the added structs/code.
Attachment #8789570 - Flags: review?(mhowell) → review+
(Assignee)

Comment 8

10 months ago
mozreview-review-reply
Comment on attachment 8789570 [details]
Bug 1301246: Allow processes to break away from their job.

https://reviewboard.mozilla.org/r/77736/#review76022

> nit: signed LONGLONG isn't used in any of the added structs/code.

Oh, right. It's how Windows defines LARGE_INTEGER, but I didn't think to remove it after I decided to use uint64_t directly.
(Assignee)

Updated

10 months ago
Keywords: dev-doc-needed
(Assignee)

Comment 9

10 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce6190b497023911c060af11dae5d3afe2df4a71
Bug 1301246: Allow processes to break away from their job. r=mhowell
(Assignee)

Comment 10

10 months ago
Comment on attachment 8789570 [details]
Bug 1301246: Allow processes to break away from their job.

Approval Request Comment
[Feature/regressing bug #]: Bug 1290598
[User impact if declined]: This prevents extensions from launching native messaging applications which need to create long-running processes on Windows. Extensions which need to do so will fail to work correctly in affected versions.
[Describe test coverage new/current, TreeHerder]: The code which this change affects is thoroughly tested. There is no automated test for the issue itself, due to the difficulty and risk of testing it on infrastructure.
[Risks and why]: Low. The change is fairly simple, and only eases a restriction which was introduced late in the last nightly cycle.
[String/UUID change made/needed]: None
Attachment #8789570 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 11

10 months ago
Thanks Kris

Comment 12

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ce6190b49702
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Tracking 51+ for the reason in Comment 4.
tracking-firefox51: ? → +
(Assignee)

Comment 14

10 months ago
Can you confirm that this is fixed in the latest nightly, and meets your needs?
Flags: needinfo?(zhongyzh)

Comment 15

10 months ago
Comment on attachment 8789570 [details]
Bug 1301246: Allow processes to break away from their job.

Fixes a bug in web extensions, Aurora50+
Attachment #8789570 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

10 months ago
tracking-firefox50: ? → +
Needs rebasing for Aurora uplift.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 17

10 months ago
It looks like Aurora isn't actually affected by this.
status-firefox50: affected → unaffected
tracking-firefox50: + → ---
Flags: needinfo?(kmaglione+bmo)

Comment 18

10 months ago
Comment on attachment 8789570 [details]
Bug 1301246: Allow processes to break away from their job.

Clearing the Aurora uplift nom.
Attachment #8789570 - Flags: approval-mozilla-aurora+
I've added a section in "Native messaging" that I hope covers this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Native_messaging#Closing_the_native_app.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 20

8 months ago
Looks good. Thanks!
Flags: needinfo?(kmaglione+bmo)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.