525741 - 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

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[Michal Novotny [:michal]]

Attachment: fix

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.

Comment 2

[(dormant account)]

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.

Comment 3

[Michal Novotny [:michal]]

(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?

Comment 4

[(dormant account)]

(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.

Comment 5

[Michal Novotny [:michal]]

Attachment: patch v2 - updated according to comments #2 and #4

Comment 6

[(dormant account)]

Comment on attachment 416481 [details] [diff] [review]
patch v2 - updated according to comments #2 and #4

Dave, could you review the zipwriter part?

Comment 8

[Dave Townsend [:mossop]]

(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 9

[Dave Townsend [:mossop]]

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.

Comment 10

[(dormant account)]

(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.

Comment 11

[Michal Novotny [:michal]]

Attachment: patch v3

(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.

Comment 12

[(dormant account)]

-PRUint16 const nsZipItem::Compression()
+PRUint16 nsZipItem::Compression()

Why did you get rid of const in all these?

Comment 13

[Michal Novotny [:michal]]

(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?

Comment 14

[(dormant account)]

(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?

Comment 15

[Michal Novotny [:michal]]

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.

Comment 16

[(dormant account)]

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.

Comment 17

[Michal Novotny [:michal]]

(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.

Comment 18

[(dormant account)]

(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 19

[Dave Townsend [:mossop]]

Comment on attachment 417810 [details] [diff] [review]
patch v3

Looks good. r=dtownsend for the zipwriter parts

Comment 20

[Michal Novotny [:michal]]

Does this patch need a super-review?

Comment 21

[(dormant account)]

(In reply to comment #20)
> Does this patch need a super-review?

no, you can land it

Comment 22

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/cc65a0921a1b

Comment 23

[Zack Weinberg (:zwol)]

Does it make sense to backport this (and the regression fix, bug 543101) to release branches?