437375 - malformed awayMsgs.txt causes failed startup

Status: RESOLVED FIXED

(Other Applications Graveyard :: ChatZilla, defect)

Description

[Glen Mailer]

Attachment: check for null value

A failure to deserialise awayMsgs.txt causes an exception on chatzilla startup.

Basically, the case where it returns null is not being handled, patch included. (Using the same approach as the networks.txt deserialisation)

Comment 1

[:Gijs (he/him)]

Comment on attachment 323851 [details] [diff] [review]
check for null value

You can have my r+, but I'm wondering if we need to give a more obvious notice to the user that something is wrong. The file will be wiped the next time we go /away, so I think we'd want to poke the user about that so they can possibly save the data they put in there? (thinking people who twiddled with the file themselves to get special away reasons in there or whatever - bit annoying if we lose their data for them)

OTOH, if the pref says we're away, IIRC we will immediately run the /away command on startup, which means the warning will be too late. Can you check that assumption and comment on the bug about what you think we should do?

Comment 2

[Glen Mailer]

Hrm, I hadn't considered the case of people tweaking it and getting it wrong. This was posted after someone came into the channel with a broken chatzilla, which was fixed by manually correcting the file.

The away message deserialisation is done in initStatic, which comes before initHandlers. So in theory we can alert the user before the list is potentially overwritten with a warning prompt - dunno what you would say on it though.


The away message file (awayMsgs.txt) is corrupted, press OK to discard the
current file, or Cancel if you wish to exit ChatZilla and edit it manually
                         [ Ok ] [ Cancel ]

Dunno if that's worth it though?

Comment 3

[James Ross]

I'd do a unique rename of awayMsgs.txt to awayMsgs.invalid. See the CEIP code for an example of a unique rename:

http://mxr.mozilla.org/seamonkey/source/extensions/irc/xul/content/ceip/ceip.js#174

Obviously, in this case, we'd set newFile.leafName = "awayMsgs.invalid" and not a time-specific number. If you're feeling dangerous, you could do a non-unique rename to awayMsgs.invalid - the XUL Cache does this.

Printing a [WARNING] saying, e.g. "Invalid file <awayMsgs.txt> renamed to <awayMsgs(1).invalid>."

Comment 4

[Glen Mailer]

Attachment: rename to unique

patch modified as per silver's comments, the setTimeout(0) workaround is because the message display stuff isn't initialised at this point.

Comment 5

[James Ross]

Comment on attachment 323896 [details] [diff] [review]
rename to unique

>Index: mozilla/extensions/irc/xul/content/static.js
>+                     

Nit: trailing whitespace.

>+            /* we have to close the file before we can move it,
>+             * hence the second if statement */
>+            if (item == null)
>+            {
>+                var invalidFile = new nsLocalFile(client.prefs["profilePath"]);
>+                invalidFile.append("awayMsgs.invalid");
>+                invalidFile.createUnique(FTYPE_FILE, 0600);
>+                var msg = getMsg(MSG_ERR_INVALID_FILE,
>+                                 [awayFile.leafName, invalidFile.leafName]);
>+                setTimeout("client.display('" + msg + "', MT_WARN)", 0);

Please use: setTimeout("client.display(" + msg.quote() + ", MT_WARN)", 0);

>+                awayFile.moveTo(null, invalidFile.leafName);
>+            }
>+            

Nit: Unnecessary line (and trailing whitespace).

>         }
>     }
> 
>     client.defaultCompletion = client.COMMAND_CHAR + "help ";
> 
>     client.deck = document.getElementById('output-deck');

Comment 6

[James Ross]

Checked in --> FIXED.