Also PrerenderBrowserTest.OpenTaskManagerAfterPrerender in https://chromium-swarm.appspot.com/task?id=3a14fbab27699910&refresh=10&show_raw=1

I see at least one test crash with this stack in every browser_tests run on Win7 Tests (dbg)(1) going back at least a week. Please prioritize fixing this. Thanks.

erikchen@: hjd@ and I believe that these tests are failing because task manager tests are not waiting for the memory instrumentation service to respond before closing the browser.

The correct fix for this is to use ScopedCallbackRunner (see https://cs.chromium.org/chromium/src/media/base/scoped_callback_runner.h?q=ScopedCallback&sq=package:chromium&l=47) or similar to run the callback before shut down.

However, as evidenced by https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-mojo/not$20called%7Csort:date/chromium-mojo/zyKcD5C7COk/BaBidUhoAAAJ, there is a push to have this sort of code in base/. Clearly this will take a while to get through the relevant processes so as a temporary fix, we think the simple fix would be just to wait a bit longer in the tests for the service to respond. Thoughts?

Whoops not sure why accidentally made owner not me. Fixed

A temporary fix https://chromium-review.googlesource.com/c/chromium/src/+/790591 that should stop the waterfall from flaky is on its way on CQ.
erikchen@ moving this back to you for properly fixing the task manager test. It seems (See #4) that is shutting down without waiting that the callbacks from memory-infra has come.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/91bd62bac6ce380da0e5e11d29da46abf2b954b8

commit 91bd62bac6ce380da0e5e11d29da46abf2b954b8
Author: Primiano Tucci <primiano@chromium.org>
Date: Tue Nov 28 01:43:35 2017

memory-infra: don't pass mojo callback to MDM from client process

Passing these callbacks from the client process code means that MDM begins
owning the callback. However, these callbacks *must* be run before
destruction. While this is not a problem with MDM per-se as MDM is a
leaky singleton, MDM ocassionally passes ownership of this callback to
a thread loop.

On destruction of the thread loop with the callback posted, the callback
could be destroyed without being run which can cause problems.

To solve this, instead of passing the callback into a situation where
it could be destroyed, we instead store the callback inside the client
process itself. As the client is also a leaky singleton, the destructor
of these callbacks will no longer be run so we do not have the above problem.

Bug:  788658 
Change-Id: I9dd65fe7748e0afaedd1bf5a3d25ac9cadfe9853
Reviewed-on: https://chromium-review.googlesource.com/790591
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Commit-Queue: Primiano Tucci <primiano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519517}
[modify] https://crrev.com/91bd62bac6ce380da0e5e11d29da46abf2b954b8/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.cc
[modify] https://crrev.com/91bd62bac6ce380da0e5e11d29da46abf2b954b8/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.h
[modify] https://crrev.com/91bd62bac6ce380da0e5e11d29da46abf2b954b8/services/resource_coordinator/public/cpp/memory_instrumentation/tracing_integration_unittest.cc

hjd@, lalitm@, thanks for looking into this!

It turns out that the error message is actually slightly misleading. When the implementation for a mojo interface receives a callback, it must do one of two things:

  1) Run the callback before destroying it.
  2) Destroy the binding before destroying the callback.

(2) is necessary because there are some cases where a callback cannot be meaningfully run [usually during destruction], as you've noticed. The CL you landed is functional, since the ClientProcessImpl is never destroyed, but it might cause issues if ClientProcessImpl ever becomes not-a-leaky-singleton. The slighter better solution is to make sure that pending_chrome_callbacks_ is destroyed after binding_.

I'll put up a quick CL.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/30171433f40d6102ab29d9233f1ffe0fcfbc2927

commit 30171433f40d6102ab29d9233f1ffe0fcfbc2927
Author: Erik Chen <erikchen@chromium.org>
Date: Wed Nov 29 11:32:54 2017

Fix destruction order for ClientProcessImpl.

pending_chrome_callbacks_ should be destroyed after binding_ to prevent the
callback destructor from hitting a DCHECK.

Bug:  788658 
Change-Id: I14f1844a12f5dbb48542863672c161fb2c43dd4e
Reviewed-on: https://chromium-review.googlesource.com/794672
Commit-Queue: Hector Dearman <hjd@chromium.org>
Reviewed-by: Hector Dearman <hjd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520073}
[modify] https://crrev.com/30171433f40d6102ab29d9233f1ffe0fcfbc2927/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.h