Closed Bug 525741 Opened 10 years ago Closed 10 years ago

two JAR tests fail if modification date of their files is on day daylight saving time (summer time) starts or ends and after time change

Categories

(Core :: Networking: JAR, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: dbaron, Assigned: michal)

References

Details

(Keywords: intermittent-failure, Whiteboard: [orange:time-bomb])

Attachments

(1 file, 2 obsolete files)

Today on tinderbox we've started seeing these failures:

TEST-UNEXPECTED-FAIL | e:\builds\moz2_slave\mozilla-central-win32-debug-unittest-everythingelse\build\xpcshell\tests\test_zipwriter\unit\test_asyncadd.js | test failed (with xpcshell return code: 3), see following log:
TEST-UNEXPECTED-FAIL | e:/builds/moz2_slave/mozilla-central-win32-debug-unittest-everythingelse/build/xpcshell/tests/test_zipwriter/unit/test_asyncadd.js | 3600000 - See following stack:
TEST-UNEXPECTED-FAIL | e:\builds\moz2_slave\mozilla-central-win32-debug-unittest-everythingelse\build\xpcshell\tests\test_zipwriter\unit\test_sync.js | test failed (with xpcshell return code: 3), see following log:
TEST-UNEXPECTED-FAIL | e:/builds/moz2_slave/mozilla-central-win32-debug-unittest-everythingelse/build/xpcshell/tests/test_zipwriter/unit/test_sync.js | 3600000 - See following stack:

These failures occur when the timestamps of the files in $(OBJDIR)/_tests/xpcshell/test_zipwriter/unit/data/ are:
 * on the day that we ended summer time
 * sometime after the time change

In other words, I can reproduce the failures if I change to the above directory and run:
touch -t200911010430.00 * */* */*/*
or:
touch -t200903080430.00 * */* */*/*
(These dates assume your system time zone is a US time zone that observes daylight saving time.  If you're in some other time zone you'll have to adjust appropriately.)

I'm guessing we didn't see these failures in the past because we weren't using packaged unit tests and none of the test boxes checked out a fresh tree on the problematic day.
Assignee: nobody → michal.novotny
Attached patch fix (obsolete) — Splinter Review
DST offset handling in function GetModTime() in nsJAR.cpp is incorrect:

  time.tm_params.tp_gmt_offset = 0;
  time.tm_params.tp_dst_offset = 0;
  PR_NormalizeTime(&time, PR_GMTParameters);
  time.tm_params = PR_LocalTimeParameters(&time);

After calling PR_NormalizeTime() variable time contains localtime from ZIP entry as UTC (without shifting, i.e. 11/01/2009 4:30 will be represented as 11/01/2009 4:30 UTC). PR_LocalTimeParameters() sets gmt_offset correctly, but dst_offset is wrong because 11/01/2009 4:30 UTC is 10/31/2009 21:30 _PDT_, but it should be treat as US local time (i.e. 11/01/2009 4:30 _PST_). The unittest fails on DST shift PST->PDT between 03/08/2009 UTC 10:00:00 - 03/08/2009 UTC 16:59:59 and PDT->PST between 11/01/2009 UTC 09:00:00 - 11/01/2009 UTC 16:59:59.


Following code is a little bit more correct:

  time.tm_params.tp_gmt_offset = 0;
  time.tm_params.tp_dst_offset = 0;
  PR_NormalizeTime(&time, PR_GMTParameters);
  time.tm_params.tp_gmt_offset = PR_LocalTimeParameters(&time).tp_gmt_offset;
  PR_NormalizeTime(&time, PR_GMTParameters);
  time.tm_params.tp_dst_offset = PR_LocalTimeParameters(&time).tp_dst_offset;

It first normalizes time as GMT, apply gmt_offset only and normalizes/converts to GMT again. This solves all problems on PST->PDT shift but can't solve problem on PDT->PST shift for local US time between 11/01/2009 01:00:00 and 11/01/2009 01:59:59 since it is valid time in PDT as well as in PST and the timezone information isn't stored in ZIP entry. This patch adds support for extended timestamp extra field in ZIP files to fix this problem.
Attachment #414063 - Flags: review?(tglek)
Whiteboard: [orange][orange:time-bomb]
Comment on attachment 414063 [details] [diff] [review]
fix




>+
>+  return NULL;
>+}
>+
>+
>+PRTime const nsZipItem::LastModTime()
>+{
>+  if (isSynthetic) return GetModTime(kSyntheticDate, kSyntheticTime);
>+
>+  // Try to read timestamp from extra field
>+  const unsigned char *tsField = GetExtraField(EXTENDED_TIMESTAMP_FIELD);
>+  if (tsField) {
>+    PRUint16 blocksize = xtoint(tsField + 2);
That's redundant. Pass blocksize as an outparam in GetExtraField

>+    if (blocksize >= 5 &&
>+        *(PRUint8*)(tsField + 4) & EXTENDED_TIMESTAMP_MODTIME) {

Afaik you don't need a cast here. tsField[4] & EXTENDED_TIMESTAMP_MODTIME

>+      PRTime conversion, retVal;
>+      LL_I2L(conversion, PR_USEC_PER_SEC);
>+      LL_I2L(retVal, xtolong(tsField + 5));
>+      LL_MUL(retVal, retVal, conversion);

LL* macros are obsolete. You can use regular */ operators.


>+  const unsigned char * GetExtraField(PRUint16 aTag);
Make this return PRUint8* and outparam for length. We need to convert the rest of the code to use PRUint8 instead of char *.




>-inline NS_HIDDEN_(PRUint8) READ8(char* buf, PRUint32* off)
>+inline NS_HIDDEN_(PRUint8) READ8(const char* buf, PRUint32* off)

This is good. I would appreciate if you could change the char* into PRUint8*s as a bonus? 

> /* readonly attribute PRTime lastModifiedTime; */
> NS_IMETHODIMP nsZipHeader::GetLastModifiedTime(PRTime *aLastModifiedTime)
> {
>     NS_ASSERTION(mInited, "Not initalised");
> 
>+    // Try to read timestamp from extra field
>+    const char *tsField = GetExtraField(ZIP_EXTENDED_TIMESTAMP_FIELD, PR_FALSE);
>+    if (tsField) {
>+        PRUint32 pos = 2;
>+        PRUint16 blocksize = READ16(tsField, &pos);
>+        if (blocksize >= 5) {
>+            PRUint8 flags;
>+            flags = READ8(tsField, &pos);
>+            if (flags & ZIP_EXTENDED_TIMESTAMP_MODTIME) {
>+                PRUint32 utc = READ32(tsField, &pos);
>+                PRTime conversion;
>+                LL_I2L(conversion, PR_USEC_PER_SEC);
>+                LL_I2L(*aLastModifiedTime, utc);
>+                LL_MUL(*aLastModifiedTime, *aLastModifiedTime, conversion);
>+                return NS_OK;
Get rid of LL* here too

> }
> 
> /* readonly attribute boolean isSynthetic; */
> NS_IMETHODIMP nsZipHeader::GetIsSynthetic(PRBool *aIsSynthetic)
>@@ -144,28 +171,48 @@ void nsZipHeader::Init(const nsACString 
> 
>     PRExplodedTime time;
>     PR_ExplodeTime(aDate, PR_LocalTimeParameters, &time);
> 
>     mTime = time.tm_sec / 2 + (time.tm_min << 5) + (time.tm_hour << 11);
>     mDate = time.tm_mday + ((time.tm_month + 1) << 5) +
>             ((time.tm_year - 1980) << 9);
> 
>+    // Store modification timestamp as extra field
>+    // First fill CDS extra field
>+    mFieldLength = 9;
>+    mExtraField = new char[mFieldLength];

needs an OOM check

>+    PRUint32 pos = 0;
>+    WRITE16(mExtraField.get(), &pos, ZIP_EXTENDED_TIMESTAMP_FIELD);
>+    WRITE16(mExtraField.get(), &pos, 5);
>+    WRITE8(mExtraField.get(), &pos, ZIP_EXTENDED_TIMESTAMP_MODTIME);
>+    PRUint32 gmtTime;
>+    PRTime conversion;
>+    LL_I2L(conversion, PR_USEC_PER_SEC);
>+    LL_DIV(conversion, aDate, conversion);
>+    LL_L2I(gmtTime, conversion);
more LL_* to get rid of.

>+
>+const char * nsZipHeader::GetExtraField(PRUint16 aTag, PRBool aLocal)
>+{
>+    const char *buf = aLocal ? mLocalExtraField : mExtraField;
>+    PRUint32 buflen = aLocal ? mLocalFieldLength : mFieldLength;
>+    PRUint32 pos = 0;
>+    PRUint32 tag, blocksize;
>+
>+    while (buf && (pos + 4) <= buflen) {
>+      tag = READ16(buf, &pos);
>+      blocksize = READ16(buf, &pos);
>+
>+      if (aTag == tag && (pos + blocksize) <= buflen)
>+        return buf + pos - 4;
>+
>+      pos += blocksize;
>+    }
>+
>+    return NULL;
>+}

Should not create indentical functions(this one has same issues as one above). Share this via a header.
Attachment #414063 - Flags: review?(tglek) → review-
(In reply to comment #2)
> >+const char * nsZipHeader::GetExtraField(PRUint16 aTag, PRBool aLocal)
> >+{
> >+    const char *buf = aLocal ? mLocalExtraField : mExtraField;
> >+    PRUint32 buflen = aLocal ? mLocalFieldLength : mFieldLength;
> >+    PRUint32 pos = 0;
> >+    PRUint32 tag, blocksize;
> >+
> >+    while (buf && (pos + 4) <= buflen) {
> >+      tag = READ16(buf, &pos);
> >+      blocksize = READ16(buf, &pos);
> >+
> >+      if (aTag == tag && (pos + blocksize) <= buflen)
> >+        return buf + pos - 4;
> >+
> >+      pos += blocksize;
> >+    }
> >+
> >+    return NULL;
> >+}
> 
> Should not create indentical functions(this one has same issues as one above).
> Share this via a header.

Is this a good idea? They aren't completely identical. They differ in buffer location, length, isSynthetic attribute and data access methods (xtoint vs. READ16). Another similar duplicity is already there: nsZipHeader::GetLastModifiedTime and nsZipItem::LastModTime. And probably much more since zipwriter doesn't use anything from libjar sources.

>  PRUint32 const LocalOffset();
>  PRUint32 const Size();
...

IMHO it doesn't make sense to return const PRUint32, PRUint16, bool, etc... Can I remove it?
(In reply to comment #3)

> Is this a good idea? They aren't completely identical. They differ in buffer
> location, length, isSynthetic attribute and data access methods (xtoint vs.
> READ16). Another similar duplicity is already there:
> nsZipHeader::GetLastModifiedTime and nsZipItem::LastModTime. And probably much
> more since zipwriter doesn't use anything from libjar sources.

You are right, sharing code here is a pain and beyond the scope of this patch.
> 
> >  PRUint32 const LocalOffset();
> >  PRUint32 const Size();
> ...
> 
> IMHO it doesn't make sense to return const PRUint32, PRUint16, bool, etc... Can
> I remove it?

Ok.
Attachment #414063 - Attachment is obsolete: true
Attachment #416481 - Flags: review?(tglek)
Attachment #416481 - Flags: review?(dtownsend)
Comment on attachment 416481 [details] [diff] [review]
patch v2 - updated according to comments #2 and #4

Dave, could you review the zipwriter part?
Duplicate of this bug: 402434
(In reply to comment #2)
> >+  const unsigned char * GetExtraField(PRUint16 aTag);
> Make this return PRUint8* and outparam for length. We need to convert the rest
> of the code to use PRUint8 instead of char *.

What is the benefit in using PRUint8 vs char?
Comment on attachment 416481 [details] [diff] [review]
patch v2 - updated according to comments #2 and #4

Awesome, I wanted to implement this a long time ago. Couple of nits while I wait for a response from Taras:

>diff --git a/modules/libjar/zipwriter/test/unit/test_asyncadd.js b/modules/libjar/zipwriter/test/unit/test_asyncadd.js
>--- a/modules/libjar/zipwriter/test/unit/test_asyncadd.js
>+++ b/modules/libjar/zipwriter/test/unit/test_asyncadd.js
>@@ -95,14 +95,15 @@ function run_test()
> {
>   zipW.open(tmpFile, PR_RDWR | PR_CREATE_FILE | PR_TRUNCATE);
> 
>   for (var i = 0; i < TESTS.length; i++) {
>     var source = do_get_file(DATA_DIR+TESTS[i].name);
>     zipW.addEntryFile(TESTS[i].name, Ci.nsIZipWriter.COMPRESSION_NONE, source,
>                       true);
>     size += ZIP_FILE_HEADER_SIZE + ZIP_CDS_HEADER_SIZE +
>+            (ZIP_EXTENDED_TIMESTAMP_SIZE*2) +

Spaces around the operator

>diff --git a/modules/libjar/zipwriter/test/unit/test_sync.js b/modules/libjar/zipwriter/test/unit/test_sync.js
>--- a/modules/libjar/zipwriter/test/unit/test_sync.js
>+++ b/modules/libjar/zipwriter/test/unit/test_sync.js
>@@ -55,16 +55,17 @@ function run_test()
>   zipW.open(tmpFile, PR_RDWR | PR_CREATE_FILE | PR_TRUNCATE);
> 
>   var size = 0;
>   for (var i = 0; i < TESTS.length; i++) {
>     var source = do_get_file(DATA_DIR + TESTS[i].name);
>     zipW.addEntryFile(TESTS[i].name, Ci.nsIZipWriter.COMPRESSION_NONE, source,
>                       false);
>     size += ZIP_FILE_HEADER_SIZE + ZIP_CDS_HEADER_SIZE +
>+            (ZIP_EXTENDED_TIMESTAMP_SIZE*2) +

Spaces around the operator.

Most of the tests check that timestamps didn't vary by more than 2 seconds (TIME_RESOLUTION) because the basic zip time format is low resolution. I think we can drop that requirement for most of those tests and make them test exact times now.
(In reply to comment #8)
> (In reply to comment #2)
> What is the benefit in using PRUint8 vs char?

Consistency with other buffers in the zip module. Obviously there is no runtime difference.
Attached patch patch v3Splinter Review
(In reply to comment #9)
> (From update of attachment 416481 [details] [diff] [review])
> >+            (ZIP_EXTENDED_TIMESTAMP_SIZE*2) +
> 
> Spaces around the operator

Fixed

> >+            (ZIP_EXTENDED_TIMESTAMP_SIZE*2) +
> 
> Spaces around the operator.

Fixed

> Most of the tests check that timestamps didn't vary by more than 2 seconds
> (TIME_RESOLUTION) because the basic zip time format is low resolution. I think
> we can drop that requirement for most of those tests and make them test exact
> times now.

Fixed. All tests pass successfully with exact timestamps.
Attachment #416481 - Attachment is obsolete: true
Attachment #417810 - Flags: review?(dtownsend)
Attachment #416481 - Flags: review?(tglek)
Attachment #416481 - Flags: review?(dtownsend)
-PRUint16 const nsZipItem::Compression()
+PRUint16 nsZipItem::Compression()

Why did you get rid of const in all these?
(In reply to comment #12)
> -PRUint16 const nsZipItem::Compression()
> +PRUint16 nsZipItem::Compression()
> 
> Why did you get rid of const in all these?

Because you allowed that :) (See comment 4)
What's the purpose of having them const?
(In reply to comment #13)
> Because you allowed that :) (See comment 4)
> What's the purpose of having them const?

I'm sorry, I misread that. I thought you were actually adding consts. Why does it not make sense to have them as consts?
Because I think that it is misleading. Correct me if I'm wrong but when function returns basic type like int, it is always "copied". I.e. the const is lost just like in this example:

const int f() { return 1; }

int i = f();
i = 2;

I couldn't find any usage where the const wasn't lost and the compiler threw an error.
The value is never "copied", it's still a normal function call. const doesn't actually provide any guarantees to the C++ compiler(because they can all be circumvented with casts), so it's mainly syntactic sugar. 

On the other hand const members are useful for that developer in that they indicate that the methods are more or less pure and wont modify object that they are called on.
(In reply to comment #16)
> On the other hand const members are useful for that developer in that they
> indicate that the methods are more or less pure and wont modify object that
> they are called on.

But the const here is related to the return value. As far as I know PRUint32 const f(); is the same as const PRUint32 f();. Const member function would be PRUint32 f() const; and I agree that this would make sense.
(In reply to comment #17)
> But the const here is related to the return value. As far as I know PRUint32
> const f(); is the same as const PRUint32 f();. Const member function would be
> PRUint32 f() const; and I agree that this would make sense.

You are right for non-member functions. That change is great, sorry for the confusion.
Comment on attachment 417810 [details] [diff] [review]
patch v3

Looks good. r=dtownsend for the zipwriter parts
Attachment #417810 - Flags: review?(dtownsend) → review+
Attachment #417810 - Flags: review+
Does this patch need a super-review?
(In reply to comment #20)
> Does this patch need a super-review?

no, you can land it
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/cc65a0921a1b
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Depends on: 543101
Does it make sense to backport this (and the regression fix, bug 543101) to release branches?
Duplicate of this bug: 552292
Blocks: 438871
Whiteboard: [orange][orange:time-bomb] → [orange:time-bomb]
You need to log in before you can comment on or make changes to this bug.