Check WindowTreeClient for empty inflight queue upon teardown

Check WindowTreeClient for empty inflight queue upon teardown

This is a follow up to https://codereview.chromium.org/2023933002/ &
https://codereview.chromium.org/2010083002/

Which verifies that the inflight queue is cleared upon teardown. After running views_mus_unittests 100 times no case was found. However if this occurs on bots there may be currently hidden bugs. This change will allow us to find them.

TEST=views_mus_unittests
BUG=577274

[jonross, 2016-06-01 20:13:24]

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

[commit-bot: I haz the power, 2016-06-01 20:14:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029743002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2029743002/1

[commit-bot: I haz the power, 2016-06-01 20:44:16]

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

[commit-bot: I haz the power, 2016-06-01 20:44:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jonross, 2016-06-13 15:57:47]

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

[commit-bot: I haz the power, 2016-06-13 15:58:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029743002/20001

[commit-bot: I haz the power, 2016-06-13 17:15:47]

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

[commit-bot: I haz the power, 2016-06-13 17:15:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jonross, 2016-06-14 14:37:19]

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

[commit-bot: I haz the power, 2016-06-14 14:37:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029743002/40001

[commit-bot: I haz the power, 2016-06-14 15:36:01]

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

[commit-bot: I haz the power, 2016-06-14 15:36:01]

Dry run: This issue passed the CQ dry run.

[jonross, 2016-06-14 15:42:48]

jonross@chromium.org changed reviewers:
+ sadrul@chromium.org

[jonross, 2016-06-14 15:42:49]

Hey Sadrul,

Mind taking a look at this change? I add a DCHECK to WindowTreeClient to make
sure that tests properly receive all messages before shutdown.

[sadrul, 2016-06-15 06:42:46]

I am having second thoughts is this is useful, since it looks like it adds
non-trivial amount of burden to individual tests. Can you give some examples of
the kind of issues you expect to find with this change?

https://codereview.chromium.org/2029743002/diff/40001/components/mus/public/c...
File components/mus/public/cpp/tests/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2029743002/diff/40001/components/mus/public/c...
components/mus/public/cpp/tests/window_tree_client_unittest.cc:1042:
setup.window_tree_client()->OnChangeCompleted(change_id1, true);
I think this adds burden to individual tests that we do not really need to
carry.

https://codereview.chromium.org/2029743002/diff/40001/components/mus/ws/windo...
File components/mus/ws/window_manager_client_unittest.cc (right):

https://codereview.chromium.org/2029743002/diff/40001/components/mus/ws/windo...
components/mus/ws/window_manager_client_unittest.cc:761:
ASSERT_TRUE(WaitForWindowToHaveFocus(window11));
Why? window11 already has focus (asserted above in line 755)

https://codereview.chromium.org/2029743002/diff/40001/components/mus/ws/windo...
components/mus/ws/window_manager_client_unittest.cc:819:
WindowTreeClientPrivate(embedded).WaitForInFlightQueue();
I don't think we should require individual tests to have to do this.

[jonross, 2016-06-15 13:59:10]

On 2016/06/15 06:42:46, sadrul wrote:
> I am having second thoughts is this is useful, since it looks like it adds
> non-trivial amount of burden to individual tests. Can you give some examples
of
> the kind of issues you expect to find with this change?
> 
>
https://codereview.chromium.org/2029743002/diff/40001/components/mus/public/c...
> File components/mus/public/cpp/tests/window_tree_client_unittest.cc (right):
> 
>
https://codereview.chromium.org/2029743002/diff/40001/components/mus/public/c...
> components/mus/public/cpp/tests/window_tree_client_unittest.cc:1042:
> setup.window_tree_client()->OnChangeCompleted(change_id1, true);
> I think this adds burden to individual tests that we do not really need to
> carry.
> 
>
https://codereview.chromium.org/2029743002/diff/40001/components/mus/ws/windo...
> File components/mus/ws/window_manager_client_unittest.cc (right):
> 
>
https://codereview.chromium.org/2029743002/diff/40001/components/mus/ws/windo...
> components/mus/ws/window_manager_client_unittest.cc:761:
> ASSERT_TRUE(WaitForWindowToHaveFocus(window11));
> Why? window11 already has focus (asserted above in line 755)
> 
>
https://codereview.chromium.org/2029743002/diff/40001/components/mus/ws/windo...
> components/mus/ws/window_manager_client_unittest.cc:819:
> WindowTreeClientPrivate(embedded).WaitForInFlightQueue();
> I don't think we should require individual tests to have to do this.

Some issues found:
  - Incorrect cleanup of capture during window deletion causing a crash
  - The fact that certain tests, which wanted to test server interactions, were
actually silently passing without any connection to the server.

I was also hoping to get a pattern in place as we expand testing. I recall we
had many interactive_ui_tests that could not be ran on linux due to the
separation of the X11 server. I didn't want to see a repeat.

[jonross, 2016-06-15 14:03:08]

https://codereview.chromium.org/2029743002/diff/40001/components/mus/public/c...
File components/mus/public/cpp/tests/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2029743002/diff/40001/components/mus/public/c...
components/mus/public/cpp/tests/window_tree_client_unittest.cc:1042:
setup.window_tree_client()->OnChangeCompleted(change_id1, true);
On 2016/06/15 06:42:46, sadrul wrote:
> I think this adds burden to individual tests that we do not really need to
> carry.

An alternate modification I considered was changing the focus/capture
InFlightChanges to include tracking the target window.

This would allow for the queued changes to be removed in
WindowTreeClient::OnWindowDestroyed

https://codereview.chromium.org/2029743002/diff/40001/components/mus/ws/windo...
File components/mus/ws/window_manager_client_unittest.cc (right):

https://codereview.chromium.org/2029743002/diff/40001/components/mus/ws/windo...
components/mus/ws/window_manager_client_unittest.cc:819:
WindowTreeClientPrivate(embedded).WaitForInFlightQueue();
On 2016/06/15 06:42:46, sadrul wrote:
> I don't think we should require individual tests to have to do this.

Yeah I dislike having certain tests with this. I could look at having the actual
test class do this during teardown. The burden then would be ensuring that the
test class has the WindowTreeClient* to wait for.

[jonross, 2016-06-16 20:06:57]

jonross@chromium.org changed reviewers:
+ sky@chromium.org

[jonross, 2016-06-16 20:06:58]

+sky@ for thoughts.

In this change I'm adding a DCHECK to WindowTreeClient to verify that the
InFlightMap is empty upon shutdown. I wanted to verify this for tests, as I'd
seen flaky tests, where the error would have caught earlier had the
client/server waited for all messages to be received.

As the review is right now this requires some extra work in the tests, which
sadrul@ dislikes. I agree with his concerns.

One concern can be addressed by moving the syncing of the queue to a test
TearDown method. Similar to how some tests perform RunAllPendingMessages.

The other concern involves the cleanup of capture/focus InFlightChanges. Since
these aren't attributed to Windows they aren't cleared out of the queue upon
window destruction. My thought is that we could update these InFlightChanges,
which would remove the need to edit some tests.

Thoughts?

[sky, 2016-06-16 22:09:14]

On Thu, Jun 16, 2016 at 1:06 PM,  <jonross@chromium.org> wrote:
> +sky@ for thoughts.
>
> In this change I'm adding a DCHECK to WindowTreeClient to verify that the
> InFlightMap is empty upon shutdown. I wanted to verify this for tests, as
> I'd
> seen flaky tests, where the error would have caught earlier had the
> client/server waited for all messages to be received.
>
> As the review is right now this requires some extra work in the tests, which
> sadrul@ dislikes. I agree with his concerns.
>
> One concern can be addressed by moving the syncing of the queue to a test
> TearDown method. Similar to how some tests perform RunAllPendingMessages.
>
> The other concern involves the cleanup of capture/focus InFlightChanges.
> Since
> these aren't attributed to Windows they aren't cleared out of the queue upon
> window destruction. My thought is that we could update these
> InFlightChanges,
> which would remove the need to edit some tests.
>
> Thoughts?

If you add the DCHECK it implies clients have to know to run all
messages, and wait for messages to be received. This can lead to
awkward shutdown behavior where a client is trying to shutdown, but
can't because they are waiting and worse yet must run a nested message
loop. Nested message loops lead to all sorts of problems and in
general are very hard to get right. We should allow clients to
shutdown at any point they want and ensure we do the right thing.

  -Scott

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

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jonross, 2016-06-17 13:35:27]

On 2016/06/16 22:09:14, sky wrote:
> On Thu, Jun 16, 2016 at 1:06 PM,  <mailto:jonross@chromium.org> wrote:
> > +sky@ for thoughts.
> >
> > In this change I'm adding a DCHECK to WindowTreeClient to verify that the
> > InFlightMap is empty upon shutdown. I wanted to verify this for tests, as
> > I'd
> > seen flaky tests, where the error would have caught earlier had the
> > client/server waited for all messages to be received.
> >
> > As the review is right now this requires some extra work in the tests, which
> > sadrul@ dislikes. I agree with his concerns.
> >
> > One concern can be addressed by moving the syncing of the queue to a test
> > TearDown method. Similar to how some tests perform RunAllPendingMessages.
> >
> > The other concern involves the cleanup of capture/focus InFlightChanges.
> > Since
> > these aren't attributed to Windows they aren't cleared out of the queue upon
> > window destruction. My thought is that we could update these
> > InFlightChanges,
> > which would remove the need to edit some tests.
> >
> > Thoughts?
> 
> If you add the DCHECK it implies clients have to know to run all
> messages, and wait for messages to be received. This can lead to
> awkward shutdown behavior where a client is trying to shutdown, but
> can't because they are waiting and worse yet must run a nested message
> loop. Nested message loops lead to all sorts of problems and in
> general are very hard to get right. We should allow clients to
> shutdown at any point they want and ensure we do the right thing.
> 
>   -Scott
> 
> >
> >
> > https://codereview.chromium.org/2029743002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

Ok, that SGTM. Closing this review. Thanks