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

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
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.

Description

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} 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+79 lines, -10 lines) Patch
M content/common/font_config_ipc_linux.h View 1 2 3 4 5 2 chunks +13 lines, -0 lines 0 comments Download
M content/common/font_config_ipc_linux.cc View 1 2 3 4 5 4 chunks +66 lines, -10 lines 0 comments Download

Messages

Total messages: 53 (17 generated)
Krzysztof Olczyk
Hi behdad, eae, you were reviewing https://codereview.chromium.org/307243002 by Dominik which introduced (revealed) the resource leak ...
6 years, 1 month ago (2014-11-04 10:46:13 UTC) #2
eae
+derat Dan would be a better reviewer for this.
6 years, 1 month ago (2014-11-06 20:04:48 UTC) #4
Daniel Erat
i don't have much experience with the underlying skia code here. do SkMemoryStreams get deleted ...
6 years, 1 month ago (2014-11-06 20:33:55 UTC) #5
Krzysztof Olczyk
https://codereview.chromium.org/697383002/diff/1/content/common/font_config_ipc_linux.cc File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/697383002/diff/1/content/common/font_config_ipc_linux.cc#newcode28 content/common/font_config_ipc_linux.cc:28: public base::SupportsWeakPtr<FontConfigIPC::FontMemoryStream> { On 2014/11/06 20:33:55, Daniel Erat wrote: ...
6 years, 1 month ago (2014-11-07 11:38:01 UTC) #6
Krzysztof Olczyk
On 2014/11/07 11:38:01, Krzysztof Olczyk wrote: > https://codereview.chromium.org/697383002/diff/1/content/common/font_config_ipc_linux.cc > File content/common/font_config_ipc_linux.cc (right): > > https://codereview.chromium.org/697383002/diff/1/content/common/font_config_ipc_linux.cc#newcode28 ...
6 years, 1 month ago (2014-11-07 11:41:48 UTC) #7
Daniel Erat
https://codereview.chromium.org/697383002/diff/20001/content/common/font_config_ipc_linux.cc File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/697383002/diff/20001/content/common/font_config_ipc_linux.cc#newcode48 content/common/font_config_ipc_linux.cc:48: uint32_t font_id() const { return font_id_; } move inlined ...
6 years, 1 month ago (2014-11-07 15:07:36 UTC) #9
Dominik Röttsches
We might be able to modify the interaction between fallback and our blink side font ...
6 years ago (2014-11-24 09:43:11 UTC) #10
Krzysztof Olczyk
Hi Daniel, I've applied your suggestions. Could you take a look once more at this ...
5 years, 11 months ago (2015-01-14 12:42:23 UTC) #11
Daniel Erat
looks okay to me with some comments, but please have someone more familiar with this ...
5 years, 11 months ago (2015-01-14 23:40:27 UTC) #12
Krzysztof Olczyk
I've uploaded further fixups based on your comments. Could you suggest who can be more ...
5 years, 11 months ago (2015-01-15 09:05:28 UTC) #13
Daniel Erat
lgtm bungeman@ and reed@ look like they worked on this in 2013; they might have ...
5 years, 11 months ago (2015-01-15 15:55:54 UTC) #15
Krzysztof Olczyk
https://codereview.chromium.org/697383002/diff/40001/content/common/font_config_ipc_linux.cc File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/697383002/diff/40001/content/common/font_config_ipc_linux.cc#newcode55 content/common/font_config_ipc_linux.cc:55: ~MappedFontFile() { On 2015/01/15 15:55:54, Daniel Erat (OOO 1-14 ...
5 years, 11 months ago (2015-01-15 17:44:11 UTC) #16
bungeman-chromium
lgtm, with the exception of the commit message (description). The last paragraph isn't really true: ...
5 years, 11 months ago (2015-01-16 22:32:42 UTC) #17
behdad_google
lgtm
5 years, 11 months ago (2015-01-16 22:36:41 UTC) #18
Krzysztof Olczyk
On 2015/01/16 22:32:42, bungeman2 wrote: > lgtm, with the exception of the commit message (description). ...
5 years, 11 months ago (2015-01-19 11:13:44 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/697383002/60001
5 years, 11 months ago (2015-01-19 11:14:35 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/36211)
5 years, 11 months ago (2015-01-19 11:17:49 UTC) #23
Krzysztof Olczyk
Integration failed due to, apparently, missing an LGTM from somebody from OWNER. @jlm, as you ...
5 years, 11 months ago (2015-01-20 12:09:16 UTC) #25
bungeman-chromium
On 2015/01/20 12:09:16, Krzysztof Olczyk wrote: > Integration failed due to, apparently, missing an LGTM ...
5 years, 11 months ago (2015-01-22 21:13:57 UTC) #26
Krzysztof Olczyk
Hi jam, jochen, it looks like you are suitable owners for these files too, could ...
5 years, 10 months ago (2015-01-26 15:02:52 UTC) #28
jam
rubberstamp lgtm based on previous reviews
5 years, 10 months ago (2015-01-26 16:04:17 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/697383002/60001
5 years, 10 months ago (2015-01-26 16:10:52 UTC) #31
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/36848)
5 years, 10 months ago (2015-01-26 16:14:05 UTC) #33
bungeman-chromium
On 2015/01/26 16:14:05, I haz the power (commit-bot) wrote: > Try jobs failed on following ...
5 years, 10 months ago (2015-01-26 17:07:08 UTC) #34
brettw
I think this is OK to add. You'll have to bypass presubmit check.
5 years, 10 months ago (2015-01-26 19:20:16 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/697383002/60001
5 years, 10 months ago (2015-01-26 19:43:00 UTC) #37
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 10 months ago (2015-01-26 19:44:29 UTC) #38
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/78db5e535ef48c596223fe272572e5679fbb44fd Cr-Commit-Position: refs/heads/master@{#313102}
5 years, 10 months ago (2015-01-26 19:46:01 UTC) #39
tbarzic
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/873213003/ by tbarzic@chromium.org. ...
5 years, 10 months ago (2015-01-26 22:52:17 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/697383002/60001
5 years, 10 months ago (2015-02-03 07:50:52 UTC) #42
commit-bot: I haz the power
Failed to apply patch for content/common/font_config_ipc_linux.cc: While running git apply --index -3 -p1; error: patch ...
5 years, 10 months ago (2015-02-03 07:51:27 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/697383002/80001
5 years, 10 months ago (2015-02-03 09:52:44 UTC) #47
commit-bot: I haz the power
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_compile_dbg_32_ng/builds/19730) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator/builds/54789) ios_rel_device_ng ...
5 years, 10 months ago (2015-02-03 09:55:53 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/697383002/100001
5 years, 10 months ago (2015-02-03 12:06:16 UTC) #51
commit-bot: I haz the power
Committed patchset #6 (id:100001)
5 years, 10 months ago (2015-02-03 12:07:31 UTC) #52
commit-bot: I haz the power
5 years, 10 months ago (2015-02-03 12:09:12 UTC) #53
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/57a1895e40238c00c372a44be32294d7de3da0ab
Cr-Commit-Position: refs/heads/master@{#314314}

Powered by Google App Engine
This is Rietveld 408576698