Closed Bug 377307 Opened 13 years ago Closed 9 years ago

nsILocalFile shows the wrong lastModifiedTime

Categories

(Core :: XPCOM, defect)

x86
Windows 2000
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b10
Tracking Status
blocking2.0 --- -

People

(Reporter: glee, Assigned: emk)

References

Details

(Keywords: testcase)

Attachments

(1 file)

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
Component: File Handling → XPCOM
Product: Firefox → Core
QA Contact: file.handling → xpcom
This was confirmed on tinderbox today when Bug 178506 landed and failed in exactly the same way on Windows only.
Blocks: 178506
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
blocking2.0: --- → ?
It's worth noting that that test uses the exact same method as comment #0, so nsIWebBrowserPersist may be involved somehow too.
Summary: nsiLocalFile shows the wrong lastModifiedTime → nsILocalFile shows the wrong lastModifiedTime
Version: unspecified → Trunk
blocking2.0: ? → -
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)
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #467561 - Flags: review?(jmathies)
Attachment #467561 - Flags: review?(jmathies) → review+
Comment on attachment 467561 [details] [diff] [review]
Use UTC consistently

I also consider backporting this to 1.9.2 branch.
Attachment #467561 - Flags: approval2.0?
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.
Attachment #467561 - Flags: approval2.0? → approval2.0-
(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?
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);
(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 on attachment 467561 [details] [diff] [review]
Use UTC consistently

renominating per comment 7
Attachment #467561 - Flags: approval2.0- → approval2.0?
Depends on: post2.0
Attachment #467561 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/0b2511671608
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
No longer depends on: post2.0
Duplicate of this bug: 226793
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.
You need to log in before you can comment on or make changes to this bug.