getLogFromFile sometimes returns the wrong log for grouped logs

RESOLVED FIXED in 1.6

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aleth, Assigned: nhnt11)

Tracking

trunk

Thunderbird Tracking Flags

(thunderbird38 fixed, thunderbird39 fixed, thunderbird40 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

4 years ago
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.
Reporter

Comment 1

4 years ago
This probably has a simple and safe fix: just use the Date.* methods that the grouped logs code uses rather than rolling your own.
Assignee

Comment 2

4 years ago
Posted 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)
Assignee

Comment 3

4 years ago
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?
Assignee

Comment 4

4 years ago
I'm puzzled by this bug though. The math seems fine to me. I'll take another look later.
Reporter

Comment 5

4 years ago
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+
Reporter

Comment 6

4 years ago
(In reply to aleth [:aleth] from comment #5)
> logTime.getMonth() && day && fullyear. Personally I think I'd prefer that,

s/day/date
Reporter

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Reporter

Comment 8

4 years ago
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.
Reporter

Comment 9

4 years ago
(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)"
Reporter

Comment 10

4 years ago
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.