Remove MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA support for inheriting stdout/stderr handles on XP

RESOLVED FIXED in Firefox 53

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 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)

Now that Firefox 53 no longer supports Windows XP or Vista, we can remove unnecessary Windows version checks for XP, Vista, and 7.
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 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 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 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+
(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.
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
Attachment #8834839 - Flags: review?(aklotz) → review+
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.