Bug 633670 (LifetimeTesting)

Need testing support for leaks that do not persist through shutdown

NEW
Unassigned

Status

Testing
General
--
enhancement
7 years ago
a year ago

People

(Reporter: khuey, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

Lately we've taken more notice of "leaks", where we hold on to an object longer than we should (but not past shutdown).  I was thinking about ways to write tests for these kinds of bugs since our traditional tests won't catch these and came up with the following.

Provide two new global functions (on SpecialPowers?  as an xpcom object?):

registerObject: takes an object that supports weak references (which includes the big ones, documents and windows), stores a weak ref, and hands back some sort of identifier.
isObjectDead: takes the identifier mentioned above, looks up the weak reference, forces a GC/CC, and then sees if the object is still alive via the weakref.

This won't let us test everything, but it would get the big objects.  Jesse, would this help you fuzz for this kind of stuff?

Thoughts, comments?
Severity: trivial → enhancement

Comment 1

7 years ago
For regression tests, this sounds excellent.  It could be used to construct an automated test for bug 610956, for example.

I'd keep the GC/CC calls separate from isObjectDead.  It can be too much, if you're hoping to check isObjectDead for a bunch of objects at once, and want it to be fast.  It can be too little, if collecting an object requires multiple CCs or a return to the event loop between CCs (e.g. bug 610166).

You might want to provide an async GC/CC API to guarantee that conservative stack scanning won't incorrectly keep things alive.

For fuzzing, I'm not sure this would be useful.  With all the crazy stuff going on, there's no obvious point at which a given {frame, tab, browser window, page in session history} "should be dead".  I can close all browser windows (or navigate away from the main fuzz page with bfcache disabled? - see bug 630072 comment 53), but then I'm happier counting the remaining windows and documents than asking whether specific windows and documents are alive.
This sounds very similar in philosophy to David Baron's (I think) ExplainLiveExpectedGarbage for the CC.  I'm not sure exactly how that works, or how effective he found it to be.

Updated

6 years ago
Blocks: 640791
Blocks: 663416

Updated

6 years ago
Whiteboard: [MemShrink:P1]

Updated

6 years ago
Assignee: nobody → khuey

Comment 3

6 years ago
> You might want to provide an async GC/CC API to guarantee that conservative
> stack scanning won't incorrectly keep things alive.

jdm added such an API in bug 661927 :)
Depends on: 661927

Updated

6 years ago
Alias: LifetimeTesting

Updated

6 years ago
Blocks: 658738
What's the status of this?
I have a patch in my queue but ran into a bug that was hard to track down and then got whisked away to other things.  I'll see if I can figure it out soon.
Created attachment 556571 [details] [diff] [review]
Patch

Heh, and with a few weeks distance it was pretty easy to find the bug.
Attachment #556571 - Flags: review?(dbaron)
Comment on attachment 556571 [details] [diff] [review]
Patch

>+already_AddRefed<mozILifetimeTester>
>+GetLifetimeTester()
>+{
>+  if (!sLifetimeTester) {
>+    // The service manager will hold us alive until shutdown.
>+    sLifetimeTester = new mozLifetimeTester();
>+    bool inited = sLifetimeTester->Init();
>+    NS_ENSURE_TRUE(inited, nsnull);
>+  }

I'm not a fan of this function, for a few reasons:
 - it holds the object without adding a reference until near the end
 - it has an unnecessary QI
 - if Init() ever does fail, this'll return null the first time and
   then non-null the rest (since you'll never actually call the destructor,
   because of the first problem)

Do you really need a moz prefix on a class inside mozilla::testing::?
That's not the convention we've been using.  (And I'm not crazy about
moz prefixes in general; I'd rather just stick with ns.)

Also, could you make ~mozLifetimeTester() explicitly private rather
than relying on the default?

In RegisterObject, why is it an error to register the same object
twice.  That seems like unnecessary pain -- why not just return the
ID?

Using the pointer as the ID doesn't seem valid -- it's subject to
pointer replay (though if you hit it you'll hit the NS_ERROR_UNEXPECTED
you currently have, but I think that's a bad idea -- could still lead to
random test failures).  I think you need to generate and track your own
IDs.  (In that case, RegisterObject could just always give out a new ID
even if the object is already registered.) If you do that you could even
use an array rather than a hashtable.

In the IDL, don't have an empty comment.  Either have none or say
something.  Preferably say something useful.



You should ask bsmedberg, as xpcom owner, if he's ok with the file
location and the mozilla::testing:: namespace, at the very least.

review- because of the pointer replay issues
Attachment #556571 - Flags: review?(dbaron) → review-
Blocks: 683953
Whiteboard: [MemShrink:P1] → [MemShrink:P2]
Summary: Need testing support for "leaks" → Need testing support for leaks

Updated

6 years ago
Summary: Need testing support for leaks → Need testing support for leaks that do not persist through shutdown

Updated

4 years ago
Blocks: 938691
Assignee: khuey → nobody
You need to log in before you can comment on or make changes to this bug.