Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(552)

Issue 600263002: Get Win32K lockdown for renderers working again on Windows 8+. (Closed)

Created:
6 years, 2 months ago by ananta
Modified:
6 years, 2 months ago
Reviewers:
dmazzoni, jam, sky, scottmg
CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Get Win32K lockdown for renderers working again on Windows 8+. This regressed recently with the change to query the device scale factor to see if it needs to be scaled back to 1.0 for display scales equal to or under 125%. user32/gdi32 API's crash or fail with the Win32K lockdown process mitigation. Fix is to ensure that the device scale factor is queried from the one set via the command line. The other change is to move the code in ContentMainRunner::Initialize which sets the global device scale factor for the process to before the content client initialization. This is because the ShellContentRendererClient in its ctor calls the ShouldUseDirectWrite function which relies on the GetDPIScale function to decide whether or not directwrite should be enabled. This causes a DCHECK in content_browsertests. To ensure that the win32k lockdown does not regress I added a simple content browsertest RendererWin32KLockdownNavigationTest which runs this on Windows 8+. The accessibility fixes are to get the browser tests to run on Windows 8. There were special cases to always enable accessibility on Windows 8 for historical reasons. These are no longer necessary as we don't rely on accessibility to display the virtual keyboard. BUG=417147 Committed: https://crrev.com/b1f9313020d4c1e8619f21b5320a0255bf313b4b Cr-Commit-Position: refs/heads/master@{#296859}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Code review comments #

Total comments: 2

Patch Set 3 : Added content_browsertests to the win8_aura bot #

Total comments: 2

Patch Set 4 : Code review comments and accessibility fixes #

Patch Set 5 : Rebased to tip #

Patch Set 6 : Add chrome_200_percent.pak to browser.isolate #

Patch Set 7 : Fix linux build failures #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+74 lines, -60 lines) Patch
M chrome/chrome.isolate View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 1 comment Download
M content/app/content_main_runner.cc View 2 chunks +15 lines, -13 lines 0 comments Download
M content/browser/accessibility/accessibility_mode_helper.cc View 1 2 3 4 3 chunks +0 lines, -27 lines 0 comments Download
M content/browser/accessibility/browser_accessibility_state_impl.cc View 1 2 3 2 chunks +0 lines, -14 lines 0 comments Download
M content/browser/renderer_host/render_process_host_browsertest.cc View 2 chunks +43 lines, -0 lines 0 comments Download
M testing/buildbot/chromium.win.json View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M testing/buildbot/chromium_win8_trybot.json View 1 chunk +1 line, -0 lines 0 comments Download
M ui/gfx/win/dpi.cc View 1 2 3 4 1 chunk +7 lines, -6 lines 0 comments Download

Messages

Total messages: 25 (7 generated)
ananta
sky for ui jam for content scottmg for everything.
6 years, 2 months ago (2014-09-24 23:49:27 UTC) #2
sky
https://codereview.chromium.org/600263002/diff/1/ui/gfx/win/dpi.cc File ui/gfx/win/dpi.cc (right): https://codereview.chromium.org/600263002/diff/1/ui/gfx/win/dpi.cc#newcode137 ui/gfx/win/dpi.cc:137: float dpi_scale = gfx::Display::HasForceDeviceScaleFactor() ? If there is a ...
6 years, 2 months ago (2014-09-24 23:51:15 UTC) #3
ananta
https://codereview.chromium.org/600263002/diff/1/ui/gfx/win/dpi.cc File ui/gfx/win/dpi.cc (right): https://codereview.chromium.org/600263002/diff/1/ui/gfx/win/dpi.cc#newcode137 ui/gfx/win/dpi.cc:137: float dpi_scale = gfx::Display::HasForceDeviceScaleFactor() ? On 2014/09/24 23:51:15, sky ...
6 years, 2 months ago (2014-09-24 23:56:00 UTC) #4
sky
LGTM
6 years, 2 months ago (2014-09-24 23:57:40 UTC) #5
scottmg
With that, lgtm. https://codereview.chromium.org/600263002/diff/20001/testing/buildbot/chromium_win8_trybot.json File testing/buildbot/chromium_win8_trybot.json (right): https://codereview.chromium.org/600263002/diff/20001/testing/buildbot/chromium_win8_trybot.json#newcode11 testing/buildbot/chromium_win8_trybot.json:11: "content_browsertests", I think this test needs ...
6 years, 2 months ago (2014-09-25 00:01:22 UTC) #6
ananta
https://codereview.chromium.org/600263002/diff/20001/testing/buildbot/chromium_win8_trybot.json File testing/buildbot/chromium_win8_trybot.json (right): https://codereview.chromium.org/600263002/diff/20001/testing/buildbot/chromium_win8_trybot.json#newcode11 testing/buildbot/chromium_win8_trybot.json:11: "content_browsertests", On 2014/09/25 00:01:21, scottmg wrote: > I think ...
6 years, 2 months ago (2014-09-25 00:17:06 UTC) #7
jam
lgtm https://codereview.chromium.org/600263002/diff/40001/testing/buildbot/chromium.win.json File testing/buildbot/chromium.win.json (right): https://codereview.chromium.org/600263002/diff/40001/testing/buildbot/chromium.win.json#newcode335 testing/buildbot/chromium.win.json:335: "content_browsertests", nit: tabbing?
6 years, 2 months ago (2014-09-25 00:42:22 UTC) #8
ananta
https://codereview.chromium.org/600263002/diff/40001/testing/buildbot/chromium.win.json File testing/buildbot/chromium.win.json (right): https://codereview.chromium.org/600263002/diff/40001/testing/buildbot/chromium.win.json#newcode335 testing/buildbot/chromium.win.json:335: "content_browsertests", On 2014/09/25 00:42:22, jam wrote: > nit: tabbing? ...
6 years, 2 months ago (2014-09-25 01:05:46 UTC) #9
ananta
+dmazzoni for accessibility fixes.
6 years, 2 months ago (2014-09-25 01:07:28 UTC) #11
dmazzoni
lgtm for accessibility I filed http://crbug.com/417758 to remove the rest of the accessibility code added ...
6 years, 2 months ago (2014-09-25 17:17:46 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/600263002/80001
6 years, 2 months ago (2014-09-25 18:07:05 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_swarming/builds/13121)
6 years, 2 months ago (2014-09-25 21:11:45 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/600263002/100001
6 years, 2 months ago (2014-09-25 23:31:52 UTC) #18
commit-bot: I haz the power
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/71600)
6 years, 2 months ago (2014-09-26 00:12:16 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/600263002/120001
6 years, 2 months ago (2014-09-26 00:52:29 UTC) #22
commit-bot: I haz the power
Committed patchset #7 (id:120001) as b84b73abdacda8281b202ff2344abb3d414aab75
6 years, 2 months ago (2014-09-26 02:20:41 UTC) #23
commit-bot: I haz the power
Patchset 7 (id:??) landed as https://crrev.com/b1f9313020d4c1e8619f21b5320a0255bf313b4b Cr-Commit-Position: refs/heads/master@{#296859}
6 years, 2 months ago (2014-09-26 02:21:18 UTC) #24
scottmg
6 years, 2 months ago (2014-09-26 02:42:55 UTC) #25
Message was sent while issue was closed.
https://codereview.chromium.org/600263002/diff/120001/chrome/chrome.isolate
File chrome/chrome.isolate (right):

https://codereview.chromium.org/600263002/diff/120001/chrome/chrome.isolate#n...
chrome/chrome.isolate:72: '<(PRODUCT_DIR)/<(version_full).manifest',
duplicate block, just move the new file down here in a followup

Powered by Google App Engine
This is Rietveld 408576698