Clean up after each MemoryCoordinatorImpl test

Clean up g_memory_coordinator after each MemoryCoordinatorImpl test

MemoryCoordinatorImpl tests fail the first time and should be re-run separately.
g_memory_coordinator need to be cleaned up between tests, otherwise the subsequent tests
will crash trying to reset g_memory_coordinator.

This CL implements reset method which called after each MemoryCoordinatorImpl in destructor.

BUG=698282
Review-Url: https://codereview.chromium.org/2726983006
Cr-Commit-Position: refs/heads/master@{#454926}
Committed: https://chromium.googlesource.com/chromium/src/+/aba30978d6c58a937eb2cf5fbdcde3e2abb7cfaa

[dimaa1, 2017-03-03 17:36:30]

Description was changed from

==========
Clean up g_memory_coordinator after each MemoryCoordinatorImpl test

MemoryCoordinatorImpl tests fail the first time and should be re-run separately.
g_memory_coordinator need to be cleaned up between tests, otherwise the
subsequent tests
will crash trying to reset g_memory_coordinator.

This CL implements reset method which called after each MemoryCoordinatorImpl in
destructor.

BUG=698282
==========

to

==========
Clean up g_memory_coordinator after each MemoryCoordinatorImpl test

MemoryCoordinatorImpl tests fail the first time and should be re-run separately.
g_memory_coordinator need to be cleaned up between tests, otherwise the
subsequent tests
will crash trying to reset g_memory_coordinator.

This CL implements reset method which called after each MemoryCoordinatorImpl in
destructor.

BUG=698282
==========

[sadrul, 2017-03-03 17:40:41]

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

[sadrul, 2017-03-03 17:40:41]

https://codereview.chromium.org/2726983006/diff/1/content/browser/memory/memo...
File content/browser/memory/memory_coordinator_impl.cc (right):

https://codereview.chromium.org/2726983006/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator_impl.cc:180:
base::MemoryCoordinatorProxy::ResetGlobals();
Maybe this could just call:
  base::MemoryCoordinatorProxy::SetMemoryCoordinator(nullptr)?

and SetMemoryCoordinator() could be updated to allow this?

[dimaa1, 2017-03-03 18:06:55]

dimaa@chromium.org changed reviewers:
+ hajimehoshi@chromium.org

[dimaa1, 2017-03-03 18:30:05]

https://codereview.chromium.org/2726983006/diff/1/content/browser/memory/memo...
File content/browser/memory/memory_coordinator_impl.cc (right):

https://codereview.chromium.org/2726983006/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator_impl.cc:180:
base::MemoryCoordinatorProxy::ResetGlobals();
On 2017/03/03 17:40:41, sadrul wrote:
> Maybe this could just call:
>   base::MemoryCoordinatorProxy::SetMemoryCoordinator(nullptr)?
> 
> and SetMemoryCoordinator() could be updated to allow this?

These tests are failing because we execute this method when globals are not
reseted. We are getting crash because of the line
"DCHECK(!g_memory_coordinator);" inside that method. That is why we need to use
different way to reset it.

[sadrul, 2017-03-03 18:45:01]

https://codereview.chromium.org/2726983006/diff/1/content/browser/memory/memo...
File content/browser/memory/memory_coordinator_impl.cc (right):

https://codereview.chromium.org/2726983006/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator_impl.cc:180:
base::MemoryCoordinatorProxy::ResetGlobals();
On 2017/03/03 18:30:05, dimaa1 wrote:
> On 2017/03/03 17:40:41, sadrul wrote:
> > Maybe this could just call:
> >   base::MemoryCoordinatorProxy::SetMemoryCoordinator(nullptr)?
> > 
> > and SetMemoryCoordinator() could be updated to allow this?
> 
> These tests are failing because we execute this method when globals are not
> reseted. We are getting crash because of the line
> "DCHECK(!g_memory_coordinator);" inside that method. That is why we need to
use
> different way to reset it.

Yep. That's what I suggested SetMemoryCoordinator() would need to also be
updated. For example, instead of:

  DCHECK(!g_memory_coordinator);

it could check:

  DCHECK(!g_memory_coordinator || !coordinator);

i.e. you can unset the global coordinator, but cannot replace the global
coordinator with another one. Which is the behaviour we want.

[dimaa1, 2017-03-03 18:52:03]

https://codereview.chromium.org/2726983006/diff/1/content/browser/memory/memo...
File content/browser/memory/memory_coordinator_impl.cc (right):

https://codereview.chromium.org/2726983006/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator_impl.cc:180:
base::MemoryCoordinatorProxy::ResetGlobals();
On 2017/03/03 18:45:01, sadrul wrote:
> On 2017/03/03 18:30:05, dimaa1 wrote:
> > On 2017/03/03 17:40:41, sadrul wrote:
> > > Maybe this could just call:
> > >   base::MemoryCoordinatorProxy::SetMemoryCoordinator(nullptr)?
> > > 
> > > and SetMemoryCoordinator() could be updated to allow this?
> > 
> > These tests are failing because we execute this method when globals are not
> > reseted. We are getting crash because of the line
> > "DCHECK(!g_memory_coordinator);" inside that method. That is why we need to
> use
> > different way to reset it.
> 
> Yep. That's what I suggested SetMemoryCoordinator() would need to also be
> updated. For example, instead of:
> 
>   DCHECK(!g_memory_coordinator);
> 
> it could check:
> 
>   DCHECK(!g_memory_coordinator || !coordinator);
> 
> i.e. you can unset the global coordinator, but cannot replace the global
> coordinator with another one. Which is the behaviour we want.

Great idea!

[sadrul, 2017-03-03 19:27:32]

cool! non-owner lgtm

[hajimehoshi, 2017-03-06 06:24:15]

hajimehoshi@chromium.org changed reviewers:
+ bashi@chromium.org

[hajimehoshi, 2017-03-06 06:24:16]

+bashi

[bashi, 2017-03-06 06:25:37]

lgtm. Thanks for the fix!

[hajimehoshi, 2017-03-06 06:32:28]

non-owner lgtm

[dimaa1, 2017-03-06 16:45:51]

dimaa@chromium.org changed reviewers:
+ mark@chromium.org

[dimaa1, 2017-03-06 16:45:52]

mark@chromium.org: Please review changes in 
  
 base/memory/memory_coordinator_proxy.cc

[Mark Mentovai, 2017-03-06 18:17:20]

LGTM

[dimaa1, 2017-03-06 18:50:22]

The CQ bit was checked by dimaa@chromium.org

[commit-bot: I haz the power, 2017-03-06 18:51:00]

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-03-06 19:57:16]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488826222396380,
"parent_rev": "e8cf2b3e52941a3d8cd632908dbd81a4a7b02470", "commit_rev":
"aba30978d6c58a937eb2cf5fbdcde3e2abb7cfaa"}

[commit-bot: I haz the power, 2017-03-06 19:57:56]

Description was changed from

==========
Clean up g_memory_coordinator after each MemoryCoordinatorImpl test

MemoryCoordinatorImpl tests fail the first time and should be re-run separately.
g_memory_coordinator need to be cleaned up between tests, otherwise the
subsequent tests
will crash trying to reset g_memory_coordinator.

This CL implements reset method which called after each MemoryCoordinatorImpl in
destructor.

BUG=698282
==========

to

==========
Clean up g_memory_coordinator after each MemoryCoordinatorImpl test

MemoryCoordinatorImpl tests fail the first time and should be re-run separately.
g_memory_coordinator need to be cleaned up between tests, otherwise the
subsequent tests
will crash trying to reset g_memory_coordinator.

This CL implements reset method which called after each MemoryCoordinatorImpl in
destructor.

BUG=698282

Review-Url: https://codereview.chromium.org/2726983006
Cr-Commit-Position: refs/heads/master@{#454926}
Committed:
https://chromium.googlesource.com/chromium/src/+/aba30978d6c58a937eb2cf5fbdcd...
==========

[commit-bot: I haz the power, 2017-03-06 19:57:58]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/aba30978d6c58a937eb2cf5fbdcd...