605908 - test_parent.js leaks objects

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Josh Matthews [:jdm]]

Attachment: Parent and child leaks

Comment 1

[[e-mail]]

Looks like we're leaking an entire nsPermissionManager. An observer's probably holding on to the whole thing and never releasing it.

Probably a result of Alon's work. Any ideas?

Comment 2

[Josh Matthews [:jdm]]

I have a patch I can't test (no ipc tests on mac).

Comment 3

[Josh Matthews [:jdm]]

Attachment: Remove message listeners to avoid leaks.

Comment 4

[Josh Matthews [:jdm]]

Try says that test_parent times out with this patch :(

Comment 5

[Alon Zakai (:azakai)]

Since

    const messages = ["TESTING:Stage2", "TESTING:Stage3"];

then

    for (let message in messages)
      mM.addMessageListener(message, messageListener);

should be

    messages.forEach(function(message) {
      mM.addMessageListener(message, messageListener);
    });

(the JavaScript for-in operator claims another victim).

But, this still doesn't fix the leak, hmm. Do we even care about such leaks - are xpcshell tests supposed to not leak? This doesn't show up as an orange anywhere that I can see.

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> But, this still doesn't fix the leak, hmm. Do we even care about such leaks -
> are xpcshell tests supposed to not leak? This doesn't show up as an orange
> anywhere that I can see.
We do care, but don't presently have a way to go orange when one test starts leaking (it's an all or nothing thing right now, which is sucky).

Comment 7

[Alon Zakai (:azakai)]

(In reply to comment #6)
> We do care, but don't presently have a way to go orange when one test starts
> leaking (it's an all or nothing thing right now, which is sucky).

Hmm, isn't that the same situation as we have in mochitests? But there we do report leaks for some reason?

Anyhow, the leak happens here even with

    var pm = Cc["@mozilla.org/permissionmanager;1"].getService(Ci.nsIPermissionManager);
    pm = null;

(and not using pm later). So it isn't the fault of the script. Must be something with nsPermissionManager::GetSingleton(). Is there a difference in how xpcshell tests destroy singletons for services, compared to running firefox normally? Does cycle collection etc. happen in xpcshell tests?

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> Hmm, isn't that the same situation as we have in mochitests? But there we do
> report leaks for some reason?
xpcshell runs each test in a new process, whereas mochitest is all the same.

> Anyhow, the leak happens here even with
> 
>     var pm =
> Cc["@mozilla.org/permissionmanager;1"].getService(Ci.nsIPermissionManager);
>     pm = null;
> 
> (and not using pm later). So it isn't the fault of the script. Must be
> something with nsPermissionManager::GetSingleton(). Is there a difference in
> how xpcshell tests destroy singletons for services, compared to running firefox
> normally? Does cycle collection etc. happen in xpcshell tests?
That's...neat.  dwitte owns that code; perhaps he has some insight here.

Comment 9

[[e-mail]]

ContentParent is leaking the permission manager. Patch coming up.

Comment 10

[[e-mail]]

Attachment: fix

I bet this fixes it.

Comment 11

[[e-mail]]

(Alon, want to test it?)

Comment 12

[Alon Zakai (:azakai)]

Attachment: current leaks

The fix gets us most of the way, but some leakiness still remains, see attachment

Note that leaking PObjectWrapperChild happens even if removing all calls to nsPermissionManager in the tests - so perhaps a separate issue?

Not sure how we could be leaking an nsPermission. Somewhere in the IPC conversion code? Looks ok to me though...

Comment 13

[Alon Zakai (:azakai)]

Another thing, the code that is fixed in |fix| was written based off of

  http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.h#290

- so is that code in error as well?

Comment 14

[Josh Matthews [:jdm]]

The PObjectWrapperChild leak is a known issue being tracked in bug 599713.

Comment 15

[[e-mail]]

(In reply to comment #12)
>Not sure how we could be leaking an nsPermission. Somewhere in the IPC
>conversion code? Looks ok to me though...

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#229
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#238

Both of those will leak -- the methods return an addrefed pointer, which you're never releasing.

(In reply to comment #13)
> - so is that code in error as well?

Nope. In the places we use it, we assign it into an already_AddRefed helper, which makes the nsCOMPtr not addref it again. (But it will release it.)

Comment 16

[[e-mail]]

Attachment: second fix

I should've caught these in review. Sorry :(

I checked most of the other patches you've committed to m-c, and couldn't find any other leaky code, so I think we're OK.

Comment 17

[Josh Matthews [:jdm]]

Comment on attachment 484887 [details] [diff] [review]
fix

Boom. Headshot.

Comment 18

[Josh Matthews [:jdm]]

Comment on attachment 485056 [details] [diff] [review]
second fix

DOUBLE HEADSHOT

Comment 19

[[e-mail]]

http://hg.mozilla.org/mozilla-central/rev/e674806e6889
http://hg.mozilla.org/mozilla-central/rev/0434c3cf6279