Closed Bug 1161477 Opened 6 years ago Closed 6 years ago

getLogFromFile sometimes returns the wrong log for grouped logs

Categories

(Chat Core :: General, defect)

defect
Not set
normal

Tracking

(thunderbird38 fixed, thunderbird39 fixed, thunderbird40 fixed)

RESOLVED FIXED
Tracking Status
thunderbird38 --- fixed
thunderbird39 --- fixed
thunderbird40 --- fixed

People

(Reporter: aleth, Assigned: nhnt11)

Details

Attachments

(1 file)

Something about the way dates are calculated/handled is incorrect in getLogFromFile.

For the log 
  2015-05-05.010932+0200.json
it returned the log 
  2015-05-04.175959+0200.json
This was neither the first log file in the grouped log for 4/5, nor the first log file in the grouped log for 5/5 (which would have been 2015-05-05.010932+0200.json).

This leads to search results not being found and displayed properly in the UI, as the wrong grouped log is shown.
This probably has a simple and safe fix: just use the Date.* methods that the grouped logs code uses rather than rolling your own.
Attached patch PatchSplinter Review
This approach uses toDateString() to compare the timestamps. I like this better than setting hours, minutes, and seconds to 0 and then using the ISO string (which is what DailyLogEnumerator does).

If you feel strongly that the approach should be consistent, maybe we can change DailyLogEnumerator to use toDateString() too? Of course, I'd be fine with just going the ISO string way in getLogFromFile too. Let me know.
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Attachment #8601521 - Flags: review?(aleth)
By the way, I haven't tested this patch because I don't have the time/motivation right now to set up a test case. Since you seem to have one already, could you please let me know if the issue is fixed?
I'm puzzled by this bug though. The math seems fine to me. I'll take another look later.
Comment on attachment 8601521 [details] [diff] [review]
Patch

Review of attachment 8601521 [details] [diff] [review]:
-----------------------------------------------------------------

Using Date methods indeed fixes the problem. I suspect your day maths didn't actually precisely line up with 00:00.

::: chat/components/src/logger.js
@@ +666,5 @@
>          return;
>        let path = aEntry.path;
>        let [logTime] = getDateFromFilename(OS.Path.basename(path));
>  
> +      if (targetDate == logTime.toDateString()) {

As we're only interested in matches and not in sorting, and not saving this platform-dependent string anywhere, fair enough.

The alternative would be to compare targetDate.getMonth() == logTime.getMonth() && day && fullyear. Personally I think I'd prefer that, it seem excessive to involve strings.
Attachment #8601521 - Flags: review?(aleth) → review+
(In reply to aleth [:aleth] from comment #5)
> logTime.getMonth() && day && fullyear. Personally I think I'd prefer that,

s/day/date
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
This bug was due to the day calculation in the previous code ignored the timezone offset (2 hours in the example in the description), which meant that getLogFromFile "days" started at 2am compared to Date days at 00:00.
(In reply to aleth [:aleth] from comment #8)
> This bug was due to the day calculation in the previous code ignored the
> timezone offset (2 hours in the example in the description), which meant
> that getLogFromFile "days" started at 2am compared to Date days at 00:00.

e.g. new Date(16559 * (86400 * 1000)) == "Mon May 04 2015 02:00:00 GMT+0200 (CEST)"
Comment on attachment 8601521 [details] [diff] [review]
Patch

[Approval Request Comment]
User impact if declined: Occasionally, when viewing the full log by clicking on a search result hit in faceted search in TB, the wrong day's logs will be displayed and the search string will not be found in the displayed log.
Risk to taking this patch (and alternatives if risky): low
Attachment #8601521 - Flags: approval-comm-beta?
Attachment #8601521 - Flags: approval-comm-aurora?
You need to log in before you can comment on or make changes to this bug.