Closed Bug 226793 Opened 21 years ago Closed 11 years ago

Setting/getting last mod time differs on some systems

Categories

(Core :: XPCOM, defect, P3)

x86
Windows 2000
defect

Tracking

()

RESOLVED DUPLICATE of bug 377307

People

(Reporter: BenB, Unassigned)

Details

(Whiteboard: [M1])

Attachments

(1 file)

Reproducability:
Unknown. Bug appears on 2 W2K systems in California, but not on my W2K system in
Germany. I don't have access to the California systems.

Reproduction:
1. W2K, set timezone to PDT winter time (-0800)
2. int t1 = 1066244633000; // == 2003-10-15 21:03:53
   nsIFile f = ...;
   f.lastModifiedFile = t1;
   int t2 = f.lastModifiedFile;
   print t2;

Actual result:
t2 == 1066248233000; // == 2003-10-15 22:03:53 == t1 + 1 hour

Expected result:
t2 == t1

Analysis:
That 1 hour made me suspect a timezone problem, that the impl. of getter and
setter differ. In fact, they do, the getter is implemented in NSPR and using a
shortcut, the setter is implemented in xpcom/io/ and uses a number of scary
local time conversions.
Status: NEW → ASSIGNED
Priority: -- → P1
Bug appears reproducably on FAT filesystems, but not on NTFS filesystems, to my
knowledge.

Compare
<http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/setfiletime.asp>:
"FAT records times on disk in local time. However, NTFS records times on disk in
UTC."
No, wrong, can't reproduce on a FAT16 partition, I ran instead into another
(documented) problem, the impreciseness of FAT's last mod time.
The California systems are FAT32. Will continue to try to reproduce.
Bug appears, if the "Adjust for daylight saving time" setting of the OS is
checked. Both on NTFS and FAT.

Cases to be considered: (any combination of)
- "Daylight saving time adjustment" on or off
- File time in during daylight saving time or winter time
- File time set during daylight saving time or winter time
- File time read during daylight saving time or winter time
- File time read by Mozilla (NSPR) or Explorer
- File time set by Mozilla (XPCOM) or Windows (edit file with Notepad)
- NTFS or FAT
= 64 cases
I didn't have time to cover them all.

Despite some quite scary code in NSPR (hardcoded offset; direct reading of
FILETIME strcture, although discouraged by MS), and a seemingly wrong call to
LocalFileTimeToFileTime(), the main culprit seems to be the
PR_LocalTimeParameters parameter to PR_ExplodeTime in XPCOM IO. Windows System
time is in UTC, so converting our NSPR time into local time and feeding that to
Windows System Time functions will give wrong results.

Changing that parameter to PR_GMTParameters seems to fix the bug.

But I also removed the LocalFileTimeToFileTime() call, to make the code match
MS's example code and because it didn't make sense to me. Strangely, it doesn't
seem to make a difference for the correctness of the result, in my testings.

Attaching fix against Mozilla 1.4.1. Use |patch -p 0| to apply.
> = 64 cases

eh, 128 :).
Attachment #137040 - Attachment description: Proposed fix, version 1. Use |patch -p 0|. → Proposed fix, version 1.
Attachment #137040 - Flags: review?(dougt)
Comment on attachment 137040 [details] [diff] [review]
Proposed fix, version 1.

wtc needs to review this.
Attachment #137040 - Flags: review?(dougt) → review?(wchang0222)
Priority: P1 → P3
Whiteboard: [M1]
no answer from wtc. dougt, what do we do?
Comment on attachment 137040 [details] [diff] [review]
Proposed fix, version 1.

r=wtc.	The fix is correct.

The comment change to nsprpub/pr/src/md/windows/w95io.c
can be omitted.

The new comment for xpcom/io/nsLocalFileWin.cpp can also
be omitted.  It is useful for reviewing this change, but
it won't really help understand the new code.  The new
code is straightforward and easy to understand.

By the way, in xpcom/io/nsLocalFileWin.cpp I am wondering
if we should pass NULL as the third argument to
SetFileTime because we should not modify the last access
time.
Attachment #137040 - Flags: review?(wchang0222) → review+
Attachment #137040 - Flags: superreview?(scc)
Attachment #137040 - Flags: superreview?(scc) → superreview?(shaver)
shaver?
Attachment #137040 - Flags: superreview?(shaver) → superreview?(dougt)
nsLocalFileWin.cpp has changed since this patch was created.  Specifically in
version 1.125.  Ben, can check out these changes and create a new diff. 
Comment on attachment 137040 [details] [diff] [review]
Proposed fix, version 1.

ben, can you provide a new patch?
Attachment #137040 - Flags: superreview?(dougt) → superreview-
Assignee: ben.bucksch → nobody
Status: ASSIGNED → NEW
QA Contact: xpcom
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Funny. The patch commited in bug 377307 in 2011 is - apart from comments and bitrot - *exactly* the same that I submitted here in 2003, 7 years earlier. The review here took 1 year (!), and by that time it had bitrotted and I had moved on to other projects.
(And I found it unbelievably sarcastic to wait with the review so long that the patch bitrots, and then deny the review *because of* this bitrot and ask more work from the contributor.)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: