Revert of tabing inconsistencies between trays in status area

Revert of tabing inconsistencies between trays in status area (patchset #5 id:80001 of https://codereview.chromium.org/2087703002/ )

Reason for revert:
Causes UaF in web_notification_tray.cc on focus_cycler_unittest.cc. The issue appears to be unclear lifetime assumptions of the system tray object: the status area widget maintains a raw pointer to it (and confusingly *sometimes* owns it, i.e. when its Shutdown() method is called), whereas focus_cycler_unittest.cc actually owns it but doesn't clear it out from the status area widget when it deletes it on TearDown(). It looks like this CL just tickled a pre-existing bad situation, but the best fix is not obvious to me as a non-expert in this part of the codebase, so I have to revert.

Example failure: https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/13929/steps/ash_unittests%20on%20Ubuntu-12.04/logs/FocusCyclerTest.CycleFocusBackward

Relevant snippet:

==7090==ERROR: AddressSanitizer: heap-use-after-free on address 0x618000021608 at pc 0x00000a32db44 bp 0x7ffdd1268ae0 sp 0x7ffdd1268ad8
WRITE of size 8 at 0x618000021608 thread T0
    #0 0xa32db43 in views::View::SetNextFocusableView(views::View*) ui/views/view.cc:1198:24
    #1 0x964d8ba in Run base/callback.h:397:12
    #2 0x964d8ba in base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask const&) base/debug/task_annotator.cc:51
    #3 0x95131cc in base::MessageLoop::RunTask(base::PendingTask const&) base/message_loop/message_loop.cc:493:19
    #4 0x951401b in base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) base/message_loop/message_loop.cc:502:5
    #5 0x9514eab in base::MessageLoop::DoWork() base/message_loop/message_loop.cc:624:13
    #6 0x95191f7 in base::MessagePumpGlib::Run(base::MessagePump::Delegate*) base/message_loop/message_pump_glib.cc:313:49
    #7 0x956bbfb in base::RunLoop::Run() base/run_loop.cc:35:10
    #8 0x127dc12 in ash::test::AshTestHelper::RunAllPendingInMessageLoop() ash/test/ash_test_helper.cc:192:12
    #9 0x127b887 in RunAllPendingInMessageLoop ash/test/ash_test_base.cc:287:21
    #10 0x127b887 in ash::test::AshTestBase::TearDown() ash/test/ash_test_base.cc:163

Original issue's description:
> Detailed reproduce steps are in comment 2 in crbug.com/468969
>
> There are several bugs in the status area tray focus advancing.
>
> (1) there is a invisible focus between system tray and web notification tray. This is because there is a focus traversal in web notification tray's child. Solution is to disable it.
> When virtual keyboard is enabled, we should also disable its child view traversal.
>
> (2) TAB will be trapped in system tray. Web notification tray is async painted. The initialization sibling relation may not be ready. Add a setnextfocusibleview call will fix that.
>
> BUG=468969
> TEST=tested on device, both tabing and shift-tabing works well on status area.
>
> There is still a bug in crbug.com/468969 comment 7. It happens in login screen only. When logged in, it works fine.
>
> Committed: https://crrev.com/133c792af9dfe8c82561bee7015efcc0b5dba22d
> Committed: https://crrev.com/ab543ef12f3b412b9e008fce7e78f4456de82822
> Cr-Original-Commit-Position: refs/heads/master@{#401765}
> Cr-Commit-Position: refs/heads/master@{#401810}

TBR=jennyz@chromium.org,falken@chromium.org,jamescook@chromium.org,warx@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=468969
Committed: https://crrev.com/83e93cf04db96326a89176429d604bf131cf9229
Cr-Commit-Position: refs/heads/master@{#401848}

[blundell, 2016-06-24 10:33:54]

The CQ bit was checked by blundell@chromium.org

[blundell, 2016-06-24 10:33:55]

Created Revert of tabing inconsistencies between trays in status area

[commit-bot: I haz the power, 2016-06-24 10:34:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2095933002/1

[commit-bot: I haz the power, 2016-06-24 10:34:48]

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

[commit-bot: I haz the power, 2016-06-24 10:34:49]

Failed to apply patch for ash/system/web_notification/web_notification_tray.cc:
While running git apply --index -3 -p1;
  error: patch failed: ash/system/web_notification/web_notification_tray.cc:505
  Falling back to three-way merge...
  Applied patch to 'ash/system/web_notification/web_notification_tray.cc' with
conflicts.
  U ash/system/web_notification/web_notification_tray.cc

Patch:       ash/system/web_notification/web_notification_tray.cc
Index: ash/system/web_notification/web_notification_tray.cc
diff --git a/ash/system/web_notification/web_notification_tray.cc
b/ash/system/web_notification/web_notification_tray.cc
index
ff40d760bc8929fcae4cf33fe1eef9806f8eab45..208187b9972a67c591f93046e8c98e7aa87d3065
100644
--- a/ash/system/web_notification/web_notification_tray.cc
+++ b/ash/system/web_notification/web_notification_tray.cc
@@ -214,7 +214,6 @@
   button_->set_triggerable_event_flags(
       ui::EF_LEFT_MOUSE_BUTTON | ui::EF_RIGHT_MOUSE_BUTTON);
   tray_container()->AddChildView(button_);
-  button_->SetFocusBehavior(FocusBehavior::NEVER);
   SetContentsBackground();
   tray_container()->SetBorder(views::Border::NullBorder());
   message_center_tray_.reset(new message_center::MessageCenterTray(
@@ -505,8 +504,6 @@
   SetVisible(IsLoggedIn());
   Layout();
   SchedulePaint();
-  if (IsLoggedIn())
-    status_area_widget()->system_tray()->SetNextFocusableView(this);
 }
 
 void WebNotificationTray::ClickedOutsideBubble() {

[blundell, 2016-06-24 11:01:06]

Adjusted the revert manually to take into account
https://codereview.chromium.org/2099603002.

[blundell, 2016-06-24 11:01:10]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2016-06-24 11:01:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2095933002/70001

[commit-bot: I haz the power, 2016-06-24 11:02:01]

Description was changed from

==========
Revert of tabing inconsistencies between trays in status area (patchset #5
id:80001 of https://codereview.chromium.org/2087703002/ )

Reason for revert:
Causes UaF in web_notification_tray.cc on focus_cycler_unittest.cc. The issue
appears to be unclear lifetime assumptions of the system tray object: the status
area widget maintains a raw pointer to it (and confusingly *sometimes* owns it,
i.e. when its Shutdown() method is called), whereas focus_cycler_unittest.cc
actually owns it but doesn't clear it out from the status area widget when it
deletes it on TearDown(). It looks like this CL just tickled a pre-existing bad
situation, but the best fix is not obvious to me as a non-expert in this part of
the codebase, so I have to revert.

Example failure:
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2...

Relevant snippet:

==7090==ERROR: AddressSanitizer: heap-use-after-free on address 0x618000021608
at pc 0x00000a32db44 bp 0x7ffdd1268ae0 sp 0x7ffdd1268ad8
WRITE of size 8 at 0x618000021608 thread T0
    #0 0xa32db43 in views::View::SetNextFocusableView(views::View*)
ui/views/view.cc:1198:24
    #1 0x964d8ba in Run base/callback.h:397:12
    #2 0x964d8ba in base::debug::TaskAnnotator::RunTask(char const*,
base::PendingTask const&) base/debug/task_annotator.cc:51
    #3 0x95131cc in base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:493:19
    #4 0x951401b in base::MessageLoop::DeferOrRunPendingTask(base::PendingTask
const&) base/message_loop/message_loop.cc:502:5
    #5 0x9514eab in base::MessageLoop::DoWork()
base/message_loop/message_loop.cc:624:13
    #6 0x95191f7 in base::MessagePumpGlib::Run(base::MessagePump::Delegate*)
base/message_loop/message_pump_glib.cc:313:49
    #7 0x956bbfb in base::RunLoop::Run() base/run_loop.cc:35:10
    #8 0x127dc12 in ash::test::AshTestHelper::RunAllPendingInMessageLoop()
ash/test/ash_test_helper.cc:192:12
    #9 0x127b887 in RunAllPendingInMessageLoop ash/test/ash_test_base.cc:287:21
    #10 0x127b887 in ash::test::AshTestBase::TearDown()
ash/test/ash_test_base.cc:163

Original issue's description:
> Detailed reproduce steps are in comment 2 in crbug.com/468969
> 
> There are several bugs in the status area tray focus advancing.
> 
> (1) there is a invisible focus between system tray and web notification tray.
This is because there is a focus traversal in web notification tray's child.
Solution is to disable it.
> When virtual keyboard is enabled, we should also disable its child view
traversal.
> 
> (2) TAB will be trapped in system tray. Web notification tray is async
painted. The initialization sibling relation may not be ready. Add a
setnextfocusibleview call will fix that.
> 
> BUG=468969
> TEST=tested on device, both tabing and shift-tabing works well on status area.
> 
> There is still a bug in crbug.com/468969 comment 7. It happens in login screen
only. When logged in, it works fine.
> 
> Committed: https://crrev.com/133c792af9dfe8c82561bee7015efcc0b5dba22d
> Committed: https://crrev.com/ab543ef12f3b412b9e008fce7e78f4456de82822
> Cr-Original-Commit-Position: refs/heads/master@{#401765}
> Cr-Commit-Position: refs/heads/master@{#401810}

TBR=jennyz@chromium.org,falken@chromium.org,jamescook@chromium.org,warx@chrom...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=468969
==========

to

==========
Revert of tabing inconsistencies between trays in status area (patchset #5
id:80001 of https://codereview.chromium.org/2087703002/ )

Reason for revert:
Causes UaF in web_notification_tray.cc on focus_cycler_unittest.cc. The issue
appears to be unclear lifetime assumptions of the system tray object: the status
area widget maintains a raw pointer to it (and confusingly *sometimes* owns it,
i.e. when its Shutdown() method is called), whereas focus_cycler_unittest.cc
actually owns it but doesn't clear it out from the status area widget when it
deletes it on TearDown(). It looks like this CL just tickled a pre-existing bad
situation, but the best fix is not obvious to me as a non-expert in this part of
the codebase, so I have to revert.

Example failure:
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2...

Relevant snippet:

==7090==ERROR: AddressSanitizer: heap-use-after-free on address 0x618000021608
at pc 0x00000a32db44 bp 0x7ffdd1268ae0 sp 0x7ffdd1268ad8
WRITE of size 8 at 0x618000021608 thread T0
    #0 0xa32db43 in views::View::SetNextFocusableView(views::View*)
ui/views/view.cc:1198:24
    #1 0x964d8ba in Run base/callback.h:397:12
    #2 0x964d8ba in base::debug::TaskAnnotator::RunTask(char const*,
base::PendingTask const&) base/debug/task_annotator.cc:51
    #3 0x95131cc in base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:493:19
    #4 0x951401b in base::MessageLoop::DeferOrRunPendingTask(base::PendingTask
const&) base/message_loop/message_loop.cc:502:5
    #5 0x9514eab in base::MessageLoop::DoWork()
base/message_loop/message_loop.cc:624:13
    #6 0x95191f7 in base::MessagePumpGlib::Run(base::MessagePump::Delegate*)
base/message_loop/message_pump_glib.cc:313:49
    #7 0x956bbfb in base::RunLoop::Run() base/run_loop.cc:35:10
    #8 0x127dc12 in ash::test::AshTestHelper::RunAllPendingInMessageLoop()
ash/test/ash_test_helper.cc:192:12
    #9 0x127b887 in RunAllPendingInMessageLoop ash/test/ash_test_base.cc:287:21
    #10 0x127b887 in ash::test::AshTestBase::TearDown()
ash/test/ash_test_base.cc:163

Original issue's description:
> Detailed reproduce steps are in comment 2 in crbug.com/468969
> 
> There are several bugs in the status area tray focus advancing.
> 
> (1) there is a invisible focus between system tray and web notification tray.
This is because there is a focus traversal in web notification tray's child.
Solution is to disable it.
> When virtual keyboard is enabled, we should also disable its child view
traversal.
> 
> (2) TAB will be trapped in system tray. Web notification tray is async
painted. The initialization sibling relation may not be ready. Add a
setnextfocusibleview call will fix that.
> 
> BUG=468969
> TEST=tested on device, both tabing and shift-tabing works well on status area.
> 
> There is still a bug in crbug.com/468969 comment 7. It happens in login screen
only. When logged in, it works fine.
> 
> Committed: https://crrev.com/133c792af9dfe8c82561bee7015efcc0b5dba22d
> Committed: https://crrev.com/ab543ef12f3b412b9e008fce7e78f4456de82822
> Cr-Original-Commit-Position: refs/heads/master@{#401765}
> Cr-Commit-Position: refs/heads/master@{#401810}

TBR=jennyz@chromium.org,falken@chromium.org,jamescook@chromium.org,warx@chrom...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=468969
==========

[commit-bot: I haz the power, 2016-06-24 11:02:02]

Committed patchset #2 (id:70001)

[commit-bot: I haz the power, 2016-06-24 11:03:28]

Description was changed from

==========
Revert of tabing inconsistencies between trays in status area (patchset #5
id:80001 of https://codereview.chromium.org/2087703002/ )

Reason for revert:
Causes UaF in web_notification_tray.cc on focus_cycler_unittest.cc. The issue
appears to be unclear lifetime assumptions of the system tray object: the status
area widget maintains a raw pointer to it (and confusingly *sometimes* owns it,
i.e. when its Shutdown() method is called), whereas focus_cycler_unittest.cc
actually owns it but doesn't clear it out from the status area widget when it
deletes it on TearDown(). It looks like this CL just tickled a pre-existing bad
situation, but the best fix is not obvious to me as a non-expert in this part of
the codebase, so I have to revert.

Example failure:
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2...

Relevant snippet:

==7090==ERROR: AddressSanitizer: heap-use-after-free on address 0x618000021608
at pc 0x00000a32db44 bp 0x7ffdd1268ae0 sp 0x7ffdd1268ad8
WRITE of size 8 at 0x618000021608 thread T0
    #0 0xa32db43 in views::View::SetNextFocusableView(views::View*)
ui/views/view.cc:1198:24
    #1 0x964d8ba in Run base/callback.h:397:12
    #2 0x964d8ba in base::debug::TaskAnnotator::RunTask(char const*,
base::PendingTask const&) base/debug/task_annotator.cc:51
    #3 0x95131cc in base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:493:19
    #4 0x951401b in base::MessageLoop::DeferOrRunPendingTask(base::PendingTask
const&) base/message_loop/message_loop.cc:502:5
    #5 0x9514eab in base::MessageLoop::DoWork()
base/message_loop/message_loop.cc:624:13
    #6 0x95191f7 in base::MessagePumpGlib::Run(base::MessagePump::Delegate*)
base/message_loop/message_pump_glib.cc:313:49
    #7 0x956bbfb in base::RunLoop::Run() base/run_loop.cc:35:10
    #8 0x127dc12 in ash::test::AshTestHelper::RunAllPendingInMessageLoop()
ash/test/ash_test_helper.cc:192:12
    #9 0x127b887 in RunAllPendingInMessageLoop ash/test/ash_test_base.cc:287:21
    #10 0x127b887 in ash::test::AshTestBase::TearDown()
ash/test/ash_test_base.cc:163

Original issue's description:
> Detailed reproduce steps are in comment 2 in crbug.com/468969
> 
> There are several bugs in the status area tray focus advancing.
> 
> (1) there is a invisible focus between system tray and web notification tray.
This is because there is a focus traversal in web notification tray's child.
Solution is to disable it.
> When virtual keyboard is enabled, we should also disable its child view
traversal.
> 
> (2) TAB will be trapped in system tray. Web notification tray is async
painted. The initialization sibling relation may not be ready. Add a
setnextfocusibleview call will fix that.
> 
> BUG=468969
> TEST=tested on device, both tabing and shift-tabing works well on status area.
> 
> There is still a bug in crbug.com/468969 comment 7. It happens in login screen
only. When logged in, it works fine.
> 
> Committed: https://crrev.com/133c792af9dfe8c82561bee7015efcc0b5dba22d
> Committed: https://crrev.com/ab543ef12f3b412b9e008fce7e78f4456de82822
> Cr-Original-Commit-Position: refs/heads/master@{#401765}
> Cr-Commit-Position: refs/heads/master@{#401810}

TBR=jennyz@chromium.org,falken@chromium.org,jamescook@chromium.org,warx@chrom...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=468969
==========

to

==========
Revert of tabing inconsistencies between trays in status area (patchset #5
id:80001 of https://codereview.chromium.org/2087703002/ )

Reason for revert:
Causes UaF in web_notification_tray.cc on focus_cycler_unittest.cc. The issue
appears to be unclear lifetime assumptions of the system tray object: the status
area widget maintains a raw pointer to it (and confusingly *sometimes* owns it,
i.e. when its Shutdown() method is called), whereas focus_cycler_unittest.cc
actually owns it but doesn't clear it out from the status area widget when it
deletes it on TearDown(). It looks like this CL just tickled a pre-existing bad
situation, but the best fix is not obvious to me as a non-expert in this part of
the codebase, so I have to revert.

Example failure:
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2...

Relevant snippet:

==7090==ERROR: AddressSanitizer: heap-use-after-free on address 0x618000021608
at pc 0x00000a32db44 bp 0x7ffdd1268ae0 sp 0x7ffdd1268ad8
WRITE of size 8 at 0x618000021608 thread T0
    #0 0xa32db43 in views::View::SetNextFocusableView(views::View*)
ui/views/view.cc:1198:24
    #1 0x964d8ba in Run base/callback.h:397:12
    #2 0x964d8ba in base::debug::TaskAnnotator::RunTask(char const*,
base::PendingTask const&) base/debug/task_annotator.cc:51
    #3 0x95131cc in base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:493:19
    #4 0x951401b in base::MessageLoop::DeferOrRunPendingTask(base::PendingTask
const&) base/message_loop/message_loop.cc:502:5
    #5 0x9514eab in base::MessageLoop::DoWork()
base/message_loop/message_loop.cc:624:13
    #6 0x95191f7 in base::MessagePumpGlib::Run(base::MessagePump::Delegate*)
base/message_loop/message_pump_glib.cc:313:49
    #7 0x956bbfb in base::RunLoop::Run() base/run_loop.cc:35:10
    #8 0x127dc12 in ash::test::AshTestHelper::RunAllPendingInMessageLoop()
ash/test/ash_test_helper.cc:192:12
    #9 0x127b887 in RunAllPendingInMessageLoop ash/test/ash_test_base.cc:287:21
    #10 0x127b887 in ash::test::AshTestBase::TearDown()
ash/test/ash_test_base.cc:163

Original issue's description:
> Detailed reproduce steps are in comment 2 in crbug.com/468969
>
> There are several bugs in the status area tray focus advancing.
>
> (1) there is a invisible focus between system tray and web notification tray.
This is because there is a focus traversal in web notification tray's child.
Solution is to disable it.
> When virtual keyboard is enabled, we should also disable its child view
traversal.
>
> (2) TAB will be trapped in system tray. Web notification tray is async
painted. The initialization sibling relation may not be ready. Add a
setnextfocusibleview call will fix that.
>
> BUG=468969
> TEST=tested on device, both tabing and shift-tabing works well on status area.
>
> There is still a bug in crbug.com/468969 comment 7. It happens in login screen
only. When logged in, it works fine.
>
> Committed: https://crrev.com/133c792af9dfe8c82561bee7015efcc0b5dba22d
> Committed: https://crrev.com/ab543ef12f3b412b9e008fce7e78f4456de82822
> Cr-Original-Commit-Position: refs/heads/master@{#401765}
> Cr-Commit-Position: refs/heads/master@{#401810}

TBR=jennyz@chromium.org,falken@chromium.org,jamescook@chromium.org,warx@chrom...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=468969

Committed: https://crrev.com/83e93cf04db96326a89176429d604bf131cf9229
Cr-Commit-Position: refs/heads/master@{#401848}
==========

[commit-bot: I haz the power, 2016-06-24 11:03:29]

Patchset 2 (id:??) landed as
https://crrev.com/83e93cf04db96326a89176429d604bf131cf9229
Cr-Commit-Position: refs/heads/master@{#401848}