Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1248713002:
ozone: ClientPixmapManager passes VGEM fd from browser to renderer. (Closed)
|
Created:
5 years, 5 months ago by dshwang Modified:
5 years, 3 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, kalyank, nasko+codewatch_chromium.org, ozone-reviews_chromium.org, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionozone: ClientPixmapManager passes VGEM fd from browser to renderer.
Browser and Renderer have the singleton ClientPixmapManager instance.
BrowserClientPixmapManager and ChildClientPixmapManager communicates
via existing Browser-Renderer IPC to transfer virtual device FD.
e.g. VGEM FD in Ozone GBM
Browser sends virtual device fd to all renderers when the channel
is established. Renderer creates the singleton ChildClientPixmapManager
at the time. It's similar to how MACOSX shares |io_surface_manager_token_|.
BUG=475633
Committed: https://crrev.com/9e59fefbb589611ce7fb9823754f21a48bd09887
Cr-Commit-Position: refs/heads/master@{#346433}
Patch Set 1 #Patch Set 2 : rebase to lastest crrev.com/1128113011 #Patch Set 3 : use one-way IPC, instead of sync round-trip IPC #Patch Set 4 : rebase to latest crrev.com/1128113011 #
Total comments: 4
Patch Set 5 : rename to ClientNativePixmapFactory #
Total comments: 10
Patch Set 6 : follow IOSurface pattern. remove wrapper classes #
Total comments: 14
Patch Set 7 : use ScopedFD instead of FileDescriptor #
Total comments: 6
Patch Set 8 : scoped_ptr<base::ScopedFD> instead of base::ScopedFD* #
Total comments: 15
Patch Set 9 : resolve reviewers' comments #
Total comments: 4
Patch Set 10 : rollback to use PlatformThreadRef #
Total comments: 1
Patch Set 11 : fix base::ThreadRestrictions::AssertWaitAllowed crash #
Total comments: 17
Patch Set 12 : Add IOSurfaceManager to ThreadRestrictions friend list. #
Total comments: 7
Patch Set 13 : remove waitable_event #
Total comments: 4
Patch Set 14 : remove local var ScopedFD #
Total comments: 1
Patch Set 15 : remove redundant lock. make content_unittests run #
Total comments: 8
Patch Set 16 : remove redundant Pass() #Messages
Total messages: 75 (16 generated)
dongseong.hwang@intel.com changed reviewers: + reveman@chromium.org, spang@chromium.org, tiago.vignatti@intel.com
reveman, spang, could you review? It's blocked by crrev.com/1128113011: ozone: Introduce ClientPixmapManager for Renderer and Browser.
Patchset #4 (id:60001) has been deleted
not lgtm https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... File content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc (right): https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc:66: pool, FROM_HERE, base::Bind(&DupVirtualDeviceFd, virtual_device_.fd), I'm pretty sure that passing access to vgem by dup()ing a shared file descriptor ends up giving all renderers a common handle namespace, which is completely unacceptable from a security perspective. We need to open a new vgem fd for each renderer.
On 2015/08/04 17:47:31, spang wrote: > not lgtm > > https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... > File content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc (right): > > https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... > content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc:66: pool, > FROM_HERE, base::Bind(&DupVirtualDeviceFd, virtual_device_.fd), > I'm pretty sure that passing access to vgem by dup()ing a shared file descriptor > ends up giving all renderers a common handle namespace, which is completely > unacceptable from a security perspective. > > We need to open a new vgem fd for each renderer. FWIW You don't need to land this infrastructure to land VgemPixmap. Until we get the sandbox whitelist updated, you already have to test with --no-sandbox. So you can just do this: ClientNativePixmapFactoryGbm() { static const char kVgemPath[] = "/dev/dri/renderD129"; vgem_fd_ = open(kVgemPath, O_RDWR); if (vgem_fd_ < 0) PLOG(ERROR) << "open: " << kVgemPath; } I've been testing using the above code successfully. I would recommend to just land the VgemPixmap & ClientNativePixmapFactoryGbm ignoring sandbox as your next step.
Thank you for nice review! https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... File content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc (right): https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc:66: pool, FROM_HERE, base::Bind(&DupVirtualDeviceFd, virtual_device_.fd), On 2015/08/04 17:47:31, spang wrote: > I'm pretty sure that passing access to vgem by dup()ing a shared file descriptor > ends up giving all renderers a common handle namespace, which is completely > unacceptable from a security perspective. > > We need to open a new vgem fd for each renderer. I don't fully catch up which point is vulnerable from security point of view. This pattern follows MacOSX code, which also gives all renderers |io_surface_manager_token_| in https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... If it's problematic, how about round-trip communication in Patch Set 2; https://codereview.chromium.org/1248713002/diff/20001/content/child/child_nat... When renderer initializes ChildNativePixmapManagerOzone, it sends sync message to browser to get vgem fd, although reveman didn't like this sync round-trip. > FWIW You don't need to land this infrastructure to land VgemPixmap. > > Until we get the sandbox whitelist updated, you already have to test with > --no-sandbox. So you can just do this: > > ClientNativePixmapFactoryGbm() { > static const char kVgemPath[] = "/dev/dri/renderD129"; > vgem_fd_ = open(kVgemPath, O_RDWR); > if (vgem_fd_ < 0) > PLOG(ERROR) << "open: " << kVgemPath; > } > > I've been testing using the above code successfully. > > I would recommend to just land the VgemPixmap & ClientNativePixmapFactoryGbm > ignoring sandbox as your next step. Ok, The next step is https://codereview.chromium.org/1134993003/ with the code. After the CL landed, I'll come back to this CL.
Patchset #5 (id:100001) has been deleted
On 2015/08/05 09:44:26, dshwang wrote: > Thank you for nice review! > > https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... > File content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc (right): > > https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... > content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc:66: pool, > FROM_HERE, base::Bind(&DupVirtualDeviceFd, virtual_device_.fd), > On 2015/08/04 17:47:31, spang wrote: > > I'm pretty sure that passing access to vgem by dup()ing a shared file > descriptor > > ends up giving all renderers a common handle namespace, which is completely > > unacceptable from a security perspective. > > > > We need to open a new vgem fd for each renderer. > > I don't fully catch up which point is vulnerable from security point of view. > This pattern follows MacOSX code, which also gives all renderers > |io_surface_manager_token_| in > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... The problem is calling DRM_IOCTL_PRIME_FD_TO_HANDLE creates a handle that is visible to all of the renderers that share the same vgem cloned via dup(). So to access another renderer's buffers all you need to do is guess a small integer. Instead of calling dup() we need to call open(). > > If it's problematic, how about round-trip communication in Patch Set 2; > https://codereview.chromium.org/1248713002/diff/20001/content/child/child_nat... > When renderer initializes ChildNativePixmapManagerOzone, it sends sync message > to browser to get vgem fd, although reveman didn't like this sync round-trip. > > > FWIW You don't need to land this infrastructure to land VgemPixmap. > > > > Until we get the sandbox whitelist updated, you already have to test with > > --no-sandbox. So you can just do this: > > > > ClientNativePixmapFactoryGbm() { > > static const char kVgemPath[] = "/dev/dri/renderD129"; > > vgem_fd_ = open(kVgemPath, O_RDWR); > > if (vgem_fd_ < 0) > > PLOG(ERROR) << "open: " << kVgemPath; > > } > > > > I've been testing using the above code successfully. > > > > I would recommend to just land the VgemPixmap & ClientNativePixmapFactoryGbm > > ignoring sandbox as your next step. > > Ok, The next step is https://codereview.chromium.org/1134993003/ with the code. > After the CL landed, I'll come back to this CL.
On 2015/08/12 19:57:26, spang wrote: > On 2015/08/05 09:44:26, dshwang wrote: > > Thank you for nice review! > > > > > https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... > > File content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc > (right): > > > > > https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... > > content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc:66: pool, > > FROM_HERE, base::Bind(&DupVirtualDeviceFd, virtual_device_.fd), > > On 2015/08/04 17:47:31, spang wrote: > > > I'm pretty sure that passing access to vgem by dup()ing a shared file > > descriptor > > > ends up giving all renderers a common handle namespace, which is completely > > > unacceptable from a security perspective. > > > > > > We need to open a new vgem fd for each renderer. > > > > I don't fully catch up which point is vulnerable from security point of view. > > This pattern follows MacOSX code, which also gives all renderers > > |io_surface_manager_token_| in > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > The problem is calling DRM_IOCTL_PRIME_FD_TO_HANDLE creates a handle that is > visible to all of the renderers that share the same vgem cloned via dup(). So to > access another renderer's buffers all you need to do is guess a small integer. > > Instead of calling dup() we need to call open(). Now it dup /dev/dri/card1, not dmabuf_fd. There is not difference between dup(vgem_device_fd) and open(/dev/dri/card1). All states are kept by kernel. DRM_IOCTL_PRIME_FD_TO_HANDLE requires both vgem device fd and dmabuf_fd to import vgem bo handle. On the other hands, GbmPixmap exports dmabuf_fd and sends it to renderer. Another renderer needs the dambuf_fd, not small integer, to see another renderer's content. This situation is same to Android SurfaceTexture and MacOS IoSurface.
On 2015/08/13 at 15:57:19, dongseong.hwang wrote: > On 2015/08/12 19:57:26, spang wrote: > > On 2015/08/05 09:44:26, dshwang wrote: > > > Thank you for nice review! > > > > > > > > https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... > > > File content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc > > (right): > > > > > > > > https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... > > > content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc:66: pool, > > > FROM_HERE, base::Bind(&DupVirtualDeviceFd, virtual_device_.fd), > > > On 2015/08/04 17:47:31, spang wrote: > > > > I'm pretty sure that passing access to vgem by dup()ing a shared file > > > descriptor > > > > ends up giving all renderers a common handle namespace, which is completely > > > > unacceptable from a security perspective. > > > > > > > > We need to open a new vgem fd for each renderer. > > > > > > I don't fully catch up which point is vulnerable from security point of view. > > > This pattern follows MacOSX code, which also gives all renderers > > > |io_surface_manager_token_| in > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > The problem is calling DRM_IOCTL_PRIME_FD_TO_HANDLE creates a handle that is > > visible to all of the renderers that share the same vgem cloned via dup(). So to > > access another renderer's buffers all you need to do is guess a small integer. > > > > Instead of calling dup() we need to call open(). > > Now it dup /dev/dri/card1, not dmabuf_fd. There is not difference between dup(vgem_device_fd) and open(/dev/dri/card1). All states are kept by kernel. > DRM_IOCTL_PRIME_FD_TO_HANDLE requires both vgem device fd and dmabuf_fd to import vgem bo handle. > On the other hands, GbmPixmap exports dmabuf_fd and sends it to renderer. Another renderer needs the dambuf_fd, not small integer, to see another renderer's content. This situation is same to Android SurfaceTexture and MacOS IoSurface. dup()ing and sending an FD to a renderer process is safe from a renderer isolation perspective as only the receiving process will have access to the resource referred to by the FD. When passing a FD using the regular IPC system in chromium, the kernel takes care of dup()ing the FD for the receiving process. Only the receiving renderer process will be able to use the FD. The FD is not an integer that another process could guess as that would not be secure at all. Note: when sending a FD to another process in chromium we typically need to dup() it as the IPC system will automatically close it (this is why fd.auto_close is for) if sending the message fails.
I'm seeing the term "VgemFd" used in some places and "VirtualDevice" in other places. Can we make this consistent? We already use VgemFd in some places before this patch and I prefer that term as it's not as vague as virtualDevice. Can we use VgemFd everywhere? https://codereview.chromium.org/1248713002/diff/120001/content/browser/gpu/br... File content/browser/gpu/browser_client_native_pixmap_factory_ozone.cc (right): https://codereview.chromium.org/1248713002/diff/120001/content/browser/gpu/br... content/browser/gpu/browser_client_native_pixmap_factory_ozone.cc:64: base::SequencedWorkerPool* pool = content::BrowserThread::GetBlockingPool(); why do we need to use blocking thread pool to dup() this fd? https://codereview.chromium.org/1248713002/diff/120001/content/browser/gpu/br... File content/browser/gpu/browser_client_native_pixmap_factory_ozone.h (right): https://codereview.chromium.org/1248713002/diff/120001/content/browser/gpu/br... content/browser/gpu/browser_client_native_pixmap_factory_ozone.h:15: class BrowserClientNativePixmapFactory : public ui::ClientNativePixmapFactory { This class doesn't seem useful to me. Afaict, it's just a large wrapper around sending one IPC message to the renderer with the vgem_fd. We can replace all this with a few lines of code in RenderProcessHostImpl. In addition to reducing code, I think it will also make the code easier to understand and easier to review and make sure it does the right thing as we'd follow the pattern already put in place for IOSurface tokens. https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... https://codereview.chromium.org/1248713002/diff/120001/content/child/child_cl... File content/child/child_client_native_pixmap_factory_message_filter_ozone.cc (right): https://codereview.chromium.org/1248713002/diff/120001/content/child/child_cl... content/child/child_client_native_pixmap_factory_message_filter_ozone.cc:25: ChildClientNativePixmapFactory::GetInstance()); How do we make sure this happens before the renderer tries to allocate a GpuMemoryBuffer? If you follow the IOSurface token pattern as suggested below it becomes easier to see what the problem is and what needs to be done to fix it. https://codereview.chromium.org/1248713002/diff/120001/content/child/child_cl... File content/child/child_client_native_pixmap_factory_ozone.h (right): https://codereview.chromium.org/1248713002/diff/120001/content/child/child_cl... content/child/child_client_native_pixmap_factory_ozone.h:14: class ChildClientNativePixmapFactory : public ui::ClientNativePixmapFactory { Same here. This is just a large wrapper around calling: ui::ClientNativePixmapFactory::GetInstance()->SetVgemFd(vgem_fd); please follow the IOSurface token pattern instead. https://codereview.chromium.org/1248713002/diff/120001/ui/ozone/public/client... File ui/ozone/public/client_native_pixmap_factory.h (right): https://codereview.chromium.org/1248713002/diff/120001/ui/ozone/public/client... ui/ozone/public/client_native_pixmap_factory.h:55: virtual void Initialize(const base::FileDescriptor& virtual_device) = 0; if this can't fail, why not pass the fd to the ctor instead?
On 2015/08/13 17:37:17, reveman wrote: > On 2015/08/13 at 15:57:19, dongseong.hwang wrote: > > On 2015/08/12 19:57:26, spang wrote: > > > On 2015/08/05 09:44:26, dshwang wrote: > > > > Thank you for nice review! > > > > > > > > > > > > https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... > > > > File content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc > > > (right): > > > > > > > > > > > > https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... > > > > content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc:66: > pool, > > > > FROM_HERE, base::Bind(&DupVirtualDeviceFd, virtual_device_.fd), > > > > On 2015/08/04 17:47:31, spang wrote: > > > > > I'm pretty sure that passing access to vgem by dup()ing a shared file > > > > descriptor > > > > > ends up giving all renderers a common handle namespace, which is > completely > > > > > unacceptable from a security perspective. > > > > > > > > > > We need to open a new vgem fd for each renderer. > > > > > > > > I don't fully catch up which point is vulnerable from security point of > view. > > > > This pattern follows MacOSX code, which also gives all renderers > > > > |io_surface_manager_token_| in > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > > > The problem is calling DRM_IOCTL_PRIME_FD_TO_HANDLE creates a handle that is > > > visible to all of the renderers that share the same vgem cloned via dup(). > So to > > > access another renderer's buffers all you need to do is guess a small > integer. > > > > > > Instead of calling dup() we need to call open(). > > > > Now it dup /dev/dri/card1, not dmabuf_fd. There is not difference between > dup(vgem_device_fd) and open(/dev/dri/card1). All states are kept by kernel. > > DRM_IOCTL_PRIME_FD_TO_HANDLE requires both vgem device fd and dmabuf_fd to > import vgem bo handle. > > On the other hands, GbmPixmap exports dmabuf_fd and sends it to renderer. > Another renderer needs the dambuf_fd, not small integer, to see another > renderer's content. This situation is same to Android SurfaceTexture and MacOS > IoSurface. > > dup()ing and sending an FD to a renderer process is safe from a renderer > isolation perspective as only the receiving process will have access to the > resource referred to by the FD. > > When passing a FD using the regular IPC system in chromium, the kernel takes > care of dup()ing the FD for the receiving process. Only the receiving renderer > process will be able to use the FD. The FD is not an integer that another > process could guess as that would not be secure at all. > > Note: when sending a FD to another process in chromium we typically need to > dup() it as the IPC system will automatically close it (this is why > fd.auto_close is for) if sending the message fails. Sorry, I should have been clearer. When I said you can guess the handle I meant GEM handle, not dmabuf fd. These handles are contained inside the DRM file structure ("struct drm_file"). We get one drm_file per call to open("/dev/dri/renderD129"). If we share that device fd to multiple renderers using dup(), we'll have defeated our security. It's easy to resolve: don't share the vgem device using dup(). Sharing dmabufs using dup() is totally OK, that's what they're for.
On 2015/08/13 19:20:03, spang wrote: > On 2015/08/13 17:37:17, reveman wrote: > > On 2015/08/13 at 15:57:19, dongseong.hwang wrote: > > > On 2015/08/12 19:57:26, spang wrote: > > > > On 2015/08/05 09:44:26, dshwang wrote: > > > > > Thank you for nice review! > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... > > > > > File content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc > > > > (right): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1248713002/diff/80001/content/browser/gpu/bro... > > > > > content/browser/gpu/browser_client_native_pixmap_manager_ozone.cc:66: > > pool, > > > > > FROM_HERE, base::Bind(&DupVirtualDeviceFd, virtual_device_.fd), > > > > > On 2015/08/04 17:47:31, spang wrote: > > > > > > I'm pretty sure that passing access to vgem by dup()ing a shared file > > > > > descriptor > > > > > > ends up giving all renderers a common handle namespace, which is > > completely > > > > > > unacceptable from a security perspective. > > > > > > > > > > > > We need to open a new vgem fd for each renderer. > > > > > > > > > > I don't fully catch up which point is vulnerable from security point of > > view. > > > > > This pattern follows MacOSX code, which also gives all renderers > > > > > |io_surface_manager_token_| in > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > > > > > The problem is calling DRM_IOCTL_PRIME_FD_TO_HANDLE creates a handle that > is > > > > visible to all of the renderers that share the same vgem cloned via dup(). > > So to > > > > access another renderer's buffers all you need to do is guess a small > > integer. > > > > > > > > Instead of calling dup() we need to call open(). > > > > > > Now it dup /dev/dri/card1, not dmabuf_fd. There is not difference between > > dup(vgem_device_fd) and open(/dev/dri/card1). All states are kept by kernel. There is a difference, and it's the same difference as with regular files. Using dup() shares more state (e.g. file offset) than calling open() multiple times. > > > DRM_IOCTL_PRIME_FD_TO_HANDLE requires both vgem device fd and dmabuf_fd to > > import vgem bo handle. > > > On the other hands, GbmPixmap exports dmabuf_fd and sends it to renderer. > > Another renderer needs the dambuf_fd, not small integer, to see another > > renderer's content. This situation is same to Android SurfaceTexture and MacOS > > IoSurface. > > > > dup()ing and sending an FD to a renderer process is safe from a renderer > > isolation perspective as only the receiving process will have access to the > > resource referred to by the FD. > > > > When passing a FD using the regular IPC system in chromium, the kernel takes > > care of dup()ing the FD for the receiving process. Only the receiving renderer > > process will be able to use the FD. The FD is not an integer that another > > process could guess as that would not be secure at all. > > > > Note: when sending a FD to another process in chromium we typically need to > > dup() it as the IPC system will automatically close it (this is why > > fd.auto_close is for) if sending the message fails. > > Sorry, I should have been clearer. When I said you can guess the handle I meant > GEM handle, not dmabuf fd. These handles are contained inside the DRM file > structure ("struct drm_file"). > > We get one drm_file per call to open("/dev/dri/renderD129"). If we share that > device fd to multiple renderers using dup(), we'll have defeated our security. > > It's easy to resolve: don't share the vgem device using dup(). Sharing dmabufs > using dup() is totally OK, that's what they're for.
On 2015/08/13 18:05:28, reveman wrote:
> I'm seeing the term "VgemFd" used in some places and "VirtualDevice" in other
> places. Can we make this consistent? We already use VgemFd in some places
before
> this patch and I prefer that term as it's not as vague as virtualDevice. Can
we
> use VgemFd everywhere?
I intentionally named "VirtualDevice", because only Ozone GBM knows VGEM. Other
ozone platforms might use or unuse the virtual device for other device file.
So "VgemFD" was used in only ui/ozone/platform/drm, and "VirtualDevice" was used
in the others.
I change all terms to "VgemFD" as requested.
spang, WDYT?
In more detail, this small issue is because we want to communicate device file
in content/ level.
content/ should not know Ozone platform (e.g. GBM), and ui/ozone/ should not
know renderer/browser process semantic.
We decided content/ control this work flow, so content/ requires to know VGEM
unfortunately.
On 2015/08/13 19:27:41, spang wrote:
> Sorry, I should have been clearer. When I said you can guess the handle I
meant
> GEM handle, not dmabuf fd. These handles are contained inside the DRM file
> structure ("struct drm_file").
>
> We get one drm_file per call to open("/dev/dri/renderD129"). If we share that
> device fd to multiple renderers using dup(), we'll have defeated our security.
>
> It's easy to resolve: don't share the vgem device using dup(). Sharing dmabufs
> using dup() is totally OK, that's what they're for.
>
> There is a difference, and it's the same difference as with regular files.
Using
> dup() shares more state (e.g. file offset) than calling open() multiple times.
Oh, "struct drm_file" is kept per opened file. Thank you for explanation.
Now I open "/dev/dri/renderD129" every time.
https://codereview.chromium.org/1248713002/diff/120001/content/browser/gpu/br...
File content/browser/gpu/browser_client_native_pixmap_factory_ozone.cc (right):
https://codereview.chromium.org/1248713002/diff/120001/content/browser/gpu/br...
content/browser/gpu/browser_client_native_pixmap_factory_ozone.cc:64:
base::SequencedWorkerPool* pool = content::BrowserThread::GetBlockingPool();
On 2015/08/13 18:05:28, reveman wrote:
> why do we need to use blocking thread pool to dup() this fd?
Done. Removed.
https://codereview.chromium.org/1248713002/diff/120001/content/browser/gpu/br...
File content/browser/gpu/browser_client_native_pixmap_factory_ozone.h (right):
https://codereview.chromium.org/1248713002/diff/120001/content/browser/gpu/br...
content/browser/gpu/browser_client_native_pixmap_factory_ozone.h:15: class
BrowserClientNativePixmapFactory : public ui::ClientNativePixmapFactory {
On 2015/08/13 18:05:28, reveman wrote:
> This class doesn't seem useful to me. Afaict, it's just a large wrapper around
> sending one IPC message to the renderer with the vgem_fd.
>
> We can replace all this with a few lines of code in RenderProcessHostImpl. In
> addition to reducing code, I think it will also make the code easier to
> understand and easier to review and make sure it does the right thing as we'd
> follow the pattern already put in place for IOSurface tokens.
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
Thank you for explaining. I removed (Browser|Child)XXXFactory wrappers.
I didn't want RenderProcessHostImpl to do more thing, but it looks
over-generalized. One thing more in RenderProcessHostImpl is practically
acceptable compared to bloating code.
https://codereview.chromium.org/1248713002/diff/120001/content/child/child_cl...
File content/child/child_client_native_pixmap_factory_message_filter_ozone.cc
(right):
https://codereview.chromium.org/1248713002/diff/120001/content/child/child_cl...
content/child/child_client_native_pixmap_factory_message_filter_ozone.cc:25:
ChildClientNativePixmapFactory::GetInstance());
On 2015/08/13 18:05:28, reveman wrote:
> How do we make sure this happens before the renderer tries to allocate a
> GpuMemoryBuffer? If you follow the IOSurface token pattern as suggested below
it
> becomes easier to see what the problem is and what needs to be done to fix it.
Ok, now Renderer waits for vgem fd on importing NativePixmap like IOSurface
pattern.
https://codereview.chromium.org/1248713002/diff/120001/content/child/child_cl...
File content/child/child_client_native_pixmap_factory_ozone.h (right):
https://codereview.chromium.org/1248713002/diff/120001/content/child/child_cl...
content/child/child_client_native_pixmap_factory_ozone.h:14: class
ChildClientNativePixmapFactory : public ui::ClientNativePixmapFactory {
On 2015/08/13 18:05:28, reveman wrote:
> Same here. This is just a large wrapper around calling:
>
> ui::ClientNativePixmapFactory::GetInstance()->SetVgemFd(vgem_fd);
>
> please follow the IOSurface token pattern instead.
Done.
https://codereview.chromium.org/1248713002/diff/120001/ui/ozone/public/client...
File ui/ozone/public/client_native_pixmap_factory.h (right):
https://codereview.chromium.org/1248713002/diff/120001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_factory.h:55: virtual void Initialize(const
base::FileDescriptor& virtual_device) = 0;
On 2015/08/13 18:05:28, reveman wrote:
> if this can't fail, why not pass the fd to the ctor instead?
Replace it with SetVgemFD()
Patchset #6 (id:140001) has been deleted
this is much easier to review, thanks! my review below, mostly nits. https://codereview.chromium.org/1248713002/diff/160001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1248713002/diff/160001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1606: int vgem_fd = ui::ClientNativePixmapFactory::GetInstance()->OpenVgemFD(); It's seems wrong that we ask the client factory to open a vgem fd. Should this not be part of a more privileged API or at least be made a static function and not part of the client side interface? https://codereview.chromium.org/1248713002/diff/160001/content/child/child_th... File content/child/child_thread_impl.cc (right): https://codereview.chromium.org/1248713002/diff/160001/content/child/child_th... content/child/child_thread_impl.cc:215: void OnSetVgemFD(const base::FileDescriptor& virtual_device) { nit: s/virtual_device/vgem_fd/ https://codereview.chromium.org/1248713002/diff/160001/content/child/child_th... content/child/child_thread_impl.cc:216: DCHECK(ui::ClientNativePixmapFactory::GetInstance()); nit: unnecessary as dereferencing null will already cause an easy to diagnose crash https://codereview.chromium.org/1248713002/diff/160001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1248713002/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:49: nit: remove blank line as this is part of the ClientNativePixmapFactory interface https://codereview.chromium.org/1248713002/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:52: if (vgem_fd_.get() < 0) { hm, are you sure this gets called multiple times? is there an easy way to avoid that? If we need to support this then I think it would be a bit cleaner to just replace vgem_fd if already set like we're doing with the token in the IOSurface case. https://codereview.chromium.org/1248713002/diff/160001/ui/ozone/public/client... File ui/ozone/public/client_native_pixmap_factory.h (right): https://codereview.chromium.org/1248713002/diff/160001/ui/ozone/public/client... ui/ozone/public/client_native_pixmap_factory.h:35: virtual int OpenVgemFD() const = 0; could this return a ScopedFD to make the ownership more clear? https://codereview.chromium.org/1248713002/diff/160001/ui/ozone/public/client... ui/ozone/public/client_native_pixmap_factory.h:38: virtual void SetVgemFD(const base::FileDescriptor& vgem_fd) = 0; Can we pass a ScopedFD to this function instead?
Thank you for rapid review! spang, we need to make decision how much other Ozone platforms are aware of vgem. See below. https://codereview.chromium.org/1248713002/diff/160001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1248713002/diff/160001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1606: int vgem_fd = ui::ClientNativePixmapFactory::GetInstance()->OpenVgemFD(); On 2015/08/14 13:19:12, reveman wrote: > It's seems wrong that we ask the client factory to open a vgem fd. Should this > not be part of a more privileged API or at least be made a static function and > not part of the client side interface? spang, what do you think? I chose ClientNativePixmapFactory because I wanted privileged API (i.e. OzonePlatform) to be not aware of VGEM. static function is not good because Ozone platform is chosen in run time. If I implement it in static function, all Ozone platforms will know the API, which mean all Ozone platforms become aware of "/dev/dri/renderD129". As we encounter repeatedly, we need to make decision how much other Ozone platforms are aware of vgem. In previous patch set, I made other platforms consider it as "virtual_device". https://codereview.chromium.org/1248713002/diff/160001/content/child/child_th... File content/child/child_thread_impl.cc (right): https://codereview.chromium.org/1248713002/diff/160001/content/child/child_th... content/child/child_thread_impl.cc:215: void OnSetVgemFD(const base::FileDescriptor& virtual_device) { On 2015/08/14 13:19:13, reveman wrote: > nit: s/virtual_device/vgem_fd/ Done. https://codereview.chromium.org/1248713002/diff/160001/content/child/child_th... content/child/child_thread_impl.cc:216: DCHECK(ui::ClientNativePixmapFactory::GetInstance()); On 2015/08/14 13:19:13, reveman wrote: > nit: unnecessary as dereferencing null will already cause an easy to diagnose > crash Done. https://codereview.chromium.org/1248713002/diff/160001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1248713002/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:49: On 2015/08/14 13:19:13, reveman wrote: > nit: remove blank line as this is part of the ClientNativePixmapFactory > interface Done. https://codereview.chromium.org/1248713002/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:52: if (vgem_fd_.get() < 0) { On 2015/08/14 13:19:13, reveman wrote: > hm, are you sure this gets called multiple times? is there an easy way to avoid > that? If we need to support this then I think it would be a bit cleaner to just > replace vgem_fd if already set like we're doing with the token in the IOSurface > case. When I test using --renderer-process-limit=2, it doesn't happen. Sorry for naive code. I replace it with DCHECK. https://codereview.chromium.org/1248713002/diff/160001/ui/ozone/public/client... File ui/ozone/public/client_native_pixmap_factory.h (right): https://codereview.chromium.org/1248713002/diff/160001/ui/ozone/public/client... ui/ozone/public/client_native_pixmap_factory.h:35: virtual int OpenVgemFD() const = 0; On 2015/08/14 13:19:13, reveman wrote: > could this return a ScopedFD to make the ownership more clear? Done. https://codereview.chromium.org/1248713002/diff/160001/ui/ozone/public/client... ui/ozone/public/client_native_pixmap_factory.h:38: virtual void SetVgemFD(const base::FileDescriptor& vgem_fd) = 0; On 2015/08/14 13:19:13, reveman wrote: > Can we pass a ScopedFD to this function instead? Done.
Patchset #7 (id:180001) has been deleted
https://codereview.chromium.org/1248713002/diff/200001/ui/ozone/public/client... File ui/ozone/public/client_native_pixmap_factory.h (right): https://codereview.chromium.org/1248713002/diff/200001/ui/ozone/public/client... ui/ozone/public/client_native_pixmap_factory.h:35: virtual base::ScopedFD OpenVgemFD() const = 0; doesn't the return type have to be scoped_ptr<base::ScopedFD>? https://codereview.chromium.org/1248713002/diff/200001/ui/ozone/public/client... ui/ozone/public/client_native_pixmap_factory.h:38: virtual void SetVgemFD(base::ScopedFD* vgem_fd) = 0; scoped_ptr<base::ScopedFD> if passing ownership?
On 2015/08/14 12:40:04, dshwang wrote:
> On 2015/08/13 18:05:28, reveman wrote:
> > I'm seeing the term "VgemFd" used in some places and "VirtualDevice" in
other
> > places. Can we make this consistent? We already use VgemFd in some places
> before
> > this patch and I prefer that term as it's not as vague as virtualDevice. Can
> we
> > use VgemFd everywhere?
>
> I intentionally named "VirtualDevice", because only Ozone GBM knows VGEM.
Other
> ozone platforms might use or unuse the virtual device for other device file.
> So "VgemFD" was used in only ui/ozone/platform/drm, and "VirtualDevice" was
used
> in the others.
> I change all terms to "VgemFD" as requested.
> spang, WDYT?
>
If we create the ClientNativePixmapFactory before the sandbox starts, I think we
can just open the device directly. It wouldn't need more IPCs.
Can we try that? Is there any obstacle?
> In more detail, this small issue is because we want to communicate device file
> in content/ level.
> content/ should not know Ozone platform (e.g. GBM), and ui/ozone/ should not
> know renderer/browser process semantic.
> We decided content/ control this work flow, so content/ requires to know VGEM
> unfortunately.
>
> On 2015/08/13 19:27:41, spang wrote:
> > Sorry, I should have been clearer. When I said you can guess the handle I
> meant
> > GEM handle, not dmabuf fd. These handles are contained inside the DRM file
> > structure ("struct drm_file").
> >
> > We get one drm_file per call to open("/dev/dri/renderD129"). If we share
that
> > device fd to multiple renderers using dup(), we'll have defeated our
security.
> >
> > It's easy to resolve: don't share the vgem device using dup(). Sharing
dmabufs
> > using dup() is totally OK, that's what they're for.
> >
> > There is a difference, and it's the same difference as with regular files.
> Using
> > dup() shares more state (e.g. file offset) than calling open() multiple
times.
>
> Oh, "struct drm_file" is kept per opened file. Thank you for explanation.
> Now I open "/dev/dri/renderD129" every time.
>
>
https://codereview.chromium.org/1248713002/diff/120001/content/browser/gpu/br...
> File content/browser/gpu/browser_client_native_pixmap_factory_ozone.cc
(right):
>
>
https://codereview.chromium.org/1248713002/diff/120001/content/browser/gpu/br...
> content/browser/gpu/browser_client_native_pixmap_factory_ozone.cc:64:
> base::SequencedWorkerPool* pool = content::BrowserThread::GetBlockingPool();
> On 2015/08/13 18:05:28, reveman wrote:
> > why do we need to use blocking thread pool to dup() this fd?
>
> Done. Removed.
>
>
https://codereview.chromium.org/1248713002/diff/120001/content/browser/gpu/br...
> File content/browser/gpu/browser_client_native_pixmap_factory_ozone.h (right):
>
>
https://codereview.chromium.org/1248713002/diff/120001/content/browser/gpu/br...
> content/browser/gpu/browser_client_native_pixmap_factory_ozone.h:15: class
> BrowserClientNativePixmapFactory : public ui::ClientNativePixmapFactory {
> On 2015/08/13 18:05:28, reveman wrote:
> > This class doesn't seem useful to me. Afaict, it's just a large wrapper
around
> > sending one IPC message to the renderer with the vgem_fd.
> >
> > We can replace all this with a few lines of code in RenderProcessHostImpl.
In
> > addition to reducing code, I think it will also make the code easier to
> > understand and easier to review and make sure it does the right thing as
we'd
> > follow the pattern already put in place for IOSurface tokens.
> >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
>
> Thank you for explaining. I removed (Browser|Child)XXXFactory wrappers.
> I didn't want RenderProcessHostImpl to do more thing, but it looks
> over-generalized. One thing more in RenderProcessHostImpl is practically
> acceptable compared to bloating code.
>
>
https://codereview.chromium.org/1248713002/diff/120001/content/child/child_cl...
> File content/child/child_client_native_pixmap_factory_message_filter_ozone.cc
> (right):
>
>
https://codereview.chromium.org/1248713002/diff/120001/content/child/child_cl...
> content/child/child_client_native_pixmap_factory_message_filter_ozone.cc:25:
> ChildClientNativePixmapFactory::GetInstance());
> On 2015/08/13 18:05:28, reveman wrote:
> > How do we make sure this happens before the renderer tries to allocate a
> > GpuMemoryBuffer? If you follow the IOSurface token pattern as suggested
below
> it
> > becomes easier to see what the problem is and what needs to be done to fix
it.
>
> Ok, now Renderer waits for vgem fd on importing NativePixmap like IOSurface
> pattern.
>
>
https://codereview.chromium.org/1248713002/diff/120001/content/child/child_cl...
> File content/child/child_client_native_pixmap_factory_ozone.h (right):
>
>
https://codereview.chromium.org/1248713002/diff/120001/content/child/child_cl...
> content/child/child_client_native_pixmap_factory_ozone.h:14: class
> ChildClientNativePixmapFactory : public ui::ClientNativePixmapFactory {
> On 2015/08/13 18:05:28, reveman wrote:
> > Same here. This is just a large wrapper around calling:
> >
> > ui::ClientNativePixmapFactory::GetInstance()->SetVgemFd(vgem_fd);
> >
> > please follow the IOSurface token pattern instead.
>
> Done.
>
>
https://codereview.chromium.org/1248713002/diff/120001/ui/ozone/public/client...
> File ui/ozone/public/client_native_pixmap_factory.h (right):
>
>
https://codereview.chromium.org/1248713002/diff/120001/ui/ozone/public/client...
> ui/ozone/public/client_native_pixmap_factory.h:55: virtual void
Initialize(const
> base::FileDescriptor& virtual_device) = 0;
> On 2015/08/13 18:05:28, reveman wrote:
> > if this can't fail, why not pass the fd to the ctor instead?
>
> Replace it with SetVgemFD()
On 2015/08/14 at 17:58:07, spang wrote: > On 2015/08/14 12:40:04, dshwang wrote: > > On 2015/08/13 18:05:28, reveman wrote: > > > I'm seeing the term "VgemFd" used in some places and "VirtualDevice" in other > > > places. Can we make this consistent? We already use VgemFd in some places > > before > > > this patch and I prefer that term as it's not as vague as virtualDevice. Can > > we > > > use VgemFd everywhere? > > > > I intentionally named "VirtualDevice", because only Ozone GBM knows VGEM. Other > > ozone platforms might use or unuse the virtual device for other device file. > > So "VgemFD" was used in only ui/ozone/platform/drm, and "VirtualDevice" was used > > in the others. > > I change all terms to "VgemFD" as requested. > > spang, WDYT? > > > > If we create the ClientNativePixmapFactory before the sandbox starts, I think we can just open the device directly. It wouldn't need more IPCs. > > Can we try that? Is there any obstacle? > +1, that would be great.
On 2015/08/16 20:32:36, reveman wrote: > On 2015/08/14 at 17:58:07, spang wrote: > > On 2015/08/14 12:40:04, dshwang wrote: > > > On 2015/08/13 18:05:28, reveman wrote: > > > > I'm seeing the term "VgemFd" used in some places and "VirtualDevice" in > other > > > > places. Can we make this consistent? We already use VgemFd in some places > > > before > > > > this patch and I prefer that term as it's not as vague as virtualDevice. > Can > > > we > > > > use VgemFd everywhere? > > > > > > I intentionally named "VirtualDevice", because only Ozone GBM knows VGEM. > Other > > > ozone platforms might use or unuse the virtual device for other device file. > > > So "VgemFD" was used in only ui/ozone/platform/drm, and "VirtualDevice" was > used > > > in the others. > > > I change all terms to "VgemFD" as requested. > > > spang, WDYT? > > > > > > > If we create the ClientNativePixmapFactory before the sandbox starts, I think > we can just open the device directly. It wouldn't need more IPCs. > > > > Can we try that? Is there any obstacle? > > > > +1, that would be great. Renderer cannot open any files even before sandboxing. Let me explain. ClientNativePixmapFactory is already created before sandboxing. ClientNativePixmapFactory creation in line 113 https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... sandboxing in line 187 https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... Nevertheless, ClientNativePixmapFactory ctor can not open any file. There are two sandboxs for renderer in linux; "User namespace sandbox (i.e. setuid)" and "Seccomp-BPF". "setuid sandbox" prevents renderer from accessing filesystem in the way similar to chroot. "Seccomp-BPF" prevents renderer from calling system call. "setuid" is already started in the zygote process via chrome-sandbox file, so renderer is under the sandbox from launching. What it means is that there is no way for renderer to open any files. It might be possible for content/zygote/zygote_main_linux.cc opens vgem device and inherits the fd to renderer, but it's ugly as well as content/ still needs to know VGEM. spang, reveman, WDYT about original question "is it ok to expose VGEM API in ClientNativePixmapFactory although VGEM is Ozone GBM's concept?" virtual scoped_ptr<base::ScopedFD> OpenVgemFD() const = 0; virtual void SetVgemFD(scoped_ptr<base::ScopedFD> vgem_fd) = 0; https://codereview.chromium.org/1248713002/diff/200001/ui/ozone/public/client... File ui/ozone/public/client_native_pixmap_factory.h (right): https://codereview.chromium.org/1248713002/diff/200001/ui/ozone/public/client... ui/ozone/public/client_native_pixmap_factory.h:35: virtual base::ScopedFD OpenVgemFD() const = 0; On 2015/08/14 16:53:16, reveman wrote: > doesn't the return type have to be scoped_ptr<base::ScopedFD>? Done. https://codereview.chromium.org/1248713002/diff/200001/ui/ozone/public/client... ui/ozone/public/client_native_pixmap_factory.h:38: virtual void SetVgemFD(base::ScopedFD* vgem_fd) = 0; On 2015/08/14 16:53:17, reveman wrote: > scoped_ptr<base::ScopedFD> if passing ownership? That's good idea, although it's the first time that scoped_ptr<base::ScopedFD> is used in whole source. Done.
https://codereview.chromium.org/1248713002/diff/80001/content/common/gpu/clie... File content/common/gpu/client_native_pixmap_manager_ozone_messages.h (right): https://codereview.chromium.org/1248713002/diff/80001/content/common/gpu/clie... content/common/gpu/client_native_pixmap_manager_ozone_messages.h:16: base::FileDescriptor /* device_fd */) Is this file still necessary? https://codereview.chromium.org/1248713002/diff/200001/ui/ozone/public/client... File ui/ozone/public/client_native_pixmap_factory.h (right): https://codereview.chromium.org/1248713002/diff/200001/ui/ozone/public/client... ui/ozone/public/client_native_pixmap_factory.h:35: virtual base::ScopedFD OpenVgemFD() const = 0; On 2015/08/17 10:28:55, dshwang wrote: > On 2015/08/14 16:53:16, reveman wrote: > > doesn't the return type have to be scoped_ptr<base::ScopedFD>? > > Done. Plain base::ScopedFD is correct, it's not necessary to wrap with scoped_ptr<> because ScopedFD already transfers ownership like a scoped_ptr<> would. https://codereview.chromium.org/1248713002/diff/220001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1248713002/diff/220001/content/browser/browse... content/browser/browser_main_loop.cc:623: auto scoped_fd(ui::ClientNativePixmapFactory::GetInstance()->OpenVgemFD()); The usual syntax is: auto scoped_fd = ui::ClientNativePixmapFactory::GetInstance()->OpenVgemFD(); https://codereview.chromium.org/1248713002/diff/220001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1248713002/diff/220001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1605: auto vgem_fd(ui::ClientNativePixmapFactory::GetInstance()->OpenVgemFD()); and here. https://codereview.chromium.org/1248713002/diff/220001/content/common/child_p... File content/common/child_process_messages.h (right): https://codereview.chromium.org/1248713002/diff/220001/content/common/child_p... content/common/child_process_messages.h:131: IPC_MESSAGE_CONTROL1(ChildProcessMsg_SetVgemFD, Maybe: ChildProcessMsg_InitializeClientNativePixmapFactory https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/common/stub_c... File ui/ozone/common/stub_client_native_pixmap_factory.cc (right): https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/common/stub_c... ui/ozone/common/stub_client_native_pixmap_factory.cc:16: // Overridden from ClientNativePixmapFactory: Don't change this // ClassName: is the style we're using in ui/ozone. https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/public/client... File ui/ozone/public/client_native_pixmap_factory.h (right): https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/public/client... ui/ozone/public/client_native_pixmap_factory.h:35: virtual scoped_ptr<base::ScopedFD> OpenVgemFD() const = 0; Just base::ScopedFD. https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/public/client... ui/ozone/public/client_native_pixmap_factory.h:38: virtual void SetVgemFD(scoped_ptr<base::ScopedFD> vgem_fd) = 0; I'd prefer virtual void Initialize(base::ScopedFD device_fd) = 0; so it's clear it's a single method you call once before doing anything else. It'd be very incorrect to call it a 2nd time.
On 2015/08/17 10:28:55, dshwang wrote: > On 2015/08/16 20:32:36, reveman wrote: > > On 2015/08/14 at 17:58:07, spang wrote: > > > On 2015/08/14 12:40:04, dshwang wrote: > > > > On 2015/08/13 18:05:28, reveman wrote: > > > > > I'm seeing the term "VgemFd" used in some places and "VirtualDevice" in > > other > > > > > places. Can we make this consistent? We already use VgemFd in some > places > > > > before > > > > > this patch and I prefer that term as it's not as vague as virtualDevice. > > Can > > > > we > > > > > use VgemFd everywhere? > > > > > > > > I intentionally named "VirtualDevice", because only Ozone GBM knows VGEM. > > Other > > > > ozone platforms might use or unuse the virtual device for other device > file. > > > > So "VgemFD" was used in only ui/ozone/platform/drm, and "VirtualDevice" > was > > used > > > > in the others. > > > > I change all terms to "VgemFD" as requested. > > > > spang, WDYT? > > > > > > > > > > If we create the ClientNativePixmapFactory before the sandbox starts, I > think > > we can just open the device directly. It wouldn't need more IPCs. > > > > > > Can we try that? Is there any obstacle? > > > > > > > +1, that would be great. > > Renderer cannot open any files even before sandboxing. Let me explain. > > ClientNativePixmapFactory is already created before sandboxing. > ClientNativePixmapFactory creation in line 113 > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > sandboxing in line 187 > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > Nevertheless, ClientNativePixmapFactory ctor can not open any file. > There are two sandboxs for renderer in linux; "User namespace sandbox (i.e. > setuid)" and "Seccomp-BPF". > "setuid sandbox" prevents renderer from accessing filesystem in the way similar > to chroot. > "Seccomp-BPF" prevents renderer from calling system call. > "setuid" is already started in the zygote process via chrome-sandbox file, so > renderer is under the sandbox from launching. What it means is that there is no > way for renderer to open any files. > Ah, right, we cannot do anything before it starts. Darn. > It might be possible for content/zygote/zygote_main_linux.cc opens vgem device > and inherits the fd to renderer, but it's ugly as well as content/ still needs > to know VGEM. > > > spang, reveman, WDYT about original question "is it ok to expose VGEM API in > ClientNativePixmapFactory although VGEM is Ozone GBM's concept?" > > virtual scoped_ptr<base::ScopedFD> OpenVgemFD() const = 0; > virtual void SetVgemFD(scoped_ptr<base::ScopedFD> vgem_fd) = 0; > I agree with not saying "vgem" in the interface. I think "device" is generic enough though. base::ScopedFD OzonePlatform::OpenClientNativePixmapDevice(); [used from browser] void ClientNativePixmapFactory::Initialize(base::ScopedFD device_fd); [used from browsre & renderers] This also moves opening the device out of the client object like reveman@ suggested. > https://codereview.chromium.org/1248713002/diff/200001/ui/ozone/public/client... > File ui/ozone/public/client_native_pixmap_factory.h (right): > > https://codereview.chromium.org/1248713002/diff/200001/ui/ozone/public/client... > ui/ozone/public/client_native_pixmap_factory.h:35: virtual base::ScopedFD > OpenVgemFD() const = 0; > On 2015/08/14 16:53:16, reveman wrote: > > doesn't the return type have to be scoped_ptr<base::ScopedFD>? > > Done. > > https://codereview.chromium.org/1248713002/diff/200001/ui/ozone/public/client... > ui/ozone/public/client_native_pixmap_factory.h:38: virtual void > SetVgemFD(base::ScopedFD* vgem_fd) = 0; > On 2015/08/14 16:53:17, reveman wrote: > > scoped_ptr<base::ScopedFD> if passing ownership? > > That's good idea, although it's the first time that scoped_ptr<base::ScopedFD> > is used in whole source. Done.
https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:107: base::PlatformThreadRef set_vgem_fd_thread_id_; Can you use base::ThreadChecker for this?
https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:107: base::PlatformThreadRef set_vgem_fd_thread_id_; On 2015/08/17 at 19:04:33, spang wrote: > Can you use base::ThreadChecker for this? I'm afraid not as we don't know what a valid thread is. All we know is that Wait() should not be called on the same thread that the fd was set on.
https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:107: base::PlatformThreadRef set_vgem_fd_thread_id_; On 2015/08/17 19:09:52, reveman wrote: > On 2015/08/17 at 19:04:33, spang wrote: > > Can you use base::ThreadChecker for this? > > I'm afraid not as we don't know what a valid thread is. All we know is that > Wait() should not be called on the same thread that the fd was set on. Ah, I see. We should use DCHECK_IS_ON() instead of !NDEBUG so that DCHECK_ALWAYS_ON enables these assertions.
https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:107: base::PlatformThreadRef set_vgem_fd_thread_id_; On 2015/08/17 at 19:17:29, spang wrote: > On 2015/08/17 19:09:52, reveman wrote: > > On 2015/08/17 at 19:04:33, spang wrote: > > > Can you use base::ThreadChecker for this? > > > > I'm afraid not as we don't know what a valid thread is. All we know is that > > Wait() should not be called on the same thread that the fd was set on. > > Ah, I see. > > We should use DCHECK_IS_ON() instead of !NDEBUG so that DCHECK_ALWAYS_ON enables these assertions. sure but if we change this to DCHECK_IS_ON(), please change ChildIOSurfaceManager too.
On 2015/08/17 18:05:39, spang wrote: > I agree with not saying "vgem" in the interface. I think "device" is generic > enough though. > > base::ScopedFD OzonePlatform::OpenClientNativePixmapDevice(); [used from > browser] > > void ClientNativePixmapFactory::Initialize(base::ScopedFD device_fd); [used > from browsre & renderers] > > This also moves opening the device out of the client object like reveman@ > suggested. Thank you for nice suggestion. It saves my day :) Could you review again? https://codereview.chromium.org/1248713002/diff/80001/content/common/gpu/clie... File content/common/gpu/client_native_pixmap_manager_ozone_messages.h (right): https://codereview.chromium.org/1248713002/diff/80001/content/common/gpu/clie... content/common/gpu/client_native_pixmap_manager_ozone_messages.h:16: base::FileDescriptor /* device_fd */) On 2015/08/17 18:03:41, spang wrote: > Is this file still necessary? It's already removed in latest CL. https://codereview.chromium.org/1248713002/diff/200001/ui/ozone/public/client... File ui/ozone/public/client_native_pixmap_factory.h (right): https://codereview.chromium.org/1248713002/diff/200001/ui/ozone/public/client... ui/ozone/public/client_native_pixmap_factory.h:35: virtual base::ScopedFD OpenVgemFD() const = 0; On 2015/08/17 18:03:41, spang wrote: > On 2015/08/17 10:28:55, dshwang wrote: > > On 2015/08/14 16:53:16, reveman wrote: > > > doesn't the return type have to be scoped_ptr<base::ScopedFD>? > > > > Done. > > Plain base::ScopedFD is correct, it's not necessary to wrap with scoped_ptr<> > because ScopedFD already transfers ownership like a scoped_ptr<> would. Done. Thx for explaining. https://codereview.chromium.org/1248713002/diff/220001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1248713002/diff/220001/content/browser/browse... content/browser/browser_main_loop.cc:623: auto scoped_fd(ui::ClientNativePixmapFactory::GetInstance()->OpenVgemFD()); On 2015/08/17 18:03:41, spang wrote: > The usual syntax is: > > auto scoped_fd = ui::ClientNativePixmapFactory::GetInstance()->OpenVgemFD(); Done. https://codereview.chromium.org/1248713002/diff/220001/content/common/child_p... File content/common/child_process_messages.h (right): https://codereview.chromium.org/1248713002/diff/220001/content/common/child_p... content/common/child_process_messages.h:131: IPC_MESSAGE_CONTROL1(ChildProcessMsg_SetVgemFD, On 2015/08/17 18:03:41, spang wrote: > Maybe: > > ChildProcessMsg_InitializeClientNativePixmapFactory Done. https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/common/stub_c... File ui/ozone/common/stub_client_native_pixmap_factory.cc (right): https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/common/stub_c... ui/ozone/common/stub_client_native_pixmap_factory.cc:16: // Overridden from ClientNativePixmapFactory: On 2015/08/17 18:03:41, spang wrote: > Don't change this > > // ClassName: > > is the style we're using in ui/ozone. Done. Ah, ok. I did change it to match to cc style, which reveman suggested in another CL. https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:107: base::PlatformThreadRef set_vgem_fd_thread_id_; On 2015/08/18 08:47:34, reveman wrote: > On 2015/08/17 at 19:17:29, spang wrote: > > On 2015/08/17 19:09:52, reveman wrote: > > > On 2015/08/17 at 19:04:33, spang wrote: > > > > Can you use base::ThreadChecker for this? > > > > > > I'm afraid not as we don't know what a valid thread is. All we know is that > > > Wait() should not be called on the same thread that the fd was set on. ThreadChecker is better, because it's just wrapper of PlatformThreadRef I replace it with ThreadChecker here as well as ChildIOSurfaceManager > > > > Ah, I see. > > > > We should use DCHECK_IS_ON() instead of !NDEBUG so that DCHECK_ALWAYS_ON > enables these assertions. > > sure but if we change this to DCHECK_IS_ON(), please change > ChildIOSurfaceManager too. As replacing it with ThreadChecker, macro isn't needed anymore because ThreadChecker has the macro internally. https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/public/client... File ui/ozone/public/client_native_pixmap_factory.h (right): https://codereview.chromium.org/1248713002/diff/220001/ui/ozone/public/client... ui/ozone/public/client_native_pixmap_factory.h:38: virtual void SetVgemFD(scoped_ptr<base::ScopedFD> vgem_fd) = 0; On 2015/08/17 18:03:41, spang wrote: > I'd prefer > > virtual void Initialize(base::ScopedFD device_fd) = 0; > > so it's clear it's a single method you call once before doing anything else. > It'd be very incorrect to call it a 2nd time. good idea. Done.
Patchset #9 (id:240001) has been deleted
https://codereview.chromium.org/1248713002/diff/260001/content/child/child_io... File content/child/child_io_surface_manager_mac.cc (right): https://codereview.chromium.org/1248713002/diff/260001/content/child/child_io... content/child/child_io_surface_manager_mac.cc:99: DCHECK(!set_token_thread_checker_->CalledOnValidThread()); this is not the same. you can't use a thread checker to verify that that some code is called on the wrong thread. it's confusing as a start but also incorrect as the thread checker is allowed to always return true and this is then broken. please change it back but use DCHECK_IS_ON instead
https://codereview.chromium.org/1248713002/diff/260001/content/child/child_io... File content/child/child_io_surface_manager_mac.cc (right): https://codereview.chromium.org/1248713002/diff/260001/content/child/child_io... content/child/child_io_surface_manager_mac.cc:99: DCHECK(!set_token_thread_checker_->CalledOnValidThread()); On 2015/08/18 14:34:27, reveman wrote: > this is not the same. you can't use a thread checker to verify that that some > code is called on the wrong thread. it's confusing as a start but also incorrect > as the thread checker is allowed to always return true and this is then broken. > please change it back but use DCHECK_IS_ON instead How it's different. The implementation is same; https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... Keep in mind |set_token_thread_checker_| is created in set_token().
https://codereview.chromium.org/1248713002/diff/260001/content/child/child_io... File content/child/child_io_surface_manager_mac.cc (right): https://codereview.chromium.org/1248713002/diff/260001/content/child/child_io... content/child/child_io_surface_manager_mac.cc:99: DCHECK(!set_token_thread_checker_->CalledOnValidThread()); On 2015/08/18 at 14:45:50, dshwang wrote: > On 2015/08/18 14:34:27, reveman wrote: > > this is not the same. you can't use a thread checker to verify that that some > > code is called on the wrong thread. it's confusing as a start but also incorrect > > as the thread checker is allowed to always return true and this is then broken. > > please change it back but use DCHECK_IS_ON instead > > How it's different. The implementation is same; https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... > Keep in mind |set_token_thread_checker_| is created in set_token(). if ENABLE_THREAD_CHECKER is false then this is the implementation used: https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... and this check will always fail.
https://codereview.chromium.org/1248713002/diff/260001/content/child/child_io... File content/child/child_io_surface_manager_mac.cc (right): https://codereview.chromium.org/1248713002/diff/260001/content/child/child_io... content/child/child_io_surface_manager_mac.cc:99: DCHECK(!set_token_thread_checker_->CalledOnValidThread()); On 2015/08/18 15:02:38, reveman wrote: > On 2015/08/18 at 14:45:50, dshwang wrote: > > On 2015/08/18 14:34:27, reveman wrote: > > > this is not the same. you can't use a thread checker to verify that that > some > > > code is called on the wrong thread. it's confusing as a start but also > incorrect > > > as the thread checker is allowed to always return true and this is then > broken. > > > please change it back but use DCHECK_IS_ON instead > > > > How it's different. The implementation is same; > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... > > Keep in mind |set_token_thread_checker_| is created in set_token(). > > if ENABLE_THREAD_CHECKER is false then this is the implementation used: > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... > and this check will always fail. ENABLE_THREAD_CHECKER is true if (!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)). https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... DCHECK is enabled only if (!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)). We check CalledOnValidThread() inside DCHECK, otherwise it's no-op. so I think it's ok. WDYT?
On 2015/08/18 at 15:41:44, dongseong.hwang wrote: > https://codereview.chromium.org/1248713002/diff/260001/content/child/child_io... > File content/child/child_io_surface_manager_mac.cc (right): > > https://codereview.chromium.org/1248713002/diff/260001/content/child/child_io... > content/child/child_io_surface_manager_mac.cc:99: DCHECK(!set_token_thread_checker_->CalledOnValidThread()); > On 2015/08/18 15:02:38, reveman wrote: > > On 2015/08/18 at 14:45:50, dshwang wrote: > > > On 2015/08/18 14:34:27, reveman wrote: > > > > this is not the same. you can't use a thread checker to verify that that > > some > > > > code is called on the wrong thread. it's confusing as a start but also > > incorrect > > > > as the thread checker is allowed to always return true and this is then > > broken. > > > > please change it back but use DCHECK_IS_ON instead > > > > > > How it's different. The implementation is same; > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... > > > Keep in mind |set_token_thread_checker_| is created in set_token(). > > > > if ENABLE_THREAD_CHECKER is false then this is the implementation used: > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... > > and this check will always fail. > > ENABLE_THREAD_CHECKER is true if (!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)). > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... > > DCHECK is enabled only if (!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)). > We check CalledOnValidThread() inside DCHECK, otherwise it's no-op. so I think it's ok. > WDYT? ThreadChecker exists for the purpose to check if some code is executing on the current thread. Using it to check the inverse is an abuse and there's no guarantee that it will not break in the future as it's not what it's meant to be used for.
On 2015/08/18 15:51:38, reveman wrote: > On 2015/08/18 at 15:41:44, dongseong.hwang wrote: > > > https://codereview.chromium.org/1248713002/diff/260001/content/child/child_io... > > File content/child/child_io_surface_manager_mac.cc (right): > > > > > https://codereview.chromium.org/1248713002/diff/260001/content/child/child_io... > > content/child/child_io_surface_manager_mac.cc:99: > DCHECK(!set_token_thread_checker_->CalledOnValidThread()); > > On 2015/08/18 15:02:38, reveman wrote: > > > On 2015/08/18 at 14:45:50, dshwang wrote: > > > > On 2015/08/18 14:34:27, reveman wrote: > > > > > this is not the same. you can't use a thread checker to verify that that > > > some > > > > > code is called on the wrong thread. it's confusing as a start but also > > > incorrect > > > > > as the thread checker is allowed to always return true and this is then > > > broken. > > > > > please change it back but use DCHECK_IS_ON instead > > > > > > > > How it's different. The implementation is same; > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... > > > > Keep in mind |set_token_thread_checker_| is created in set_token(). > > > > > > if ENABLE_THREAD_CHECKER is false then this is the implementation used: > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... > > > and this check will always fail. > > > > ENABLE_THREAD_CHECKER is true if (!defined(NDEBUG) || > defined(DCHECK_ALWAYS_ON)). > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... > > > > DCHECK is enabled only if (!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)). > > We check CalledOnValidThread() inside DCHECK, otherwise it's no-op. so I think > it's ok. > > WDYT? > > ThreadChecker exists for the purpose to check if some code is executing on the > current thread. Using it to check the inverse is an abuse and there's no > guarantee that it will not break in the future as it's not what it's meant to be > used for. Ok, I'll roll back. On the other hands, PlatformThreadRef should be used with base::Lock to ensure visibility between threads as ThreadChecker implementation. I'll roll back with Lock protection.
https://codereview.chromium.org/1248713002/diff/280001/content/child/child_io... File content/child/child_io_surface_manager_mac.cc (right): https://codereview.chromium.org/1248713002/diff/280001/content/child/child_io... content/child/child_io_surface_manager_mac.cc:100: DCHECK(!(set_token_thread_id_ == base::PlatformThread::CurrentRef())); I rollback to PlatformThreadRef. I don't use base::Lock because base::WaitableEvent makes sure visibility before this line. Thank you for patient explanation.
lgtm
dongseong.hwang@intel.com changed reviewers: + brettw@chromium.org, jam@chromium.org
dongseong.hwang@intel.com changed reviewers: - brettw@chromium.org, jam@chromium.org
dongseong.hwang@intel.com changed reviewers: + brettw@chromium.org, dcheng@chromium.org, jam@chromium.org, sievers@chromium.org
dongseong.hwang@intel.com changed reviewers: - dcheng@chromium.org, sievers@chromium.org
As https://codereview.chromium.org/1263323004 and https://codereview.chromium.org/1134993003/ are landed, resume this CL's review. spang, thx for reviewing. reveman, could you review overall change? jam, brettw, could you review base/threading/thread_restrictions.h? dcheng, could you review content/common/child_process_messages.h? https://codereview.chromium.org/1248713002/diff/300001/base/threading/thread_... File base/threading/thread_restrictions.h (right): https://codereview.chromium.org/1248713002/diff/300001/base/threading/thread_... base/threading/thread_restrictions.h:198: friend class ui::ClientNativePixmapFactoryGbm; jam, brettw, could you allow this? Renderer needs to wait in impl thread only one time in whole process lifecycle. See ClientNativePixmapFactoryGbm::ImportFromHandle(). https://codereview.chromium.org/1248713002/diff/300001/content/child/child_io... File content/child/child_io_surface_manager_mac.cc (right): https://codereview.chromium.org/1248713002/diff/300001/content/child/child_io... content/child/child_io_surface_manager_mac.cc:99: set_token_event_.Wait(); reveman, it should require "base::ThreadRestrictions::ScopedAllowWait allow_wait;" not to crash, right? https://codereview.chromium.org/1248713002/diff/300001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1248713002/diff/300001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:76: base::ThreadRestrictions::ScopedAllowWait allow_wait; It waits only one time in whole render process lifecycle at the begining. It's called in impl thread in the same manner of ChildIOSurfaceManager::AcquireIOSurface().
dongseong.hwang@intel.com changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/1248713002/diff/300001/content/child/child_io... File content/child/child_io_surface_manager_mac.cc (right): https://codereview.chromium.org/1248713002/diff/300001/content/child/child_io... content/child/child_io_surface_manager_mac.cc:99: set_token_event_.Wait(); On 2015/08/21 at 13:04:10, dshwang_ooo_5.9-27.9 wrote: > reveman, it should require "base::ThreadRestrictions::ScopedAllowWait allow_wait;" not to crash, right? Only if this is called on a thread where wait is not allowed. Is it? We typically call this on a worker thread.
https://codereview.chromium.org/1248713002/diff/300001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1248713002/diff/300001/content/browser/browse... content/browser/browser_main_loop.cc:625: auto scoped_fd = Use the full typename rather than auto (ScopedFD isn't that long, and makes this a lot clearer) https://codereview.chromium.org/1248713002/diff/300001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1248713002/diff/300001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1612: auto device_fd = Same. https://codereview.chromium.org/1248713002/diff/300001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): https://codereview.chromium.org/1248713002/diff/300001/ui/ozone/platform/drm/... ui/ozone/platform/drm/ozone_platform_gbm.cc:159: int vgem_fd = open(kVgemPath, O_RDWR | O_CLOEXEC); base::ScopedFD vgem_fd(open(...)); https://codereview.chromium.org/1248713002/diff/300001/ui/ozone/platform/drm/... ui/ozone/platform/drm/ozone_platform_gbm.cc:160: DCHECK_GE(vgem_fd, 0) << "Failed to open: " << kVgemPath; DCHECK(vgem_fd.is_valid()) https://codereview.chromium.org/1248713002/diff/300001/ui/ozone/platform/drm/... ui/ozone/platform/drm/ozone_platform_gbm.cc:161: return base::ScopedFD(vgem_fd); return vgem_fd
https://codereview.chromium.org/1248713002/diff/300001/content/child/child_io... File content/child/child_io_surface_manager_mac.cc (right): https://codereview.chromium.org/1248713002/diff/300001/content/child/child_io... content/child/child_io_surface_manager_mac.cc:99: set_token_event_.Wait(); On 2015/08/21 14:40:31, reveman wrote: > On 2015/08/21 at 13:04:10, dshwang_ooo_5.9-27.9 wrote: > > reveman, it should require "base::ThreadRestrictions::ScopedAllowWait > allow_wait;" not to crash, right? > > Only if this is called on a thread where wait is not allowed. Is it? We > typically call this on a worker thread. Ah, compositor creates GMB in worker thread, not impl thread. sorry for misleading. By the way, GMB is created in renderer_gpu_video_accelerator_factories.cc and CreateGpuMemoryBufferImageCHROMIUM in gles2_implementation also. So I think each platform GBM implementation needs base::ThreadRestrictions::ScopedAllowWait
https://codereview.chromium.org/1248713002/diff/300001/content/child/child_io... File content/child/child_io_surface_manager_mac.cc (right): https://codereview.chromium.org/1248713002/diff/300001/content/child/child_io... content/child/child_io_surface_manager_mac.cc:99: set_token_event_.Wait(); On 2015/08/21 15:21:18, dshwang_ooo_5.9-27.9 wrote: > On 2015/08/21 14:40:31, reveman wrote: > > On 2015/08/21 at 13:04:10, dshwang_ooo_5.9-27.9 wrote: > > > reveman, it should require "base::ThreadRestrictions::ScopedAllowWait > > allow_wait;" not to crash, right? > > > > Only if this is called on a thread where wait is not allowed. Is it? We > > typically call this on a worker thread. > > Ah, compositor creates GMB in worker thread, not impl thread. sorry for > misleading. > By the way, GMB is created in renderer_gpu_video_accelerator_factories.cc and > CreateGpuMemoryBufferImageCHROMIUM in gles2_implementation also. > So I think each platform GBM implementation needs > base::ThreadRestrictions::ScopedAllowWait And crash happens in worker thread in chromeos. For some reasons, worker thread of chromeos is not allowed to wait by default. Is MacOS fine?
On 2015/08/21 at 15:56:51, dongseong.hwang wrote: > https://codereview.chromium.org/1248713002/diff/300001/content/child/child_io... > File content/child/child_io_surface_manager_mac.cc (right): > > https://codereview.chromium.org/1248713002/diff/300001/content/child/child_io... > content/child/child_io_surface_manager_mac.cc:99: set_token_event_.Wait(); > On 2015/08/21 15:21:18, dshwang_ooo_5.9-27.9 wrote: > > On 2015/08/21 14:40:31, reveman wrote: > > > On 2015/08/21 at 13:04:10, dshwang_ooo_5.9-27.9 wrote: > > > > reveman, it should require "base::ThreadRestrictions::ScopedAllowWait > > > allow_wait;" not to crash, right? > > > > > > Only if this is called on a thread where wait is not allowed. Is it? We > > > typically call this on a worker thread. > > > > Ah, compositor creates GMB in worker thread, not impl thread. sorry for > > misleading. > > By the way, GMB is created in renderer_gpu_video_accelerator_factories.cc and > > CreateGpuMemoryBufferImageCHROMIUM in gles2_implementation also. > > So I think each platform GBM implementation needs > > base::ThreadRestrictions::ScopedAllowWait > > And crash happens in worker thread in chromeos. For some reasons, worker thread of chromeos is not allowed to wait by default. Is MacOS fine? Ok. we probably need to add this then.
https://codereview.chromium.org/1248713002/diff/300001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1248713002/diff/300001/content/browser/browse... content/browser/browser_main_loop.cc:625: auto scoped_fd = On 2015/08/21 14:44:06, dcheng wrote: > Use the full typename rather than auto (ScopedFD isn't that long, and makes this > a lot clearer) Done. https://codereview.chromium.org/1248713002/diff/300001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1248713002/diff/300001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1612: auto device_fd = On 2015/08/21 14:44:06, dcheng wrote: > Same. Done. https://codereview.chromium.org/1248713002/diff/300001/content/child/child_io... File content/child/child_io_surface_manager_mac.cc (right): https://codereview.chromium.org/1248713002/diff/300001/content/child/child_io... content/child/child_io_surface_manager_mac.cc:99: set_token_event_.Wait(); On 2015/08/21 15:56:50, dshwang_ooo_5.9-27.9 wrote: > On 2015/08/21 15:21:18, dshwang_ooo_5.9-27.9 wrote: > > On 2015/08/21 14:40:31, reveman wrote: > > > On 2015/08/21 at 13:04:10, dshwang_ooo_5.9-27.9 wrote: > > > > reveman, it should require "base::ThreadRestrictions::ScopedAllowWait > > > allow_wait;" not to crash, right? > > > > > > Only if this is called on a thread where wait is not allowed. Is it? We > > > typically call this on a worker thread. > > > > Ah, compositor creates GMB in worker thread, not impl thread. sorry for > > misleading. > > By the way, GMB is created in renderer_gpu_video_accelerator_factories.cc and > > CreateGpuMemoryBufferImageCHROMIUM in gles2_implementation also. > > So I think each platform GBM implementation needs > > base::ThreadRestrictions::ScopedAllowWait > > And crash happens in worker thread in chromeos. For some reasons, worker thread > of chromeos is not allowed to wait by default. Is MacOS fine? On 2015/08/21 16:33:28, reveman wrote: > Ok. we probably need to add this then. Added. https://codereview.chromium.org/1248713002/diff/300001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): https://codereview.chromium.org/1248713002/diff/300001/ui/ozone/platform/drm/... ui/ozone/platform/drm/ozone_platform_gbm.cc:159: int vgem_fd = open(kVgemPath, O_RDWR | O_CLOEXEC); On 2015/08/21 14:44:06, dcheng wrote: > base::ScopedFD vgem_fd(open(...)); Done. https://codereview.chromium.org/1248713002/diff/300001/ui/ozone/platform/drm/... ui/ozone/platform/drm/ozone_platform_gbm.cc:160: DCHECK_GE(vgem_fd, 0) << "Failed to open: " << kVgemPath; On 2015/08/21 14:44:06, dcheng wrote: > DCHECK(vgem_fd.is_valid()) Done. https://codereview.chromium.org/1248713002/diff/300001/ui/ozone/platform/drm/... ui/ozone/platform/drm/ozone_platform_gbm.cc:161: return base::ScopedFD(vgem_fd); On 2015/08/21 14:44:06, dcheng wrote: > return vgem_fd Done.
lgtm with nit https://codereview.chromium.org/1248713002/diff/320001/content/common/child_p... File content/common/child_process_messages.h (right): https://codereview.chromium.org/1248713002/diff/320001/content/common/child_p... content/common/child_process_messages.h:133: // Sent ClientNativePixmap device file descriptor to child processes to nit: comment is a bit confusing as "Sent" doesn't refer to the message but the fd when structures like this. Maybe something like: // Sent to child processes to initialize ClientNativePixmapFactory using // a device file descriptor.
the mac code should be split into a different change. I would like to learn more abbout why the mac code has that waiting on an event code. i.e. why can't the IPC that carries the necessary information be guaranteed to arrive before when it's needed? This seems like it should be possible. https://codereview.chromium.org/1248713002/diff/320001/content/child/child_th... File content/child/child_thread_impl.cc (right): https://codereview.chromium.org/1248713002/diff/320001/content/child/child_th... content/child/child_thread_impl.cc:218: ui::ClientNativePixmapFactory::GetInstance()->Initialize( do you specifically want this to happen on the IO thread? is the class you're calling thread safe? i.e. why are you using a filter?
On 2015/08/24 at 15:00:32, jam wrote: > the mac code should be split into a different change. > > I would like to learn more abbout why the mac code has that waiting on an event code. i.e. why can't the IPC that carries the necessary information be guaranteed to arrive before when it's needed? This seems like it should be possible. IOSurfaceManager code uses Mach IPC to request an IOSurface reference from the browser process. IOSurfaceManager needs a secure token before it can send this Mach message. This token is sent from the browser process using the regular IPC after spawning the child process and we need to make sure it has arrived before sending a Mach IPC message to the browser. Alternatively, RenderThreadImpl initialization can be deferred until this message has arrived but I'm not sure that's such a good idea or even possible without major changes to the code. > > https://codereview.chromium.org/1248713002/diff/320001/content/child/child_th... > File content/child/child_thread_impl.cc (right): > > https://codereview.chromium.org/1248713002/diff/320001/content/child/child_th... > content/child/child_thread_impl.cc:218: ui::ClientNativePixmapFactory::GetInstance()->Initialize( > do you specifically want this to happen on the IO thread? is the class you're calling thread safe? > > i.e. why are you using a filter?
On 2015/08/24 15:54:25, reveman wrote: > On 2015/08/24 at 15:00:32, jam wrote: > > the mac code should be split into a different change. > > > > I would like to learn more abbout why the mac code has that waiting on an > event code. i.e. why can't the IPC that carries the necessary information be > guaranteed to arrive before when it's needed? This seems like it should be > possible. > > IOSurfaceManager code uses Mach IPC to request an IOSurface reference from the > browser process. Is this because it's not possible to send an IOSurface over chrome ipc? > IOSurfaceManager needs a secure token before it can send this > Mach message. If the answer to my previous question is yes, then I have more questions: -i'm confused why a browser starts a renderer, then gives it a token, and then the renderer asks the browser for something with that token. i.e. the browser side code knows if an ipc is coming from a specific renderer, why is the token needed? -is there a design doc or background reading somewhere for this new use of mach ipc in chrome? is the reason mach ipc is used here is because it's an unspoofable method of capability passing? > This token is sent from the browser process using the regular IPC > after spawning the child process and we need to make sure it has arrived before > sending a Mach IPC message to the browser. Alternatively, RenderThreadImpl > initialization can be deferred until this message has arrived but I'm not sure > that's such a good idea or even possible without major changes to the code. > > > > > > https://codereview.chromium.org/1248713002/diff/320001/content/child/child_th... > > File content/child/child_thread_impl.cc (right): > > > > > https://codereview.chromium.org/1248713002/diff/320001/content/child/child_th... > > content/child/child_thread_impl.cc:218: > ui::ClientNativePixmapFactory::GetInstance()->Initialize( > > do you specifically want this to happen on the IO thread? is the class you're > calling thread safe? > > > > i.e. why are you using a filter?
reveman@chromium.org changed reviewers: + rsesek@chromium.org
+rsesek
On 2015/08/24 at 16:52:15, jam wrote: > On 2015/08/24 15:54:25, reveman wrote: > > On 2015/08/24 at 15:00:32, jam wrote: > > > the mac code should be split into a different change. > > > > > > I would like to learn more abbout why the mac code has that waiting on an > > event code. i.e. why can't the IPC that carries the necessary information be > > guaranteed to arrive before when it's needed? This seems like it should be > > possible. > > > > IOSurfaceManager code uses Mach IPC to request an IOSurface reference from the > > browser process. > > Is this because it's not possible to send an IOSurface over chrome ipc? Yes, the alternative is making them global and accessible by any process in the system. That's what we used to do until this IOSurfaceManager system was introduced. > > > IOSurfaceManager needs a secure token before it can send this > > Mach message. > > If the answer to my previous question is yes, then I have more questions: > -i'm confused why a browser starts a renderer, then gives it a token, and then the renderer asks the browser for something with that token. i.e. the browser side code knows if an ipc is coming from a specific renderer, why is the token needed? The browser side code doesn't really know in the case of Mach IPC. It knows what PID the message came from but the way Mach IPC messages are handled in the browser makes it very hard to have an up to date view of child process -> PID. The secure token approach for verifying the identity of a child process Mach message was eventually chosen as the preferred method. See https://codereview.chromium.org/1137453002 for more about this. > -is there a design doc or background reading somewhere for this new use of mach ipc in chrome? is the reason mach ipc is used here is because it's an unspoofable method of capability passing? There's https://docs.google.com/a/chromium.org/document/d/1L8wZtEDtQ2S6dLfNcZHopMgZVz... for the general concept of using Mach IPC to share IOSurfaces instead of using the old insecure mechanism where we made them global. However, this doc doesn't include the details of using a browser side unguessable token in favor of checking the PID as that evolved from the code review.
On 2015/08/24 17:10:07, reveman wrote: > On 2015/08/24 at 16:52:15, jam wrote: > > On 2015/08/24 15:54:25, reveman wrote: > > > On 2015/08/24 at 15:00:32, jam wrote: > > > > the mac code should be split into a different change. > > > > > > > > I would like to learn more abbout why the mac code has that waiting on an > > > event code. i.e. why can't the IPC that carries the necessary information be > > > guaranteed to arrive before when it's needed? This seems like it should be > > > possible. > > > > > > IOSurfaceManager code uses Mach IPC to request an IOSurface reference from > the > > > browser process. > > > > Is this because it's not possible to send an IOSurface over chrome ipc? > > Yes, the alternative is making them global and accessible by any process in the > system. That's what we used to do until this IOSurfaceManager system was > introduced. > > > > > > IOSurfaceManager needs a secure token before it can send this > > > Mach message. > > > > If the answer to my previous question is yes, then I have more questions: > > -i'm confused why a browser starts a renderer, then gives it a token, and then > the renderer asks the browser for something with that token. i.e. the browser > side code knows if an ipc is coming from a specific renderer, why is the token > needed? > > The browser side code doesn't really know in the case of Mach IPC. It knows what > PID the message came from but the way Mach IPC messages are handled in the > browser makes it very hard to have an up to date view of child process -> PID. > The secure token approach for verifying the identity of a child process Mach > message was eventually chosen as the preferred method. See > https://codereview.chromium.org/1137453002 for more about this. Can you link to specific messages? there are 40 there and I searched for token and pid and didn't find the discussion. > > > -is there a design doc or background reading somewhere for this new use of > mach ipc in chrome? is the reason mach ipc is used here is because it's an > unspoofable method of capability passing? > > There's > https://docs.google.com/a/chromium.org/document/d/1L8wZtEDtQ2S6dLfNcZHopMgZVz... > for the general concept of using Mach IPC to share IOSurfaces instead of using > the old insecure mechanism where we made them global. However, this doc doesn't > include the details of using a browser side unguessable token in favor of > checking the PID as that evolved from the code review.
On 2015/08/24 at 21:36:10, jam wrote: > On 2015/08/24 17:10:07, reveman wrote: > > On 2015/08/24 at 16:52:15, jam wrote: > > > On 2015/08/24 15:54:25, reveman wrote: > > > > On 2015/08/24 at 15:00:32, jam wrote: > > > > > the mac code should be split into a different change. > > > > > > > > > > I would like to learn more abbout why the mac code has that waiting on an > > > > event code. i.e. why can't the IPC that carries the necessary information be > > > > guaranteed to arrive before when it's needed? This seems like it should be > > > > possible. > > > > > > > > IOSurfaceManager code uses Mach IPC to request an IOSurface reference from > > the > > > > browser process. > > > > > > Is this because it's not possible to send an IOSurface over chrome ipc? > > > > Yes, the alternative is making them global and accessible by any process in the > > system. That's what we used to do until this IOSurfaceManager system was > > introduced. > > > > > > > > > IOSurfaceManager needs a secure token before it can send this > > > > Mach message. > > > > > > If the answer to my previous question is yes, then I have more questions: > > > -i'm confused why a browser starts a renderer, then gives it a token, and then > > the renderer asks the browser for something with that token. i.e. the browser > > side code knows if an ipc is coming from a specific renderer, why is the token > > needed? > > > > The browser side code doesn't really know in the case of Mach IPC. It knows what > > PID the message came from but the way Mach IPC messages are handled in the > > browser makes it very hard to have an up to date view of child process -> PID. > > The secure token approach for verifying the identity of a child process Mach > > message was eventually chosen as the preferred method. See > > https://codereview.chromium.org/1137453002 for more about this. > > Can you link to specific messages? there are 40 there and I searched for token and pid and didn't find the discussion. Sorry, looks like the code review never captured the discussion Robert and I had regarding this. I've updated the design doc with a "Child Process Verification" section that hopefully explains it. Please comment on the doc if you still have questions.
https://codereview.chromium.org/1248713002/diff/320001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1248713002/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:82: DCHECK_GE(vgem_fd_.get(), 0); I think this DCHECK_GE is all we need after discussing this with jam@. As the InitializeClientNativePixmapFactory message is being handled on the IO thread, there's no way this could be called without that first being processed. It would require that we tried to allocate a buffer before a widget is created and that should not happen today and can be considered unsupported. I think the need in the IOSurfaceManager case is from a time when this message was not handled on the IO thread. I'll change the IOSurfaceManager code to use a DCHECK too instead of a Wait in a separate CL.
dongseong.hwang@intel.com changed reviewers: + sievers@chromium.org - brettw@chromium.org
reveman, jam, thx for reviewing. sievers, could you review content/? dcheng, could you review content/common/child_process_messages.h? https://codereview.chromium.org/1248713002/diff/320001/content/common/child_p... File content/common/child_process_messages.h (right): https://codereview.chromium.org/1248713002/diff/320001/content/common/child_p... content/common/child_process_messages.h:133: // Sent ClientNativePixmap device file descriptor to child processes to On 2015/08/22 14:15:05, reveman wrote: > nit: comment is a bit confusing as "Sent" doesn't refer to the message but the > fd when structures like this. Maybe something like: > > // Sent to child processes to initialize ClientNativePixmapFactory using > // a device file descriptor. Done. Thx. https://codereview.chromium.org/1248713002/diff/320001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1248713002/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:82: DCHECK_GE(vgem_fd_.get(), 0); On 2015/08/25 17:06:19, reveman wrote: > I think this DCHECK_GE is all we need after discussing this with jam@. As the > InitializeClientNativePixmapFactory message is being handled on the IO thread, > there's no way this could be called without that first being processed. It would > require that we tried to allocate a buffer before a widget is created and that > should not happen today and can be considered unsupported. > > I think the need in the IOSurfaceManager case is from a time when this message > was not handled on the IO thread. I'll change the IOSurfaceManager code to use a > DCHECK too instead of a Wait in a separate CL. That's excellent conclusion which everybody is happy. Thank you for your investigation. I updated.
content lgtm https://codereview.chromium.org/1248713002/diff/340001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1248713002/diff/340001/content/browser/browse... content/browser/browser_main_loop.cc:627: ui::ClientNativePixmapFactory::GetInstance()->Initialize(scoped_fd.Pass()); nit: you don't need the local var https://codereview.chromium.org/1248713002/diff/340001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1248713002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1615: base::FileDescriptor(device_fd.Pass()))); nit: same here
thanks for reviewing! dcheng, could you review content/common/child_process_messages.h? https://codereview.chromium.org/1248713002/diff/340001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1248713002/diff/340001/content/browser/browse... content/browser/browser_main_loop.cc:627: ui::ClientNativePixmapFactory::GetInstance()->Initialize(scoped_fd.Pass()); On 2015/08/26 22:07:21, sievers wrote: > nit: you don't need the local var Done. https://codereview.chromium.org/1248713002/diff/340001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1248713002/diff/340001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1615: base::FileDescriptor(device_fd.Pass()))); On 2015/08/26 22:07:21, sievers wrote: > nit: same here Done.
dcheng, could you review content/common/child_process_messages.h? https://codereview.chromium.org/1248713002/diff/360001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1248713002/diff/360001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:43: base::AutoLock lock(vgem_fd_lock_); remove this lock to fit the decision of https://codereview.chromium.org/1312353002/
Patchset #15 (id:380001) has been deleted
https://codereview.chromium.org/1248713002/diff/320001/base/threading/thread_... File base/threading/thread_restrictions.h (right): https://codereview.chromium.org/1248713002/diff/320001/base/threading/thread_... base/threading/thread_restrictions.h:182: friend class content::ChildIOSurfaceManager; Did you talk to jam or brettw about adding this line? https://codereview.chromium.org/1248713002/diff/400001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1248713002/diff/400001/content/browser/browse... content/browser/browser_main_loop.cc:626: ui::OzonePlatform::GetInstance()->OpenClientNativePixmapDevice().Pass()); Pass() here is unnecessary. https://codereview.chromium.org/1248713002/diff/400001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1248713002/diff/400001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1606: .Pass()))); Pass() here is also unnecessary. https://codereview.chromium.org/1248713002/diff/400001/content/test/content_t... File content/test/content_test_suite.cc (right): https://codereview.chromium.org/1248713002/diff/400001/content/test/content_t... content/test/content_test_suite.cc:122: .Pass()); Pass() here should also be unnecessary. https://codereview.chromium.org/1248713002/diff/400001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): https://codereview.chromium.org/1248713002/diff/400001/ui/ozone/platform/drm/... ui/ozone/platform/drm/ozone_platform_gbm.cc:161: return vgem_fd.Pass(); Pass() here is also unnecessary (NRVO applies)
dcheng, thanks for reviewing. could you review content/common/child_process_messages.h again? https://codereview.chromium.org/1248713002/diff/320001/base/threading/thread_... File base/threading/thread_restrictions.h (right): https://codereview.chromium.org/1248713002/diff/320001/base/threading/thread_... base/threading/thread_restrictions.h:182: friend class content::ChildIOSurfaceManager; On 2015/08/31 17:40:53, dcheng wrote: > Did you talk to jam or brettw about adding this line? you reviewed old and deprecated patch set. current patch set (15th) doesn't touch thread_restrictions.h https://codereview.chromium.org/1248713002/diff/400001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1248713002/diff/400001/content/browser/browse... content/browser/browser_main_loop.cc:626: ui::OzonePlatform::GetInstance()->OpenClientNativePixmapDevice().Pass()); On 2015/08/31 17:40:53, dcheng wrote: > Pass() here is unnecessary. Done. I didn't know that ScopedGeneric has move constructor. Thx. https://codereview.chromium.org/1248713002/diff/400001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1248713002/diff/400001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1606: .Pass()))); On 2015/08/31 17:40:53, dcheng wrote: > Pass() here is also unnecessary. Done. https://codereview.chromium.org/1248713002/diff/400001/content/test/content_t... File content/test/content_test_suite.cc (right): https://codereview.chromium.org/1248713002/diff/400001/content/test/content_t... content/test/content_test_suite.cc:122: .Pass()); On 2015/08/31 17:40:53, dcheng wrote: > Pass() here should also be unnecessary. Done. https://codereview.chromium.org/1248713002/diff/400001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/ozone_platform_gbm.cc (right): https://codereview.chromium.org/1248713002/diff/400001/ui/ozone/platform/drm/... ui/ozone/platform/drm/ozone_platform_gbm.cc:161: return vgem_fd.Pass(); On 2015/08/31 17:40:53, dcheng wrote: > Pass() here is also unnecessary (NRVO applies) Done.
Oops, sorry for the stale comment. lgtm
On 2015/08/31 18:22:13, dcheng wrote: > Oops, sorry for the stale comment. lgtm thx for reviewing!
The CQ bit was checked by dongseong.hwang@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org, reveman@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1248713002/#ps420001 (title: "remove redundant Pass()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1248713002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248713002/420001
Message was sent while issue was closed.
Committed patchset #16 (id:420001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/9e59fefbb589611ce7fb9823754f21a48bd09887 Cr-Commit-Position: refs/heads/master@{#346433} |
