Closed
Bug 1330496
Opened 6 years ago
Closed 6 years ago
Remove MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA support for inheriting stdout/stderr handles on XP
Categories
(Core :: DOM: Content Processes, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: cpeterson, Assigned: cpeterson)
References
Details
Attachments
(4 files)
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•6 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•6 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•6 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•6 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 7•6 years ago
|
||
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•6 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.
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•6 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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 11•5 years ago
|
||
Attachment #8834839 -
Flags: review?(aklotz)
Updated•5 years ago
|
Attachment #8834839 -
Flags: review?(aklotz) → review+
Comment 12•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c355b3756b5cd529ea878bd978ec0082f7b1656
Comment 13•5 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
Comment 14•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/87c98844baf6
You need to log in
before you can comment on or make changes to this bug.
Description
•