Last Comment Bug 777873 - Viewing some old Twitter logs breaks log viewer
: Viewing some old Twitter logs breaks log viewer
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: 15 Branch
: x86 Windows 7
: -- major (vote)
: Thunderbird 17.0
Assigned To: Florian Quèze [:florian] [:flo]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-26 13:00 PDT by Mike Conley (:mconley) - (needinfo me!)
Modified: 2012-09-20 09:09 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
Patch (5.04 KB, patch)
2012-08-08 09:51 PDT, Florian Quèze [:florian] [:flo]
clokep: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Mike Conley (:mconley) - (needinfo me!) 2012-07-26 13:00:25 PDT
STR:

In the Chat tab, choose a previous conversation from several weeks ago

What happens?

The log is not displayed. What's more, viewing *any* logs at this point is broken.

What's expected?

The log viewer should work.
Comment 1 Mike Conley (:mconley) - (needinfo me!) 2012-07-26 13:00:49 PDT
This appears in the Error Console:

Timestamp: 26/07/2012 3:58:01 PM
Error: TypeError: aConv is null
Source File: chrome://chat/content/convbrowser.xml
Line: 713
Comment 2 Mike Conley (:mconley) - (needinfo me!) 2012-07-26 13:05:06 PDT
This seems to affect the current beta.
Comment 3 Mike Conley (:mconley) - (needinfo me!) 2012-07-26 13:08:32 PDT
(I'm unsure how critical an error this is - perhaps my old logs are corrupt somehow...they go all the way back to April 1st. Regardless, we've clearly got a hole in our error handling here.)
Comment 4 Florian Quèze [:florian] [:flo] 2012-08-08 09:51:28 PDT
Created attachment 650187 [details] [diff] [review]
Patch

This patch handles 3 error cases:
- log files that aren't in the JSON format (I think Mike may have tested a very old try server build of IM in Tb that still wrote plain text logs. The error in the console in that case is exactly what Mike reported)
- random files in the log folder that don't have a name matching the expected format (these files are now just ignored, instead of breaking the list of logs)
- random junk inside a log file with a correct name and a .json extension. (The log file will be ignored if the first line is not valid JSON. If the junk starts later, only the lines that aren't valid JSON will be ignored).
Comment 5 Patrick Cloke [:clokep] 2012-08-08 09:59:26 PDT
Comment on attachment 650187 [details] [diff] [review]
Patch

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> This patch handles 3 error cases:
> - log files that aren't in the JSON format (I think Mike may have tested a
> very old try server build of IM in Tb that still wrote plain text logs. The
> error in the console in that case is exactly what Mike reported)

Shouldn't these go into the text log reader we have then or am I misunderstanding that?

The changes here look fine though!
Comment 6 Florian Quèze [:florian] [:flo] 2012-08-08 10:33:52 PDT
(In reply to Patrick Cloke [:clokep] from comment #5)

> > - log files that aren't in the JSON format (I think Mike may have tested a
> > very old try server build of IM in Tb that still wrote plain text logs. The
> > error in the console in that case is exactly what Mike reported)
> 
> Shouldn't these go into the text log reader we have then or am I
> misunderstanding that?

Thunderbird never logged in the plain text format (except for a few very early testing builds) so it doesn't have the text log reader that Instantbird has.
Comment 7 Florian Quèze [:florian] [:flo] 2012-08-10 09:23:09 PDT
Comment on attachment 650187 [details] [diff] [review]
Patch

[Approval Request Comment]
User impact if declined: parts of the chat UI will be broken if the user has random junk in a log folder.

Risk to taking this patch: low.

Should we take this for Tb15? I really don't know. The risk is low (the patch mostly adds null checks and try/catch in a few places), but the benefit seems only marginal (ie, why would users have random invalid log files in their log folders?).

I'm requesting approval mostly so that someone actually make a decision here.
Comment 8 Mark Banner (:standard8) 2012-08-14 03:49:16 PDT
Comment on attachment 650187 [details] [diff] [review]
Patch

Although there's probably not too many people with corrupted logs, I think keeping working if we get into that situation is probably a good idea. We don't know yet the full extent of what we'll hit, so lets take it and have less risk there.
Comment 10 Florian Quèze [:florian] [:flo] 2012-09-20 09:09:35 PDT
The chat/ part landed for Instantbird as http://hg.instantbird.org/instantbird/rev/8ec7e45f71fd and the UI changes were dealt with in https://bugzilla.instantbird.org/show_bug.cgi?id=1448

Note You need to log in before you can comment on or make changes to this bug.