515713 - reply-all to email that has a newsgroup header as well is broken

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[David Ascher (:davida)]

If I reply-all to an email that also has a newsgroup: header, I get a dialog saying something like "we can't send to both mail and newsgroup at once. hitting ok will ignore the newsgroup".  There's a checkbox saying "don't ask me again".

if you check the checkbox and hit OK, then sending doesn't happen.

Comment 1

[David Ascher (:davida)]

Attachment: patch

god this is awful code.  Anyway, this code path was never going to work with an undefined var.

Comment 2

[David Ascher (:davida)]

Attachment: better patch

may as well not make the code worse.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 400695 [details] [diff] [review]
better patch

The patch works fine. I'd really like to say indent the lines in the try part, but 200 lines is quite a lot.

Do you really need the try/catch? When I tried without your patch the details were pretty clear without needed additional logging.

Comment 4

[David Ascher (:davida)]

Comment on attachment 400695 [details] [diff] [review]
better patch

I don't need the try catch for this code -- I want the try catch for any future failure in this code which would be completely silent otherwise.

We have to stop letting exceptions go to /dev/null at some point, but the core bug there feels futuristic.  try/catch around scary long and undertested code feels like a good "braces & suspenders" model to me.  But if you don't want it, i can take it out.

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> (From update of attachment 400695 [details] [diff] [review])
> I don't need the try catch for this code -- I want the try catch for any future
> failure in this code which would be completely silent otherwise.

Not quite, I've just done a bit more digging. On 1.9.1 I get an error dumped to the command line console. On trunk the error goes to the error console. 1.9.2 is the same as trunk.

The culprit is globalOverlay.js - bug 490150 improved it to use the Components.utils.reportError function.

> We have to stop letting exceptions go to /dev/null at some point, but the core
> bug there feels futuristic.  try/catch around scary long and undertested code
> feels like a good "braces & suspenders" model to me.  But if you don't want it,
> i can take it out.

I agree. I think for 1.9.1 we need the try catch and for 1.9.2 and later we can loose it.

Therefore my proposal is that we leave the indentation as-is and stick a comment in along the lines of:

// This try/catch ensures errors make it to the error console on the
// MOZILLA_1_9_1_BRANCH. On 1.9.2 and later, globalOverlay.js has been fixed
// to put the errors to the error console for us and therefore this try/catch
// can be removed.

Then when we come to clean up from the 1.9.1 ifdefs we should find the comment and clean up the try/catch that is no longer necessary.

Comment 6

[David Ascher (:davida)]

standard8, my recollection is you were going to do something w/ this bug at this point, no?

Comment 7

[Mark Banner (:standard8)]

Yep, I've added my suggested comments to the patch and checked it in:

http://hg.mozilla.org/comm-central/rev/be58ebcea946

We should probably have a litmus test for something like this, as it is easy to miss if it does get broken. Eventually we may be able to pull it across into a mozmill test, but I'm not convinced we've got the necessary set up at the moment (e.g. newsgroups).

Comment 8

[Ludovic Hirlimann [:Usul]]

litmus test 9544 covers it