1116195 - test_bug846906.xul fails by leaking when run as a standalone directory

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Vaibhav (:vaibhav1994)]

in bug 1110982, we are looking to enable tests where we run a fresh browser instance per directory.  Usually what happens is that a few tests fail because they accidentally depend on the state of the browser from an earlier test.

In the try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c9fa9885207d

This test causes a leak in linux debug, windows xp debug, windows 7 debug, windows 8 x64 debug.

some leaks are like this:

 TEST-UNEXPECTED-FAIL | leakcheck | default process: 2356 bytes leaked (ClientLayerManager, ClientLayerManager::MemoryPressureObserv, DrawTargetCairo, LayerManager, PuppetWidget, ...) 

 TEST-UNEXPECTED-FAIL | leakcheck | default process: 2620 bytes leaked (ClientLayerManager, ClientLayerManager::MemoryPressureObserv, DrawTargetD2D, LayerManager, PuppetWidget, ...)

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

I verified this locally on linux64 debug build (downloaded the build and tests from the try push and ran things locally, I assume building debug locally would produce the same bits).

Neil, can you take a look at this?  Maybe if this isn't your area, you could help find someone who knows more about this.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

to reproduce this on a debug build do this:
./mach mochitest-chrome docshell/test/chrome

Comment 3

[Neil Deakin]

Don't know about this. Probably best to ask the author of bug 846906.

Comment 4

[Eddy Bruel [:ejpbruel]]

I wrote the patch for bug 846906 ages ago, and even back then it was a hack I didn't fully understand. I have absolutely no idea what is causing these leaks.

The person that helped me through the parts I didn't understand at the time was bz. He might have a better shot at knowing what's going on.

Comment 5

[Boris Zbarsky [:bzbarsky]]

What's going on is clearly that a PuppetWidget is leaking, right?

Someone who can reproduce this locally should do a refcount-balance on PuppetWidget and see who's keeping it alive.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

bz, if you can give me a pointer on how to do a refcount-balance, I would be happy to do it!

Comment 7

[Boris Zbarsky [:bzbarsky]]

http://www-archive.mozilla.org/performance/leak-tutorial.html describes how to do it.  The output of make-tree.pl is the interesting bit here for a start.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: refcnt stack from a debug build

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

this is still an issue with the latest bits:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e4065c13a17

I can still reproduce this locally as well!

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

We only have a few tests left, I would like to disable this test case for all debug platforms so we can move forward with --run-by-dir by the end of the week.

If we can hack on this test case, I would be happy to work on fixing this vs disabling it.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Sorry, I'd missed comment 8.

Is that stack from a debug + optimized build or something?  It's clearly missing some stackframes...  If it _is_ debug+optimized, get you get a stack for debug without optimization?

In any case, it _looks_ like we're taking two references to a PuppetWidget: one from a view, and one from a parent PuppetWidget.   The former then goes away via nsView::DestroyWidget but the latter sticks around.  But that leaves the question of who keeps the parent PuppetWidget alive.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

I had thought this was a debug build + debug symbols (built locally).  I normally build in opt mode as I do test harness changes and related bits.

Do you have anything you would like me to try?  patches, directions for me to start hacking on, etc.?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Did your mozconfig have --disable-optimize in addition to --enable-debug?  If not, could you do that and then get the refcount log again?

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: refcnt stack from a debug build

ran this again, this time I added --disable-optimize to my mozconfig file.  I get a similar stack, not sure if I am missing something else in the picture.

Comment 15

[Boris Zbarsky [:bzbarsky]]

That's a quite different set of stacks!

That's showing PuppetWidget ending up with a reference to it from an nsDeviceContext.

Does the nsDeviceContext leak?  What does the refcount balance tree look like for it?

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: refcnt stack from a debug build for nsDeviceContext

do let me know if this is useful- I am a bit lost on understanding this fully, but can keep getting new sets of data :)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8559363 [details]
refcnt stack from a debug build for nsDeviceContext

Alright, now I'm lost.

The nsDeviceContext has a ref to mWidget.

The widget (a different widget?) has a ref to mContext.

What breaks the cycle, or why is this not a cycle?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think the idea is that a nsDeviceContext owned by a widget does not have a reference to that widget. But this code is terribly confusing.

nsBaseWidget::GetDeviceContext violates that invariant, but as far as I can tell nsIWidget::GetDeviceContext is never called. I'll try removing it...

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That works. Then it seems to me that nsBaseWidget::mContext is never really used.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I've landed patches in bug 1129774 which remove nsBaseWidget::mContext. I don't know whether they will fix this bug, but at least we won't have a cycle involving mContext anymore :-).

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

Thanks!  I can test this out!

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

wow, this solved the leak issue, thanks for cleaning it up!