Skip to content

Commit

Permalink
[unseasoned-pdf] Patch GetFontData() in renderers
Browse files Browse the repository at this point in the history
Patches the GetFontData() call in all renderer processes to fix a crash
on Windows when trying to load the unseasoned PDF viewer. GetFontData()
was patched in PPAPI processes, but the unseasoned PDF viewer runs
inside a renderer process now.

Also renames MaybeInitializeGDI() to something less misleading.

Unfortunately, it's not possible to replicate this issue in a browser
test at this time; see crbug.com/1238982 for details.

Fixed: 1238982
Change-Id: Ifd5ad8c1d7820edd4f50807959029e25e10a7594
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3091845
Auto-Submit: K. Moon <kmoon@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Commit-Queue: K. Moon <kmoon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#911565}
  • Loading branch information
kmoon-work authored and Chromium LUCI CQ committed Aug 12, 2021
1 parent fab1b18 commit a0fe0fd
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 6 deletions.
2 changes: 1 addition & 1 deletion chrome/app/chrome_main_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1170,7 +1170,7 @@ void ChromeMainDelegate::PreSandboxStartup() {
crash_keys::SetCrashKeysFromCommandLine(command_line);

#if BUILDFLAG(ENABLE_PDF)
MaybeInitializeGDI();
MaybePatchGdiGetFontData();
#endif
}

Expand Down
11 changes: 8 additions & 3 deletions chrome/child/pdf_child_init.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ DWORD WINAPI GetFontDataPatch(HDC hdc,

} // namespace

void MaybeInitializeGDI() {
void MaybePatchGdiGetFontData() {
#if defined(OS_WIN)
// Only patch utility processes which explicitly need GDI.
sandbox::policy::SandboxType service_sandbox_type =
Expand All @@ -59,6 +59,11 @@ void MaybeInitializeGDI() {
service_sandbox_type == sandbox::policy::SandboxType::kPpapi ||
service_sandbox_type == sandbox::policy::SandboxType::kPrintCompositor ||
service_sandbox_type == sandbox::policy::SandboxType::kPdfConversion;

// TODO(crbug.com/1239330): Only apply to renderers containing PDF content.
if (service_sandbox_type == sandbox::policy::SandboxType::kRenderer)
need_gdi = true;

if (!need_gdi)
return;

Expand All @@ -69,8 +74,8 @@ void MaybeInitializeGDI() {
HMODULE module = CURRENT_MODULE();
#endif // defined(COMPONENT_BUILD)

// Need to patch GetFontData() for font loading to work correctly. This can be
// removed once PDFium switches to use Skia. https://crbug.com/pdfium/11
// Need to patch GetFontData() for font loading to work correctly.
// TODO(crbug.com/pdfium/11): Can be removed once PDFium switches to use Skia.
static base::NoDestructor<base::win::IATPatchFunction> patch_get_font_data;
patch_get_font_data->PatchFromModule(
module, "gdi32.dll", "GetFontData",
Expand Down
4 changes: 2 additions & 2 deletions chrome/child/pdf_child_init.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#error "PDF must be enabled"
#endif

// Initializes child-process specific code for the PDF module.
void MaybeInitializeGDI();
// Possibly patches GDI's `GetFontData()` for use by PDFium.
void MaybePatchGdiGetFontData();

#endif // CHROME_CHILD_PDF_CHILD_INIT_H_

0 comments on commit a0fe0fd

Please sign in to comment.