Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12918004:
mmap the opened file descriptor in the renderer process, close the fd in the browser process. (Closed)
|
Created:
7 years, 9 months ago by reed1 Modified:
7 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, bungeman-skia Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
Descriptionmmap the opened file descriptor in the renderer process, close the fd in the browser process.
This avoids running out of available file descriptors when many tabs are opened, since we now
never leave a file open for very long, and rely on the kernel to clean up the mmap'd regions
when the tab is closed (or the stream is safely destroyed by the font cache).
BUG=196549
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188813
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 3
Patch Set 3 : #
Total comments: 15
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #
Total comments: 1
Messages
Total messages: 27 (0 generated)
https://codereview.chromium.org/12918004/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_sandbox_host_linux.cc (right): https://codereview.chromium.org/12918004/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_sandbox_host_linux.cc:229: close(result_fd); Please, add HANDLE_EINTR + DCHECK https://codereview.chromium.org/12918004/diff/1/content/common/font_config_ip... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/12918004/diff/1/content/common/font_config_ip... content/common/font_config_ipc_linux.cc:162: return new SkFileDescriptorStream(result_fd); You say that the caller should close the fd, but the caller doesn't close the fd. DCHECK(HANDLE_EINTR(close(result_fd)));
> DCHECK(HANDLE_EINTR(close(result_fd))); DCHECK(! ...) of course, sorry.
https://codereview.chromium.org/12918004/diff/1/content/common/font_config_ip... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/12918004/diff/1/content/common/font_config_ip... content/common/font_config_ipc_linux.cc:162: return new SkFileDescriptorStream(result_fd); On 2013/03/18 16:07:21, Julien Tinnes wrote: > You say that the caller should close the fd, but the caller doesn't close the > fd. Good point. It is not the imnmediate caller (this line) that closes it, but the browser process that performed the open (render_sandbox_host_linux.cc:229). > > DCHECK(HANDLE_EINTR(close(result_fd)));
On 2013/03/18 16:12:47, reed1 wrote: > Good point. It is not the imnmediate caller (this line) that closes it, but the > browser process that performed the open (render_sandbox_host_linux.cc:229). Yes, be careful that file descriptor are per-process. They don't have any cross-process meaning. Speaking of scoping, you might want to use a ScopedFD (see base/).
Ah, got it. Thanks all. I did not realize it was being dup'd in SendRendererReply. I thought we were just pickling the 'int' in the raw. New patch also performs the close in the stream constructor.
On 2013/03/18 16:16:51, Julien Tinnes wrote: > On 2013/03/18 16:12:47, reed1 wrote: > > > Good point. It is not the imnmediate caller (this line) that closes it, but > the > > browser process that performed the open (render_sandbox_host_linux.cc:229). > > Yes, be careful that file descriptor are per-process. They don't have any > cross-process meaning. Just to make sure I'm clear: you DO need to close it here as well.
On 2013/03/18 16:19:24, Julien Tinnes wrote: > On 2013/03/18 16:16:51, Julien Tinnes wrote: > > On 2013/03/18 16:12:47, reed1 wrote: > > > > > Good point. It is not the imnmediate caller (this line) that closes it, but > > the > > > browser process that performed the open (render_sandbox_host_linux.cc:229). > > > > Yes, be careful that file descriptor are per-process. They don't have any > > cross-process meaning. > > Just to make sure I'm clear: you DO need to close it here as well. Done. close() is called in the stream constructor (as it was in the original code).
https://codereview.chromium.org/12918004/diff/7001/content/browser/renderer_h... File content/browser/renderer_host/render_sandbox_host_linux.cc (right): https://codereview.chromium.org/12918004/diff/7001/content/browser/renderer_h... content/browser/renderer_host/render_sandbox_host_linux.cc:229: close(result_fd); Please, add HANDLE_EINTR and DCHECK (or CHECK) here. DCHECK(!HANDLE_EINTR(close(result_fd))); https://codereview.chromium.org/12918004/diff/7001/content/common/font_config... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/12918004/diff/7001/content/common/font_config... content/common/font_config_ipc_linux.cc:39: close(fd); You need to HANDLE_EINTR and DCHECK (or CHECK) here. DCHECK(!HANDLE_EINTR(close(fd)));
https://codereview.chromium.org/12918004/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_sandbox_host_linux.cc (right): https://codereview.chromium.org/12918004/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_sandbox_host_linux.cc:229: close(result_fd); On 2013/03/18 16:07:21, Julien Tinnes wrote: > Please, add HANDLE_EINTR + DCHECK Done. https://codereview.chromium.org/12918004/diff/7001/content/browser/renderer_h... File content/browser/renderer_host/render_sandbox_host_linux.cc (right): https://codereview.chromium.org/12918004/diff/7001/content/browser/renderer_h... content/browser/renderer_host/render_sandbox_host_linux.cc:229: close(result_fd); On 2013/03/18 16:26:06, Julien Tinnes wrote: > Please, add HANDLE_EINTR and DCHECK (or CHECK) here. > > DCHECK(!HANDLE_EINTR(close(result_fd))); Done.
https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... content/common/font_config_ipc_linux.cc:24: class SkFileDescriptorStream : public SkStream { General nit: why are you using a 4 spaces indent? https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... content/common/font_config_ipc_linux.cc:27: memory_ = NULL; You don't want to use an initialization list instead? https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... content/common/font_config_ipc_linux.cc:30: // this ensures that if we fail in the constructor, we will safely Nit: s/this/This https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... content/common/font_config_ipc_linux.cc:31: // ignore all subsequent calls to read() because we will always trim I don't think this currently works. You just do remaining = length_ - offset_, with remaining being unsigned. If you don't want to have an explicit Init() method, make everything very clear by having a bool initialized_ and adding a CHECK() for it in your methods maybe? https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... content/common/font_config_ipc_linux.cc:32: // the requested size down to 0 Nit: final "." https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... content/common/font_config_ipc_linux.cc:36: if (fstat(fd, &st)) close the fd here as well ? https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... content/common/font_config_ipc_linux.cc:48: virtual ~SkFileDescriptorStream() { This works, because length == 0 will make the kernel EINVAL, but wouldn't it be nicer to if (!memory_) ? Also I would at least DCHECK it to catch bugs. https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... content/common/font_config_ipc_linux.cc:67: if (buffer) Shouldn't we just CHECK buffer at the start of this function or here ?
https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... content/common/font_config_ipc_linux.cc:24: class SkFileDescriptorStream : public SkStream { On 2013/03/18 18:25:37, Julien Tinnes wrote: > General nit: why are you using a 4 spaces indent? Done. https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... content/common/font_config_ipc_linux.cc:27: memory_ = NULL; On 2013/03/18 18:25:37, Julien Tinnes wrote: > You don't want to use an initialization list instead? Done. https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... content/common/font_config_ipc_linux.cc:30: // this ensures that if we fail in the constructor, we will safely On 2013/03/18 18:25:37, Julien Tinnes wrote: > Nit: s/this/This Done. https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... content/common/font_config_ipc_linux.cc:32: // the requested size down to 0 On 2013/03/18 18:25:37, Julien Tinnes wrote: > Nit: final "." Done. https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... content/common/font_config_ipc_linux.cc:36: if (fstat(fd, &st)) On 2013/03/18 18:25:37, Julien Tinnes wrote: > close the fd here as well ? Done. https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... content/common/font_config_ipc_linux.cc:48: virtual ~SkFileDescriptorStream() { On 2013/03/18 18:25:37, Julien Tinnes wrote: > This works, because length == 0 will make the kernel EINVAL, but wouldn't it be > nicer to if (!memory_) ? > > Also I would at least DCHECK it to catch bugs. Done. https://codereview.chromium.org/12918004/diff/10001/content/common/font_confi... content/common/font_config_ipc_linux.cc:67: if (buffer) On 2013/03/18 18:25:37, Julien Tinnes wrote: > Shouldn't we just CHECK buffer at the start of this function or here ? buffer == NULL is defined to mean "skip", so it is valid for it to be null.
patchset #5 just fixes indents. PTAL
lgtm https://codereview.chromium.org/12918004/diff/13001/content/common/font_confi... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/12918004/diff/13001/content/common/font_confi... content/common/font_config_ipc_linux.cc:25: public: Nit: public should be only indented with one space, and the rest indented one space from it.
Also in your commit message make sure you use "BUG=196549". I don't think the current syntax works.
On 2013/03/18 19:23:03, Julien Tinnes wrote: > Also in your commit message make sure you use "BUG=196549". I don't think the > current syntax works. Done
https://codereview.chromium.org/12918004/diff/13001/content/common/font_confi... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/12918004/diff/13001/content/common/font_confi... content/common/font_config_ipc_linux.cc:25: public: On 2013/03/18 19:21:28, Julien Tinnes wrote: > Nit: public should be only indented with one space, and the rest indented one > space from it. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/reed@google.com/12918004/8002
Presubmit check for 12918004-8002 failed and returned exit status 1.
INFO:root:Found 2 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
content/browser/renderer_host/render_sandbox_host_linux.cc
adding piman for OWNER
https://codereview.chromium.org/12918004/diff/8002/content/browser/renderer_h... File content/browser/renderer_host/render_sandbox_host_linux.cc (right): https://codereview.chromium.org/12918004/diff/8002/content/browser/renderer_h... content/browser/renderer_host/render_sandbox_host_linux.cc:231: DCHECK(!HANDLE_EINTR(close(result_fd))); I'm not the biggest fan of side-effects inside of DCHECK - we compile them out on official builds. https://codereview.chromium.org/12918004/diff/8002/content/common/font_config... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/12918004/diff/8002/content/common/font_config... content/common/font_config_ipc_linux.cc:29: DCHECK(!HANDLE_EINTR(close(fd))); no side effects inside of DCHECK please. https://codereview.chromium.org/12918004/diff/8002/content/common/font_config... content/common/font_config_ipc_linux.cc:34: DCHECK(!HANDLE_EINTR(close(fd))); no side effects inside of DCHECK please. https://codereview.chromium.org/12918004/diff/8002/content/common/font_config... content/common/font_config_ipc_linux.cc:157: return new SkFileDescriptorStream(result_fd); If the map failed inside of the constructor, shouldn't we return NULL here?
On 2013/03/18 19:49:14, piman wrote: > I'm not the biggest fan of side-effects inside of DCHECK - we compile them out > on official builds. I can't believe I'm discovering that now. I was certain we guaranteed CHECK-like functions to execute what's inside. Bad news. (And sorry about that Mike).
https://codereview.chromium.org/12918004/diff/8002/content/browser/renderer_h... File content/browser/renderer_host/render_sandbox_host_linux.cc (right): https://codereview.chromium.org/12918004/diff/8002/content/browser/renderer_h... content/browser/renderer_host/render_sandbox_host_linux.cc:231: DCHECK(!HANDLE_EINTR(close(result_fd))); On 2013/03/18 19:49:14, piman wrote: > I'm not the biggest fan of side-effects inside of DCHECK - we compile them out > on official builds. Done. https://codereview.chromium.org/12918004/diff/8002/content/common/font_config... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/12918004/diff/8002/content/common/font_config... content/common/font_config_ipc_linux.cc:157: return new SkFileDescriptorStream(result_fd); On 2013/03/18 19:49:14, piman wrote: > If the map failed inside of the constructor, shouldn't we return NULL here? Done.
lgtm
Thanks all. In hindsight, I think all of the changes to font_config_ipc_linux.cc were unnecessary to fix the bug at hand. However, I am fine with the (revised) MMapStream as well, and it restores the older mmap behavior, so overall it makes the entire set-of-changes smaller. The biggest assist in testing this was the following shell command (thanks bungeman) lsof | grep fonts | grep chrome | wc Which showed me when I was leaking descriptors after closing tabs.
Message was sent while issue was closed.
Committed patchset #6 manually as r188813 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/12918004/diff/3006/content/browser/renderer_h... File content/browser/renderer_host/render_sandbox_host_linux.cc (right): https://codereview.chromium.org/12918004/diff/3006/content/browser/renderer_h... content/browser/renderer_host/render_sandbox_host_linux.cc:231: DCHECK(!HANDLE_EINTR(close(result_fd))); We missed moving this out of DCHECK. And it's arguably the most important. Sorry again about that.
Message was sent while issue was closed.
On 2013/03/19 10:23:27, Julien Tinnes wrote: > https://codereview.chromium.org/12918004/diff/3006/content/browser/renderer_h... > File content/browser/renderer_host/render_sandbox_host_linux.cc (right): > > https://codereview.chromium.org/12918004/diff/3006/content/browser/renderer_h... > content/browser/renderer_host/render_sandbox_host_linux.cc:231: > DCHECK(!HANDLE_EINTR(close(result_fd))); > We missed moving this out of DCHECK. And it's arguably the most important. Sorry > again about that. Good catch. Naturally, I did my testing in a debug build, so that close() was executed, and I missed the problem. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
