nsILocalFile shows the wrong lastModifiedTime

RESOLVED FIXED in mozilla2.0b10

Status

()

Core
XPCOM
RESOLVED FIXED
11 years ago
5 years ago

People

(Reporter: George Lee, Assigned: emk)

Tracking

({testcase})

Trunk
mozilla2.0b10
x86
Windows 2000
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment)

(Reporter)

Description

11 years ago
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

Updated

8 years ago
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: ? → -
(Assignee)

Comment 3

7 years ago
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)

Comment 4

7 years ago
Created attachment 467561 [details] [diff] [review]
Use UTC consistently
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #467561 - Flags: review?(jmathies)

Updated

7 years ago
Attachment #467561 - Flags: review?(jmathies) → review+
(Assignee)

Comment 5

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

Comment 7

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

Comment 8

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

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

Updated

7 years ago
Depends on: 610267
Attachment #467561 - Flags: approval2.0? → approval2.0+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/0b2511671608
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10

Updated

7 years ago
No longer depends on: 610267
Duplicate of this bug: 226793

Comment 13

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