ThreadedTest that a non-null OutputSurface that fails initialization immediately doesn't crash

ThreadedTest that a non-null OutputSurface that fails initialization immediately doesn't crash

Unit test to catch things like bug 168691

BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175632

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176184

[danakj, 2013-01-08 00:49:08]

LGTM and thanks for the test.

https://codereview.chromium.org/11782026/diff/1/cc/layer_tree_host_unittest_c...
File cc/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/11782026/diff/1/cc/layer_tree_host_unittest_c...
cc/layer_tree_host_unittest_context.cc:810: new
FakeWebGraphicsContext3DMakeCurrentFails);
I think you could just create a FakeWebGraphicsContext3d and call
loseContextCHROMIUM() on it immediately to get the same effect without needing
to make your own subclass.

[jamesr, 2013-01-08 00:50:49]

This crashes pre-http://src.chromium.org/viewvc/chrome?view=rev&revision=175416
as expected.

After that rev, it doesn't crash but it also never completes because
commitPendingForTesting() is always true - we can't commit without a valid
context.

[danakj, 2013-01-08 00:52:32]

On 2013/01/08 00:50:49, jamesr wrote:
> This crashes
pre-http://src.chromium.org/viewvc/chrome?view=rev&revision=175416
> as expected.
> 
> After that rev, it doesn't crash but it also never completes because
> commitPendingForTesting() is always true - we can't commit without a valid
> context.

When you endTest() have it also set a flag to allow contexts to be made for
which makeCurrent() succeeds?

[jamesr1, 2013-01-08 00:55:06]

On Mon, Jan 7, 2013 at 4:52 PM, <danakj@chromium.org> wrote:

> On 2013/01/08 00:50:49, jamesr wrote:
>
>> This crashes
>>
>
pre-http://src.chromium.org/**viewvc/chrome?view=rev&**revision=175416<http://src.chromium.org/viewvc/chrome?view=rev&revision=175416>
>
>> as expected.
>>
>
>  After that rev, it doesn't crash but it also never completes because
>> commitPendingForTesting() is always true - we can't commit without a valid
>> context.
>>
>
> When you endTest() have it also set a flag to allow contexts to be made for
> which makeCurrent() succeeds?
>

That's not testing the same thing, then.  We should successfully get out of
here even if we never get a valid context.  I could try triggering the
normal shutdown logic, since that's normally what happens in
didRecreateOutputSurface(false) callbacks


>
>
https://codereview.chromium.**org/11782026/<https://codereview.chromium.org/1...
>

[danakj, 2013-01-08 01:02:59]

On 2013/01/08 00:55:06, jamesr1 wrote:
> That's not testing the same thing, then.  We should successfully get out of
> here even if we never get a valid context.  I could try triggering the
> normal shutdown logic, since that's normally what happens in
> didRecreateOutputSurface(false) callbacks

Why not? When you endTest() you're saying you've gotten to the point where you
are happy with the results. What happens from there to clean up shouldn't matter
as it's not part of the test, no?

[jamesr, 2013-01-08 01:03:42]

How about this? The fact that this test is looking for pending commits is pretty
bogus since this test isn't about the commit flow, really, it's about
initialization so it's a bit irritating to have to muck with that.

[danakj, 2013-01-08 01:06:50]

https://codereview.chromium.org/11782026/diff/10003/cc/layer_tree_host_unitte...
File cc/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/11782026/diff/10003/cc/layer_tree_host_unitte...
cc/layer_tree_host_unittest_context.cc:783: class
LayerTreeHostContextTestFailsImmediately : public ThreadedTest {
Oh, subclass from LayerTreHostContextTest here. And set
times_to_create_and_lose_ = 10000 in beginTest() and you can skip your whole
createOutputSurface method. (Also don't need the constructor).

https://codereview.chromium.org/11782026/diff/10003/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://codereview.chromium.org/11782026/diff/10003/cc/thread_proxy.cc#newcod...
cc/thread_proxy.cc:1015: if (m_layerTreeHostImpl->outputSurface())
Ya, I was thinking about something like this also. I'm fine with this, as it
shouldn't introduce races. You know that no commit is possibly sitting in a
message loop queue, so that sounds reasonable to me.

[jamesr, 2013-01-08 01:09:54]

On 2013/01/08 01:06:50, danakj wrote:
>
https://codereview.chromium.org/11782026/diff/10003/cc/layer_tree_host_unitte...
> File cc/layer_tree_host_unittest_context.cc (right):
> 
>
https://codereview.chromium.org/11782026/diff/10003/cc/layer_tree_host_unitte...
> cc/layer_tree_host_unittest_context.cc:783: class
> LayerTreeHostContextTestFailsImmediately : public ThreadedTest {
> Oh, subclass from LayerTreHostContextTest here. And set
> times_to_create_and_lose_ = 10000 in beginTest() and you can skip your whole
> createOutputSurface method. (Also don't need the constructor).

Due to this code in LayerTreeHostContextTest::createOutputSurface():
    if (times_to_create_and_lose_) {
      --times_to_create_and_lose_;
      // Make the context get lost during reinitialization.
      // The number of times MakeCurrent succeeds is not important, and
      // can be changed if needed to make this pass with future changes.
      context3d_->set_times_make_current_succeeds(2);
    }

that definitely will not test what I want, since what's important is that the
very first call to makeContextCurrent() fails.  If I change this it'll change
the meaning of other tests.

[danakj, 2013-01-08 01:11:48]

On 2013/01/08 01:09:54, jamesr wrote:
> On 2013/01/08 01:06:50, danakj wrote:
> >
>
https://codereview.chromium.org/11782026/diff/10003/cc/layer_tree_host_unitte...
> > File cc/layer_tree_host_unittest_context.cc (right):
> > 
> >
>
https://codereview.chromium.org/11782026/diff/10003/cc/layer_tree_host_unitte...
> > cc/layer_tree_host_unittest_context.cc:783: class
> > LayerTreeHostContextTestFailsImmediately : public ThreadedTest {
> > Oh, subclass from LayerTreHostContextTest here. And set
> > times_to_create_and_lose_ = 10000 in beginTest() and you can skip your whole
> > createOutputSurface method. (Also don't need the constructor).
> 
> Due to this code in LayerTreeHostContextTest::createOutputSurface():
>     if (times_to_create_and_lose_) {
>       --times_to_create_and_lose_;
>       // Make the context get lost during reinitialization.
>       // The number of times MakeCurrent succeeds is not important, and
>       // can be changed if needed to make this pass with future changes.
>       context3d_->set_times_make_current_succeeds(2);
>     }
> 
> that definitely will not test what I want, since what's important is that the
> very first call to makeContextCurrent() fails.  If I change this it'll change
> the meaning of other tests.

Ooh.. sorry. I forgot that it did that. Well, you could override CreateContext3D
and call lose on the thing you create there, then. That'd be less code still.
Details, anyhow.

[commit-bot: I haz the power, 2013-01-08 01:30:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11782026/21001

[commit-bot: I haz the power, 2013-01-08 20:58:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11782026/21001

[commit-bot: I haz the power, 2013-01-09 00:28:12]

Change committed as 175632

[danakj, 2013-01-09 01:39:59]

https://chromiumcodereview.appspot.com/11782026/diff/21001/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://chromiumcodereview.appspot.com/11782026/diff/21001/cc/thread_proxy.cc...
cc/thread_proxy.cc:1016: request->commitPending =
m_schedulerOnImplThread->commitPending();
else commitPending = false;

I suspect you're leaving tests hanging because this variable is left
uninitialized.

[jamesr, 2013-01-09 19:35:41]

https://chromiumcodereview.appspot.com/11782026/diff/21001/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://chromiumcodereview.appspot.com/11782026/diff/21001/cc/thread_proxy.cc...
cc/thread_proxy.cc:1016: request->commitPending =
m_schedulerOnImplThread->commitPending();
On 2013/01/09 01:39:59, danakj wrote:
> else commitPending = false;
> 
> I suspect you're leaving tests hanging because this variable is left
> uninitialized.

D'oh!  PTAL

[danakj, 2013-01-09 20:56:13]

Ya, LGTM. I think this will address the seemingly random hangs, but I'm not
certain if it'll solve why this test itself hung on a mac bot. I'm a little
confused by that. Give it a shot and see?

[commit-bot: I haz the power, 2013-01-10 18:47:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11782026/30001

[commit-bot: I haz the power, 2013-01-10 22:42:02]

Change committed as 176184