Commit 57a1895e authored by kolczyk's avatar kolczyk Committed by Commit bot

Fix for font files being mmaped multiple times (Fontconfig).

Before this change, when there were a lot of font fallbacks happening on
the web site, the fallback font was mmaped multiple times, reaching up to
200 times in the TC url: http://jsfiddle.net/p5pe81vs/, leading to OOMs
and crashes for renderer process.

This happens after the change introduced in
https://codereview.chromium.org/307243002

This CL keeps track of the mmaps for each font ID to avoid any further
unnecessary IPC requests and mmaps for future requests of the same font
ID that would result in new FD.

BUG=430021

NOPRESUBMIT=true
brettw indicates this use of ScopedAllowIO is acceptable.
Previous code eluded IO checks, this change makes the IO use find-able.

Committed: https://crrev.com/78db5e535ef48c596223fe272572e5679fbb44fd
Cr-Commit-Position: refs/heads/master@{#313102}

Review URL: https://codereview.chromium.org/697383002

Cr-Commit-Position: refs/heads/master@{#314314}
parent 12ac7fa8
......@@ -13,8 +13,11 @@
#include <unistd.h>
#include "base/files/file_util.h"
#include "base/files/memory_mapped_file.h"
#include "base/memory/ref_counted.h"
#include "base/pickle.h"
#include "base/posix/unix_domain_socket_linux.h"
#include "base/threading/thread_restrictions.h"
#include "base/trace_event/trace_event.h"
#include "skia/ext/refptr.h"
#include "skia/ext/skia_utils_base.h"
......@@ -24,14 +27,45 @@
namespace content {
// Return a stream from the file descriptor, or NULL on failure.
SkStreamAsset* StreamFromFD(int fd) {
skia::RefPtr<SkData> data = skia::AdoptRef(SkData::NewFromFD(fd));
if (!data) {
return NULL;
class FontConfigIPC::MappedFontFile
: public base::RefCountedThreadSafe<MappedFontFile> {
public:
explicit MappedFontFile(uint32_t font_id) : font_id_(font_id) {}
uint32_t font_id() const { return font_id_; }
bool Initialize(int fd) {
base::ThreadRestrictions::ScopedAllowIO allow_mmap;
return mapped_font_file_.Initialize(base::File(fd));
}
SkMemoryStream* CreateMemoryStream() {
DCHECK(mapped_font_file_.IsValid());
auto data = skia::AdoptRef(SkData::NewWithProc(
mapped_font_file_.data(), mapped_font_file_.length(),
&MappedFontFile::ReleaseProc, this));
if (!data)
return nullptr;
AddRef();
return new SkMemoryStream(data.get());
}
private:
friend class base::RefCountedThreadSafe<MappedFontFile>;
~MappedFontFile() {
auto font_config = static_cast<FontConfigIPC*>(FontConfigIPC::RefGlobal());
font_config->RemoveMappedFontFile(this);
}
return new SkMemoryStream(data.get());
}
static void ReleaseProc(const void* ptr, size_t length, void* context) {
base::ThreadRestrictions::ScopedAllowIO allow_munmap;
static_cast<MappedFontFile*>(context)->Release();
}
uint32_t font_id_;
base::MemoryMappedFile mapped_font_file_;
};
void CloseFD(int fd) {
int err = IGNORE_EINTR(close(fd));
......@@ -97,6 +131,14 @@ bool FontConfigIPC::matchFamilyName(const char familyName[],
SkStreamAsset* FontConfigIPC::openStream(const FontIdentity& identity) {
TRACE_EVENT0("sandbox_ipc", "FontConfigIPC::openStream");
{
base::AutoLock lock(lock_);
auto mapped_font_files_it = mapped_font_files_.find(identity.fID);
if (mapped_font_files_it != mapped_font_files_.end())
return mapped_font_files_it->second->CreateMemoryStream();
}
Pickle request;
request.WriteInt(METHOD_OPEN);
request.WriteUInt32(identity.fID);
......@@ -119,9 +161,23 @@ SkStreamAsset* FontConfigIPC::openStream(const FontIdentity& identity) {
return NULL;
}
SkStreamAsset* stream = StreamFromFD(result_fd);
CloseFD(result_fd);
return stream;
scoped_refptr<MappedFontFile> mapped_font_file =
new MappedFontFile(identity.fID);
if (!mapped_font_file->Initialize(result_fd))
return nullptr;
{
base::AutoLock lock(lock_);
auto mapped_font_files_it =
mapped_font_files_.insert(std::make_pair(mapped_font_file->font_id(),
mapped_font_file.get())).first;
return mapped_font_files_it->second->CreateMemoryStream();
}
}
void FontConfigIPC::RemoveMappedFontFile(MappedFontFile* mapped_font_file) {
base::AutoLock lock(lock_);
mapped_font_files_.erase(mapped_font_file->font_id());
}
} // namespace content
......
......@@ -6,6 +6,8 @@
#define CONTENT_COMMON_FONT_CONFIG_IPC_LINUX_H_
#include "base/compiler_specific.h"
#include "base/containers/hash_tables.h"
#include "base/synchronization/lock.h"
#include "third_party/skia/include/core/SkStream.h"
#include "third_party/skia/include/core/SkTypeface.h"
#include "third_party/skia/include/ports/SkFontConfigInterface.h"
......@@ -41,7 +43,18 @@ class FontConfigIPC : public SkFontConfigInterface {
};
private:
class MappedFontFile;
// Removes |mapped_font_file| from |mapped_font_files_|.
// Does not delete the passed-in object.
void RemoveMappedFontFile(MappedFontFile* mapped_font_file);
const int fd_;
// Lock preventing multiple threads from opening font file and accessing
// |mapped_font_files_| map at the same time.
base::Lock lock_;
// Maps font identity ID to the memory-mapped file with font data.
base::hash_map<uint32_t, MappedFontFile*> mapped_font_files_;
};
} // namespace content
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment