CRD Webapp intermittently crashes on some machines

CRD Webapp intermittently crashes on some machines

This issue appeared during our M56 release and affected a subset of
machines.  Some users would hit this problem after a few seconds and
others could spend several hours using the app without problems.

I was able to track the problem down to a checkin from last year:
https://codereview.chromium.org/2096643003/

After a bit of additional debugging, I believe I know what the problem
is.  The actual problem is caused by a call to OnPictureReady() in the
PepperVideoRenderer3D class that occurs before we have a decoded frame
ready.  This leads to a cascade of issues where we try to splice an
element from the empty decoded_frames_ list which causes the size of the
list to underflow to 2^32 and inserts a null FrameTracker into the
next_picture_frames_ list.  This null FrameTracker is eventually
dereferenced which causes a crash.

Why does this only happen on certain machines?  I believe the problem is
in the call to GetNextPicture().  This method is called in two places,
once when we finish decoding a frame and again after we have retrieved a
decoded frame.  The machine I have been debugging the crash on is a dual
core celeron which is quite slow.  All of the test machines have at
least a core i5 in it.  My theory is that on the faster machines, the
decoder is fast enough to complete its work before the extra call to
GetNextPicture() results in the PictureReady callback being signalled.
On the slow machines. we set up our callback which ends up triggering
before the decoding completes.

My fix is to prevent setting up the OnPictureReady callback if we do not
have any decoded frames.  A simple fix for a difficult to debug problem.

BUG=689229
Review-Url: https://codereview.chromium.org/2692703002
Cr-Commit-Position: refs/heads/master@{#450132}
Committed: https://chromium.googlesource.com/chromium/src/+/838ec64ba5d7602699ec7c02182c5b17fa06075d

[joedow, 2017-02-11 20:11:43]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-11 20:11:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-11 21:01:36]

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

[commit-bot: I haz the power, 2017-02-11 21:01:36]

Dry run: This issue passed the CQ dry run.

[joedow, 2017-02-13 20:05:24]

joedow@chromium.org changed reviewers:
+ sergeyu@chromium.org, wez@chromium.org

[joedow, 2017-02-13 20:05:24]

joedow@chromium.org changed required reviewers:
+ sergeyu@chromium.org

[joedow, 2017-02-13 20:05:25]

PTAL!

[Sergey Ulanov, 2017-02-13 20:46:57]

LGTM
It appears that the problem is that the plugin doesn't handle the case when
GetPicture() completes before the corresponding Decode() call. From interface
documentation (see ppapi/api/ppb_video_decoder.idl) it appears to be legal
behavior for a decoder. The software video decoder actually sends decoded
pictures before notifying that decoding has finished (see
content/renderer/pepper/video_decoder_shim.cc).
Thanks for debugging this issue!

https://codereview.chromium.org/2692703002/diff/1/remoting/client/plugin/pepp...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/2692703002/diff/1/remoting/client/plugin/pepp...
remoting/client/plugin/pepper_video_renderer_3d.cc:336: if (get_picture_pending_
|| decoded_frames_.empty()) {
Add a comment to explain why we need to check decoded_frames_ (we don't want to
call GetPicture() before OnDecodeDone() as OnPictureReady() expects to be called
after OnDecodeDone() for the corresponding frame).

[joedow, 2017-02-13 22:33:47]

Thanks Sergey!

I'll file a bug for the video shim problem.

Joe

[joedow, 2017-02-13 22:34:29]

The CQ bit was checked by joedow@chromium.org

[joedow, 2017-02-13 22:34:30]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2692703002/#ps20001
(title: "Added comment")

[Sergey Ulanov, 2017-02-13 22:34:44]

On 2017/02/13 22:33:47, joedow wrote:
> Thanks Sergey!
> 
> I'll file a bug for the video shim problem.

I don't think we need a bug. I think shim behavior is correct.

> 
> Joe

[commit-bot: I haz the power, 2017-02-13 22:35:35]

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

[commit-bot: I haz the power, 2017-02-13 22:52:31]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487025269887340,
"parent_rev": "f4fa46dbc7d5dc7e9de701c27aa956a2da440e59", "commit_rev":
"838ec64ba5d7602699ec7c02182c5b17fa06075d"}

[commit-bot: I haz the power, 2017-02-13 22:53:08]

Description was changed from

==========
CRD Webapp intermittently crashes on some machines

This issue appeared during our M56 release and affected a subset of
machines.  Some users would hit this problem after a few seconds and
others could spend several hours using the app without problems.

I was able to track the problem down to a checkin from last year:
https://codereview.chromium.org/2096643003/

After a bit of additional debugging, I believe I know what the problem
is.  The actual problem is caused by a call to OnPictureReady() in the
PepperVideoRenderer3D class that occurs before we have a decoded frame
ready.  This leads to a cascade of issues where we try to splice an
element from the empty decoded_frames_ list which causes the size of the
list to underflow to 2^32 and inserts a null FrameTracker into the
next_picture_frames_ list.  This null FrameTracker is eventually
dereferenced which causes a crash.

Why does this only happen on certain machines?  I believe the problem is
in the call to GetNextPicture().  This method is called in two places,
once when we finish decoding a frame and again after we have retrieved a
decoded frame.  The machine I have been debugging the crash on is a dual
core celeron which is quite slow.  All of the test machines have at
least a core i5 in it.  My theory is that on the faster machines, the
decoder is fast enough to complete its work before the extra call to
GetNextPicture() results in the PictureReady callback being signalled.
On the slow machines. we set up our callback which ends up triggering
before the decoding completes.

My fix is to prevent setting up the OnPictureReady callback if we do not
have any decoded frames.  A simple fix for a difficult to debug problem.

BUG=689229
==========

to

==========
CRD Webapp intermittently crashes on some machines

This issue appeared during our M56 release and affected a subset of
machines.  Some users would hit this problem after a few seconds and
others could spend several hours using the app without problems.

I was able to track the problem down to a checkin from last year:
https://codereview.chromium.org/2096643003/

After a bit of additional debugging, I believe I know what the problem
is.  The actual problem is caused by a call to OnPictureReady() in the
PepperVideoRenderer3D class that occurs before we have a decoded frame
ready.  This leads to a cascade of issues where we try to splice an
element from the empty decoded_frames_ list which causes the size of the
list to underflow to 2^32 and inserts a null FrameTracker into the
next_picture_frames_ list.  This null FrameTracker is eventually
dereferenced which causes a crash.

Why does this only happen on certain machines?  I believe the problem is
in the call to GetNextPicture().  This method is called in two places,
once when we finish decoding a frame and again after we have retrieved a
decoded frame.  The machine I have been debugging the crash on is a dual
core celeron which is quite slow.  All of the test machines have at
least a core i5 in it.  My theory is that on the faster machines, the
decoder is fast enough to complete its work before the extra call to
GetNextPicture() results in the PictureReady callback being signalled.
On the slow machines. we set up our callback which ends up triggering
before the decoding completes.

My fix is to prevent setting up the OnPictureReady callback if we do not
have any decoded frames.  A simple fix for a difficult to debug problem.

BUG=689229

Review-Url: https://codereview.chromium.org/2692703002
Cr-Commit-Position: refs/heads/master@{#450132}
Committed:
https://chromium.googlesource.com/chromium/src/+/838ec64ba5d7602699ec7c02182c...
==========

[commit-bot: I haz the power, 2017-02-13 22:53:09]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/838ec64ba5d7602699ec7c02182c...

[Wez, 2017-02-13 22:59:41]

Can you trim the CL description down to describe just the bug and what was
changed to fix it, so it's not so verbose?

[Wez, 2017-02-13 23:03:36]

https://codereview.chromium.org/2692703002/diff/20001/remoting/client/plugin/...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/2692703002/diff/20001/remoting/client/plugin/...
remoting/client/plugin/pepper_video_renderer_3d.cc:341: if (get_picture_pending_
|| decoded_frames_.empty()) {
I'd suggest either adding the decoded_frames_ check in OnPictureReady(),
instead, since that's the case where it's required - the OnDecodeDone() should
never hit this - or renaming this function to GetNextPictureIfReady() and making
explicit that it's always safe to call and will only do work if there is work to
be done.

[joedow, 2017-02-13 23:45:35]

I will look into Wez's comments in a follow-up CL since this one is already in :
)