Fixes a DCHECK in tools_menu_button_observer_bridge during tests

Fixes a DCHECK in tools_menu_button_observer_bridge during tests

A DCHECK was observed during omnibox_perftest. It traces down to
-[ToolsMenuButtonObserverBridge readingListModelBeingDeleted:] being
called twice and the second call triggered the DCHECK.

DependencyManager::DestroyContextServices() calls ContextShutdown()
and then ContextDestroyed() on all dependencies. Each of the call
entered -readingListModelBeingDeleted: because there is no distinction
between Shutdown and Destroyed.

BUG=717300
Review-Url: https://codereview.chromium.org/2863303002
Cr-Commit-Position: refs/heads/master@{#472825}
Committed: https://chromium.googlesource.com/chromium/src/+/52ab15efe9499088cd3c2a2474887671630b0311

[pkl (ping after 24h if needed), 2017-05-08 13:03:32]

pkl@chromium.org changed reviewers:
+ noyau@chromium.org, rohitrao@chromium.org

[pkl (ping after 24h if needed), 2017-05-08 13:03:33]

Please review:
noyau: components/reading_list/
rohitrao: ios/chrome/browser/ui/ OWNERS and overall

[noyau (Ping after 24h), 2017-05-09 13:27:11]

lgtm

https://codereview.chromium.org/2863303002/diff/20001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/tools_menu_button_observer_bridge.mm (right):

https://codereview.chromium.org/2863303002/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/tools_menu_button_observer_bridge.mm:66: }
Is this method really necessary? In C++ this would be removed at link time, but
in objectiveC the empty method will stay here. Just for a DCHECK that is very
unlikely to be hit.

[rohitrao (ping after 24h), 2017-05-09 14:52:09]

ios/ lgtm but I'm not very familiar with the reading list code.  It seems weird
that the BeingDeleted() method was called twice in the first place, and it
doesn't seem like there's any real need for the BeingShutdown() function,
especially since the only implementation is sort of a no-op.

Could we just delete one of the calls to BeingDeleted() and not replace it with
anything?  I don't know enough about reading list to reason about the effects of
that.  (Although as written, this CL effectively deletes the first call, since
no one implements BeingShutdown()).

https://codereview.chromium.org/2863303002/diff/20001/components/reading_list...
File components/reading_list/core/reading_list_model_unittest.cc (right):

https://codereview.chromium.org/2863303002/diff/20001/components/reading_list...
components/reading_list/core/reading_list_model_unittest.cc:317: model_.reset();
Why does the model need to be reset here?  This is unexpected, so it could use a
comment.

https://codereview.chromium.org/2863303002/diff/20001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/tools_menu_button_observer_bridge.mm (right):

https://codereview.chromium.org/2863303002/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/tools_menu_button_observer_bridge.mm:69:
DCHECK(model == _model);
Does anyone know why this DCHECK exists or what it's trying to protect/enforce?

[pkl (ping after 24h if needed), 2017-05-09 15:04:39]

On 2017/05/09 14:52:09, rohitrao (ping after 24h) wrote:
> ios/ lgtm but I'm not very familiar with the reading list code.  It seems
weird
> that the BeingDeleted() method was called twice in the first place, and it
> doesn't seem like there's any real need for the BeingShutdown() function,
> especially since the only implementation is sort of a no-op.
> 
> Could we just delete one of the calls to BeingDeleted() and not replace it
with
> anything?  I don't know enough about reading list to reason about the effects
of
> that.  (Although as written, this CL effectively deletes the first call, since
> no one implements BeingShutdown()).
> 
>
https://codereview.chromium.org/2863303002/diff/20001/components/reading_list...
> File components/reading_list/core/reading_list_model_unittest.cc (right):
> 
>
https://codereview.chromium.org/2863303002/diff/20001/components/reading_list...
> components/reading_list/core/reading_list_model_unittest.cc:317:
model_.reset();
> Why does the model need to be reset here?  This is unexpected, so it could use
a
> comment.
> 
>
https://codereview.chromium.org/2863303002/diff/20001/ios/chrome/browser/ui/t...
> File ios/chrome/browser/ui/toolbar/tools_menu_button_observer_bridge.mm
(right):
> 
>
https://codereview.chromium.org/2863303002/diff/20001/ios/chrome/browser/ui/t...
> ios/chrome/browser/ui/toolbar/tools_menu_button_observer_bridge.mm:69:
> DCHECK(model == _model);
> Does anyone know why this DCHECK exists or what it's trying to
protect/enforce?

rohitrao: Being called twice is indeed the bug. It's called once through
Shutdown (see
https://codereview.chromium.org/2863303002/diff/20001/components/reading_list...
) and then again through the destructor. That's why I introduced the new
Shutdown path.

noyau: Do you think we should hold this CL until Olivier can review it? Or jif?

[noyau (Ping after 24h), 2017-05-15 14:36:12]

Just land it. I'd remove the implementation just doing a DCHECK, but other than
that, it's pretty straightforward.

https://codereview.chromium.org/2863303002/diff/20001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/tools_menu_button_observer_bridge.mm (right):

https://codereview.chromium.org/2863303002/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/tools_menu_button_observer_bridge.mm:69:
DCHECK(model == _model);
On 2017/05/09 14:52:08, rohitrao (ping after 24h) wrote:
> Does anyone know why this DCHECK exists or what it's trying to
protect/enforce?

It's enforcing the fact that this method should only be called once. Peter what
do you think about moving the _model=nullptr to the shutdown method instead of
delete?

[pkl (ping after 24h if needed), 2017-05-18 04:51:04]

Added more comments and answered questions. Submitting.

https://codereview.chromium.org/2863303002/diff/20001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/tools_menu_button_observer_bridge.mm (right):

https://codereview.chromium.org/2863303002/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/tools_menu_button_observer_bridge.mm:66: }
On 2017/05/09 13:27:10, noyau (Ping after 24h) wrote:
> Is this method really necessary? In C++ this would be removed at link time,
but
> in objectiveC the empty method will stay here. Just for a DCHECK that is very
> unlikely to be hit.

Removing this method since it is not doing anything really useful.

https://codereview.chromium.org/2863303002/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/tools_menu_button_observer_bridge.mm:69:
DCHECK(model == _model);
On 2017/05/15 14:36:12, noyau (Ping after 24h) wrote:
> On 2017/05/09 14:52:08, rohitrao (ping after 24h) wrote:
> > Does anyone know why this DCHECK exists or what it's trying to
> protect/enforce?
> 
> It's enforcing the fact that this method should only be called once. Peter
what
> do you think about moving the _model=nullptr to the shutdown method instead of
> delete?

This should really be here. After rebasing, I noticed that a new
_modelBridge.reset() is added here and I believe that it actually belongs here.
_model should be nulled at the same place as _modelBridge.

[pkl (ping after 24h if needed), 2017-05-18 04:51:28]

The CQ bit was checked by pkl@chromium.org

[pkl (ping after 24h if needed), 2017-05-18 04:51:28]

The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org,
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2863303002/#ps40001
(title: "delete unnecessary implementation of Shutdown()")

[commit-bot: I haz the power, 2017-05-18 04:52:58]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-18 06:15:22]

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

[commit-bot: I haz the power, 2017-05-18 06:15:23]

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

[pkl (ping after 24h if needed), 2017-05-18 14:08:04]

The CQ bit was checked by pkl@chromium.org

[commit-bot: I haz the power, 2017-05-18 14:08:38]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-18 15:42:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495116484142840,
"parent_rev": "7498e9b31a8ed8fd1d1fac071916cf824e48e2c5", "commit_rev":
"52ab15efe9499088cd3c2a2474887671630b0311"}

[commit-bot: I haz the power, 2017-05-18 15:42:49]

Description was changed from

==========
Fixes a DCHECK in tools_menu_button_observer_bridge during tests

A DCHECK was observed during omnibox_perftest. It traces down to
-[ToolsMenuButtonObserverBridge readingListModelBeingDeleted:] being
called twice and the second call triggered the DCHECK.

DependencyManager::DestroyContextServices() calls ContextShutdown()
and then ContextDestroyed() on all dependencies. Each of the call
entered -readingListModelBeingDeleted: because there is no distinction
between Shutdown and Destroyed.

BUG=717300
==========

to

==========
Fixes a DCHECK in tools_menu_button_observer_bridge during tests

A DCHECK was observed during omnibox_perftest. It traces down to
-[ToolsMenuButtonObserverBridge readingListModelBeingDeleted:] being
called twice and the second call triggered the DCHECK.

DependencyManager::DestroyContextServices() calls ContextShutdown()
and then ContextDestroyed() on all dependencies. Each of the call
entered -readingListModelBeingDeleted: because there is no distinction
between Shutdown and Destroyed.

BUG=717300

Review-Url: https://codereview.chromium.org/2863303002
Cr-Commit-Position: refs/heads/master@{#472825}
Committed:
https://chromium.googlesource.com/chromium/src/+/52ab15efe9499088cd3c2a247488...
==========

[commit-bot: I haz the power, 2017-05-18 15:42:51]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/52ab15efe9499088cd3c2a247488...