43591 - nsBasicDecoderSupport held past XPCOM shutdown in nsFSStringConversion

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

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

An nsCOMPtr<nsIUnicodeDecoder> pointing to an nsBasicDecoderSupport is held 
past XPCOM shutdown by the global variable "nsFSStringConversion gConverter" 
defined at:
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileCommon.cpp#124
See bug 43561 for more info.

Comment 1

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

This object holds on to an nsUnicodeDecodeHelper too.

Comment 2

[Ray Whitmer]

My first guess is that this is a string issue that scc would handle better than
I.  Reassigning to scc.

Comment 3

[Scott Collins]

Un-related to string stuff, Ray.  This is just an ordinary case of someone 
keeping an owning reference in a static global, which delays the referents 
destruction beyond the point where needed resources are already gone.  You need 
to figure out a way to shorten its lifetime.

Comment 4

[Ray Whitmer]

I can create a listener to shorten the lifecycle, if everyone is willing to live 
without conversion services in the I/O subsystem after xpcom shuts down.

Is there a document describing which services should be available when?

This seems to be part of the bigger question of whether string conversion 
services should be tied to the initialization state of XPCOM.

Scott, any opinions?  Am I owner of file I/O and string conversion services, so 
I should fix it?

Comment 5

[Ray Whitmer]

I think this has been mistagged as a memory leak.  It is one case where XPCOM 
file I/O does not release all resources during shutdown, and it is not clear 
that it should.  This may get cleared up if we can get time to better-describe 
XPCOM lifecycle and relationship to conversion facilities.  Marking futute.

Comment 6

[Scott Collins]

I think we are willing to live without any but the basic conversions (between 
UTF8, ASCII, and UCS2 or whatever our double-byte format is) which I will ensure 
live outside the range of XPCOM initialization.

Comment 7

[Ray Whitmer]

Then we will have no conversion between native filesystem names and unicode.

Comment 8

[Judson Valeski]

can't we make this a static raw ptr and let some service w/ scope release it 
when it goes away?

Comment 9

[Ray Whitmer]

Why is it a static global variable if it is owned by some other object with 
scope which comes and goes?  We seem to lack an object model and discipline to 
do it this way.  I do not think spaghetti interdependencies help us.

If it is a static variable that must be managed by the scope of some singleton 
object, it should be in the scope of that singleton object.

Comment 10

[Warren Harris]

Edward: Welcome to xpcom!

Comment 11

[Warren Harris]

Once again... attempting to reassign from Ray to Edward.

Comment 12

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

Attachment: an ugly fix, in the hopes someone will tell me a better way

Comment 13

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

Attachment: fix that avoids static construction

Comment 14

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

It seems the above patch also fixes bug 49575 (probably because OpenBSD has
problems with static constructors).

Comment 15

[Doug Turner (:dougt)]

nice work.  David, you should add an NS_StartUpLocalFile that basically noops to
compilment the NS_ShutdownLocalFile.  or you could also allocate this class in
the heap in this startup method...

Fix this and you have an r=dougt

Comment 16

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

dougt:  Where in NS_InitXPCOM2 would you want NS_StartupLocalFile to be called?
 Without knowing what it would do, I'm not sure where to put it.

(While I'm here, assigning the bug to myself since I'm working on a fix.)

Comment 17

[Doug Turner (:dougt)]

Put it as early as possible; after the nsMemoryImpl::Startup would be perfect. 
It would be for system initialization.  For example, I would like to factor out 
(on windows) the CoInitialize and CoUninitialize.  

Comment 18

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

Attachment: revised patch

Comment 19

[Doug Turner (:dougt)]

r=dougt.  

--> SCC <-- can you please approve this change?

Comment 20

[Doug Turner (:dougt)]

Chris could you approved this leak fix?  

Comment 21

[Chris Waterson]

sr=waterson

Comment 22

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

Fix checked in 2000-12-08 20:11 PST.

Comment 23

[John Bandhauer]

Your fix broke XPCOM_STANDALONE. I'll attach a patch. If I can get a quick r=
and sr= I'll check it in and reclose this bug.

Comment 24

[John Bandhauer]

Attachment: a patch to un-bust XPCOM_STANDALONE

Comment 25

[Brendan Eich [:brendan]]

r or sr=[e-mail] on jband's XPCOM_STANDALONE patch.

/be

Comment 26

[Doug Turner (:dougt)]

duh!  sorry about that jband.  thanks for the quick patch.

Comment 27

[John Bandhauer]

dougt: turn that 'duh!' into an r= and I'll check it in when the tree clears.

Comment 28

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

r=dbaron  Sorry about that.

Comment 29

[John Bandhauer]

No problem. Thanks for the quick reviews. Fix to XPCOM_STANDALONE checked in.

Comment 30

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

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