Fix crash in XServerPixelBuffer.

Fix crash in XServerPixelBuffer.

XServerPixelBuffer contains x_image_ field, that may be allocated using
XGetImage() or XShmCreateImage() depending how the last frame was captured.
x_image_ is passed XShmGetImage(), which may crash if it gets image
allocated with XGetImage(). Added x_shm_image_ to ensure that SHM and
non-SHM capture paths are separate and XShmGetImage() is allways called
with the correct XImage.

The linked bug appears to be a regressiona after
https://codereview.webrtc.org/2044693002

BUG=chromium:697823
Review-Url: https://codereview.webrtc.org/2796673002
Cr-Commit-Position: refs/heads/master@{#17518}
Committed: https://chromium.googlesource.com/external/webrtc/+/0786c04f972cd15f5dfb2e5882aba5c6efb214e3

[Sergey Ulanov, 2017-04-03 18:14:18]

sergeyu@chromium.org changed reviewers:
+ gyzhou@chromium.org, zijiehe@chromium.org

[Hzj_jie, 2017-04-03 18:43:19]

The change itself has clearly done what mentioned in the description. But I
think the reason is not so persuasive. The XShmGetImage() only calls when
shm_segment_info_ is not nullptr, i.e. the x_image_ should become
XShmCreateImage() in this scenario.

[Sergey Ulanov, 2017-04-03 18:54:21]

On 2017/04/03 18:43:19, Hzj_jie wrote:
> The change itself has clearly done what mentioned in the description. But I
> think the reason is not so persuasive. The XShmGetImage() only calls when
> shm_segment_info_ is not nullptr, i.e. the x_image_ should become
> XShmCreateImage() in this scenario.

If XShmGetImage() fails then the capturer falls back to the non-SHM capturing,
which means that x_image_ is replaced with a new image created by XGetImage(),
see XServerPixelBuffer::CaptureRect(). shm_segment_info_ is not reset in that
case.

[Hzj_jie, 2017-04-03 18:58:39]

On 2017/04/03 18:54:21, Sergey Ulanov wrote:
> On 2017/04/03 18:43:19, Hzj_jie wrote:
> > The change itself has clearly done what mentioned in the description. But I
> > think the reason is not so persuasive. The XShmGetImage() only calls when
> > shm_segment_info_ is not nullptr, i.e. the x_image_ should become
> > XShmCreateImage() in this scenario.
> 
> If XShmGetImage() fails then the capturer falls back to the non-SHM capturing,
> which means that x_image_ is replaced with a new image created by XGetImage(),
> see XServerPixelBuffer::CaptureRect(). shm_segment_info_ is not reset in that
> case.

Do you mean once the shm_pixmap_ == null && xshm_get_image_succeeded_ == false?
If so, should we call ReleaseSharedMemorySegment() around line 260
(https://cs.chromium.org/chromium/src/third_party/webrtc/modules/desktop_captu...),
or do we still prefer to try to use XShmGetImage() again next time?

[GeorgeZ, 2017-04-03 18:59:58]

lgtm

LGTM with a nit.

https://codereview.webrtc.org/2796673002/diff/1/webrtc/modules/desktop_captur...
File webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.cc (right):

https://codereview.webrtc.org/2796673002/diff/1/webrtc/modules/desktop_captur...
webrtc/modules/desktop_capture/x11/x_server_pixel_buffer.cc:61: // In
FastBlit(), the operation is effectively a memcpy.
Please explain a little of SlowBlit().

[Sergey Ulanov, 2017-04-03 23:28:23]

On 2017/04/03 18:58:39, Hzj_jie wrote:
> On 2017/04/03 18:54:21, Sergey Ulanov wrote:
> > On 2017/04/03 18:43:19, Hzj_jie wrote:
> > > The change itself has clearly done what mentioned in the description. But
I
> > > think the reason is not so persuasive. The XShmGetImage() only calls when
> > > shm_segment_info_ is not nullptr, i.e. the x_image_ should become
> > > XShmCreateImage() in this scenario.
> > 
> > If XShmGetImage() fails then the capturer falls back to the non-SHM
capturing,
> > which means that x_image_ is replaced with a new image created by
XGetImage(),
> > see XServerPixelBuffer::CaptureRect(). shm_segment_info_ is not reset in
that
> > case.
> 
> Do you mean once the shm_pixmap_ == null && xshm_get_image_succeeded_ ==
false?
> If so, should we call ReleaseSharedMemorySegment() around line 260
>
(https://cs.chromium.org/chromium/src/third_party/webrtc/modules/desktop_captu...),
> or do we still prefer to try to use XShmGetImage() again next time?

I think we still want to try to use SHM later. XShmGetImage() fails when the
window is moved partially out of the screen, but it may be moved back later.

[Hzj_jie, 2017-04-04 00:07:48]

On 2017/04/03 23:28:23, Sergey Ulanov wrote:
> On 2017/04/03 18:58:39, Hzj_jie wrote:
> > On 2017/04/03 18:54:21, Sergey Ulanov wrote:
> > > On 2017/04/03 18:43:19, Hzj_jie wrote:
> > > > The change itself has clearly done what mentioned in the description.
But
> I
> > > > think the reason is not so persuasive. The XShmGetImage() only calls
when
> > > > shm_segment_info_ is not nullptr, i.e. the x_image_ should become
> > > > XShmCreateImage() in this scenario.
> > > 
> > > If XShmGetImage() fails then the capturer falls back to the non-SHM
> capturing,
> > > which means that x_image_ is replaced with a new image created by
> XGetImage(),
> > > see XServerPixelBuffer::CaptureRect(). shm_segment_info_ is not reset in
> that
> > > case.
> > 
> > Do you mean once the shm_pixmap_ == null && xshm_get_image_succeeded_ ==
> false?
> > If so, should we call ReleaseSharedMemorySegment() around line 260
> >
>
(https://cs.chromium.org/chromium/src/third_party/webrtc/modules/desktop_captu...),
> > or do we still prefer to try to use XShmGetImage() again next time?
> 
> I think we still want to try to use SHM later. XShmGetImage() fails when the
> window is moved partially out of the screen, but it may be moved back later.

Thanks for the explanation. This change lgtm.

[Sergey Ulanov, 2017-04-04 00:22:13]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2017-04-04 00:22:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-04 01:08:08]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1491265333766370, "parent_rev":
"4eeb53748a47157077c37f69c4aed79feb931b61", "commit_rev":
"0786c04f972cd15f5dfb2e5882aba5c6efb214e3"}

[commit-bot: I haz the power, 2017-04-04 01:08:12]

Description was changed from

==========
Fix crash in XServerPixelBuffer.

XServerPixelBuffer contains x_image_ field, that may be allocated using
XGetImage() or XShmCreateImage() depending how the last frame was captured.
x_image_ is passed XShmGetImage(), which may crash if it gets image
allocated with XGetImage(). Added x_shm_image_ to ensure that SHM and
non-SHM capture paths are separate and XShmGetImage() is allways called
with the correct XImage.

The linked bug appears to be a regressiona after
https://codereview.webrtc.org/2044693002

BUG=chromium:697823
==========

to

==========
Fix crash in XServerPixelBuffer.

XServerPixelBuffer contains x_image_ field, that may be allocated using
XGetImage() or XShmCreateImage() depending how the last frame was captured.
x_image_ is passed XShmGetImage(), which may crash if it gets image
allocated with XGetImage(). Added x_shm_image_ to ensure that SHM and
non-SHM capture paths are separate and XShmGetImage() is allways called
with the correct XImage.

The linked bug appears to be a regressiona after
https://codereview.webrtc.org/2044693002

BUG=chromium:697823

Review-Url: https://codereview.webrtc.org/2796673002
Cr-Commit-Position: refs/heads/master@{#17518}
Committed:
https://chromium.googlesource.com/external/webrtc/+/0786c04f972cd15f5dfb2e588...
==========

[commit-bot: I haz the power, 2017-04-04 01:08:12]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/external/webrtc/+/0786c04f972cd15f5dfb2e588...