65150 - leaking nsPrintOptions

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

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

done's checkin on 2001-11-01 14:14 or 14:17 caused an nsPrintOptions object to
show up on the tinderbox leak stats.

Comment 1

[Peter "jag" Annema]

Attachment: [patch] fix leak and do some cleaning up

Comment 2

[dcone (gone)]

I will fix my bug, but I will leave all the rest of the cleanup to the module 
owner of nsAppRunner.cpp.

Comment 3

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

Hmmm.   Registering services in nsViewerApp / nsAppRunner seems like the
wrong thing to do.  Why isn't being listed in CreateInstance/components in
gfx/src/{mac,windows,gtk}/nsGfxFactory{Mac,Windows,GTK}.cpp enough?  Isn't
what's enough for CreateInstance enough for GetService?  See
http://lxr.mozilla.org/mozilla/source/xpcom/doc/xpcom-brownbag-1.html
http://lxr.mozilla.org/mozilla/source/xpcom/doc/xpcom-code-faq.html#ComponentManager%20Vs%20ServiceManager

Comment 4

[Peter "jag" Annema]

dcone: no problem :-)

Comment 5

[dcone (gone)]

Attachment: Patch to fix leak.

Comment 6

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

Could you explain why you need this RegisterService call at all.  I don't see
why.  Taking it out of nsAppRunner/nsViewerApp would also fix the leak.  (And do
you have the same leak in both places?)

Comment 7

[dcone (gone)]

David, because we wanted this to be a singleton accessible from more than just 
GFX.
I read the documents you pointed at and I believe we fulfill those requirments.

Comment 8

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

I don't think you need to call RegisterService to get that to happen.  You
shouldn't need to change nsAppRunner to make that kind of change.  The
ServiceManager will call CreateInstance for you.  I think you only need to use
RegisterService if:
 * the ServiceManager can't call CreateInstance
 * you want to override the object that would be created by CreateInstance

Comment 9

[dcone (gone)]

Ok.. that would be great actually.. let me do some research and I will get it to 
the right thing..

Comment 10

[dcone (gone)]

Attachment: Patch to fix leak..

Comment 11

[dcone (gone)]

The above patch fixes the leak by taking the RegisterService of the 
nsIPrintOptions out.  I confirmed that a singleton will still be constructed.
Thanks for the help Dave..

Comment 12

[buster]

sr=buster

Comment 13

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

r=dbaron on that, although you should also do the same for viewer

Comment 14

[dcone (gone)]

I did not start the service for viewer.. so nothing needs to be done there.

Comment 15

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

Um...ok, I guess I'll just let viewer be weird since it looks weird already...

jag:  Are you going to file another bug on that cleanup?

Comment 16

[Peter "jag" Annema]

Yeah, bug 65660.

Comment 17

[dcone (gone)]

Checked in the fix.

Comment 18

[shrirang khanzode]

verified tha tthe patch went in in version 1.256 in lxr.