133383 - nsMetaCharsetObserver destructor shouldn't call End

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[timeless]

Something tries to create a parser service on shutdown.  I filed a bug to convert parser service from CID to ContractID so others could see, and I guessed it was intl, it was.

Law wrote that destructors shouldn't call remove observer. Here's a patch, I'll test it from home (my build broke and i'm out of time).

Comment 1

[timeless]

Attachment: patch

Comment 2

[timeless]

requires patch from other bug (although a reworked version wouldn't)

Comment 3

[Rui Xu]

Should IQA be involved in this bug? 

Comment 4

[timeless]

not really, this is a code only bug.

tao/ftang: comments?

Comment 5

[timeless]

Attachment: compiling patch

Comment 6

[Bill Law]

For some reason (because I had spoken out on a similar issue long ago), timeless
spoke to me about this issue.

After careful analysis, we've come to the conclusion that:

1. The implementations of both nsIObserverService and nsIParserService ensure
that they "remove" any still registered observers when they are destroyed
(typically, at Mozilla shutdown).

2. Therefore, it is not necessary for any object to do that unregistering at
Mozilla shutdown, since it happens anyway.

3. It is more wrong to do it in the observer's destructor, since by definition
said observer would have to have already been unregistered.

Bottom line is that I endorse the general strategy of removing the observer
removal code from the destructor (which timeless's patch does).  I have not
fully reviewed his patch, however (although I suspect he might ask me to do that
soon :-).

Comment 7

[Roy Yokoyama]

changing qa contact to me:)

Comment 8

[timeless]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 76163 [details] [diff] [review]
compiling patch

Bill is not likely to actually review it; maybe try someone from intl?

Comment 10

[Alec Flett]

Comment on attachment 76163 [details] [diff] [review]
compiling patch

r=alecf

Comment 11

[timeless]

checked in