Joel Maher ( :jmaher ) (UTC -8) Reporter
	Description • 11 years ago

Instead of running browser-chrome as a single chunk, I am running it per subdirectory, in this case --test-path browser/components/search/test.

I get this in the log file:
leakcheck | 5319 bytes leaked (DOMStorageManager, EventTokenBucket, Mutex, ReentrantMonitor, SpdySession31, ...)

This can be seen on try:
https://tbpl.mozilla.org/?tree=Try&rev=b561d98e8d72

I was able to download the build/tests from there and reproduce locally. I verified this file by deleting it from the test tree and the harness did not report a leak.

	Neil Deakin
	Comment 1 • 10 years ago

The leak is caused because the search bar uses nsISpeculativeConnect to precache the suggestion and search list when the search bar is focused. I assume these are still loading when the test finishes, or are failing to clean up in some way.

	Matthew N. [:MattN] Assignee
	Comment 2 • 10 years ago

I don't know of a way to force the connections to be cleaned up so instead I just disable the connections in search tests that I saw focusing the searchBar.

	Matthew N. [:MattN] Assignee
	Comment 3 • 10 years ago

An alternative is to have speculative connections disabled in tests by default and have specific tests opt-in. I worry about not testing what we're shipping though.

	Phil Ringnalda (:philor)
	Comment 4 • 10 years ago

"try" push: https://tbpl.mozilla.org/?tree=Cedar&showall=1&rev=a670ee5f5860

	Matthew N. [:MattN] Assignee
	Comment 5 • 10 years ago

(In reply to Matthew N. [:MattN] from comment #3)
> An alternative is to have speculative connections disabled in tests by
> default and have specific tests opt-in. I worry about not testing what we're
> shipping though.

This is widespread enough that I think it's the most manageable way at this point. I just found more bugs that are caused by the same problem since UITour also focuses the search box and we also use speculative connections on the new tab page tiles.

New tab page: bug 964358 and bug 990713
Search box: This bug and bug 980746

I suspect more tests will get reported as leaking in the future if we require tests to disable the connections themselves as people might not realize the cause.

	Ed Lee :Mardak
	Comment 6 • 10 years ago

The tricky part for us so far in bug 990713 is that we don't even know which test is causing the nsISpeculativeConnect -> leak. It seems that some mochitest VMs happen to have the cursor in the "wrong" spot triggering the speculative connect now that we populate tiles with default links.

mcmanus/mayhemer: Is there a good way to have these speculative connections cleaned up when the tests finish? Or potentially there's a leak somewhere?

	Ed Lee :Mardak
	Comment 7 • 10 years ago

Actually, looking at all the leak bugs MattN linked, they all seem to include SpdySession31. Perhaps there's a real leak with the Spdy implementation.

	Matthew N. [:MattN] Assignee
	Comment 8 • 10 years ago

(In reply to Ed Lee :Mardak from comment #7)
> Actually, looking at all the leak bugs MattN linked, they all seem to
> include SpdySession31. Perhaps there's a real leak with the Spdy
> implementation.

It's possible but I'm not sure you understand how this is supposed to work: My understanding of speculative connections is that the connection is opened and intentionally left open with the idea that it will be used shortly and the overhead of opening the connection is already done, thus making the action faster. Having the connection still open just means that it's doing its job and the reason I don't believe there is a real permanent leak is because the failures I saw were when the search bar was focused shortly before the test ended. When I put a delay before the test ends and checks for leaks, no leaks a reported, presumably because the pre-connection timed out.

	Matthew N. [:MattN] Assignee
	Comment 9 • 10 years ago

For example, this doesn't leak for me on OS X but it does without the patch: https://pastebin.mozilla.org/4767956

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 10 • 10 years ago

It may be that we just need to adjust the speculative connection implementation to play nicer with our leak detecting scripts.

	Matthew N. [:MattN] Assignee
	Comment 11 • 10 years ago

Right, that's another option which would allow us to test what we ship.

A downside is that it doesn't avoid opening network connections during these tests which I thought was bad. Perhaps it's fine when the tests don't break when the network resource isn't available like in this case?

I've put a patch in bug 992611 to disable the connections in all mochitests by default. The patch here was just handling search tests.

	Patrick McManus [:mcmanus]
	Comment 12 • 10 years ago

Comment on attachment 8402258 [details] [diff] [review]
v.1 Disable speculative connections for the search bar in tests

Review of attachment 8402258 [details] [diff] [review]:
-----------------------------------------------------------------

agree that 992611 is the way to go

	Matthew N. [:MattN] Assignee
	Comment 13 • 10 years ago

Comment on attachment 8402258 [details] [diff] [review]
v.1 Disable speculative connections for the search bar in tests

Fixed by disabling speculative connections in mochitests in bug 992611

	David Baron :dbaron:
	Comment 15 • 10 years ago

Perhaps these speculative connections should be cleaning themselves up at shutdown, possibly #ifdef NS_FREE_PERMANENT_DATA ?

	Patrick McManus [:mcmanus]
	Comment 16 • 10 years ago

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #15)
> Perhaps these speculative connections should be cleaning themselves up at
> shutdown, possibly #ifdef NS_FREE_PERMANENT_DATA ?

I think you're right.. they aren't aborting correctly at shutdown. Largely harmless - but not right. I'll file a separate bug.

the "fix" in here is also correct though - the tests aren't supposed to be accessing external sites and that's definitely what is happening.