1330496 - Remove MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA support for inheriting stdout/stderr handles on XP

Status: RESOLVED FIXED

Description

[Chris Peterson [:cpeterson]]

Now that Firefox 53 no longer supports Windows XP or Vista, we can remove unnecessary Windows version checks for XP, Vista, and 7.

Comment 1

[Chris Peterson [:cpeterson]]

Created attachment 8826032 [details]
Bug 1330496 - Part 1: Remove MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA support for inheriting stdout/stderr handles on XP.

Review commit: https://reviewboard.mozilla.org/r/104086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104086/

Comment 2

[Chris Peterson [:cpeterson]]

Created attachment 8826033 [details]
Bug 1330496 - Part 2: Remove mochitest use of MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA.

Review commit: https://reviewboard.mozilla.org/r/104088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104088/

Comment 3

[Chris Peterson [:cpeterson]]

Created attachment 8826034 [details]
Bug 1330496 - Part 3: Remove other ipc checks for Windows Vista or 7+.

Review commit: https://reviewboard.mozilla.org/r/104090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104090/

Comment 4

[Masatoshi Kimura [:emk]]

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

[Bob Owen (:bobowen)]

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.

Comment 6

[Joel Maher ( :jmaher ) (UTC-4)]

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!

Comment 7

[Aaron Klotz [:aklotz]]

Comment on attachment 8826034 [details]
Bug 1330496 - Part 3: Remove other ipc checks for Windows Vista or 7+.

Thanks!

Comment 8

[Chris Peterson [:cpeterson]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/605d3b2bd202
https://hg.mozilla.org/mozilla-central/rev/5515c1da32d9
https://hg.mozilla.org/mozilla-central/rev/c78097a585e0

Comment 11

[Masatoshi Kimura [:emk]]

Created attachment 8834839 [details] [diff] [review]
Followup patch to remove redundant parenthses

Comment 12

[Masatoshi Kimura [:emk]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87c98844baf6
Followup patch to remove redundant parenthses. r=aklotz

Comment 14

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/87c98844baf6