1231123 - Remove BSD code no longer used downstream

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Jan Beich]

All BSDs support posix_spawn API nowadays. The last one to use fork/exec was NetBSD 5.x [1]. By removing it process_util_bsd.cc becomes close to process_util_mac.mm. Let's also remove __dso_public as it shouldn't be used outside of BSDs source.

[1] https://blog.netbsd.org/tnf/entry/end_of_life_for_netbsd

Comment 1

[Jan Beich]

Attachment: MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?[e-mail]

Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?[e-mail]

Comment 2

[Jan Beich]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa44d567b669

Comment 3

[Landry Breuil (:gaston)]

I have no opinion on this, but consistency is always better :) I know some of our developers dont like this API but since it is in POSIX....

Comment 4

[Landry Breuil (:gaston)]

Comment on attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?[e-mail]

Wont be able to test it soon since last i tried m-c was broken on openbsd, but i welcome this change :)

Comment 5

[Landry Breuil (:gaston)]

Comment on attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?[e-mail]

I even managed to build it fine on OpenBSD.

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?[e-mail]

https://reviewboard.mozilla.org/r/27401/#review25249

::: ipc/chromium/src/base/process_util_bsd.cc:69
(Diff revision 1)
>    while(environ[pos] != NULL) {

Not your change, but accessing `environ` like that is racy.  I'll file a bug to fix that once bug 1133073 lands and it's fixable.

Comment 7

[Jan Beich]

Also land in Aurora mainly to ease maintenance of ESR45 downstream. Completely NPOTB unlike bug 1231109.

Comment 8

[Pulsebot]

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

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Jan Beich]

a=NPOTB DONTBUILD per comment 7 or whiteboard. process_util_bsd.cc is not built on Tier1.

Comment 11

[Carsten Book [:Tomcat]]

(In reply to Jan Beich from comment #10)
> a=NPOTB DONTBUILD per comment 7 or whiteboard. process_util_bsd.cc is not
> built on Tier1.

this landed in comment 8 or ? For aurora not sure but i guess this need approval

Comment 12

[Jan Beich]

Comment on attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?[e-mail]

To reduce maintenance cost of ESR45 downstream.

Approval Request Comment
[Feature/regressing bug #]: None, dead code; originally added by bug 753046
[User impact if declined]: None for supported *BSD releases
[Describe test coverage new/current, TreeHerder]: landed in m-c
[Risks and why]: NPOTB, may break build on some BSDs
[String/UUID change made/needed]: None

(In reply to Carsten Book [:Tomcat] from comment #11)
> For aurora not sure but i guess this need approval

In the past NPOTB patches could land under implicit approval e.g., bug 985848 comment 6, bug 986357 comment 6, bug 969932 comment 5. Looking at mercurial log occasional a=NPOTB for other folks still happen.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?[e-mail]

No, I do want the normal uplift process to be used for this kind of changes.
NPOTB is more for documentation or this kind of things.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c3e2b78a0fe3