Skip to content

Commit

Permalink
Move Blink Sandbox IPC to Mojo Calls
Browse files Browse the repository at this point in the history
This is a reland of
https://chromium-review.googlesource.com/c/chromium/src/+/1109964
Tbr'ing previous reviewers from that CL as the exact change has been
previously reviewed there.

The revert was done manually in response to flakiness of viz_browser
tests in MSAN. See issue https://crbug.com/860349 - my analysis is in
issue https://crbug.com/860445 where I disable this test. In short, I
believe my CL exposed a previously existing race condition in that test.

Instead of Chromium IPC macro-defined messages or Mojo, Chrome on Linux
uses hand-pickled IPC messages through a special purpose file descriptor
to send messages from the renderer to the browser host in order to
access FontConfig for font matching and font fallback. This system is
described in docs/linux_sandbox_ipc.md.

For the "Font Matching by Full Font Name / PS Name" effort, see issue
828317, additional out of process font methods are needed. Instead of
adding them to this legacy hand-written IPC, we modernize the Linux
Sandbox IPC mechanism and upgrade it to using Mojo interface definitions
and a service architecture, in which a font service running in an
unsandboxed utility process answers FontConfig requests from the
renderer.

Previous CLs [1], [2] prepared the Font Service to have testing and
additional font fallback and render-style-for-strike methods. Now we can
move Blink over to using this Mojo interface and remove the traditional
sandbox IPC handlers since we do not use the file descriptor based IPC
anymore for FontConfig acces.

For more details, please refer to the design doc in issue 839344.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1091754
[2] https://chromium-review.googlesource.com/c/chromium/src/+/1087951

Bug: 855021
Change-Id: I74663c5685a7797089e4d69354453146c245e20a
Tbr: skyostil@chromium.org, michaelpg@chromium.org, rsesek@chromium.org, halliwell@chromium.org, thestig@chromium.org, piman@chromium.org, eae@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/1127028
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572930}
  • Loading branch information
drott authored and Commit Bot committed Jul 6, 2018
1 parent 48898fe commit ac24004
Show file tree
Hide file tree
Showing 60 changed files with 379 additions and 1,170 deletions.
5 changes: 5 additions & 0 deletions chrome/app/generated_resources.grd
Original file line number Diff line number Diff line change
Expand Up @@ -3184,6 +3184,11 @@ are declared in build/common.gypi.
<message name="IDS_UTILITY_PROCESS_FILE_UTILITY_NAME" desc="The name of the utility process used for various Chrome specific file operations.">
Chrome File Utilities
</message>
<if expr="is_linux">
<message name="IDS_UTILITY_PROCESS_FONT_SERVICE_UTILITY_NAME" desc="The name of the utility process used for FontConfig operations for Chrome on Linux.">
Linux Font Service
</message>
</if>
<if expr="not is_android">
<message name="IDS_UTILITY_PROCESS_PROFILE_IMPORTER_NAME" desc="The name of the utility process used for importing profiles.">
Profile Importer
Expand Down
10 changes: 10 additions & 0 deletions chrome/browser/chrome_content_browser_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,10 @@
#include "chrome/browser/offline_pages/offline_page_url_loader_request_interceptor.h"
#endif

#if defined(OS_LINUX)
#include "components/services/font/public/interfaces/constants.mojom.h"
#endif

using base::FileDescriptor;
using content::BrowserThread;
using content::BrowserURLHandler;
Expand Down Expand Up @@ -3638,6 +3642,12 @@ void ChromeContentBrowserClient::RegisterOutOfProcessServices(
&l10n_util::GetStringUTF16, IDS_UTILITY_PROCESS_FILE_UTILITY_NAME);
#endif

#if defined(OS_LINUX)
(*services)[font_service::mojom::kServiceName] =
base::BindRepeating(&l10n_util::GetStringUTF16,
IDS_UTILITY_PROCESS_FONT_SERVICE_UTILITY_NAME);
#endif

(*services)[patch::mojom::kServiceName] = base::BindRepeating(
&l10n_util::GetStringUTF16, IDS_UTILITY_PROCESS_PATCH_NAME);

Expand Down
62 changes: 40 additions & 22 deletions chrome/renderer/pepper/pepper_flash_font_file_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,38 @@
#include "ppapi/host/ppapi_host.h"
#include "ppapi/proxy/ppapi_messages.h"
#include "ppapi/proxy/serialized_structs.h"

#if defined(OS_LINUX) || defined(OS_OPENBSD)
#include "content/public/child/child_process_sandbox_support_linux.h"
#include "content/public/common/common_sandbox_support_linux.h"
#elif defined(OS_WIN)
#include "third_party/skia/include/core/SkFontMgr.h"

#if !defined(OS_WIN)
namespace {

SkFontStyle::Weight PepperWeightToSkWeight(int pepper_weight) {
switch (pepper_weight) {
case PP_BROWSERFONT_TRUSTED_WEIGHT_100:
return SkFontStyle::kThin_Weight;
case PP_BROWSERFONT_TRUSTED_WEIGHT_200:
return SkFontStyle::kExtraLight_Weight;
case PP_BROWSERFONT_TRUSTED_WEIGHT_300:
return SkFontStyle::kLight_Weight;
case PP_BROWSERFONT_TRUSTED_WEIGHT_400:
return SkFontStyle::kNormal_Weight;
case PP_BROWSERFONT_TRUSTED_WEIGHT_500:
return SkFontStyle::kMedium_Weight;
case PP_BROWSERFONT_TRUSTED_WEIGHT_600:
return SkFontStyle::kSemiBold_Weight;
case PP_BROWSERFONT_TRUSTED_WEIGHT_700:
return SkFontStyle::kBold_Weight;
case PP_BROWSERFONT_TRUSTED_WEIGHT_800:
return SkFontStyle::kExtraBold_Weight;
case PP_BROWSERFONT_TRUSTED_WEIGHT_900:
return SkFontStyle::kBlack_Weight;
default:
NOTREACHED();
return SkFontStyle::kInvisible_Weight;
}
}

} // namespace
#endif

PepperFlashFontFileHost::PepperFlashFontFileHost(
Expand All @@ -28,24 +54,23 @@ PepperFlashFontFileHost::PepperFlashFontFileHost(
const ppapi::proxy::SerializedFontDescription& description,
PP_PrivateFontCharset charset)
: ResourceHost(host->GetPpapiHost(), instance, resource) {
#if defined(OS_LINUX) || defined(OS_OPENBSD)
fd_.reset(content::MatchFontWithFallback(
description.face,
description.weight >= PP_BROWSERFONT_TRUSTED_WEIGHT_BOLD,
description.italic,
charset,
PP_BROWSERFONT_TRUSTED_FAMILY_DEFAULT));
#elif defined(OS_WIN)
int weight = description.weight;
int weight;
#if defined(OS_WIN)
// TODO(https://crbug.com/857388): Figure out why this diverges from the
// #else block.
weight = description.weight;
if (weight == FW_DONTCARE)
weight = SkFontStyle::kNormal_Weight;
#else
weight = PepperWeightToSkWeight(description.weight);
#endif
SkFontStyle style(weight, SkFontStyle::kNormal_Width,
description.italic ? SkFontStyle::kItalic_Slant
: SkFontStyle::kUpright_Slant);
sk_sp<SkFontMgr> font_mgr(SkFontMgr::RefDefault());

typeface_ = sk_sp<SkTypeface>(
font_mgr->matchFamilyStyle(description.face.c_str(), style));
#endif // defined(OS_LINUX) || defined(OS_OPENBSD)
}

PepperFlashFontFileHost::~PepperFlashFontFileHost() {}
Expand All @@ -64,12 +89,6 @@ bool PepperFlashFontFileHost::GetFontData(uint32_t table,
void* buffer,
size_t* length) {
bool result = false;
#if defined(OS_LINUX) || defined(OS_OPENBSD)
int fd = fd_.get();
if (fd != -1)
result = content::GetFontTable(fd, table, 0 /* offset */,
reinterpret_cast<uint8_t*>(buffer), length);
#elif defined(OS_WIN)
if (typeface_) {
table = base::ByteSwap(table);
if (buffer == NULL) {
Expand All @@ -82,7 +101,6 @@ bool PepperFlashFontFileHost::GetFontData(uint32_t table,
result = true;
}
}
#endif
return result;
}

Expand Down
8 changes: 0 additions & 8 deletions chrome/renderer/pepper/pepper_flash_font_file_host.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,8 @@
#include "ppapi/c/private/pp_private_font_charset.h"
#include "ppapi/host/resource_host.h"

#if defined(OS_LINUX) || defined(OS_OPENBSD)
#include "base/files/scoped_file.h"
#elif defined(OS_WIN)
#include "third_party/skia/include/core/SkRefCnt.h"
#include "third_party/skia/include/core/SkTypeface.h"
#endif

namespace content {
class RendererPpapiHost;
Expand Down Expand Up @@ -50,11 +46,7 @@ class PepperFlashFontFileHost : public ppapi::host::ResourceHost {
uint32_t table);
bool GetFontData(uint32_t table, void* buffer, size_t* length);

#if defined(OS_LINUX) || defined(OS_OPENBSD)
base::ScopedFD fd_;
#elif defined(OS_WIN)
sk_sp<SkTypeface> typeface_;
#endif

DISALLOW_COPY_AND_ASSIGN(PepperFlashFontFileHost);
};
Expand Down
4 changes: 4 additions & 0 deletions chrome/utility/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,10 @@ static_library("utility") {
if (is_android || enable_extensions) {
deps += [ "//chrome/services/media_gallery_util:lib" ]
}

if (is_linux && !is_android) {
deps += [ "//components/services/font:lib" ]
}
}

if (!is_android) {
Expand Down
1 change: 1 addition & 0 deletions chromecast/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ cast_source_set("browser") {

deps += [
"//components/metrics:serialization",
"//components/services/font:lib",
"//third_party/fontconfig",
]
}
Expand Down
1 change: 1 addition & 0 deletions chromecast/browser/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ include_rules = [
"+components/prefs",
"+components/pref_registry",
"+components/proxy_config",
"+components/services/font",
"+components/storage_monitor",
"+components/user_prefs",
"+components/version_info",
Expand Down
13 changes: 13 additions & 0 deletions chromecast/browser/cast_content_browser_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,11 @@
#include "media/mojo/services/media_service.h" // nogncheck
#endif // ENABLE_MOJO_MEDIA_IN_BROWSER_PROCESS

#if defined(OS_LINUX)
#include "components/services/font/font_service_app.h" // nogncheck
#include "components/services/font/public/interfaces/constants.mojom.h" // nogncheck
#endif

#if defined(OS_LINUX) || defined(OS_ANDROID)
#include "components/crash/content/app/breakpad_linux.h"
#include "components/crash/content/browser/crash_handler_host_linux.h"
Expand Down Expand Up @@ -683,6 +688,14 @@ void CastContentBrowserClient::RegisterInProcessServices(
#endif
}

void CastContentBrowserClient::RegisterOutOfProcessServices(
OutOfProcessServiceMap* services) {
#if defined(OS_LINUX)
(*services)[font_service::mojom::kServiceName] =
base::BindRepeating(&base::ASCIIToUTF16, "Font Service");
#endif
}

std::unique_ptr<base::Value>
CastContentBrowserClient::GetServiceManifestOverlay(
base::StringPiece service_name) {
Expand Down
1 change: 1 addition & 0 deletions chromecast/browser/cast_content_browser_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ class CastContentBrowserClient : public content::ContentBrowserClient {
void RegisterInProcessServices(
StaticServiceMap* services,
content::ServiceManagerConnection* connection) override;
void RegisterOutOfProcessServices(OutOfProcessServiceMap* services) override;
std::unique_ptr<base::Value> GetServiceManifestOverlay(
base::StringPiece service_name) override;
void GetAdditionalMappedFilesForChildProcess(
Expand Down
9 changes: 7 additions & 2 deletions components/services/font/public/cpp/font_service_thread.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ FontServiceThread::FontServiceThread(mojom::FontServicePtr font_service)
: base::Thread(kFontThreadName),
font_service_info_(font_service.PassInterface()),
weak_factory_(this) {
DETACH_FROM_THREAD(thread_checker_);
Start();
}

Expand All @@ -31,7 +32,7 @@ bool FontServiceThread::MatchFamilyName(
SkFontConfigInterface::FontIdentity* out_font_identity,
SkString* out_family_name,
SkFontStyle* out_style) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK_NE(GetThreadId(), base::PlatformThread::CurrentId());

bool out_valid = false;
// This proxies to the other thread, which proxies to mojo. Only on the reply
Expand All @@ -54,6 +55,7 @@ bool FontServiceThread::FallbackFontForCharacter(
std::string* out_family_name,
bool* out_is_bold,
bool* out_is_italic) {
DCHECK_NE(GetThreadId(), base::PlatformThread::CurrentId());
bool out_valid = false;
base::WaitableEvent done_event;
task_runner()->PostTask(
Expand All @@ -74,6 +76,7 @@ bool FontServiceThread::FontRenderStyleForStrike(
bool is_bold,
float device_scale_factor,
font_service::mojom::FontRenderStylePtr* out_font_render_style) {
DCHECK_NE(GetThreadId(), base::PlatformThread::CurrentId());
bool out_valid = false;
base::WaitableEvent done_event;
task_runner()->PostTask(
Expand All @@ -92,6 +95,7 @@ void FontServiceThread::MatchFontWithFallback(
uint32_t charset,
uint32_t fallback_family_type,
base::File* out_font_file_handle) {
DCHECK_NE(GetThreadId(), base::PlatformThread::CurrentId());
base::WaitableEvent done_event;
task_runner()->PostTask(
FROM_HERE,
Expand All @@ -103,7 +107,7 @@ void FontServiceThread::MatchFontWithFallback(

scoped_refptr<MappedFontFile> FontServiceThread::OpenStream(
const SkFontConfigInterface::FontIdentity& identity) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK_NE(GetThreadId(), base::PlatformThread::CurrentId());

base::File stream_file;
// This proxies to the other thread, which proxies to mojo. Only on the
Expand Down Expand Up @@ -210,6 +214,7 @@ void FontServiceThread::OpenStreamImpl(base::WaitableEvent* done_event,
void FontServiceThread::OnOpenStreamComplete(base::WaitableEvent* done_event,
base::File* output_file,
base::File file) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
pending_waitable_events_.erase(done_event);
*output_file = std::move(file);
done_event->Signal();
Expand Down
1 change: 1 addition & 0 deletions content/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ include_rules = [
# settings, packaging details, installation or crash reporting.

"+components/services/filesystem",
"+components/services/font",

"+crypto",
"+grit/blink_resources.h",
Expand Down
5 changes: 0 additions & 5 deletions content/app/content_main_runner_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,11 @@
#if defined(OS_LINUX)
#include "base/native_library.h"
#include "base/rand_util.h"
#include "content/common/font_config_ipc_linux.h"
#include "services/service_manager/zygote/common/common_sandbox_support_linux.h"
#include "third_party/blink/public/platform/web_font_render_style.h"
#include "third_party/boringssl/src/include/openssl/crypto.h"
#include "third_party/boringssl/src/include/openssl/rand.h"
#include "third_party/skia/include/core/SkFontMgr.h"
#include "third_party/skia/include/ports/SkFontConfigInterface.h"
#include "third_party/skia/include/ports/SkFontMgr_android.h"
#include "third_party/webrtc_overrides/init_webrtc.h" // nogncheck

Expand Down Expand Up @@ -389,9 +387,6 @@ void PreSandboxInit() {
#endif
InitializeWebRtcModule();

SkFontConfigInterface::SetGlobal(
sk_make_sp<FontConfigIPC>(service_manager::GetSandboxFD()));

// Set the android SkFontMgr for blink. We need to ensure this is done
// before the sandbox is initialized to allow the font manager to access
// font configuration files on disk.
Expand Down
5 changes: 3 additions & 2 deletions content/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1249,8 +1249,6 @@ jumbo_source_set("browser") {
"renderer_host/event_with_latency_info.h",
"renderer_host/file_utilities_host_impl.cc",
"renderer_host/file_utilities_host_impl.h",
"renderer_host/font_utils_linux.cc",
"renderer_host/font_utils_linux.h",
"renderer_host/frame_connector_delegate.cc",
"renderer_host/frame_connector_delegate.h",
"renderer_host/frame_metadata_util.cc",
Expand Down Expand Up @@ -2046,6 +2044,9 @@ jumbo_source_set("browser") {
if (use_pangocairo) {
sources += [ "renderer_host/pepper/pepper_truetype_font_list_pango.cc" ]
}
if (is_linux && !is_android) {
deps += [ "//components/services/font:ppapi_fontconfig_matching" ]
}
}

if (enable_library_cdms) {
Expand Down
Loading

0 comments on commit ac24004

Please sign in to comment.