668442 - Date smoke tests broken by Tamarin Foundation changes

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

The Player smoke tests have broken due to the change to VMPI_getDaylightSavingsTA.

In particular, the following expression:
  new Date(-114457500000)
used to return a date object with string rep:
  "Mon May 16 23:15:00 GMT-0700 1966"
but now it returns a date object with string rep:
  "Mon May 16 22:15:00 GMT-0800 1966"

and this behavior seems to have started immediately following TR 6409:c86f37feef86 aka Bug 645878

I am planning to immediately put back in the prior (which I'll call "original") implementation.  Stan mentioned that there was quite a tangle of callbacks in the original.  It looks like the "best" way to put back the previous version would require also migrating the time functions to AVMPI, ugh.

(There were some other also-unpalatable options listed, see below)

----

I'm listing below the back-and-forth discussion about the VMPI_getDaylightSavingsTA rewrite, which should provide some context as to how we got here.

Bug 645878, comment 36
> I am experiencing some tangle between avmcore and vmpi on Windows in the
> surprisingly involved function VMPI_getDaylightSavingsTA(), which makes
> calls back into (ahem!) locally-declared avmplus functions:
> 
> namespace avmplus
> {
>     int WeekDay(double t);
>     double MakeDate(double day, double time);
> 
> I'm tempted to move this to the "AVMPI" side of things and punt, but first
> I'm wondering why this doesn't just use localtime and tm_isdst like the
> other ports do.
> 
> If I do punt should I just move that one function, or all the time/date set
> of functions?

Bug 645878, comment 44
> One potentially substantive change aligns the Windows implementation of
> VMPI_getDaylightSavingsTA with the other platforms. The existing
> implementation (on Windows only) involved a call back into avmplus logic
> that I needed to resolve. I speculate that this is a vestige from the Win95
> era when the localtime() API might have been less dependable.

Bug 645878, comment 57
> Abundance of caution: I would be interested in thoughts on this. AFIK it is
> the only change that isn't purely structural.

Bug 645878, comment 60
> [...]
> 
> The division turns out to be reasonably rational, and if you think that
> moving certain categories makes it more rational, I can attempt that (there
> may be dependencies I haven't discovered, though). For instance, moving the
> time functions to AVMPI would allow me to restore the previous Windows
> implementation of VMPI_getDaylightSavingsTA, reducing the risk). But I think
> the current division represents a reasonable balance of general
> functionality versus more specific MMgc and VM support. support.

Bug 645878, comment 70
> [...]
> - WinPortUtils.cpp, VMPI_getDaylightSavingsTA has been rewritten in a big
> way.
>   This method is notoriously difficult to get right.  I don't know why I
> should
>   belive that the rewritten version is correct / equivalent to the old
> version,
>   need to look into that further.

Bug 645878, comment 71
> I'm in favor of R+, but I will await feedback on the motivation for the
> rewrite of VMPI_getDaylightSavingsTA.

Bug 645878, comment 73
> [...]
> Well, yes, it's a big change but the net effect of it is a massive
> simplification and to rely on the system implementation the exactly same way
> as the other platforms do. I mentioned this in comment 44 in a previous
> review:
> 
>   "The existing implementation (on Windows only) involved a call back into
> avmplus logic that I needed to resolve. I speculate that this is a vestige
> from the Win95 era when the localtime() API might have been less dependable."
> 
> The greater mystery is why the Windows implementation of localtime() was
> deemed inadequate. I strongly suspect this code to be a historical vestige.
> You can argue that _not_ using localtime() risks causing a skew between the
> host OS/browser's concept of the time and the VM's.
> 
> Anyway, the solutions that seem feasible are:
>  - take this change and demystify the code (my preference, obviously)
>  - duplicate a bunch of timezone code from AVM into WinPortUtils (yuck).
>  - move all the time-related functionality to AVMPI, which can call into AVM
> core if it must.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

(Obviously its *possible* that the smoke tests are wrong.  But it seems unlikely in this case; even if the smoke tests were found to be incorrect and the change to invoke localtime is right, the change in behavior I wrote in the Description above certainly seems like it would have to be guarded by a bug-compatibility switch.)

Anyway, for the short term my main objective is to get the old behavior back, and thus hopefully get the player smoke tests passing again.

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Okay, I accomplished the short-term goal in P4 by reverting to the old code.  (See CL 932494).

But even if we wanted to apply that work-around in TR, we cannot, because it breaks gtest: all the code from Date would have to be linked into the gtest_vmbase library.  (I discovered this when I tried to actually do a sandbox run.) 

But the highest-priority issue (fixing the player's smoke test) is resolved, I think.  Now we just need to figure out the clean way to achieve the same effect.

Comment 3

[Stan Switzer]

(In reply to comment #0)
> The Player smoke tests have broken due to the change to
> VMPI_getDaylightSavingsTA.
> 
> In particular, the following expression:
>   new Date(-114457500000)
> used to return a date object with string rep:
>   "Mon May 16 23:15:00 GMT-0700 1966"
> but now it returns a date object with string rep:
>   "Mon May 16 22:15:00 GMT-0800 1966"

So apparently, unlike the Posix implementations, on Windows localtime does not work correctly (or at any event works differently) for dates before the epoch.

Intriguingly, the Wikipedia page on time_t (currently) says, "Some systems correctly handle negative time values, while others do not." So there you go. Mystery solved.

Comment 4

[Stan Switzer]

It would appear that the Tamarin test cases don't cover pre-epoch timezone adjustments but the Player ones do.

Comment 5

[Stan Switzer]

Related code in WebKit avoids pre-epoch dates for a different reason. A comment claims "the JavaScript standard explicitly dictates that historical information should not be considered when determining DST". So it takes pains to pick an "equivalent" year in the next 28 years (but no later than 2037). See equivalentYearForDST and calculateDSTOffset in DateMath.cpp.

Comment 6

[Tamarin Bot]

changeset: 6452:a81addf0b027
user:      Felix S Klock II <[e-mail]>
summary:   Bug 668442: rollback slice of patch for Bug 645878 to match FP code state (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/a81addf0b027

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #6)
> changeset: 6452:a81addf0b027
> user:      Felix S Klock II <[e-mail]>
> summary:   Bug 668442: rollback slice of patch for Bug 645878 to match FP
> code state (r=fklockii).
> 
> http://hg.mozilla.org/tamarin-redux/rev/a81addf0b027

(I pushed the rollback mostly to make the TR -> FR merge for this week, and the immediate future beyond this week, sane.)

Comment 8

[Stan Switzer]

A quick thought how this might be avoided, assuming you're in a position to give it a try.

//time is passed in as milliseconds from UTC.
double VMPI_getDaylightSavingsTA(double newtime)
{
    struct tm broken_down_time;
    
    //convert time from milliseconds
    newtime=newtime/kMsecPerSecond;
    
    time_t time_t_time=(time_t)newtime;
    
    // Windows localtime_s doesn't deal with times before the epoch, so
    // pick a date after tghe epoch that's a multiple of 28 years later.
    if (time_t_time <= 0)
    {
        static const int64_t secondsIn28Years = (365*3+366)*7*60*60*24;
        int64_t cycles = int64_t(-time_t_time)/secondsIn28Years + 1;
        int64_t incr = secondsIn28Years * cycles;
        time_t_time += incr;
    }

    //pull out a struct tm
    if (localtime_s( &broken_down_time, &time_t_time ) != 0)
    {
        return 0;
    }
    
    if (broken_down_time.tm_isdst > 0)
    {
        //daylight saving is definitely in effect.
        return kMsecPerHour;
    }
    
    //either daylight saving is not in effect, or we don't know (if tm_isdst is negative).
    return 0;
}

Comment 9

[Tamarin Bot]

changeset: 6461:0064647d010a
user:      Felix S Klock II <[e-mail]>
summary:   Bug 668442: rollback slice of patch for Bug 645878 to match FP code state (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/0064647d010a

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.