Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
697383002:
Fix for the font files being maped multiple times (Fontconfig). (Closed)
|
Created:
6 years, 1 month ago by Krzysztof Olczyk Modified:
5 years, 10 months ago Reviewers:
bungeman-chromium, Daniel Erat, eae, jam, reed2, Dominik Röttsches, jln (very slow on Chromium), behdad_google, jochen (gone - plz use gerrit), brettw CC:
chromium-reviews, darin-cc_chromium.org, jam, Mostyn Bramley-Moore Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix 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}
Committed: https://crrev.com/57a1895e40238c00c372a44be32294d7de3da0ab
Cr-Commit-Position: refs/heads/master@{#314314}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rewritten CL to address the fact that streams can be created and passed between different threads. #
Total comments: 13
Patch Set 3 : Fixes after review. #
Total comments: 10
Patch Set 4 : Further fixups #Patch Set 5 : Rebased to master #Patch Set 6 : Rebase to HEAD #
Messages
Total messages: 53 (17 generated)
kolczyk@opera.com changed reviewers: + behdad@google.com, brettw@chromium.org, dominik.rottsches@intel.com, eae@chromium.org
Hi behdad, eae, you were reviewing https://codereview.chromium.org/307243002 by Dominik which introduced (revealed) the resource leak in Linux I'm trying to fix here. Could you please take a look at this CL? Thanks in advance!
eae@chromium.org changed reviewers: + derat@chromium.org
+derat Dan would be a better reviewer for this.
i don't have much experience with the underlying skia code here. do SkMemoryStreams get deleted behind-the-scenes by something in skia? when does that happen, and on which thread? https://codereview.chromium.org/697383002/diff/1/content/common/font_config_i... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/697383002/diff/1/content/common/font_config_i... content/common/font_config_ipc_linux.cc:28: public base::SupportsWeakPtr<FontConfigIPC::FontMemoryStream> { do you actually need SupportsWeakPtr here and the PurgeFontStreamCache() call below, or could you just make this class's d'tor remove the corresponding stale entry from the cache? that seems like it'd avoid a lot of unnecessary iterating through the cache.
https://codereview.chromium.org/697383002/diff/1/content/common/font_config_i... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/697383002/diff/1/content/common/font_config_i... content/common/font_config_ipc_linux.cc:28: public base::SupportsWeakPtr<FontConfigIPC::FontMemoryStream> { On 2014/11/06 20:33:55, Daniel Erat wrote: > do you actually need SupportsWeakPtr here and the PurgeFontStreamCache() call > below, or could you just make this class's d'tor remove the corresponding stale > entry from the cache? that seems like it'd avoid a lot of unnecessary iterating > through the cache. Yes, there was also the problem with the fact that openStream is called from different threads and the streams are passed between threads. I've upload the new CL which just mmaps the font files by itself and create memory streams out of it. Could you take a look please?
On 2014/11/07 11:38:01, Krzysztof Olczyk wrote: > https://codereview.chromium.org/697383002/diff/1/content/common/font_config_i... > File content/common/font_config_ipc_linux.cc (right): > > https://codereview.chromium.org/697383002/diff/1/content/common/font_config_i... > content/common/font_config_ipc_linux.cc:28: public > base::SupportsWeakPtr<FontConfigIPC::FontMemoryStream> { > On 2014/11/06 20:33:55, Daniel Erat wrote: > > do you actually need SupportsWeakPtr here and the PurgeFontStreamCache() call > > below, or could you just make this class's d'tor remove the corresponding > stale > > entry from the cache? that seems like it'd avoid a lot of unnecessary > iterating > > through the cache. > > Yes, there was also the problem with the fact that openStream is called from > different threads and the streams are passed between threads. > > I've upload the new CL which just mmaps the font files by itself and create > memory streams out of it. > > Could you take a look please? Just for completeness, the threads that call openStream() and manipulate it are: * render thread * CompositorRaster thread
derat@chromium.org changed reviewers: + bungeman@chromium.org
https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... content/common/font_config_ipc_linux.cc:48: uint32_t font_id() const { return font_id_; } move inlined method up, just after c'tor https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... content/common/font_config_ipc_linux.cc:53: ~MappedFontFile() { override https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... content/common/font_config_ipc_linux.cc:131: base::AutoLock lock(stream_opening_lock_); i'm nervous about this lock being held across the duration of the method, and across an IPC. if two requests come in on different threads around the same time, is it okay if we actually load the font file twice? if so, it may be better/faster to do: { base::AutoLock // check cache, return if present } // do IPC, initialize MappedFontFile { base::AutoLock // insert into |mapped_font_files_| and return } https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... File content/common/font_config_ipc_linux.h (right): https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... content/common/font_config_ipc_linux.h:45: void RemoveMappedFontFile(MappedFontFile* mapped_font_file); add a comment: // Removes |mapped_font_file| from |mapped_font_files_|. // Does not delete the passed-in object. https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... content/common/font_config_ipc_linux.h:48: base::Lock stream_opening_lock_; please add a comment documenting what this lock protects https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... content/common/font_config_ipc_linux.h:49: // Maps font identity ID to the memory-mapped file with font data: s/:/./
We might be able to modify the interaction between fallback and our blink side font cache to not cause creation of new SkTypeface instances just for font size changes.
Hi Daniel, I've applied your suggestions. Could you take a look once more at this old review? https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... content/common/font_config_ipc_linux.cc:48: uint32_t font_id() const { return font_id_; } On 2014/11/07 at 15:07:36, Daniel Erat (OOO 1-14 to 1-19) wrote: > move inlined method up, just after c'tor Done https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... content/common/font_config_ipc_linux.cc:53: ~MappedFontFile() { On 2014/11/07 at 15:07:36, Daniel Erat (OOO 1-14 to 1-19) wrote: > override This destructor is not virtual. https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... content/common/font_config_ipc_linux.cc:131: base::AutoLock lock(stream_opening_lock_); On 2014/11/07 at 15:07:36, Daniel Erat (OOO 1-14 to 1-19) wrote: > i'm nervous about this lock being held across the duration of the method, and across an IPC. if two requests come in on different threads around the same time, is it okay if we actually load the font file twice? if so, it may be better/faster to do: > > { > base::AutoLock > // check cache, return if present > } > // do IPC, initialize MappedFontFile > { > base::AutoLock > // insert into |mapped_font_files_| and return > } Done https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... File content/common/font_config_ipc_linux.h (right): https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... content/common/font_config_ipc_linux.h:45: void RemoveMappedFontFile(MappedFontFile* mapped_font_file); On 2014/11/07 at 15:07:36, Daniel Erat (OOO 1-14 to 1-19) wrote: > add a comment: > > // Removes |mapped_font_file| from |mapped_font_files_|. > // Does not delete the passed-in object. Done https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... content/common/font_config_ipc_linux.h:48: base::Lock stream_opening_lock_; On 2014/11/07 at 15:07:36, Daniel Erat (OOO 1-14 to 1-19) wrote: > please add a comment documenting what this lock protects Done https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... content/common/font_config_ipc_linux.h:49: // Maps font identity ID to the memory-mapped file with font data: On 2014/11/07 at 15:07:36, Daniel Erat (OOO 1-14 to 1-19) wrote: > s/:/./ Done https://codereview.chromium.org/697383002/diff/20001/content/common/font_conf... content/common/font_config_ipc_linux.h:49: // Maps font identity ID to the memory-mapped file with font data: On 2014/11/07 at 15:07:36, Daniel Erat (OOO 1-14 to 1-19) wrote: > s/:/./ Done
looks okay to me with some comments, but please have someone more familiar with this code also take a look (i'll be ooo for a few days, too) https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... content/common/font_config_ipc_linux.cc:55: ~MappedFontFile() { as i understand it, you should be using 'override' on the d'tor whenever the class derives from some other class https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... File content/common/font_config_ipc_linux.h (right): https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... content/common/font_config_ipc_linux.h:43: friend class MappedFontFile; (mostly a note for myself: i initially thought that this 'friend' would be unnecessary, but apparently this is a bug that was addressed in c++11) https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... content/common/font_config_ipc_linux.h:52: base::Lock stream_opening_lock_; nit: just rename this to |lock_| since it protects the map in addition to stream-opening?
I've uploaded further fixups based on your comments. Could you suggest who can be more familiar with this? https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... content/common/font_config_ipc_linux.cc:55: ~MappedFontFile() { On 2015/01/14 at 23:40:27, Daniel Erat (OOO 1-14 to 1-19) wrote: > as i understand it, you should be using 'override' on the d'tor whenever the class derives from some other class When it derives from base class which has virtual constructor. If I add override here, I'm getting, unsurprisingly, the following error: ../../content/common/font_config_ipc_linux.cc:55:21: error: only virtual member functions can be marked 'override' https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... File content/common/font_config_ipc_linux.h (right): https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... content/common/font_config_ipc_linux.h:43: friend class MappedFontFile; On 2015/01/14 at 23:40:27, Daniel Erat (OOO 1-14 to 1-19) wrote: > (mostly a note for myself: i initially thought that this 'friend' would be unnecessary, but apparently this is a bug that was addressed in c++11) Oh, I wasn't aware it was addressed in C++11, thanks for noting. I will remove this "friend" as all toolchains are C++11 now. https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... content/common/font_config_ipc_linux.h:52: base::Lock stream_opening_lock_; On 2015/01/14 at 23:40:27, Daniel Erat (OOO 1-14 to 1-19) wrote: > nit: just rename this to |lock_| since it protects the map in addition to stream-opening? Done.
derat@chromium.org changed reviewers: + reed@chromium.org
lgtm bungeman@ and reed@ look like they worked on this in 2013; they might have opinions. https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... content/common/font_config_ipc_linux.cc:55: ~MappedFontFile() { On 2015/01/15 09:05:28, Krzysztof Olczyk wrote: > On 2015/01/14 at 23:40:27, Daniel Erat (OOO 1-14 to 1-19) wrote: > > as i understand it, you should be using 'override' on the d'tor whenever the > class derives from some other class > > When it derives from base class which has virtual constructor. If I add override > here, I'm getting, unsurprisingly, the following error: > > ../../content/common/font_config_ipc_linux.cc:55:21: error: only virtual member > functions can be marked 'override' ah, thanks; i didn't realize that RefCountedThreadSafe doesn't declare a virtual d'tor. does 'virtual' work here? (maybe none of this matters since this d'tor is private.) https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... File content/common/font_config_ipc_linux.h (right): https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... content/common/font_config_ipc_linux.h:43: friend class MappedFontFile; On 2015/01/15 09:05:28, Krzysztof Olczyk wrote: > On 2015/01/14 at 23:40:27, Daniel Erat (OOO 1-14 to 1-19) wrote: > > (mostly a note for myself: i initially thought that this 'friend' would be > unnecessary, but apparently this is a bug that was addressed in c++11) > > Oh, I wasn't aware it was addressed in C++11, thanks for noting. I will remove > this "friend" as all toolchains are C++11 now. sorry, i meant it the other way around: apparently nested classes automatically got access to their outer class's members before, but maybe don't now. (if the bots compile without 'friend', though, feel free to leave it out!)
https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... content/common/font_config_ipc_linux.cc:55: ~MappedFontFile() { On 2015/01/15 15:55:54, Daniel Erat (OOO 1-14 to 1-19) wrote: > On 2015/01/15 09:05:28, Krzysztof Olczyk wrote: > > On 2015/01/14 at 23:40:27, Daniel Erat (OOO 1-14 to 1-19) wrote: > > > as i understand it, you should be using 'override' on the d'tor whenever the > > class derives from some other class > > > > When it derives from base class which has virtual constructor. If I add > override > > here, I'm getting, unsurprisingly, the following error: > > > > ../../content/common/font_config_ipc_linux.cc:55:21: error: only virtual > member > > functions can be marked 'override' > > ah, thanks; i didn't realize that RefCountedThreadSafe doesn't declare a virtual > d'tor. does 'virtual' work here? > > (maybe none of this matters since this d'tor is private.) It uses CRTP in order to match right destructor instead. I don't think we need a virtual destructor as this class is not going to be inherited. https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... File content/common/font_config_ipc_linux.h (right): https://codereview.chromium.org/697383002/diff/40001/content/common/font_conf... content/common/font_config_ipc_linux.h:43: friend class MappedFontFile; On 2015/01/15 15:55:54, Daniel Erat (OOO 1-14 to 1-19) wrote: > On 2015/01/15 09:05:28, Krzysztof Olczyk wrote: > > On 2015/01/14 at 23:40:27, Daniel Erat (OOO 1-14 to 1-19) wrote: > > > (mostly a note for myself: i initially thought that this 'friend' would be > > unnecessary, but apparently this is a bug that was addressed in c++11) > > > > Oh, I wasn't aware it was addressed in C++11, thanks for noting. I will remove > > this "friend" as all toolchains are C++11 now. > > sorry, i meant it the other way around: apparently nested classes automatically > got access to their outer class's members before, but maybe don't now. (if the > bots compile without 'friend', though, feel free to leave it out!) I think it's other way round. I'm pretty sure friend was needed in C++03, but looks like it is no longer needed in C++11. Bots do compile.
lgtm, with the exception of the commit message (description). The last paragraph isn't really true: > This CL keeps track of the MemoryStreams created for given > font ID in FontConfigIPC and returns the existing one if > it already exists. This is possible because SkMemoryStream > is refcounted and Typeface just ref()s it. What this CL actually does is keep track of the mmaps for each font ID, and then creates a new stream wrapper around that mmap on future requests to openStream for that font ID instead of doing a full IPC request and mmapping the resulting new fd. This caught my attention because SkStreams will soon *not* be ref counted (it never made sense to begin with).
lgtm
On 2015/01/16 22:32:42, bungeman2 wrote: > lgtm, with the exception of the commit message (description). The last paragraph > isn't really true: > > > This CL keeps track of the MemoryStreams created for given > > font ID in FontConfigIPC and returns the existing one if > > it already exists. This is possible because SkMemoryStream > > is refcounted and Typeface just ref()s it. > > What this CL actually does is keep track of the mmaps for each font ID, and then > creates a new stream wrapper around that mmap on future requests to openStream > for that font ID instead of doing a full IPC request and mmapping the resulting > new fd. > > This caught my attention because SkStreams will soon *not* be ref counted (it > never made sense to begin with). Right, thanks for pointing this out. Updated title.
The CQ bit was checked by kolczyk@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/697383002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
kolczyk@opera.com changed reviewers: + jln@chromium.org
Integration failed due to, apparently, missing an LGTM from somebody from OWNER. @jlm, as you are listed in the owners for content/common, could you please take a look at this one?
On 2015/01/20 12:09:16, Krzysztof Olczyk wrote: > Integration failed due to, apparently, missing an LGTM from somebody from OWNER. > @jlm, as you are listed in the owners for content/common, could you please take > a look at this one? Ping: jln, could you take a look at this, for some sites it means a large decrease in memory consumption.
kolczyk@opera.com changed reviewers: + jam@chromium.org, jochen@chromium.org
Hi jam, jochen, it looks like you are suitable owners for these files too, could you please take a look at this change?
rubberstamp lgtm based on previous reviews
The CQ bit was checked by bungeman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/697383002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/01/26 16:14:05, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) From the log: ** Presubmit ERRORS ** Banned functions were used. content/common/font_config_ipc_linux.cc:37: New code should not use ScopedAllowIO. Post a task to the blocking pool or the FILE thread instead. content/common/font_config_ipc_linux.cc:61: New code should not use ScopedAllowIO. Post a task to the blocking pool or the FILE thread instead. It appears that in the past this code was avoiding all Chromium IO checks in general by using Skia to map and unmap (manage) the file descriptor.
I think this is OK to add. You'll have to bypass presubmit check.
The CQ bit was checked by bungeman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/697383002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/78db5e535ef48c596223fe272572e5679fbb44fd Cr-Commit-Position: refs/heads/master@{#313102}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/873213003/ by tbarzic@chromium.org. The reason for reverting is: Suspected for breaking security_OpenFDs on cros (http://crbug.com/452227): 01/26 13:31:23.658 ERROR|security_O:0147| Found unexpected fds in chrome type=renderer: set(['0500 /usr/share/fonts/croscore/Tinos-Regular.ttf', '0500 /usr/share/fonts/noto/NotoSans-Regular.ttc', '0500 /usr/share/fonts/noto/NotoSans-Bold.ttc']) 01/26 13:31:23.658 DEBUG|security_O:0138| Found pid 8280 for chrome 01/26 13:31:23.660 ERROR|security_O:0147| Found unexpected fds in chrome type=renderer: set(['0500 /usr/share/fonts/noto/NotoSans-Regular.ttc']) .
The CQ bit was checked by kolczyk@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/697383002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for content/common/font_config_ipc_linux.cc:
While running git apply --index -3 -p1;
error: patch failed: content/common/font_config_ipc_linux.cc:14
Falling back to three-way merge...
Applied patch to 'content/common/font_config_ipc_linux.cc' with conflicts.
U content/common/font_config_ipc_linux.cc
Patch: content/common/font_config_ipc_linux.cc
Index: content/common/font_config_ipc_linux.cc
diff --git a/content/common/font_config_ipc_linux.cc
b/content/common/font_config_ipc_linux.cc
index
c64aebf66a57fa3c432646d1906e5a15bf12b2ef..f24e06caeaafe1a0e2135475052668e1a75d733c
100644
--- a/content/common/font_config_ipc_linux.cc
+++ b/content/common/font_config_ipc_linux.cc
@@ -14,8 +14,11 @@
#include "base/debug/trace_event.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 "skia/ext/refptr.h"
#include "skia/ext/skia_utils_base.h"
#include "third_party/skia/include/core/SkData.h"
@@ -23,14 +26,45 @@
namespace content {
-// Return a stream from the file descriptor, or NULL on failure.
-SkStream* 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));
@@ -96,6 +130,14 @@ bool FontConfigIPC::matchFamilyName(const char familyName[],
SkStream* 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);
@@ -118,9 +160,23 @@ SkStream* FontConfigIPC::openStream(const FontIdentity&
identity) {
return NULL;
}
- SkStream* 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
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by kolczyk@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/697383002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...) ios_rel_device_ninja_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kolczyk@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/697383002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/57a1895e40238c00c372a44be32294d7de3da0ab Cr-Commit-Position: refs/heads/master@{#314314} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
