DeleteSoon in test objects after MessageLoop::RunAllPending

[James Hawkins]

Context:
Flaky leak in WebAuthFlowTest:

09:05:30 memcheck_analyze.py [ERROR] Command:
Leak_DefinitelyLost
14,249 (8 direct, 14,241 indirect) bytes in 1 blocks are definitely lost in loss record 34,768 of 35,169
operator new(unsigned long) (m_replacemalloc/vg_replace_malloc.c:1074)
SSLManager::SSLManager(NavigationControllerImpl*) (content/browser/ssl/ssl_manager.cc:84)
NavigationControllerImpl::NavigationControllerImpl(WebContentsImpl*, content::BrowserContext*, SessionStorageNamespaceImpl*) (content/browser/web_contents/navigation_controller_impl.cc:186)
WebContentsImpl::WebContentsImpl(content::BrowserContext*, content::SiteInstance*, int, WebContentsImpl const*, WebContentsImpl*, SessionStorageNamespaceImpl*) (content/browser/web_contents/web_contents_impl.cc:294)
content::TestWebContents::TestWebContents(content::BrowserContext*, content::SiteInstance*) (content/browser/web_contents/test_web_contents.cc:34)
content::WebContentsTester::CreateTestWebContents(content::BrowserContext*, content::SiteInstance*) (content/test/web_contents_tester.cc:63)
(anonymous namespace)::MockWebAuthFlow::CreateWebContents() (chrome/browser/extensions/api/identity/web_auth_flow_unittest.cc:62)
extensions::WebAuthFlow::Start() (chrome/browser/extensions/api/identity/web_auth_flow.cc:82)
WebAuthFlowTest_SilentRedirectToChromiumAppUrl_Test::TestBody() (chrome/browser/extensions/api/identity/web_auth_flow_unittest.cc:140)

Object Containment Hierarchy (indentation implies containment).

WebAuthFlow

 - WebContentsImpl |contents_|

  - NavigationControllerImpl

   - SSLManager

    - SSLPolicy* (LEAKED)

SSLManager deletes the SSLPolicy in its destructor, but the SSLManager is not being destructed.

~WebAuthFlow() calls MessageLoop::DeleteSoon(contents_).

WebAuthFlowTest inherits from ChromeRenderViewHostTestHarness whose TearDown eventually calls MessageLoop::RunAllPending().

Issue:  MessageLoop::RunAllPending() is called before ~WebAuthFlow(), i.e. before a task is posted to the MessageLoop to delete some object.

This must be a common pattern with an obvious solution (that I'm missing).  Can anyone send pointers my way (heh)?

Thanks,

James

[Rachel Blum]

simply call flow_.reset() in TearDown()? 

Rachel 

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[James Hawkins]

Calling flow_.reset() *before* calling up to the ChromeRenderViewTestHarness::TearDown() fixes the leak; however, this seems extremely fragile (I'm committing this fix, though, with an appropriate comment).

Is this really the best solution?

Thanks,

James

[Reid Kleckner]

This is a very, very common source of leaks, from what I've seen. 

Maybe some kind of ScopedMessageLoop that (for testing only!) runs all its tasks when it is destroyed?  You'd have to do something clever when one loop queues a message onto another which sends one back to the first.

Reid

[Munjal Doshi]

Actually I had the following code in destructor of WebAuthFlowTest but I removed it since my run on valgrind did not reveal any leaks without that code (and I didn't realize the leak is happening):

  // Delete the flow before running pendings tasks in message loop since ~WebAuthFlow posts

  // a task to lazily delete web contents.

  flow_.reset();

  MessageLoop::current()->RunAllPending();

I think that is a slightly better approach since in that case we don't depend on test harness tear down running the message loop.

Do you think it is worth using that approach?

-Munjal

[James Hawkins]

Doesn't matter to me.  My CL was committed, but I can review a proposed alteration.

James