1408552 - Permaleak of CacheIOThread in dom/base mochitests (still)

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P2)

Description

[Andrew McCreight [:mccr8]]

This leak still exists. Bug 1404486 fixed the case of running the test by itself, but I'm still seeing the leak in dom/base. I'm assuming it is still related to this particular cache test but I haven't investigated in depth yet. On windows, it is in Mochitest-e10s 1.

+++ This bug was initially created as a clone of Bug #1404486 +++

This is a permaleak, somewhere in the dom/base directory, of what looks like a networking cache:

TEST-INFO | leakcheck | tab process: leaked 1 CacheFileHandles
TEST-INFO | leakcheck | tab process: leaked 1 CacheFileIOManager
TEST-INFO | leakcheck | tab process: leaked 1 CacheIOThread
TEST-INFO | leakcheck | tab process: leaked 2 CondVar
TEST-INFO | leakcheck | tab process: leaked 2 Mutex
TEST-INFO | leakcheck | tab process: leaked 1 ThreadEventTarget
TEST-INFO | leakcheck | tab process: leaked 1 ThreadTargetSink
TEST-INFO | leakcheck | tab process: leaked 17 nsTArray_base
TEST-INFO | leakcheck | tab process: leaked 1 nsThread

This is present at least on Linux and Linux64, but it might also be on other platforms. I'll file this in the cache component, but it might be that some DOM thing is causing the leak.

Comment 1

[Andrew McCreight [:mccr8]]

I'm going to move this to the Networking: Cache component, because the leak involves the cache, though it is a DOM test of a kind of JS engine feature, and probably the fault of the way the test is written so maybe that's not quite fair...

Comment 2

[Valentin Gosu [:valentin] (he/him)]

I see a reference the the cache storage - I don't know how relevant this is:
http://searchfox.org/mozilla-central/rev/01970ed92d74f82d4e94a1e4365892bbcc593889/dom/base/test/test_bug482935.html#14

Setting ni? to check this.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

I checked that removing the code to clear cache gets rid of the memory leak.
Honza, any idea if that code is really necessary (the test passes without it), or if there is a better way of achieving the same thing?

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #3)
> I checked that removing the code to clear cache gets rid of the memory leak.
> Honza, any idea if that code is really necessary (the test passes without
> it), or if there is a better way of achieving the same thing?

OK.  That's really not the question.  There is a leak we should find and fix.  This MUST work w/o a leak.  Only if it would become too complicated to find or fix the leak I would change the test.

Comment 5

[Andrew McCreight [:mccr8]]

This leak is 1264 bytes on Linux64, 1336 bytes on OSX, 1320 bytes on Win64, 812 bytes on Linux32, 800 bytes on Win32.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> (In reply to Valentin Gosu [:valentin] from comment #3)
> > I checked that removing the code to clear cache gets rid of the memory leak.
> > Honza, any idea if that code is really necessary (the test passes without
> > it), or if there is a better way of achieving the same thing?
> 
> OK.  That's really not the question.  There is a leak we should find and
> fix.  This MUST work w/o a leak.  Only if it would become too complicated to
> find or fix the leak I would change the test.

I just realized - the test is running in the child process. So the cache storage service is being instantiated in the child process and the leaks happen in the child process.
I'm not sure if the child process gets the 'xpcom-shutdown' notification when it's shutdown, but clearly the CacheStorageService is not really designed to run there.

I think we can either remove the code that calls clear (since it appears the test still passes without it), or make it so that it's only called when run in the main process. Maybe we should also add an assertion in the CacheStorageService that it's instantiated in the main process?

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Valentin Gosu [:valentin] from comment #6)
> I'm not sure if the child process gets the 'xpcom-shutdown' notification
> when it's shutdown, but clearly the CacheStorageService is not really
> designed to run there.

xpcom-shutdown is something that the child process gets. It doesn't get any of the profile-related notifications.

> Maybe we should also add an assertion in the
> CacheStorageService that it's instantiated in the main process?

That sounds like a good idea.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1408552 - Make sure we only instantiate CacheStorageService in the main process

- A memory leak occurs when this happens in the content process
- I added an assertion that we only create it in the parent process

Review commit: https://reviewboard.mozilla.org/r/189878/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189878/

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8918994 [details]
Bug 1408552 - Make sure we only instantiate CacheStorageService in the main process

https://reviewboard.mozilla.org/r/189878/#review195014

::: dom/base/test/test_bug482935.html:16
(Diff revision 1)
>  var url = "bug482935.sjs";
>  
>  function clearCache() {
> +    if (SpecialPowers.isMainProcess()) {
> -    SpecialPowers.Cc["@mozilla.org/netwerk/cache-storage-service;1"].
> +      SpecialPowers.Cc["@mozilla.org/netwerk/cache-storage-service;1"].
> -               getService(SpecialPowers.Ci.nsICacheStorageService).
> +                    getService(SpecialPowers.Ci.nsICacheStorageService);

and the .clear() call?  if removed, then why to keep this code at all?

::: netwerk/cache2/CacheStorageService.cpp:124
(Diff revision 1)
>  , mDiskPool(MemoryPool::DISK)
>  , mMemoryPool(MemoryPool::MEMORY)
>  {
>    CacheFileIOManager::Init();
>  
> +  MOZ_ASSERT(XRE_IsParentProcess());

try run please :)))

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8918994 [details]
Bug 1408552 - Make sure we only instantiate CacheStorageService in the main process

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189878/diff/1-2/

Comment 11

[Valentin Gosu [:valentin] (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #9)
> > -               getService(SpecialPowers.Ci.nsICacheStorageService).
> > +                    getService(SpecialPowers.Ci.nsICacheStorageService);
>
> and the .clear() call?  if removed, then why to keep this code at all?

Fixed it.

> ::: netwerk/cache2/CacheStorageService.cpp:124
> > +  MOZ_ASSERT(XRE_IsParentProcess());
> 
> try run please :)))

There were a couple of instances of xpcshell-tests that crashed. I fixed them.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8918994 [details]
Bug 1408552 - Make sure we only instantiate CacheStorageService in the main process

dropping r? then

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8918994 [details]
Bug 1408552 - Make sure we only instantiate CacheStorageService in the main process

https://reviewboard.mozilla.org/r/189878/#review195448

::: netwerk/test/unit/test_synthesized_response.js:26
(Diff revision 2)
> +    let appInfo = Cc["@mozilla.org/xre/app-info;1"];
> +    return (!appInfo || appInfo.getService(Ci.nsIXULRuntime).processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT);
> +}
> +
> +if (isParentProcess()) {
> -// ensure the cache service is prepped when running the test
> +  // ensure the cache service is prepped when running the test

would be good to know why this is needed...

::: netwerk/test/unit_ipc/test_alt-data_simple_wrap.js:4
(Diff revision 2)
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +// needs to be rooted
> +var cacheFlushObserver = cacheFlushObserver = { observe: function() {

assigning twice?

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8918994 [details]
Bug 1408552 - Make sure we only instantiate CacheStorageService in the main process

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189878/diff/2-3/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dfb238189002
Make sure we only instantiate CacheStorageService in the main process r=mayhemer

Comment 16

[Andrew McCreight [:mccr8]]

Thanks for the fix!

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for asserting in xpcshell's netwerk/test/unit_ipc/test_cache-entry-id_wrap.js:

https://hg.mozilla.org/integration/autoland/rev/6e97f881e41137ce44ccb7e7b97965a54e64e561

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e14d982a968e0c610850211e3bdd8358d1a61400&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=137872991&repo=autoland

[task 2017-10-18T13:30:40.489Z] 13:30:40     INFO -  TEST-PASS | netwerk/test/unit_ipc/test_cache-entry-id_wrap.js | check - [check : 68] "response body" == "response body"
[task 2017-10-18T13:30:40.490Z] 13:30:40     INFO -  TEST-PASS | netwerk/test/unit_ipc/test_cache-entry-id_wrap.js | check - [check : 69] "" == ""
[task 2017-10-18T13:30:40.491Z] 13:30:40     INFO -  TEST-PASS | netwerk/test/unit_ipc/test_cache-entry-id_wrap.js | check - [check : 70] false == false
[task 2017-10-18T13:30:40.491Z] 13:30:40     INFO -  TEST-PASS | netwerk/test/unit_ipc/test_cache-entry-id_wrap.js | check - [check : 71] true == true
[task 2017-10-18T13:30:40.492Z] 13:30:40     INFO -  PID 12265 | Assertion failure: XRE_IsParentProcess(), at /builds/worker/workspace/build/src/netwerk/cache2/CacheStorageService.cpp:125
[task 2017-10-18T13:31:00.917Z] 13:31:00     INFO -  PID 12265 | #01: CacheStorageServiceConstructor [mfbt/RefPtr.h:54]
[task 2017-10-18T13:31:00.919Z] 13:31:00     INFO -  PID 12265 | #02: nsComponentManagerImpl::CreateInstance [xpcom/components/nsComponentManager.cpp:1004]
[task 2017-10-18T13:31:00.920Z] 13:31:00     INFO -  PID 12265 | #03: nsComponentManagerImpl::GetService [xpcom/components/nsComponentManager.cpp:1246]
[task 2017-10-18T13:31:00.921Z] 13:31:00     INFO -  PID 12265 | #04: nsJSCID::GetService [js/xpconnect/src/XPCJSID.cpp:699]
[task 2017-10-18T13:31:00.922Z] 13:31:00     INFO -  PID 12265 | #05: NS_InvokeByIndex
[task 2017-10-18T13:31:00.923Z] 13:31:00     INFO -  PID 12265 | #06: CallMethodHelper::Call [js/xpconnect/src/xpcprivate.h:775]
[task 2017-10-18T13:31:00.924Z] 13:31:00     INFO -  PID 12265 | #07: XPCWrappedNative::CallMethod [js/xpconnect/src/XPCWrappedNative.cpp:1282]
[task 2017-10-18T13:31:00.925Z] 13:31:00     INFO -  PID 12265 | #08: XPC_WN_CallMethod [js/xpconnect/src/XPCWrappedNativeJSOps.cpp:929]
[task 2017-10-18T13:31:00.926Z] 13:31:00     INFO -  PID 12265 | #09: js::CallJSNative [js/src/jscntxtinlines.h:292]
[task 2017-10-18T13:31:00.927Z] 13:31:00     INFO -  PID 12265 | #10: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:459]
[task 2017-10-18T13:31:00.928Z] 13:31:00     INFO -  PID 12265 | #11: InternalCall [js/src/vm/Interpreter.cpp:523]
[task 2017-10-18T13:31:00.929Z] 13:31:00     INFO -  PID 12265 | #12: Interpret [js/src/vm/Interpreter.cpp:3067]
[task 2017-10-18T13:31:00.930Z] 13:31:00     INFO -  PID 12265 | #13: js::RunScript [js/src/vm/Interpreter.cpp:423]
[task 2017-10-18T13:31:00.931Z] 13:31:00     INFO -  PID 12265 | #14: js::InternalCallOrConstruct [js/src/vm/Interpreter.h:205]
[task 2017-10-18T13:31:00.932Z] 13:31:00     INFO -  PID 12265 | #15: InternalCall [js/src/vm/Interpreter.cpp:523]
[task 2017-10-18T13:31:00.933Z] 13:31:00     INFO -  PID 12265 | #16: js::Call [js/src/vm/Interpreter.cpp:541]
[task 2017-10-18T13:31:00.934Z] 13:31:00     INFO -  PID 12265 | #17: js::fun_call [js/public/RootingAPI.h:835]
[task 2017-10-18T13:31:00.935Z] 13:31:00     INFO -  PID 12265 | #18: js::fun_apply [js/src/jsfun.cpp:1262]
[task 2017-10-18T13:31:00.936Z] 13:31:00     INFO -  PID 12265 | #19: js::CallJSNative [js/src/jscntxtinlines.h:292]
[task 2017-10-18T13:31:00.937Z] 13:31:00     INFO -  PID 12265 | #20: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:459]
[task 2017-10-18T13:31:00.938Z] 13:31:00     INFO -  PID 12265 | #21: InternalCall [js/src/vm/Interpreter.cpp:523]
[task 2017-10-18T13:31:00.939Z] 13:31:00     INFO -  PID 12265 | #22: js::jit::DoCallFallback [js/src/jit/BaselineIC.cpp:2534]
[task 2017-10-18T13:31:00.940Z] 13:31:00     INFO -  PID 12265 | #23: ??? (???:???)
[task 2017-10-18T13:31:00.941Z] 13:31:00     INFO -  (xpcshell/head.js) | test finished (1)
[task 2017-10-18T13:31:00.942Z] 13:31:00     INFO -  exiting test
[task 2017-10-18T13:31:00.943Z] 13:31:00     INFO -  PID 12265 | JavaScript error: resource://gre/modules/osfile/ospath_unix.jsm, line 90: TypeError: invalid path component
[task 2017-10-18T13:31:00.944Z] 13:31:00     INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "TypeError: invalid path component" {file: "resource://gre/modules/osfile/ospath_unix.jsm" line: 90}]"
[task 2017-10-18T13:31:00.945Z] 13:31:00  WARNING -  TEST-UNEXPECTED-FAIL | netwerk/test/unit_ipc/test_cache-entry-id_wrap.js | assertNoUncaughtRejections - [assertNoUncaughtRejections : 265] A promise chain failed to handle a rejection: invalid path component - stack: _do_main@/builds/worker/workspace/build/tests/xpcshell/head.js:221:3
[task 2017-10-18T13:31:00.946Z] 13:31:00     INFO -  _execute_test@/builds/worker/workspace/build/tests/xpcshell/head.js:544:5
[task 2017-10-18T13:31:00.947Z] 13:31:00     INFO -  @-e:1:1
[task 2017-10-18T13:31:00.948Z] 13:31:00     INFO -  Rejection date: Wed Oct 18 2017 13:30:40 GMT+0000 (UTC) - false == true
[task 2017-10-18T13:31:00.949Z] 13:31:00     INFO -  resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:265
[task 2017-10-18T13:31:00.950Z] 13:31:00     INFO -  /builds/worker/workspace/build/tests/xpcshell/head.js:_execute_test:545
[task 2017-10-18T13:31:00.951Z] 13:31:00     INFO -  -e:null:1
[task 2017-10-18T13:31:00.952Z] 13:31:00     INFO -  exiting test

Comment 18

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8918994 [details]
Bug 1408552 - Make sure we only instantiate CacheStorageService in the main process

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189878/diff/3-4/

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Pushed to autoland again. Let's it sticks this time.
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bf32a14dfacff945c3dbc516525c497663f9e467

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/bf32a14dfacf