Fix BrowserActionsBarBrowserTest.OverflowedBrowserActionPopupTestRemoval in ASan with new libc++.

Fix BrowserActionsBarBrowserTest.OverflowedBrowserActionPopupTestRemoval in ASan with new libc++.

superseded by https://codereview.chromium.org/2901193002

BUG=717554, 599467

[Nico, 2017-05-02 15:33:22]

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

[commit-bot: I haz the power, 2017-05-02 15:33:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2017-05-02 15:36:34]

thakis@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Nico, 2017-05-02 15:36:34]

\noideanico, seems to work though.

[commit-bot: I haz the power, 2017-05-02 16:22:27]

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

[commit-bot: I haz the power, 2017-05-02 16:22:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Devlin, 2017-05-02 16:37:38]

https://codereview.chromium.org/2857653002/diff/1/chrome/browser/ui/toolbar/t...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/2857653002/diff/1/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:663:
delegate_->RemoveViewForAction(removed_action.get());
This doesn't seem quite right to me.

The UndoPopOut() will trigger a redraw, but during the redraw, the set of views
owned by the UI won't match the controllers here, so our reordering could
potentially be incorrect, leading to different bugs/crashes.

The problem here is that in calling RemoveViewForAction(), we end up deleting
the ui view, which triggers a flow to undo the pop out, and eventually winds up
trying to paint the view we just removed.  This actually seems like it's
fundamentally a problem with the flow of RemoveViewForAction() ->
~ToolbarActionView().  My guess is the reason this started showing up could be
if the new libc++ changes the vector::erase + std::unique_ptr destruction
implementation (which means there's a bug here, but it was just hidden).

I think that a good refactoring is probably in order to make this whole thing
less... explosive... but, as a stop gap, a short patch might be to move the call
of UndoPopOut to the UI delegate's implementation in RemoveViewForAction(). 
e.g.,

void BrowserActionsContainer::RemoveViewForAction(
    ToolbarActionViewController* action) {
-  for (ToolbarActionViews::iterator iter = toolbar_action_views_.begin();
-       iter != toolbar_action_views_.end(); ++iter) {
-    if ((*iter)->view_controller() == action) {
-      toolbar_action_views_.erase(iter);
-      break;
-    }
-  }
+  auto iter = toolbar_action_views_.find_if(
+      [action](const std::unique_ptr<ToolbarActionView>& view) {
+        return view->view_controller() == action;
+  });
+  DCHECK(iter != toolbar_action_views_.end());
+  iter->get()->HidePopup();
+  toolbar_action_views_.erase(iter);
}

And then in ExtensionActionViewController:

void ExtensionActionViewController::SetDelegate(
    ToolbarActionViewDelegate* delegate) {
  DCHECK((delegate == nullptr) ^ (view_delegate_ == nullptr));
  if (delegate) {
    view_delegate_ = delegate;
    platform_delegate_->OnDelegateSet();
  } else {
+    DCHECK(!is_showing_popup());
-    if (is_showing_popup())
-      HidePopup();
    platform_delegate_.reset();
    view_delegate_ = nullptr;
  }
}

and something similar in the cocoa.

I think that should solve the problem and avoid the necessity of shaving a much
larger yak.

[Nico, 2017-05-03 20:51:45]

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

[commit-bot: I haz the power, 2017-05-03 20:52:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2017-05-03 21:01:32]

Done in patch set 2, thanks for the detailed notes. (The classes look like xplat
model code, so I'm not sure if I need to change any cocoa code.)

[Nico, 2017-05-03 21:11:43]

Hm, the test still fails on mac, so i guess something does need to change in
some cocoa file
(https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2...)

[Devlin, 2017-05-03 21:19:05]

On 2017/05/03 21:11:43, Nico wrote:
> Hm, the test still fails on mac, so i guess something does need to change in
> some cocoa file
>
(https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2...)

Yeah, I think we need to make a similar call in -[BrowserActionsController
removeViewForAction] or in -[BrowserActionButton onRemoved].

Also, with this change, I think we can remove the `if (popped_out_action_ ==
removed_action.get()) UndoPopOut();` part of
ToolbarActionsBar::OnToolbarActionRemoved().

[Nico, 2017-05-03 21:21:50]

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

[commit-bot: I haz the power, 2017-05-03 21:22:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2017-05-03 21:56:33]

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

[commit-bot: I haz the power, 2017-05-03 21:57:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2017-05-03 22:23:59]

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

[commit-bot: I haz the power, 2017-05-03 22:24:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2017-05-03 22:24:45]

On 2017/05/03 21:19:05, Devlin wrote:
> On 2017/05/03 21:11:43, Nico wrote:
> > Hm, the test still fails on mac, so i guess something does need to change in
> > some cocoa file
> >
>
(https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2...)
> 
> Yeah, I think we need to make a similar call in -[BrowserActionsController
> removeViewForAction] or in -[BrowserActionButton onRemoved].
> 
> Also, with this change, I think we can remove the `if (popped_out_action_ ==
> removed_action.get()) UndoPopOut();` part of
> ToolbarActionsBar::OnToolbarActionRemoved().

I attempted that in patch sets 4 and 5. 4 makes one test pass and another fail,
and 5 makes 2 different tests fail in interesting ways. It looks like views and
cocoa ownership is subtly different here somehow. I'll debug it some.

[commit-bot: I haz the power, 2017-05-03 23:01:14]

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

[commit-bot: I haz the power, 2017-05-03 23:01:15]

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

[Nico, 2017-05-04 15:34:31]

(mostly talking to myself)

I think the problem here is that OnToolbarActionRemoved is called after
ToolbarActionsModel::RemoveItem() has already removed the ToolbarItem from its
toolbar_items_, but UndoPopOut() calls ToolbarActionsBar::GetIconCount() which
does:

    DCHECK_EQ(model_->toolbar_items().size(), toolbar_actions_.size());

So because of that, UndoPopOut() has to be called after also removing the
ToolbarActionViewController from toolbar_actions_ (like you said). But if I do
it later, then BrowsreActionsBarBrowserTest.RemovePoppedOutAction fails on Mac
like so:


[ RUN      ] BrowserActionsBarBrowserTest.RemovePoppedOutAction
[96286:775:0504/111812.455102:FATAL:toolbar_actions_bar.cc(310)] Check failed:
actions.size() != index (2 vs. 2)
0   libbase.dylib                       0x000000010bfdad5c
base::debug::StackTrace::StackTrace(unsigned long) + 28
1   libbase.dylib                       0x000000010c0062a0
logging::LogMessage::~LogMessage() + 224
2   browser_tests                       0x0000000104d681a6
ToolbarActionsBar::GetActions() const + 326
3   browser_tests                       0x0000000104f56533
-[BrowserActionsController redraw] + 211
4   browser_tests                       0x0000000104d6abe6
ToolbarActionsBar::OnToolbarActionRemoved(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 662
5   browser_tests                       0x0000000104625764
ToolbarActionsModel::RemoveItem(ToolbarActionsModel::ToolbarItem const&) + 1124
6   browser_tests                       0x000000010462445a
ToolbarActionsModel::OnExtensionUnloaded(content::BrowserContext*,
extensions::Extension const*, extensions::UnloadedExtensionReason) + 202
7   browser_tests                       0x0000000102c70f7f
extensions::ExtensionRegistry::TriggerOnUnloaded(extensions::Extension const*,
extensions::UnloadedExtensionReason) + 351
8   browser_tests                       0x00000001047b81b2
ExtensionService::UnloadExtension(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
extensions::UnloadedExtensionReason) + 434
9   browser_tests                       0x000000010265b3ff
BrowserActionsBarBrowserTest_RemovePoppedOutAction_Test::RunTestOnMainThread() +
2079
10  browser_tests                       0x0000000103c36083
content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() + 403
11  browser_tests                       0x0000000103402d58
ChromeBrowserMainParts::PreMainMessageLoopRunImpl() + 4824
12  browser_tests                       0x000000010340197e
ChromeBrowserMainParts::PreMainMessageLoopRun() + 62
13  libcontent.dylib                    0x0000000110c542e3
content::BrowserMainLoop::PreMainMessageLoopRun() + 67
14  libcontent.dylib                    0x00000001110c5517
content::StartupTaskRunner::RunAllTasksNow() + 39

I guess closing the bubble confuses something on Mac, I'm still debugging this.

Also, with this change, some media router tests start failing on all platforms.
in_reply_to: 5681717746597888

[Nico, 2017-05-24 18:42:33]

Description was changed from

==========
Fix BrowserActionsBarBrowserTest.OverflowedBrowserActionPopupTestRemoval in ASan
with new libc++.

BUG=717554,599467
==========

to

==========
Fix BrowserActionsBarBrowserTest.OverflowedBrowserActionPopupTestRemoval in ASan
with new libc++.

superceded by https://codereview.chromium.org/2901193002

BUG=717554,599467
==========

[Nico, 2017-05-24 18:42:42]

Description was changed from

==========
Fix BrowserActionsBarBrowserTest.OverflowedBrowserActionPopupTestRemoval in ASan
with new libc++.

superceded by https://codereview.chromium.org/2901193002

BUG=717554,599467
==========

to

==========
Fix BrowserActionsBarBrowserTest.OverflowedBrowserActionPopupTestRemoval in ASan
with new libc++.

superseded by https://codereview.chromium.org/2901193002

BUG=717554,599467
==========