253936 - incorrect use of assertions with UTF-8 checking

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

There are two problems with the assertions introduced in 213035: 

a) they assert if "line" is null with a message about UTF8-ness.  If line is
null, UTF8ness (or lack thereof) is not what's going on.  Note that the old code
did not assert in this case at all.

b) assertions have a specific meaning: they are supposed to indicate "if this
assertion is not true, there is a bug in the code".  However, in an MUA, having
to handle badly formatted mail messages is a completely normal use case.

Most importantly, though, they introduce unneccessary noise on the console when
looking for real code problems.  I believe they should go away.

Comment 1

[Jungshik Shin]

(In reply to comment #0)
> There are two problems with the assertions introduced in 213035: 

  |NS_ASSERTION| was added in bug 213035, but assertions had been there long
before that. I just moved them. 
 
> a) they assert if "line" is null with a message about UTF8-ness.  If line is
> null, UTF8ness (or lack thereof) is not what's going on.  Note that the old code
> did not assert in this case at all.

You misread the code. It doesn't assert when line is null. It asserts only if
line is NOT null and line is not UTF8. The current code does exactly the same
except that it asserts where the "problem" is instead of nsReadableUtils.


> b) assertions have a specific meaning: they are supposed to indicate "if this
> assertion is not true, there is a bug in the code".  However, in an MUA, having
> to handle badly formatted mail messages is a completely normal use case.

 I shouldn't have blindly 'moved' assertions from nsReadableUtils to
nsMsgHeaderParser. 
 
> Most importantly, though, they introduce unneccessary noise on the console when
> looking for real code problems.  I believe they should go away.

 I agree and am gonna get rid of them. 

Comment 2

[Jungshik Shin]

Attachment: patch

Comment 3

[Jungshik Shin]

Comment on attachment 155489 [details] [diff] [review]
patch

asking for r/sr

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 155489 [details] [diff] [review]
patch

r=dmose.  One question... is there anything better we could be doing in the
situation where somebody sends a message which contains bogus encodings of
characters, and thus would currently trigger these asserts?

Comment 5

[Jungshik Shin]

marking as fixed. it was checked in August. 

>is there anything better we could be doing in th
I think we're doing our best down the road and this assertion had been useless
for a long time