aura: Ensure OwnedMailbox not used with invalid or destroyed GLHelper.

aura: Ensure OwnedMailbox not used with invalid or destroyed GLHelper.

There are 2 scenarios when the GLHelper becomes invalid:

1. On lost context. In this case, the OwnedMailbox is reset and the
pointer is destroyed. However, the copy request result code would
continue trying to use the OwnedMailbox regardless. Instead we can
use this as a hint that the mailbox is no longer valid and abort the
readback.

2. On shutdown. Once the RenderWidgetHostViewAura is shutdown, there
is no way to easily tell if the GLHelper attached to the OwnedMailbox
being stored in copy request callbacks is still alive. Since a readback
on the RenderWidgetHostViewAura can not complete once the RWHVA class
is destroyed anyways, we can destroy the contents of the OwnedMailboxes
in flight at that time. This way when the readback is later aborted,
the OwnedMailbox does not try to clean itself up with a potentially
destroyed GLHelper.

R=piman@chromium.org
BUG=270918
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243971

[danakj, 2014-01-07 19:53:44]

My other thought here was to add a OnShutdown() method to the
ImageTransportFactoryObserver class, and then have the OwnedMailbox destroy it's
resources at that time. But that seemed really late in the shutdown sequence,
and tracking the active OwnedMailboxes on RWHVA also does the trick.

[danakj, 2014-01-08 00:46:12]

Hm, there's a failure I need to look at it seems, will come back to it tomorrow.

[danakj, 2014-01-08 18:21:26]

PTAL

[piman, 2014-01-08 21:53:04]

LGTM. Is there a way to unit test the RWHVA changes?

[danakj, 2014-01-08 22:03:56]

On Wed, Jan 8, 2014 at 4:53 PM, <piman@chromium.org> wrote:

> LGTM. Is there a way to unit test the RWHVA changes?
>

There's various browser tests that are executing the code and hitting
DCHECKs when things go wrong. For instance, ordering can be weird with the
callback if the RWHVA is destroyed, while another callback is in the middle
of the async yuv readback, which showed up here:
http://build.chromium.org/p/tryserver.chromium/builders/linux_aura/builds/110...
.
So I just changed the active textures to be a <set> instead of a <deque>.
 This was all triggered by shutdown crashes in other tab capture browser
tests once they use the compositor as well.

I can add some indirection to unit test this more directly though, I think.
Let me see...


>
> https://codereview.chromium.org/120913004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-01-09 00:07:07]

Please have a look at the test

[piman, 2014-01-09 00:23:28]

LGTM, thanks!

https://codereview.chromium.org/120913004/diff/420001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/120913004/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1332: //
LOG(ERROR)<<"Running";
nit: remove.

[danakj, 2014-01-09 00:34:55]

https://codereview.chromium.org/120913004/diff/420001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/120913004/diff/420001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1332: //
LOG(ERROR)<<"Running";
On 2014/01/09 00:23:29, piman wrote:
> nit: remove.

oops, thanks. doesn't need the while either.

[commit-bot: I haz the power, 2014-01-09 00:42:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/120913004/600001

[commit-bot: I haz the power, 2014-01-09 02:10:19]

Retried try job too often on linux_aura for step(s) app_list_unittests,
aura_unittests, browser_tests, compositor_unittests, content_browsertests,
content_unittests, events_unittests, interactive_ui_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2014-01-09 18:40:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/120913004/910001

[commit-bot: I haz the power, 2014-01-09 21:05:13]

Change committed as 243971