Remove BSD code no longer used downstream

RESOLVED FIXED in Firefox 45

Status

()

Core
IPC
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Jan Beich, Assigned: Jan Beich)

Tracking

(Blocks: 1 bug)

Trunk
mozilla46
Unspecified
FreeBSD
Points:
---

Firefox Tracking Flags

(firefox45 fixed, firefox46 fixed)

Details

(Whiteboard: [npotb] )

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
Created attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?martin@NetBSD.ORG

Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?martin@NetBSD.ORG
Attachment #8696684 - Flags: review?(jld)
(Assignee)

Comment 2

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa44d567b669
(Assignee)

Updated

2 years ago
Attachment #8696684 - Flags: feedback?(martin)
Attachment #8696684 - Flags: feedback?(landry)
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 on attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?martin@NetBSD.ORG

Wont be able to test it soon since last i tried m-c was broken on openbsd, but i welcome this change :)
Attachment #8696684 - Flags: feedback?(landry) → feedback+
Comment on attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?martin@NetBSD.ORG

I even managed to build it fine on OpenBSD.

Updated

2 years ago
Attachment #8696684 - Flags: feedback?(martin) → feedback+
Comment on attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?martin@NetBSD.ORG

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.
Attachment #8696684 - Flags: review?(jld) → review+
See Also: → bug 1233275
(Assignee)

Comment 7

2 years ago
Also land in Aurora mainly to ease maintenance of ESR45 downstream. Completely NPOTB unlike bug 1231109.
Assignee: nobody → jbeich
Status: NEW → ASSIGNED
Whiteboard: [npotb] → [npotb] [checkin-needed-aurora]

Comment 8

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

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf2c46cbeeb3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(Assignee)

Comment 10

2 years ago
a=NPOTB DONTBUILD per comment 7 or whiteboard. process_util_bsd.cc is not built on Tier1.
Keywords: checkin-needed
(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
Keywords: checkin-needed
(Assignee)

Comment 12

2 years ago
Comment on attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?martin@NetBSD.ORG

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.
Attachment #8696684 - Flags: approval-mozilla-aurora?
Whiteboard: [npotb] [checkin-needed-aurora] → [npotb]
Comment on attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?martin@NetBSD.ORG

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.
Attachment #8696684 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 14

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/c3e2b78a0fe3
status-firefox45: affected → fixed
You need to log in before you can comment on or make changes to this bug.