295145 - nsUCSupport releases wrong pointer

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

569 NS_IMETHODIMP nsEncoderSupport::SetOutputErrorBehavior(
570                                 PRInt32 aBehavior, 
571                                 nsIUnicharEncoder * aEncoder, 
572                                 PRUnichar aChar)
[...]
577   NS_IF_RELEASE(aEncoder);
578   mErrEncoder = aEncoder;
579   NS_IF_ADDREF(aEncoder);

the release should probably refer to mErrEncoder.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

and it looks like other implementations of this functions get this wrong as well...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Biesi, is this code not used or something?  How come it't not crashing later
when using mErrEncoder?

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

I assume few, if any, callers set an aEncoder, yes.

Comment 4

[Frank Wein [:mcsmurf]]

Attachment: Patch

Comment 5

[Frank Wein [:mcsmurf]]

Attachment: Patch

Comment 6

[Jungshik Shin]

Comment on attachment 190856 [details] [diff] [review]
Patch

r=jshin

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 190856 [details] [diff] [review]
Patch

would it be more readable if this:
   NS_IF_ADDREF(aEncoder);
were changed to:
   NS_IF_ADDREF(mErrEncoder); 
?

Comment 8

[Simon Montagu :smontagu]

Is there any reason not to check this in?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Past Frank not having CVS access?  No.

Comment 10

[Simon Montagu :smontagu]

Checked in with the change from comment 7.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 190856 [details] [diff] [review]
Patch

I think we should take this for 1.8.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Checked in on 1.8 branch.

Comment 13

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

I think this caused bug 306493 because none of these classes' constructors
actually initialize mErrEncoder to null.  It should just be an nsCOMPtr anyway...