377307 - nsILocalFile shows the wrong lastModifiedTime

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[George Lee]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3 XPCOMViewer/0.9.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3 XPCOMViewer/0.9.5

For certain files the lastModifedTime returns the wrong time.  I download a file using NSIWebBrowserPersist and update the lastModifedTime so that I can sync the time with the time of the file on the server.  This results with a file with the following timings:

Created: Today, April 11, 2007, 5:52:52 PM
Modified: Saturday, March 03, 2007, 11:30:38 AM
Accessed: Today, April 11, 2007, 6:31:57 PM

Unfortunately I can't set/read the created time via nsiLocalFile but all I really need is the last modified time so I ignore the creation time. 

When I look at the file through the OS.  The epoch time is: 
  1172950238000
however when I use nsiLocalFile.lastModifiedTime it shows the following time:
  1172946638000

It's off by exactly one hour (3600 seconds).  Somehow an extra hour is either being pushed/removed.  Daylight savings code?


Reproducible: Always

Steps to Reproduce:
1. Create a file with the following timings specified above (c:\temp\foo.txt)
  Created: Today, April 11, 2007, 5:52:52 PM
  Modified: Saturday, March 03, 2007, 11:30:38 AM
  Accessed: Today, April 11, 2007, 6:31:57 PM

2. Create a nsiLocalFile and set it to c:\temp\foo.txt
3. Look at the lastModifiedTime of the nsiLocalFile
Actual Results:  
1172946638000

Expected Results:  
1172950238000

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This was confirmed on tinderbox today when Bug 178506 landed and failed in exactly the same way on Windows only.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It's worth noting that that test uses the exact same method as comment #0, so nsIWebBrowserPersist may be involved somehow too.

Comment 3

[Masatoshi Kimura [:emk]]

This problem is reproducable only when the time zone is using Daylight savings.
nsLocalFileWin converts localtime to UTC using SystemTimeToFileTime & LocalFileTimeToFileTime combo.
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileWin.cpp#1998
However, SystemTimeToFileTime is not usable to convert localtime because it cannot determine whether the given date is local or not. So it cannot handle Daylight savings correctly.
Why nsLocalFile::SetModDate converts UTC to localtime (using PR_LocalTimeParameters), then convert back to UTC again? (using LocalFileTimeToFileTime)

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: Use UTC consistently

Comment 5

[Masatoshi Kimura [:emk]]

Comment on attachment 467561 [details] [diff] [review]
Use UTC consistently

I also consider backporting this to 1.9.2 branch.

Comment 6

[Benjamin Smedberg]

I think this patch introduces an unexpected behavior change, in that localfile.lastModifiedTime no longer matches the time returned by new Date(). I don't think that this is an acceptable change in general, at least not without some serious changes in the docs and perhaps a change in the method name. It certainly is not something I can approve this close to the FF4 release.

Comment 7

[Masatoshi Kimura [:emk]]

(In reply to comment #6)
> I think this patch introduces an unexpected behavior change, in that
This patch doesn't change the behavior. This patch just fixes the inconsistency between SetLastModifiedTime and GetLastModifiedTime.
> localfile.lastModifiedTime no longer matches the time returned by new Date().
What the word "match" means? This patch doesn't change GetLastModifiedTime at all. This patch just use fix the bug of SetLastModifiedTime on Windows.
> I don't think that this is an acceptable change in general, at least not 
> without some serious changes in the docs and perhaps a change in the method
> name.
This is a Windows only behavior. Do you say that Firefox for Mac and Linux have the bug? Or is there a document which states about the difference between platforms?

Comment 8

[Masatoshi Kimura [:emk]]

This patch doesn't break the following test (see xpcom/tests/unit/test_localfile.js). I don't really understand what "localfile.lastModifiedTime no longer matches the time returned by new Date()" means.
>  var now = Date.now();
>  file.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0644);
>  do_check_true(file.exists());
>
>  // Modification time may be out by up to 2 seconds on FAT filesystems. Test
>  // with a bit of leeway, close enough probably means it is correct.
>  var diff = Math.abs(file.lastModifiedTime - now);
>  do_check_true(diff < MAX_TIME_DIFFERENCE);

Comment 9

[Dave Garrett]

(In reply to comment #5)
> I also consider backporting this to 1.9.2 branch.

If this gets backported to 1.9.2 please also backport to 1.9.1.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 467561 [details] [diff] [review]
Use UTC consistently

renominating per comment 7

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/0b2511671608

Comment 13

[Ben Bucksch (:BenB)]

Funny. In bug 226793, I had created almost exactly the same patch in 2003, but the patch got stuck in too long review queues - 1 year. By the time it was reviewed, the patch had rotted and I had moved on to other projects.