Fix potential use-after-free bug in VideoImageGenerator::onGetYUV8Planes.

Fix potential use-after-free bug in VideoImageGenerator::onGetYUV8Planes.

onGetYUV8Planes() exposes internal data of a VideoFrame and then delete the
VideoFrame. VideoImageGenerator must keep the VideoFrame until exposed data is
used.

BUG=91208
Committed: https://crrev.com/c4d8a6fbf8ac266cc26ab7b5bf45c760456fec73
Cr-Commit-Position: refs/heads/master@{#299144}

[dshwang, 2014-10-02 18:21:42]

dongseong.hwang@intel.com changed reviewers:
+ rileya@google.com, scherkus@chromium.org

[dshwang, 2014-10-02 18:21:43]

Could you review? Before I rebase https://codereview.chromium.org/445013002/ to
ToT, I wanna remove generator_ member variable.

[dshwang, 2014-10-02 18:23:52]

This CL is follow-up CL of rileya's
https://codereview.chromium.org/531353002
https://codereview.chromium.org/569313003

https://codereview.chromium.org/624633002/diff/1/media/filters/skcanvas_video...
File media/filters/skcanvas_video_renderer.cc (left):

https://codereview.chromium.org/624633002/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:278: planes[plane] =
frame_->data(plane) + offset;
It exposes VideoFrame's data to caller

https://codereview.chromium.org/624633002/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:282: frame_ = NULL;
Removing frame_ can cause seg fault. So I remove |frame_| after
SkCanvas::flush()

[dshwang, 2014-10-03 11:01:46]

dongseong.hwang@intel.com changed reviewers:
+ rileya@chromium.org
- rileya@google.com

[rileya (GONE FROM CHROMIUM), 2014-10-07 17:55:51]

Sorry for the delay, this got lost in my inbox! lgtm.

Those trybots look a bit red though, any idea what's up there?

[dshwang, 2014-10-09 19:59:57]

On 2014/10/07 17:55:51, rileya wrote:
> Sorry for the delay, this got lost in my inbox! lgtm.

Thank you for review!

scherkus@, could you review as owner?

https://codereview.chromium.org/624633002/diff/1/media/filters/skcanvas_video...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/624633002/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:367: generator->discard_frame();
> Those trybots look a bit red though, any idea what's up there?
This line caused use-after-free because SkCanvas::flush() can delete |generator|
already, so remove it.

[scherkus (not reviewing), 2014-10-09 20:01:32]

lgtm

[dshwang, 2014-10-10 14:13:55]

Thank you for review.
I rebase this CL to ToT after https://codereview.chromium.org/619343003/

https://codereview.chromium.org/624633002/diff/1/media/filters/skcanvas_video...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/624633002/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:367: generator->discard_frame();
On 2014/10/09 19:59:57, dshwang wrote:
> > Those trybots look a bit red though, any idea what's up there?
> This line caused use-after-free because SkCanvas::flush() can delete
|generator|
> already, so remove it.

It's wrong analysis. |generator| is deleted by replacing SkBitmap as follows:
last_frame_ = SkBitmapOperations::Rotate(last_frame_ =
SkBitmapOperations::Rotate(last_frame_, SkBitmapOperations::ROTATION_90_CW);
However replacement code is deleted in
https://codereview.chromium.org/619343003/, so this line is resurrectted

[dshwang, 2014-10-10 14:14:16]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-10-10 14:15:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/624633002/140001

[commit-bot: I haz the power, 2014-10-10 15:13:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-10 15:13:22]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[dshwang, 2014-10-10 16:50:20]

Give up removing |generator_| member because |last_frame_| will generate pixel
again.

https://codereview.chromium.org/624633002/diff/450001/media/filters/skcanvas_...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/624633002/diff/450001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:336:
generator_->set_frame(video_frame);
This line is needed because |generator_| generates pixel or texture every time
when |last_frame_| is used. As the name "discardable pixel ref", |last_frame_|
can cache or not the converted pixel or texture. If |last_frame_| discard the
pixel, |generator_| needs the video frame again.

[dshwang, 2014-10-10 16:50:31]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-10-10 16:51:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/624633002/450001

[dshwang, 2014-10-10 16:56:35]

FYI
onGetYUV8Planes() can be covered by following tests:
WebRtcGetUserMediaBrowserTest.TwoGetUserMediaAndVerifyFrameRate
WebRtcGetUserMediaBrowserTest.TwoGetUserMediaWithEqualConstraints

run in local machine
> out/Release/content_browsertests
--gtest_filter=WebRtcGetUserMediaBrowserTest.TwoGetUserMedia*


Following tests generates pixel multiple times. It means software-software copy
might be regressed after introducing generator.
conformance_extensions_oes_texture_float_with_video
conformance_extensions_oes_texture_half_float_with_video
conformance_textures_tex_image_and_sub_image_2d_with_video
conformance_textures_tex_image_and_sub_image_2d_with_video_rgb565
conformance_textures_tex_image_and_sub_image_2d_with_video_rgba4444
conformance_textures_tex_image_and_sub_image_2d_with_video_rgba5551
conformance_textures_texture_npot_video

run in local machine
> ./content/test/gpu/run_gpu_test.py webgl_conformance --browser=release
--page-filter=WebglConformance.conformance_textures_tex_image_and_sub_image_2d_with_video

[commit-bot: I haz the power, 2014-10-10 18:28:56]

Committed patchset #4 (id:450001)

[commit-bot: I haz the power, 2014-10-10 18:29:42]

Patchset 4 (id:??) landed as
https://crrev.com/c4d8a6fbf8ac266cc26ab7b5bf45c760456fec73
Cr-Commit-Position: refs/heads/master@{#299144}