test_parent.js leaks objects

RESOLVED FIXED

Status

()

Core
Networking: Cookies
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jdm, Assigned: dwitte@gmail.com)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [xpcshell-leak])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Created attachment 484756 [details]
Parent and child leaks
(Assignee)

Comment 1

8 years ago
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?
(Reporter)

Comment 2

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

Comment 3

8 years ago
Created attachment 484776 [details] [diff] [review]
Remove message listeners to avoid leaks.
(Reporter)

Comment 4

8 years ago
Try says that test_parent times out with this patch :(

Comment 5

8 years ago
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.
(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

8 years ago
(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?
(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.
(Assignee)

Comment 9

8 years ago
ContentParent is leaking the permission manager. Patch coming up.
(Assignee)

Comment 10

8 years ago
Created attachment 484887 [details] [diff] [review]
fix

I bet this fixes it.
Attachment #484887 - Flags: review?(azakai)
(Assignee)

Comment 11

8 years ago
(Alon, want to test it?)
Created attachment 484914 [details]
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...

Updated

8 years ago
Attachment #484914 - Attachment mime type: application/octet-stream → text/plain
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?
(Reporter)

Comment 14

8 years ago
The PObjectWrapperChild leak is a known issue being tracked in bug 599713.
(Reporter)

Updated

8 years ago
Blocks: 606067
No longer blocks: 606067
Depends on: 469523
(Assignee)

Comment 15

8 years ago
(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.)
(Assignee)

Comment 16

8 years ago
Created attachment 485056 [details] [diff] [review]
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.
Assignee: nobody → dwitte
Status: NEW → ASSIGNED
Attachment #485056 - Flags: review?(azakai)
(Reporter)

Comment 17

8 years ago
Comment on attachment 484887 [details] [diff] [review]
fix

Boom. Headshot.
Attachment #484887 - Flags: review?(azakai) → review+
(Reporter)

Comment 18

8 years ago
Comment on attachment 485056 [details] [diff] [review]
second fix

DOUBLE HEADSHOT
Attachment #485056 - Flags: review?(azakai) → review+
(Reporter)

Updated

8 years ago
Attachment #484776 - Attachment is obsolete: true

Updated

8 years ago
Attachment #484887 - Flags: approval2.0+

Updated

8 years ago
Attachment #485056 - Flags: approval2.0+
(Assignee)

Comment 19

8 years ago
http://hg.mozilla.org/mozilla-central/rev/e674806e6889
http://hg.mozilla.org/mozilla-central/rev/0434c3cf6279
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.