129794 - leaks up on 3/8/02 8pm build

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect)

Description

[John Bandhauer]

Leaks on the bradtinderbox machine jumped from 4.84k to 5.75k.
This looks like it is from the fix to bug 104127.

--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
LiteralImpl                                      16          -
nsSupportsArray                                 104          -
RDFContainerImpl                                 16          -
RDF_Assertion                                    84          -
nsWindowCreator                                   8          -
RDFServiceImpl                                  208          -
InMemoryDataSource                                4          -
RDFContainerUtilsImpl                             8          -
nsGenericFactory                                 16          -
nsRDFResource                                   160          -
nsWindowMediator                                 52          -
nsWindowWatcher                                  36          -
nsVoidArray                                      40      66.67%
TOTAL                                           752

Comment 1

[Robert John Churchill]

Bummer.

Comment 2

[Robert John Churchill]

Attachment: Initially null out-param

Test

Comment 3

[Robert John Churchill]

Attachment: Break circular memory references

Now that the window mediator adds itself as an observer into its mInner, we
need to be able to break that cycle appropriately

Comment 4

[Robert John Churchill]

Verbal r=dp

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 73398 [details] [diff] [review]
Break circular memory references

sr=[e-mail] via irc earlier...

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 73398 [details] [diff] [review]
Break circular memory references

a=roc+moz

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bug 130271 has stacks showing why the above patch was backed out (although it
was backed out before the bug was filed, due to tinderbox orange).

I see two problems with the patch above:

 1) It says --gRefCnt instead of --mRefCnt.  (However, if this object is
supposed to be used as a *service*, which it seems to be judging by the stack in
bug 130271, I wonder why it uses global variables instead of members.) 
Presumably there is more than one instance created, though

 2) There's no refcount stabilization for the |mRefCnt == 0| case, which should
probably be written as:
  mRefCnt = 1;
  delete this;
  return 0;
(RDFServiceImpl::UnregisterDataSource doesn't currently do anything that causes
its argument to be refcounted, but that could change in the future.)

I don't *think* the |mRefCnt == 1| case needs any changes, though.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I just noticed that the crash only started after #1 was fixed, so I don't think
either of those was the cause of the crash.

Comment 9

[Robert John Churchill]

Attachment: Break circular memory references part deux

The gRefCnt vs mRefCnt typo was covering up a second circular reference from
the 104127 checkin. Tweaking the typo last night caused ::Release() to fire
early, clearing mInner to null which caused the crash.

This patch takes both circular references from 104127 into account. Ran with
patch on Mac, Linux and Windows with no problems... and verified on Linux that
mRefCnt stabilizes to 0 at shutdown.

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 73792 [details] [diff] [review]
Break circular memory references part deux

sr=[e-mail]

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 73792 [details] [diff] [review]
Break circular memory references part deux

>+  NS_PRECONDITION(target != nsnull, "null ptr");
>+  if (! target)
>+    return(NS_ERROR_NULL_POINTER);
>+  *target = nsnull;

I don't care much for NS_PRECONDITION.	And I don't
care much for that sort of NULL check.	And I've never
really liked comparing pointers to nsnull explicitly.

But I can overlook all of that, as long as you don't mix
|target != nsnull| and |!target| on consecutive lines!

Pick one (guided by dominant style in that file, of course),
and you're golden.  Stamping r=shaver, because I know you
won't let me down.

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 73792 [details] [diff] [review]
Break circular memory references part deux

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 13

[Robert John Churchill]

OK, leak #s on tinderbox are back down to where they were.  Marking FIXED, baby!

Comment 14

[Robert John Churchill]

*** Bug 130234 has been marked as a duplicate of this bug. ***