Fix deadlock in VAAPI video decoder error cases.

Fix deadlock in VAAPI video decoder error cases.

RETURN_AND_NOTIFY_ON_FAILURE in VaapiVideoDecodeAccelerator calls
Cleanup() directly in the case that it's running in the main thread.
Unfortunately, this deadlocks when Cleanup() tries to recursively
acquire the member lock_.

BUG=None
TEST=local build, run on lumpy

Change-Id: I8055d5db12c6758f356be9616c27f083a5d3a7bf


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158069

[sheu, 2012-09-18 01:26:19]

Ran across this deadlock while debugging VDA-related stuff.

[Pawel Osciak, 2012-09-18 02:19:40]

On 2012/09/18 01:26:19, sheu wrote:
> Ran across this deadlock while debugging VDA-related stuff.

I don't understand, what's the difference between explicitly posting and posting
if we are on the wrong loop?

[Ami GONE FROM CHROMIUM, 2012-09-18 02:21:28]

https://chromiumcodereview.appspot.com/10943003/diff/1/content/common/gpu/med...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/10943003/diff/1/content/common/gpu/med...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:25:
message_loop_->PostTask(FROM_HERE, base::Bind(                \
This will delay clearing client_ potentially for longer than we want.

IWBN to instead post the Cleanup() call in NotifyError, instead of posting this
here.

[Ami GONE FROM CHROMIUM, 2012-09-18 02:21:58]

On Mon, Sep 17, 2012 at 7:19 PM, <posciak@chromium.org> wrote:

> I don't understand, what's the difference between explicitly posting and
> posting
> if we are on the wrong loop?
>

Explicitly posting always posts, even if you're on the right loop.

[Pawel Osciak, 2012-09-18 02:24:37]

On 2012/09/18 02:21:58, Ami Fischman wrote:
> On Mon, Sep 17, 2012 at 7:19 PM, <mailto:posciak@chromium.org> wrote:
> 
> > I don't understand, what's the difference between explicitly posting and
> > posting
> > if we are on the wrong loop?
> >
> 
> Explicitly posting always posts, even if you're on the right loop.

Of course. I still don't understand what kind of deadlock scenario this fixes...
John, could you give an example please?

[Ami GONE FROM CHROMIUM, 2012-09-18 02:30:38]

> Of course. I still don't understand what kind of deadlock scenario this
> fixes...
>

Any method running on message_loop_ that calls NOTIFY_AND_RETURN_ON_FAILURE
while holding lock_.  Decode() & AssignPictureBuffers().

[sheu, 2012-09-19 08:21:35]

https://chromiumcodereview.appspot.com/10943003/diff/1/content/common/gpu/med...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/10943003/diff/1/content/common/gpu/med...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:25:
message_loop_->PostTask(FROM_HERE, base::Bind(                \
That would possibly be racy as well.  I imagine the client might call Destroy()
on this upon receiving the error notification, and a posted Cleanup() task
afterwards would Have A Bad Time.

In any case, this is an error case, and we might be ok with not clearing client_
for a while?

[sheu, 2012-09-19 08:58:57]

Second patchset up.

https://chromiumcodereview.appspot.com/10943003/diff/1/content/common/gpu/med...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/10943003/diff/1/content/common/gpu/med...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:25:
message_loop_->PostTask(FROM_HERE, base::Bind(                \
Oh!  I see what you're doing there.

Posting the cleanup with weak_this_ should do the trick.  Also, as to clearing
client_: as I understand it, the point of clearing client_ptr_factory_ is to
invalidate all the previous callbacks that haven't completed yet, since they've
all been posted with client_.

[Ami GONE FROM CHROMIUM, 2012-09-19 16:52:30]

lgtm

[Pawel Osciak, 2012-09-19 17:01:00]

lgtm

[commit-bot: I haz the power, 2012-09-19 19:45:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/10943003/6001

[commit-bot: I haz the power, 2012-09-20 04:43:19]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win_rel, revision is HEAD

[sheu, 2012-09-21 19:17:29]

Tests have been flaky lately.  Not like anything VAAPI-related should be causing
Windows test failures...

[commit-bot: I haz the power, 2012-09-21 19:17:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/10943003/6001

[commit-bot: I haz the power, 2012-09-21 20:45:55]

Change committed as 158069