[Webkit-unassigned] [Bug 130967] erroneous date calculations on march 30, 2014 (CET/CEST time zone) the day on which daylight savings time changes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=130967


Mark Lam <mark.lam at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #229741|review?                     |review-
               Flag|                            |




--- Comment #23 from Mark Lam <mark.lam at apple.com>  2014-05-23 14:32:38 PST ---
(From update of attachment 229741)
View in context: https://bugs.webkit.org/attachment.cgi?id=229741&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        There are duplicataions on WTF and JavaScript Core,
> +        so we remove the Timezone conversion and call the function in WTF.

You misspelled "duplications" here.  In addition, from looking at the code, I'm not sure that it is really a duplication because the underlying implementation of local time offset computation is different between what's in JSDateMath and WTF::DateMath.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:236
> -static double parseDateFromNullTerminatedCharacters(VM& vm, const char* dateString)
> +static double parseDateFromNullTerminatedCharacters(const char* dateString)
>  {
> -    bool haveTZ;
> -    int offset;
> -    double ms = WTF::parseDateFromNullTerminatedCharacters(dateString, haveTZ, offset);
> +    double ms = WTF::parseDateFromNullTerminatedCharacters(dateString);
>      if (std::isnan(ms) || fabs(ms) > WTF::maxECMAScriptTime)
>          return std::numeric_limits<double>::quiet_NaN();
>  
> -    // fall back to local timezone
> -    if (!haveTZ)
> -        offset = localTimeOffset(vm, ms).offset / WTF::msPerMinute;
> -
> -    return ms - (offset * WTF::msPerMinute);
> +    return ms;

Here, you are effectively delegating the localTimeOffset computation to the WTF implementation.  However, at least on the surface, I see that the WTF implementation of calculateLocalTimeOffset() differs from the JSDateMath localTimeOffset().  I learned that ES5 has different rules for how JS is supposed to handle day light savings time computations, and I think the local time offset is responsible for that difference.  I'm not too familiar with what the ES5 spec says specifically here, but I'd be reluctant to go with this change unless we can have a greater understanding of what the ES5 spec says in this area, and are able to show that the WTF implementation satisfies the ES5 requirements.

Another thing that clued me into this issue is that, after applying your patch, there are still other parts of JSDateMath that uses the JSDateMath localTineOffset() and not the WTF version.  If it can be shown that the WTF implementation is correct and spec compliant for ES5, then everything should be switched over so that we don't have inconsistent behavior in different parts of the code.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.