Remove MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA support for inheriting stdout/stderr handles on XP

RESOLVED FIXED in Firefox 53

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

51 Branch
mozilla53
Unspecified
Windows Vista
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
Now that Firefox 53 no longer supports Windows XP or Vista, we can remove unnecessary Windows version checks for XP, Vista, and 7.
(Assignee)

Updated

2 years ago
Priority: -- → P3
Summary: Remove MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA support → Remove MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA support for inheriting stdout/stderr handles on XP
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8826032 [details]
Bug 1330496 - Part 1: Remove MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA support for inheriting stdout/stderr handles on XP.

https://reviewboard.mozilla.org/r/104086/#review104816

security/sandbox/chromium/sandbox/win/src/broker_services.cc is a sandbox code from upstream. We should update it from upstream rather than patching here.

(We can change files under ipc/chromium because we already gave up to sync them from upstream.)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8826032 [details]
Bug 1330496 - Part 1: Remove MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA support for inheriting stdout/stderr handles on XP.

https://reviewboard.mozilla.org/r/104086/#review104866

r+ with the change to modifications-to-chromium-to-reapply-after-upstream-merge.txt and maybe the comment update.

(In reply to Masatoshi Kimura [:emk] from comment #4)
> Comment on attachment 8826032 [details]
> Bug 1330496 - Part 1: Remove MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA support
> for inheriting stdout/stderr handles on XP.
> 
> https://reviewboard.mozilla.org/r/104086/#review104816
> 
> security/sandbox/chromium/sandbox/win/src/broker_services.cc is a sandbox
> code from upstream. We should update it from upstream rather than patching
> here.

Right, but as far as I can tell that's effectively what Chris's patch does.

::: ipc/chromium/src/base/process_util_win.cc:280
(Diff revision 1)
>    // where only specified handles are inherited - so we use this technique if
>    // we can.  If that technique isn't available (or it fails), we just don't
> -  // inherit anything.  This can cause us a problem for Windows XP testing,
> +  // inherit anything.

nit: given in theory it is always available now, maybe end with:

  // where only specified handles are inherited - so we use this technique.
  // If that fails we just don't inherit anything.

::: security/sandbox/chromium/sandbox/win/src/broker_services.cc
(Diff revision 1)
>        startup_info.startup_info()->hStdError = stderr_handle;
>        // Allowing inheritance of handles is only secure now that we
>        // have limited which handles will be inherited.
>        inherit_handles = true;
>      }
> -  } else if (getenv("MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA")) {

Please remove the bug1287426part4.patch line from security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.txt as well.
Attachment #8826032 - Flags: review?(bobowencode) → review+

Comment 6

2 years ago
mozreview-review
Comment on attachment 8826033 [details]
Bug 1330496 - Part 2: Remove mochitest use of MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA.

https://reviewboard.mozilla.org/r/104088/#review104884

thanks for cleaning this up!
Attachment #8826033 - Flags: review?(jmaher) → review+
Comment on attachment 8826034 [details]
Bug 1330496 - Part 3: Remove other ipc checks for Windows Vista or 7+.

Thanks!
Attachment #8826034 - Flags: review?(aklotz) → review+
(Assignee)

Comment 8

2 years ago
(In reply to Bob Owen (:bobowen) from comment #5)
> r+ with the change to
> modifications-to-chromium-to-reapply-after-upstream-merge.txt and maybe the
> comment update.

Thanks! I'll update the comments and patch file before landing.

> > security/sandbox/chromium/sandbox/win/src/broker_services.cc is a sandbox
> > code from upstream. We should update it from upstream rather than patching
> > here.
> 
> Right, but as far as I can tell that's effectively what Chris's patch does.

Yes. My patch is just reverting some of our downstream modifications of Chromium's sandbox code.

Comment 9

2 years ago
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/605d3b2bd202
Part 1: Remove MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA support for inheriting stdout/stderr handles on XP. r=bobowen
https://hg.mozilla.org/integration/mozilla-inbound/rev/5515c1da32d9
Part 2: Remove mochitest use of MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA. r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/c78097a585e0
Part 3: Remove other ipc checks for Windows Vista or 7+. r=aklotz

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/605d3b2bd202
https://hg.mozilla.org/mozilla-central/rev/5515c1da32d9
https://hg.mozilla.org/mozilla-central/rev/c78097a585e0
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Created attachment 8834839 [details] [diff] [review]
Followup patch to remove redundant parenthses
Attachment #8834839 - Flags: review?(aklotz)

Updated

2 years ago
Attachment #8834839 - Flags: review?(aklotz) → review+

Comment 13

2 years ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87c98844baf6
Followup patch to remove redundant parenthses. r=aklotz
You need to log in before you can comment on or make changes to this bug.