Bug 213884 - [WK2] Add API to allow embedder to set a timezone override
Summary: [WK2] Add API to allow embedder to set a timezone override
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-02 08:20 PDT by Philippe Normand
Modified: 2022-05-11 18:23 PDT (History)
19 users (show)

See Also:


Attachments
Patch (41.09 KB, patch)
2020-07-02 08:33 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (41.88 KB, patch)
2020-07-03 04:58 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (45.02 KB, patch)
2020-07-08 09:12 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (45.09 KB, patch)
2020-07-08 09:15 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (39.25 KB, patch)
2020-07-17 03:51 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (46.41 KB, patch)
2020-09-17 04:37 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (46.32 KB, patch)
2020-09-17 08:44 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (46.29 KB, patch)
2020-10-16 01:17 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (44.32 KB, patch)
2022-04-15 12:05 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (44.76 KB, patch)
2022-04-15 18:22 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (49.44 KB, patch)
2022-04-18 18:45 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (57.26 KB, patch)
2022-04-27 15:28 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (57.29 KB, patch)
2022-04-27 16:06 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (57.29 KB, patch)
2022-04-27 19:01 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (47.09 KB, patch)
2022-04-29 19:11 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (47.26 KB, patch)
2022-05-02 11:22 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2020-07-02 08:20:05 PDT
There are ways to configure the timezone with the TZ environment variable, but having a generic API for this in the WebsiteDataStoreConfiguration would be nice to have as well. It would also allow fine-grained control over multiple pages, for instance it's not possible currently to have two pages in different timezones. For automation and testing purposes this is a limitation currently.
Comment 1 Philippe Normand 2020-07-02 08:33:32 PDT
Created attachment 403368 [details]
Patch
Comment 2 EWS Watchlist 2020-07-02 08:34:29 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Philippe Normand 2020-07-03 04:58:02 PDT
Created attachment 403451 [details]
Patch
Comment 4 Alex Christensen 2020-07-06 09:56:06 PDT
Comment on attachment 403451 [details]
Patch

This isn't really website data or associated with a network session.  I think this should go on WKPageConfigurationRef/WKWebViewConfiguration.
Comment 5 Philippe Normand 2020-07-08 09:12:14 PDT
Created attachment 403787 [details]
Patch
Comment 6 Philippe Normand 2020-07-08 09:15:55 PDT
Created attachment 403789 [details]
Patch
Comment 7 Philippe Normand 2020-07-10 05:31:56 PDT
Comment on attachment 403789 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403789&action=review

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:486
> +        WTF::setTimeZoneOverride(*parameters.timeZoneOverride);

There's no error handling here. If this returns false it might make sense to send an error message back to the UIProcess...
Comment 8 Philippe Normand 2020-07-15 12:01:38 PDT
Comment on attachment 403789 [details]
Patch

V3 coming ... soon.
Comment 9 Philippe Normand 2020-07-17 03:51:28 PDT
Created attachment 404553 [details]
Patch
Comment 10 Philippe Normand 2020-07-28 06:21:50 PDT
Ping?
Comment 11 Philippe Normand 2020-08-12 03:08:28 PDT
(In reply to Alex Christensen from comment #4)
> Comment on attachment 403451 [details]
> Patch
> 
> This isn't really website data or associated with a network session.  I
> think this should go on WKPageConfigurationRef/WKWebViewConfiguration.

This is the case now. A new review would be most welcome ;)
Comment 12 Alex Christensen 2020-08-12 09:09:17 PDT
I'm curious what the motivation for the addition of this API is.  Who needs this ability?
Comment 13 Philippe Normand 2020-08-13 01:58:25 PDT
It's often needed for testing purposes. See these posts for instance:

https://stackoverflow.com/questions/16448754/how-to-use-a-custom-time-in-browser-to-test-for-client-vs-server-time-difference
https://stackoverflow.com/questions/12220717/how-to-mock-the-browsers-timezone/31379288#comment60641443_31379288
https://stackoverflow.com/questions/11453740/fake-time-zone-for-web-app-testing

One way to simulate different timezone is to set the TZ env variable but it doesn't work the same way on all platforms, and it affects all webviews of the browser context. With this API you can simulate timezones on a per-webview basis.
Comment 14 Alex Christensen 2020-08-13 11:20:20 PDT
Comment on attachment 404553 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404553&action=review

> Source/WTF/wtf/DateMath.cpp:339
> +    if (tz.cal) {

I believe this uses uninitialized memory.  Add a { nullptr } to its declaration.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TimeZoneOverride.mm:7
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.

Is there a reason this is not using the 2-clause BSD we usually use for new files?  I think I'd prefer that unless there's a good reason to do this.
Comment 15 Carlos Garcia Campos 2020-08-14 02:48:14 PDT
Comment on attachment 404553 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404553&action=review

> Source/WTF/wtf/DateMath.cpp:1096
> +    // Timezone is ascii.
> +    Vector<UChar> buffer(timeZone.length());
> +    UChar* bufferStart = buffer.data();
> +    CString ctz = timeZone.utf8();
> +    if (!Unicode::convertUTF8ToUTF16(ctz.data(), ctz.data() + ctz.length(), &bufferStart, bufferStart + timeZone.length()))
> +        return false;
> +
> +    Vector<UChar, 32> canonicalBuffer(32);
> +    UErrorCode status = U_ZERO_ERROR;
> +    auto canonicalLength = ucal_getCanonicalTimeZoneID(buffer.data(), buffer.size(), canonicalBuffer.data(), canonicalBuffer.size(), nullptr, &status);
> +    if (status == U_BUFFER_OVERFLOW_ERROR) {
> +        status = U_ZERO_ERROR;
> +        canonicalBuffer.grow(canonicalLength);
> +        ucal_getCanonicalTimeZoneID(buffer.data(), buffer.size(), canonicalBuffer.data(), canonicalLength, nullptr, &status);
> +    }
> +    if (!U_SUCCESS(status))
> +        return false;

This is duplicated in isTimeZoneValid(). Could we make isTimeZoneValid() return an Optional<Vector<UChar, 32>>, for example?

> Source/WTF/wtf/DateMath.cpp:1119
> +String timeZoneOverride()

String& ? or do we really need to always copy?

> Source/WTF/wtf/DateMath.cpp:1124
> +String timeZoneDisplayNameOverride()

Ditto.

> Source/WebKit/Shared/WebPageCreationParameters.cpp:165
> +    encoder << timeZoneOverride;

The time zone is applied globally to the web process, not specific to a page, so I think it would be better in WebProcessCreationParameters

> Source/WebKit/UIProcess/API/APIPageConfiguration.h:167
> +    const WTF::String timeZoneOverride() const { return m_timeZoneOverride; }

String&

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1289
> +- (void)_setTimeZoneOverride:(NSString *)timeZone

I don't know if the cocoa api ensures one process per page nowadays, but if it doesn't I think this isn't the right place to set it.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:354
> +        g_value_set_string(value, context->priv->timeZoneOverride.data());

I would add a public getter for the property.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:387
> +        context->priv->timeZoneOverride = g_value_get_string(value);

I think we should validate the given timezone here, before setting it.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:580
> +     * The timezone override for this web context.

We should explain here what the timezone override is for and the expected format.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1939
> +    pageConfiguration->setTimeZoneOverride(context->priv->timeZoneOverride.data());

String::fromUTF8()

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:468
> +static void testWebContextTimeZone(WebViewTest* test, gconstpointer)

I would call this testWebContextDefaultTimeZone or NoOverride. You could also probably use a single test and just create two web contexts.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:475
> +    // By default the test harness uses the Pacific/Los_Angeles timezone which is +7 hours (420
> +    // minutes) compared to GMT.

This could be one line.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:476
> +    g_assert_cmpint(WebViewTest::javascriptResultToNumber(javascriptResult), ==, 420);

Doesn't this depend on the current locale? or do we change it always for tests?

> Tools/TestWebKitAPI/glib/WebKitGLib/TestMain.cpp:32
> +const char* Test::timeZoneOverride = "";

Better leave it uninitialized an nullptr will be passed when not set.

> Tools/TestWebKitAPI/glib/WebKitGLib/TestMain.h:144
> +            "time-zone-override", timeZoneOverride,

We don't need this is you simply create a custom web context in the test.

> Tools/flatpak/flatpakutils.py:720
> -                "TZ": "PST8PDT",
> +                "TZ": "America/Los_Angeles",

Ah! here it's. So, the test will always fail when run manually or using jhbuild. We shouldn't make the tests depend on this. Can't we change the environment variable in TestMain.cpp?
Comment 16 Philippe Normand 2020-08-14 04:06:30 PDT
Comment on attachment 404553 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404553&action=review

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1289
>> +- (void)_setTimeZoneOverride:(NSString *)timeZone
> 
> I don't know if the cocoa api ensures one process per page nowadays, but if it doesn't I think this isn't the right place to set it.

Alex, what do you think about this?
Comment 17 Philippe Normand 2020-08-14 05:11:44 PDT
Comment on attachment 404553 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404553&action=review

>> Source/WTF/wtf/DateMath.cpp:1096
>> +        return false;
> 
> This is duplicated in isTimeZoneValid(). Could we make isTimeZoneValid() return an Optional<Vector<UChar, 32>>, for example?

The canonicalLength would need to be returned as well. We could return an Optional<Vector<...>, int32_t>... I suppose. I'm not sure it would pay off though.

>> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:468
>> +static void testWebContextTimeZone(WebViewTest* test, gconstpointer)
> 
> I would call this testWebContextDefaultTimeZone or NoOverride. You could also probably use a single test and just create two web contexts.

But then I can't reuse the API of WebViewTest to run JS code and wait for the result.

>> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:476
>> +    g_assert_cmpint(WebViewTest::javascriptResultToNumber(javascriptResult), ==, 420);
> 
> Doesn't this depend on the current locale? or do we change it always for tests?

I can sprinkle a setenv call before this code and hope for the best!
Comment 18 Philippe Normand 2020-08-14 05:25:53 PDT
Comment on attachment 404553 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404553&action=review

>> Source/WebKit/Shared/WebPageCreationParameters.cpp:165
>> +    encoder << timeZoneOverride;
> 
> The time zone is applied globally to the web process, not specific to a page, so I think it would be better in WebProcessCreationParameters

This contradicts comment 4 then? Before moving this flag from APIPageConfiguration to APIProcessPoolConfiguration I would like to reach an agreement between the reviewers involved here.
Comment 19 Carlos Garcia Campos 2020-08-14 06:16:47 PDT
Comment on attachment 404553 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404553&action=review

>>> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:468
>>> +static void testWebContextTimeZone(WebViewTest* test, gconstpointer)
>> 
>> I would call this testWebContextDefaultTimeZone or NoOverride. You could also probably use a single test and just create two web contexts.
> 
> But then I can't reuse the API of WebViewTest to run JS code and wait for the result.

Yes, it's just a function call, and you can use a lambda for the callback. I think it's weird to have two test cases for the same thing.
Comment 20 Alex Christensen 2020-08-14 08:18:04 PDT
(In reply to Philippe Normand from comment #16)
> Comment on attachment 404553 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404553&action=review
> 
> >> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1289
> >> +- (void)_setTimeZoneOverride:(NSString *)timeZone
> > 
> > I don't know if the cocoa api ensures one process per page nowadays, but if it doesn't I think this isn't the right place to set it.
> 
> Alex, what do you think about this?

Not quite, but close.  This property should definitely be on the WKWebViewConfiguration instead of WebsiteDataStoreConfiguration.
Comment 21 Carlos Garcia Campos 2020-08-14 08:44:36 PDT
(In reply to Alex Christensen from comment #20)
> (In reply to Philippe Normand from comment #16)
> > Comment on attachment 404553 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=404553&action=review
> > 
> > >> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1289
> > >> +- (void)_setTimeZoneOverride:(NSString *)timeZone
> > > 
> > > I don't know if the cocoa api ensures one process per page nowadays, but if it doesn't I think this isn't the right place to set it.
> > 
> > Alex, what do you think about this?
> 
> Not quite, but close.  This property should definitely be on the
> WKWebViewConfiguration instead of WebsiteDataStoreConfiguration.

Not website data sore, but process pool, since it's global to the web process, it's not specific to the web view.
Comment 22 Philippe Normand 2020-08-24 04:47:25 PDT
Hi Alex, back to this patch after a week off. So, would you agree to have this API in _WKProcessPoolConfiguration as Carlos is suggesting?
Comment 23 Alex Christensen 2020-08-24 10:17:43 PDT
The implementation currently uses a process-global override in the web process.  There is no reason it must be process-global, right?  It seems like an implementation shortcut.
Comment 24 Philippe Normand 2020-08-24 10:35:41 PDT
(In reply to Alex Christensen from comment #23)
> The implementation currently uses a process-global override in the web
> process.  There is no reason it must be process-global, right?  It seems
> like an implementation shortcut.

It's a process-global implementation because the DateTime functions like formatDateTime() are not context-aware, AFAICT. The timezone information is retrieved from ICU or from the override static struct.
Comment 25 Alex Christensen 2020-08-24 10:50:39 PDT
formatDateTime might not be context aware now, but could it be made context aware?
Comment 26 Philippe Normand 2020-08-25 03:23:16 PDT
(In reply to Alex Christensen from comment #25)
> formatDateTime might not be context aware now, but could it be made context
> aware?

Maybe yes, the TimeZoneOverride struct could move to the JSC::VM, for instance. But then another issue is with calculateLocalTimeOffset() which is called from various places in WebCore, that does look like a non-trivial amount of work.
Comment 27 Alex Christensen 2020-08-25 10:37:03 PDT
I agree it's not a trivial amount of work, but I think it's better than cementing us into an API shape that is based on how convenient it is to implement a certain way.
Comment 28 Carlos Garcia Campos 2020-09-09 02:10:12 PDT
I think it makes sense to override the timezone globally, since we are emulating the system has a different timezone. This is just for testing and should be documented as such, so I don't think it's worth the effort to make the timezone per web view just for this.
Comment 29 Philippe Normand 2020-09-17 04:37:01 PDT
Created attachment 409021 [details]
Patch
Comment 30 EWS Watchlist 2020-09-17 04:38:09 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 31 Philippe Normand 2020-09-17 08:44:29 PDT
Created attachment 409036 [details]
Patch
Comment 32 Darin Adler 2020-09-20 11:11:10 PDT
Comment on attachment 409036 [details]
Patch

Test is failing on macOS. Need a revised version that resolves that.
Comment 33 Philippe Normand 2020-10-16 01:17:38 PDT
Created attachment 411545 [details]
Patch
Comment 34 Philippe Normand 2020-10-16 03:18:14 PDT
Comment on attachment 411545 [details]
Patch

Failed
    TestWebKitAPI.TimeZoneOverrideTest.TimeZoneOverride
        _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
        
        /Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/TimeZoneOverride.mm:66
        Expected equality of these values:
          [result stringValue]
            Which is: "420"
          "-120"
Comment 35 Yury Semikhatsky 2022-04-15 12:05:42 PDT
Created attachment 457711 [details]
Patch
Comment 36 Yury Semikhatsky 2022-04-15 12:08:33 PDT
(In reply to Darin Adler from comment #32)
> Comment on attachment 409036 [details]
> Patch
> 
> Test is failing on macOS. Need a revised version that resolves that.

Resurrecting this patch. I've updated the implementation to make it work on all platforms, the macOS test is no passing.

Darin, Alex, can you do a review?
Comment 37 Darin Adler 2022-04-15 13:20:27 PDT
Comment on attachment 457711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457711&action=review

Looks fine assuming all tests pass, but I think they’re not all passing yet

> Source/JavaScriptCore/ChangeLog:3
> +        [WK2] Add API to allow embedder to set a timezone override

I’m not sure I love the use of "override" here as a noun meaning "an explicitly set value that overrides the normal automatic time zone determination". I wish there was a less peculiar term of art. But for now, I just went with it.

> Source/JavaScriptCore/runtime/DateConversion.cpp:101
> +            String timeZoneOverride = WTF::timeZoneDisplayNameOverride();

This is not how we currently handle namespace in WTF. There should be a "using" in the header and no WTF prefix here.

> Source/JavaScriptCore/runtime/DateConversion.cpp:102
> +            if (!timeZoneOverride.isEmpty())

Nice style to scope the value by putting it inside the if, also allows using a one word name:

    if (auto& override = timeZoneDisplayNameOverride(); !override.isEmpty())
        timeZoneName = override;

> Source/JavaScriptCore/runtime/DateConversion.cpp:109
> +                const WCHAR* winTimeZoneName = t.isDST() ? timeZoneInformation.DaylightName : timeZoneInformation.StandardName;
> +                timeZoneName = String(winTimeZoneName);

Can we merge these into one line?

    timeZoneName = String { t.isDST() ? timeZoneInformation.DaylightName : timeZoneInformation.StandardName };

> Source/JavaScriptCore/runtime/DateConversion.cpp:114
> +                struct tm gtm = t;
> +                char tzName[70];
> +                strftime(tzName, sizeof(tzName), "%Z", &gtm);
> +                timeZoneName = String::fromUTF8(tzName);

This code is, and always has been, incorrect for cases where the time zone doesn’t fit into the buffer. In that case, strftime returns 0, and the contents of the buffer are *indeterminate*. We should fix this small mistake in how we are calling strftime by not looking at tzName if strftime returns 0.

We also could give the local variables better names, but that’s not required.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:337
> +    String tz = WTF::timeZoneOverride();
> +    if (!tz.isEmpty())
> +        return tz;

In new code, please follow WebKit coding style and use words, not abbreviations, for variable names:

    if (auto& override = timeZoneOverride; !override.isEmpty())
        return override;

> Source/JavaScriptCore/runtime/JSDateMath.cpp:421
> +        m_timeZoneCache = std::unique_ptr<OpaqueICUTimeZone, OpaqueICUTimeZoneDeleter>(bitwise_cast<OpaqueICUTimeZone*>(timezone));

Frustrating that this requires direct use of bitwise_cast. We should find a safer way to encapsulate that.

I’d also like this to be more like a one-liner the way the code just below is; want the memory allocation and the taking ownership to be as tightly bound as possible. If the line is too long, we can help by naming the specific std::unique_ptr with arguments so we don’t have to write it out.

> Source/WTF/wtf/DateMath.cpp:343
> +        int32_t offset = ucal_get(tz.cal, UCAL_ZONE_OFFSET, &status);
> +        int32_t dstOffset = ucal_get(tz.cal, UCAL_DST_OFFSET, &status);

May need some error checking here. We are just ignoring status. That’s not necessarily safe.

> Source/WTF/wtf/DateMath.cpp:1046
> +    // Timezone is ascii.

Not clear on the purpose of this comment.

> Source/WTF/wtf/DateMath.cpp:1051
> +    UChar* bufferStart = buffer.data();
> +    CString ctz = timeZone.utf8();
> +    if (!Unicode::convertUTF8ToUTF16(ctz.data(), ctz.data() + ctz.length(), &bufferStart, bufferStart + timeZone.length()))
> +        return std::nullopt;

This is a very strange way to convert a WTF::String to UTF-16. There is no reason to first convert the UTF-8 and then back to UTF-16. A better way is:

  StringView { timeZone }.getCharactersWithUpconvert(buffer.data());

We can possibly optimize even further. Changing the argument to this function to already be StringView will optimize still further.

> Source/WTF/wtf/DateMath.h:396
> +WTF_EXPORT_PRIVATE std::optional<Vector<UChar, 32>> isTimeZoneValid(const String&);

I suggest changing the argument type here to StringView, since this function takes no advantage of the fact that it’s already in a String.

This is not the correct name for a function that parses a time zone and returns the canonical form. I would call this parseTimeZone or computeCanonicalTimeZone, or something like that.

If the external caller only wants to check *if* the time zone is valid, I suggest publishing only the version that returns a bool rather than returning the parsed time zone to that caller. That can be a cover that just converts the optional to a boolean on the way out.

> Source/WTF/wtf/DateMath.h:397
> +WTF_EXPORT_PRIVATE bool setTimeZoneOverride(const String&);

Callers are making WTF::String just to pass to this function, but that is unnecessary. Given how it’s used, I suggest we instead have this function take a StringView, or even a const char*. It would be easy for the callers to deal with either of those. If we must use UTF-16, then I suggest StringView rather than const String&, but I think const char* would work well.

> Source/WTF/wtf/DateMath.h:399
> +WTF_EXPORT_PRIVATE String& timeZoneOverride();
> +WTF_EXPORT_PRIVATE String& timeZoneDisplayNameOverride();

These should return const String&, unlesss we intend callers to use the returned references to modify the values.

> Source/WebKit/Shared/WebProcessCreationParameters.h:259
> +    std::optional<String> timeZoneOverride;

I suggest we use String here, since String has a null value. Using std::optional<String> is unnecessary. When the code is not generic across multiple types, I strongly suggest not ever using optional<String>.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:400
> +    case PROP_TIME_ZONE_OVERRIDE: {
> +        const char* timeZoneOverride = g_value_get_string(value);
> +        if (timeZoneOverride)
> +            webkit_web_context_set_time_zone_override(context, timeZoneOverride);
> +        break;
> +    }

No braces needed:

    if (auto* override = g_value_get_string(value))
        webkit_web_context_set_time_zone_override(context, override);
    break;

> Source/WebKit/WebProcess/WebProcess.cpp:505
> +    if (parameters.timeZoneOverride)
> +        WTF::setTimeZoneOverride(*parameters.timeZoneOverride);
> +    else
> +        WTF::setTimeZoneOverride({ });

Should not have WTF:: prefixes here. See my comments above.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TimeZoneOverride.mm:28
> +#if PLATFORM(MAC)

This should be COCOA instead of MAC. Unless there’s some reason this doesn’t work on iOS?
Comment 38 Darin Adler 2022-04-15 17:00:19 PDT
Comment on attachment 457711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457711&action=review

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:337
>> +        return tz;
> 
> In new code, please follow WebKit coding style and use words, not abbreviations, for variable names:
> 
>     if (auto& override = timeZoneOverride; !override.isEmpty())
>         return override;

Or if you prefer the word "zone" to "override", or even "timeZone".
Comment 39 Yury Semikhatsky 2022-04-15 18:22:00 PDT
Comment on attachment 457711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457711&action=review

>> Source/JavaScriptCore/ChangeLog:3
>> +        [WK2] Add API to allow embedder to set a timezone override
> 
> I’m not sure I love the use of "override" here as a noun meaning "an explicitly set value that overrides the normal automatic time zone determination". I wish there was a less peculiar term of art. But for now, I just went with it.

Some of the methods in the code use *ForAutomation suffix, we could employ it here as well to make its intent clear (e.g. timeZoneForAutomation/setTimeZoneForAutomation), it makes less sense in the public WebKit API though.

>> Source/JavaScriptCore/runtime/DateConversion.cpp:101
>> +            String timeZoneOverride = WTF::timeZoneDisplayNameOverride();
> 
> This is not how we currently handle namespace in WTF. There should be a "using" in the header and no WTF prefix here.

Fixed.

>> Source/JavaScriptCore/runtime/DateConversion.cpp:102
>> +            if (!timeZoneOverride.isEmpty())
> 
> Nice style to scope the value by putting it inside the if, also allows using a one word name:
> 
>     if (auto& override = timeZoneDisplayNameOverride(); !override.isEmpty())
>         timeZoneName = override;

Done.

>> Source/JavaScriptCore/runtime/DateConversion.cpp:109
>> +                timeZoneName = String(winTimeZoneName);
> 
> Can we merge these into one line?
> 
>     timeZoneName = String { t.isDST() ? timeZoneInformation.DaylightName : timeZoneInformation.StandardName };

Done.

>> Source/JavaScriptCore/runtime/DateConversion.cpp:114
>> +                timeZoneName = String::fromUTF8(tzName);
> 
> This code is, and always has been, incorrect for cases where the time zone doesn’t fit into the buffer. In that case, strftime returns 0, and the contents of the buffer are *indeterminate*. We should fix this small mistake in how we are calling strftime by not looking at tzName if strftime returns 0.
> 
> We also could give the local variables better names, but that’s not required.

Fixed.

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:337
>> +        return tz;
> 
> In new code, please follow WebKit coding style and use words, not abbreviations, for variable names:
> 
>     if (auto& override = timeZoneOverride; !override.isEmpty())
>         return override;

Done.

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:421
>> +        m_timeZoneCache = std::unique_ptr<OpaqueICUTimeZone, OpaqueICUTimeZoneDeleter>(bitwise_cast<OpaqueICUTimeZone*>(timezone));
> 
> Frustrating that this requires direct use of bitwise_cast. We should find a safer way to encapsulate that.
> 
> I’d also like this to be more like a one-liner the way the code just below is; want the memory allocation and the taking ownership to be as tightly bound as possible. If the line is too long, we can help by naming the specific std::unique_ptr with arguments so we don’t have to write it out.

Changed this into one-liner, the line length is ok-ish (shorter than several other in this file).

As for bitwise_cast I don't see an easy way to change this considering the comment in the header file that icu::TimeZone cannot be exposed in the header. Leaving it consistent with the existing code.

>> Source/WTF/wtf/DateMath.cpp:343
>> +        int32_t dstOffset = ucal_get(tz.cal, UCAL_DST_OFFSET, &status);
> 
> May need some error checking here. We are just ignoring status. That’s not necessarily safe.

Added checks that return 0 offset in case of error.

>> Source/WTF/wtf/DateMath.cpp:1046
>> +    // Timezone is ascii.
> 
> Not clear on the purpose of this comment.

Removed.

>> Source/WTF/wtf/DateMath.cpp:1051
>> +        return std::nullopt;
> 
> This is a very strange way to convert a WTF::String to UTF-16. There is no reason to first convert the UTF-8 and then back to UTF-16. A better way is:
> 
>   StringView { timeZone }.getCharactersWithUpconvert(buffer.data());
> 
> We can possibly optimize even further. Changing the argument to this function to already be StringView will optimize still further.

Done. Updated parameter type to StringView.

>> Source/WTF/wtf/DateMath.h:396
>> +WTF_EXPORT_PRIVATE std::optional<Vector<UChar, 32>> isTimeZoneValid(const String&);
> 
> I suggest changing the argument type here to StringView, since this function takes no advantage of the fact that it’s already in a String.
> 
> This is not the correct name for a function that parses a time zone and returns the canonical form. I would call this parseTimeZone or computeCanonicalTimeZone, or something like that.
> 
> If the external caller only wants to check *if* the time zone is valid, I suggest publishing only the version that returns a bool rather than returning the parsed time zone to that caller. That can be a cover that just converts the optional to a boolean on the way out.

Changed argument to StringView, renamed the method to validateTimeZone and made it private to DateMath.cpp. Only isTimeZoneValid exposed in the header now.

>> Source/WTF/wtf/DateMath.h:397
>> +WTF_EXPORT_PRIVATE bool setTimeZoneOverride(const String&);
> 
> Callers are making WTF::String just to pass to this function, but that is unnecessary. Given how it’s used, I suggest we instead have this function take a StringView, or even a const char*. It would be easy for the callers to deal with either of those. If we must use UTF-16, then I suggest StringView rather than const String&, but I think const char* would work well.

Changed the argument type to StringView for consistency, but this method is only invoked during WebProcess initialization with the value that comes from the creating parameters, i.e. always a String.

>> Source/WTF/wtf/DateMath.h:399
>> +WTF_EXPORT_PRIVATE String& timeZoneDisplayNameOverride();
> 
> These should return const String&, unlesss we intend callers to use the returned references to modify the values.

Done.

>> Source/WebKit/Shared/WebProcessCreationParameters.h:259
>> +    std::optional<String> timeZoneOverride;
> 
> I suggest we use String here, since String has a null value. Using std::optional<String> is unnecessary. When the code is not generic across multiple types, I strongly suggest not ever using optional<String>.

Good point. Changed to String.

>> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:400
>> +    }
> 
> No braces needed:
> 
>     if (auto* override = g_value_get_string(value))
>         webkit_web_context_set_time_zone_override(context, override);
>     break;

Done.

>> Source/WebKit/WebProcess/WebProcess.cpp:505
>> +        WTF::setTimeZoneOverride({ });
> 
> Should not have WTF:: prefixes here. See my comments above.

Done.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/TimeZoneOverride.mm:28
>> +#if PLATFORM(MAC)
> 
> This should be COCOA instead of MAC. Unless there’s some reason this doesn’t work on iOS?

Changed to COCOA.
Comment 40 Yury Semikhatsky 2022-04-15 18:22:31 PDT
Created attachment 457739 [details]
Patch
Comment 41 Yury Semikhatsky 2022-04-15 18:24:17 PDT
(In reply to Darin Adler from comment #38)
> Comment on attachment 457711 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457711&action=review
> 
> >> Source/JavaScriptCore/runtime/JSDateMath.cpp:337
> >> +        return tz;
> > 
> > In new code, please follow WebKit coding style and use words, not abbreviations, for variable names:
> > 
> >     if (auto& override = timeZoneOverride; !override.isEmpty())
> >         return override;
> 
> Or if you prefer the word "zone" to "override", or even "timeZone".

"override" compiled well, leaving it.
Comment 42 Darin Adler 2022-04-15 18:27:33 PDT
Comment on attachment 457711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457711&action=review

>>> Source/JavaScriptCore/runtime/JSDateMath.cpp:421
>>> +        m_timeZoneCache = std::unique_ptr<OpaqueICUTimeZone, OpaqueICUTimeZoneDeleter>(bitwise_cast<OpaqueICUTimeZone*>(timezone));
>> 
>> Frustrating that this requires direct use of bitwise_cast. We should find a safer way to encapsulate that.
>> 
>> I’d also like this to be more like a one-liner the way the code just below is; want the memory allocation and the taking ownership to be as tightly bound as possible. If the line is too long, we can help by naming the specific std::unique_ptr with arguments so we don’t have to write it out.
> 
> Changed this into one-liner, the line length is ok-ish (shorter than several other in this file).
> 
> As for bitwise_cast I don't see an easy way to change this considering the comment in the header file that icu::TimeZone cannot be exposed in the header. Leaving it consistent with the existing code.

The change I was suggesting, sorry I was not more explicit, for bitwise_cast is to *encapsulate* the technique in a single pair of inline functions to convert between icu::TimeZone* and OpaqueICUTimeZone*; all the call sites would use this in a type-safe way and wouldn’t literally have individual calls to bitwise_cast.
Comment 43 Yusuke Suzuki 2022-04-15 18:49:49 PDT
Comment on attachment 457739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457739&action=review

Commented.

> Source/JavaScriptCore/runtime/DateConversion.cpp:104
> +            String timeZoneName;
> +            String timeZoneOverride = timeZoneDisplayNameOverride();
> +            if (auto& override = timeZoneDisplayNameOverride(); !override.isEmpty())
> +                timeZoneName = override;
> +            else {

Can we avoid String ref-churn here? (And we need to do that since String is not thread safe).

> Source/WTF/wtf/DateMath.cpp:101
> +    UCalendar* cal { nullptr };

Use std::unique_ptr<UCalendar, ICUDeleter<ucal_close>> instead.

> Source/WTF/wtf/DateMath.cpp:103
> +    String id;
> +    String displayName;

This does not work since String is not thread safe.
If it is used by multiple threads, we need another way to keep String.
Maybe Vector<UChar, ...> instead?

> Source/WTF/wtf/DateMath.cpp:108
> +    static NeverDestroyed<TimeZoneOverride> timeZoneOverride;

static in WebKit C++ is not thread safe. Let's use

LazyNeverDestroyed<TimeZoneOverride>

and std::call_once to initialize it once.

> Source/WTF/wtf/DateMath.cpp:1080
> +    if (innerTimeZoneOverride().cal) {
> +        ucal_close(innerTimeZoneOverride().cal);
> +        innerTimeZoneOverride().cal = nullptr;
> +    }

Let's not do that, and instead, use std::unique_ptr.

> Source/WTF/wtf/DateMath.cpp:1093
> +    UCalendar* cal = ucal_open(canonicalBuffer->data(), canonicalLength, nullptr, UCAL_TRADITIONAL, &status);

For ICU data structures, we use

std::unique_ptr<UCalendar, ICUDeleter<ucal_close>>

etc. to avoid memory leak. See Intl code in JSC.

> Source/WTF/wtf/DateMath.cpp:1105
> +    Vector<UChar, 32> displayNameBuffer(32);
> +    auto displayNameLength = ucal_getTimeZoneDisplayName(cal, UCAL_STANDARD, defaultLanguage().utf8().data(), displayNameBuffer.data(), displayNameBuffer.size(), &status);
> +    if (status == U_BUFFER_OVERFLOW_ERROR) {
> +        status = U_ZERO_ERROR;
> +        displayNameBuffer.grow(displayNameLength);
> +        ucal_getTimeZoneDisplayName(cal, UCAL_STANDARD, defaultLanguage().utf8().data(), displayNameBuffer.data(), displayNameLength, &status);
> +    }
> +    if (!U_SUCCESS(status))
> +        return false;

We should use callBufferProducingFunction.
Comment 44 Yusuke Suzuki 2022-04-15 18:51:17 PDT
Comment on attachment 457739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457739&action=review

> Source/WTF/wtf/DateMath.cpp:1075
> +bool setTimeZoneOverride(const StringView& timeZone)
> +{

This function is modifying global state. Do we need to have Lock to ensure this function can be a thread safe?
If so, then, it is also true that accessors (timeZoneOverride etc.) also need to take a lock.
Comment 45 Yury Semikhatsky 2022-04-15 20:16:46 PDT
Comment on attachment 457739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457739&action=review

>> Source/WTF/wtf/DateMath.cpp:108
>> +    static NeverDestroyed<TimeZoneOverride> timeZoneOverride;
> 
> static in WebKit C++ is not thread safe. Let's use
> 
> LazyNeverDestroyed<TimeZoneOverride>
> 
> and std::call_once to initialize it once.

I believe it is thread-safe since c++11, see "Static local variables" section in https://en.cppreference.com/w/cpp/language/storage_duration: "If multiple threads attempt to initialize the same static local variable concurrently, the initialization occurs exactly once (similar behavior can be obtained for arbitrary functions with std::call_once)."

>> Source/WTF/wtf/DateMath.cpp:1075
>> +{
> 
> This function is modifying global state. Do we need to have Lock to ensure this function can be a thread safe?
> If so, then, it is also true that accessors (timeZoneOverride etc.) also need to take a lock.

This function itself is only called once when WebProcess start, so perhaps we can rename it to initializeTimeZoneOverride and make sure it's called only once. If I understand correctly, web workers will have JSDateMath instances that are used on different threads and they will access timeZoneOverride() concurrently. Even though the access to timeZoneOverride() is always on a slow path it's a bit annoying that we need to introduce locks to access it. I thought maybe there is a trick/technique that could be used here to avoid locking and e.g. just get the override in the DateCache/VM constructor at some point where another (global?) lock is already held? If not I will just introduce locks.
Comment 46 Darin Adler 2022-04-16 12:49:11 PDT
Comment on attachment 457739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457739&action=review

>>> Source/WTF/wtf/DateMath.cpp:108
>>> +    static NeverDestroyed<TimeZoneOverride> timeZoneOverride;
>> 
>> static in WebKit C++ is not thread safe. Let's use
>> 
>> LazyNeverDestroyed<TimeZoneOverride>
>> 
>> and std::call_once to initialize it once.
> 
> I believe it is thread-safe since c++11, see "Static local variables" section in https://en.cppreference.com/w/cpp/language/storage_duration: "If multiple threads attempt to initialize the same static local variable concurrently, the initialization occurs exactly once (similar behavior can be obtained for arbitrary functions with std::call_once)."

It is *not*, because WebKit is compiled in a non-standard mode that turns that thread safety off, at least for Apple platforms.

The option is -fno-threadsafe-statics
Comment 47 Yury Semikhatsky 2022-04-18 18:41:06 PDT
Comment on attachment 457711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457711&action=review

>>>> Source/JavaScriptCore/runtime/JSDateMath.cpp:421
>>>> +        m_timeZoneCache = std::unique_ptr<OpaqueICUTimeZone, OpaqueICUTimeZoneDeleter>(bitwise_cast<OpaqueICUTimeZone*>(timezone));
>>> 
>>> Frustrating that this requires direct use of bitwise_cast. We should find a safer way to encapsulate that.
>>> 
>>> I’d also like this to be more like a one-liner the way the code just below is; want the memory allocation and the taking ownership to be as tightly bound as possible. If the line is too long, we can help by naming the specific std::unique_ptr with arguments so we don’t have to write it out.
>> 
>> Changed this into one-liner, the line length is ok-ish (shorter than several other in this file).
>> 
>> As for bitwise_cast I don't see an easy way to change this considering the comment in the header file that icu::TimeZone cannot be exposed in the header. Leaving it consistent with the existing code.
> 
> The change I was suggesting, sorry I was not more explicit, for bitwise_cast is to *encapsulate* the technique in a single pair of inline functions to convert between icu::TimeZone* and OpaqueICUTimeZone*; all the call sites would use this in a type-safe way and wouldn’t literally have individual calls to bitwise_cast.

Moved bitwise_cast into 2 functions as you suggested.
Comment 48 Yury Semikhatsky 2022-04-18 18:45:18 PDT
Created attachment 457847 [details]
Patch
Comment 49 Yury Semikhatsky 2022-04-18 18:47:09 PDT
Comment on attachment 457739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457739&action=review

>> Source/JavaScriptCore/runtime/DateConversion.cpp:104
>> +            else {
> 
> Can we avoid String ref-churn here? (And we need to do that since String is not thread safe).

Changed timeZoneDisplayNameOverride to return a new instance of the string.

>> Source/WTF/wtf/DateMath.cpp:101
>> +    UCalendar* cal { nullptr };
> 
> Use std::unique_ptr<UCalendar, ICUDeleter<ucal_close>> instead.

Done.

>> Source/WTF/wtf/DateMath.cpp:103
>> +    String displayName;
> 
> This does not work since String is not thread safe.
> If it is used by multiple threads, we need another way to keep String.
> Maybe Vector<UChar, ...> instead?

Done.

>>>> Source/WTF/wtf/DateMath.cpp:108
>>>> +    static NeverDestroyed<TimeZoneOverride> timeZoneOverride;
>>> 
>>> static in WebKit C++ is not thread safe. Let's use
>>> 
>>> LazyNeverDestroyed<TimeZoneOverride>
>>> 
>>> and std::call_once to initialize it once.
>> 
>> I believe it is thread-safe since c++11, see "Static local variables" section in https://en.cppreference.com/w/cpp/language/storage_duration: "If multiple threads attempt to initialize the same static local variable concurrently, the initialization occurs exactly once (similar behavior can be obtained for arbitrary functions with std::call_once)."
> 
> It is *not*, because WebKit is compiled in a non-standard mode that turns that thread safety off, at least for Apple platforms.
> 
> The option is -fno-threadsafe-statics

Got it, updated the code to use a lock while accessing innerTimeZoneOverride.

>> Source/WTF/wtf/DateMath.cpp:1080
>> +    }
> 
> Let's not do that, and instead, use std::unique_ptr.

Done.

>> Source/WTF/wtf/DateMath.cpp:1093
>> +    UCalendar* cal = ucal_open(canonicalBuffer->data(), canonicalLength, nullptr, UCAL_TRADITIONAL, &status);
> 
> For ICU data structures, we use
> 
> std::unique_ptr<UCalendar, ICUDeleter<ucal_close>>
> 
> etc. to avoid memory leak. See Intl code in JSC.

Done.

>> Source/WTF/wtf/DateMath.cpp:1105
>> +        return false;
> 
> We should use callBufferProducingFunction.

Done. Thanks for the pointers!
Comment 50 Yury Semikhatsky 2022-04-18 18:50:56 PDT
Comment on attachment 457739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457739&action=review

>>>>> Source/WTF/wtf/DateMath.cpp:108
>>>>> +    static NeverDestroyed<TimeZoneOverride> timeZoneOverride;
>>>> 
>>>> static in WebKit C++ is not thread safe. Let's use
>>>> 
>>>> LazyNeverDestroyed<TimeZoneOverride>
>>>> 
>>>> and std::call_once to initialize it once.
>>> 
>>> I believe it is thread-safe since c++11, see "Static local variables" section in https://en.cppreference.com/w/cpp/language/storage_duration: "If multiple threads attempt to initialize the same static local variable concurrently, the initialization occurs exactly once (similar behavior can be obtained for arbitrary functions with std::call_once)."
>> 
>> It is *not*, because WebKit is compiled in a non-standard mode that turns that thread safety off, at least for Apple platforms.
>> 
>> The option is -fno-threadsafe-statics
> 
> Got it, updated the code to use a lock while accessing innerTimeZoneOverride.

I also added a test that runs the code in workers, it should provide some coverage for the multi-threaded case.
Comment 51 Yury Semikhatsky 2022-04-20 11:25:54 PDT
Darin, Yusuke, I've addressed your comments, can you have another look at the patch?
Comment 52 Darin Adler 2022-04-20 13:21:03 PDT
Comment on attachment 457847 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457847&action=review

I want to be sure that we have been careful enough about performance implications here. How are we measuring the performance impact? Typically when touching code like this I want to improve performance and to me it seems here there is a risk of slowing things down

Typically getting thread safety right while preserving high performance is a challenge; the thread safety is difficult to test and may have to be assessed by code inspection, but the performance typically can be tested

I’m mostly concerned about performance when there is no override, not performance when there is an override, but that is a secondary concern

> Source/JavaScriptCore/runtime/DateConversion.cpp:101
> +            if (auto override = timeZoneDisplayNameOverride(); !override.isEmpty())

So we are now taking and releasing a lock every time this function is called. Have we proven that is that OK for JavaScript performance? Yusuke, maybe you have a sense of this. Maybe strftime is already slow enough that this is not a concern?

> Source/JavaScriptCore/runtime/DateConversion.cpp:102
> +                timeZoneName = override;

Quite unfortunate to convert this to a temporary WTF::String every time; that’s not needed to append a time zone name to a StringBuilder. I’m really unclear on how performance critical this code is. The way it’s currently working is overusing String. There’s no need to create and destroy a String to append some character to a StringBuilder. This applies to the strftime code below just as much to this override code.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:345
> +    if (auto override = timeZoneOverride(); !override.isEmpty())
> +        return String::adopt(WTFMove(override));

Seems we would want this to be fast when called multiple times, and this object is called a "cache". Can we find a way to not lock/unlock every time this is called just to check if an override is in effect?

> Source/JavaScriptCore/runtime/JSDateMath.cpp:399
> +    Vector<UChar> override = timeZoneOverride();

Seems unfortunate we make no attempt to optimize this to not use the heap. These are short strings and always doing malloc/free is costly.

> Source/WTF/wtf/DateMath.cpp:1055
> +static std::optional<Vector<UChar>> validateTimeZone(const StringView& timeZone)

Type should be StringView, not const StringView&, in all these cases. In production builds a StringView fits into registers, and passing const StringView& instead typically just hurts performance and has no benefit.

> Source/WTF/wtf/DateMath.cpp:1057
> +    Vector<UChar> buffer(timeZone.length());

In a case like this we almost always use inline capacity so there’s no unnecessary heap allocation. This allocates/deallocates every time, which is unnecessary.

> Source/WTF/wtf/DateMath.cpp:1061
> +    canonicalBuffer.reserveCapacity(buffer.size());

Not sure this is a good optimization. It could cause a problem if the canonical ID is often slightly larger than the non-canonical.

> Source/WTF/wtf/DateMath.cpp:1065
> +    return canonicalBuffer;

I believe we may need WTFMove here to avoid allocating a new heap buffer, since the return type is not Vector<UChar> but std::optional<Vector<UChar>>.

> Source/WTF/wtf/DateMath.cpp:1068
> +bool isTimeZoneValid(const StringView& timeZone)

StringView

> Source/WTF/wtf/DateMath.cpp:1073
> +bool setTimeZoneOverride(const StringView& timeZone)

StringView

> Source/WTF/wtf/DateMath.cpp:1116
> +    auto& displayName = innerTimeZoneOverride().displayName;
> +    return String(displayName);

This should be a single line.

> Source/WTF/wtf/DateMath.h:432
> +using WTF::isTimeZoneValid;
> +using WTF::setTimeZoneOverride;
> +using WTF::timeZoneOverride;
> +using WTF::timeZoneDisplayNameOverride;
>  using WTF::calculateLocalTimeOffset;
>  using WTF::timeClip;
>  using WTF::jsCurrentTime;

We sort these alphabetically. The new ones should be sorted in with the existing ones.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TimeZoneOverride.mm:2
> + * Copyright (C) 2020 Igalia S.L.

2022?
Comment 53 Yury Semikhatsky 2022-04-25 17:34:26 PDT
Comment on attachment 457847 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457847&action=review

>> Source/JavaScriptCore/runtime/DateConversion.cpp:101
>> +            if (auto override = timeZoneDisplayNameOverride(); !override.isEmpty())
> 
> So we are now taking and releasing a lock every time this function is called. Have we proven that is that OK for JavaScript performance? Yusuke, maybe you have a sense of this. Maybe strftime is already slow enough that this is not a concern?

Looking at this code I see that GregorianDateTime already contains timezone information (in utcOffsetInMinute()) and we should not need to pass the name separately as it should have been picked at the construction time. However, `strftime` below seems to ignore timezone information in tm.tm_gmtoff and always returns 'system' value of the timezone. The code that overrides time zone can (and I think should) override that value as well. But maybe the general code can be switched to use ucal_getTimeZoneDisplayName or something similar from ucal instead of strftime? Do you know what's the reason strftime is called here to format the timezone name while rest of the code uses ucal to manipulate date/time?
Comment 54 Yury Semikhatsky 2022-04-27 15:27:13 PDT
Comment on attachment 457847 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457847&action=review

>>> Source/JavaScriptCore/runtime/DateConversion.cpp:101
>>> +            if (auto override = timeZoneDisplayNameOverride(); !override.isEmpty())
>> 
>> So we are now taking and releasing a lock every time this function is called. Have we proven that is that OK for JavaScript performance? Yusuke, maybe you have a sense of this. Maybe strftime is already slow enough that this is not a concern?
> 
> Looking at this code I see that GregorianDateTime already contains timezone information (in utcOffsetInMinute()) and we should not need to pass the name separately as it should have been picked at the construction time. However, `strftime` below seems to ignore timezone information in tm.tm_gmtoff and always returns 'system' value of the timezone. The code that overrides time zone can (and I think should) override that value as well. But maybe the general code can be switched to use ucal_getTimeZoneDisplayName or something similar from ucal instead of strftime? Do you know what's the reason strftime is called here to format the timezone name while rest of the code uses ucal to manipulate date/time?

Removed this call altogether in favor of setting "TZ" environment variable, it will change result of strftime accordingly. On Windows though explicit override for the timezone name is used but it now uses override value stored in DateCache once.

>> Source/JavaScriptCore/runtime/DateConversion.cpp:102
>> +                timeZoneName = override;
> 
> Quite unfortunate to convert this to a temporary WTF::String every time; that’s not needed to append a time zone name to a StringBuilder. I’m really unclear on how performance critical this code is. The way it’s currently working is overusing String. There’s no need to create and destroy a String to append some character to a StringBuilder. This applies to the strftime code below just as much to this override code.

Switched the code to StringView to avoid extra copies.

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:345
>> +        return String::adopt(WTFMove(override));
> 
> Seems we would want this to be fast when called multiple times, and this object is called a "cache". Can we find a way to not lock/unlock every time this is called just to check if an override is in effect?

Removed this call altogether. The override is now only accessed in DateCache::timeZoneCacheSlow which should be called only once per DateCache instance unless system timezone changes.

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:399
>> +    Vector<UChar> override = timeZoneOverride();
> 
> Seems unfortunate we make no attempt to optimize this to not use the heap. These are short strings and always doing malloc/free is costly.

Changed to use Vector<UChar, 32> and pass it as param to timeZoneOverride().

>> Source/WTF/wtf/DateMath.cpp:1055
>> +static std::optional<Vector<UChar>> validateTimeZone(const StringView& timeZone)
> 
> Type should be StringView, not const StringView&, in all these cases. In production builds a StringView fits into registers, and passing const StringView& instead typically just hurts performance and has no benefit.

Done.

>> Source/WTF/wtf/DateMath.cpp:1057
>> +    Vector<UChar> buffer(timeZone.length());
> 
> In a case like this we almost always use inline capacity so there’s no unnecessary heap allocation. This allocates/deallocates every time, which is unnecessary.

changed type to Vector<UChar, 32>

>> Source/WTF/wtf/DateMath.cpp:1061
>> +    canonicalBuffer.reserveCapacity(buffer.size());
> 
> Not sure this is a good optimization. It could cause a problem if the canonical ID is often slightly larger than the non-canonical.

Removed reserveCapacity call, instead changed type to Vector<UChar, 32>.

>> Source/WTF/wtf/DateMath.cpp:1065
>> +    return canonicalBuffer;
> 
> I believe we may need WTFMove here to avoid allocating a new heap buffer, since the return type is not Vector<UChar> but std::optional<Vector<UChar>>.

Good catch! Done.

>> Source/WTF/wtf/DateMath.cpp:1068
>> +bool isTimeZoneValid(const StringView& timeZone)
> 
> StringView

Done.

>> Source/WTF/wtf/DateMath.cpp:1073
>> +bool setTimeZoneOverride(const StringView& timeZone)
> 
> StringView

Done.

>> Source/WTF/wtf/DateMath.cpp:1116
>> +    return String(displayName);
> 
> This should be a single line.

Merged this method with timeZoneOverride.

>> Source/WTF/wtf/DateMath.h:432
>>  using WTF::jsCurrentTime;
> 
> We sort these alphabetically. The new ones should be sorted in with the existing ones.

Done. Restored alphabetic order.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/TimeZoneOverride.mm:2
>> + * Copyright (C) 2020 Igalia S.L.
> 
> 2022?

Done.
Comment 55 Yury Semikhatsky 2022-04-27 15:28:19 PDT
Created attachment 458474 [details]
Patch
Comment 56 Yury Semikhatsky 2022-04-27 15:42:02 PDT
Darin, PTAL.

I've update the code to make sure the reading of the timezone override that requires a lock is only performed from DateCache::timeZoneCacheSlow(). In other places cached information is used and doesn't require locking.

On a side note, I don't know the history behind formatDateTime and why it uses platform calls (GetTimeZoneInformation on Windows and strftime on other systems) to format the time zone name but having it use ICU similar to the rest of the code base would allow to simplify the overrides logic. Switching that to ICU would affect the timezone formatting in the Web API though (make it similar to what Firefox and Chromium produce but still), so probably not worth it.
Comment 57 Darin Adler 2022-04-27 15:49:58 PDT
(In reply to Yury Semikhatsky from comment #56)
> On a side note, I don't know the history behind formatDateTime and why it
> uses platform calls (GetTimeZoneInformation on Windows and strftime on other
> systems) to format the time zone name but having it use ICU similar to the
> rest of the code base would allow to simplify the overrides logic.

Not sure the history matters much at this point. I suspect it had to do with getting good formatting that adapts to the user’s locale and preferences, and long predates significant support for this in ICU.

> Switching
> that to ICU would affect the timezone formatting in the Web API though

This is the thing that matters now. If the result is better with ICU, we can make the change, and the consistency with other browsers would be great! But if the result debatably not as good at respecting user’s preferences and locale with ICU, then we would have to think more before making a change.
Comment 58 Yury Semikhatsky 2022-04-27 16:06:34 PDT
Created attachment 458478 [details]
Patch
Comment 59 Darin Adler 2022-04-27 16:23:44 PDT
Comment on attachment 458478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458478&action=review

> Source/JavaScriptCore/runtime/JSDateMath.cpp:112
> +static icu::TimeZone* toICUTimezZone(OpaqueICUTimeZone* timeZone)

I think TimezZone is a typo here?
Comment 60 Yury Semikhatsky 2022-04-27 19:00:48 PDT
Comment on attachment 458478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458478&action=review

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:112
>> +static icu::TimeZone* toICUTimezZone(OpaqueICUTimeZone* timeZone)
> 
> I think TimezZone is a typo here?

Fixed.
Comment 61 Yury Semikhatsky 2022-04-27 19:01:28 PDT
Created attachment 458485 [details]
Patch
Comment 62 Yury Semikhatsky 2022-04-27 19:12:31 PDT
(In reply to Yury Semikhatsky from comment #60)
> Comment on attachment 458478 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458478&action=review
> 
> >> Source/JavaScriptCore/runtime/JSDateMath.cpp:112
> >> +static icu::TimeZone* toICUTimezZone(OpaqueICUTimeZone* timeZone)
> > 
> > I think TimezZone is a typo here?
> 
> Fixed.

Is there anything else you want me to improve in this patch?


(In reply to Darin Adler from comment #57)
> (In reply to Yury Semikhatsky from comment #56)
> > Switching
> > that to ICU would affect the timezone formatting in the Web API though
> 
> This is the thing that matters now. If the result is better with ICU, we can
> make the change, and the consistency with other browsers would be great! But
> if the result debatably not as good at respecting user’s preferences and
> locale with ICU, then we would have to think more before making a change.

I can give it a try in a separate patch but I can't assess how many changes it entails and if the result will be acceptable, so I'd rather not commit to it. Given that this patch preserves existing behavior can we proceed with it?
Comment 63 Darin Adler 2022-04-28 13:05:25 PDT
(In reply to Yury Semikhatsky from comment #62)
> Is there anything else you want me to improve in this patch?

Sorry, haven’t yet had time to carefully read it through again. I plan to when I have an opportunity.
Comment 64 Yury Semikhatsky 2022-04-29 19:11:15 PDT
Created attachment 458624 [details]
Patch
Comment 65 Yury Semikhatsky 2022-04-29 19:12:58 PDT
(In reply to Yury Semikhatsky from comment #64)
> Created attachment 458624 [details]
> Patch

Rebased on top of r293625
Comment 66 Yury Semikhatsky 2022-05-02 11:22:36 PDT
Created attachment 458694 [details]
Patch
Comment 67 Yury Semikhatsky 2022-05-02 11:23:17 PDT
(In reply to Yury Semikhatsky from comment #66)
> Created attachment 458694 [details]
> Patch

Updated tests to not depend on current time as it would produce different result during winter time.
Comment 68 Yusuke Suzuki 2022-05-02 18:16:33 PDT
Comment on attachment 458694 [details]
Patch

r=me
Comment 69 EWS 2022-05-03 10:07:13 PDT
Committed r293729 (250217@main): <https://commits.webkit.org/250217@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458694 [details].
Comment 70 Radar WebKit Bug Importer 2022-05-03 10:08:17 PDT
<rdar://problem/92676360>
Comment 71 Michael Catanzaro 2022-05-11 17:05:07 PDT
Comment on attachment 458694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458694&action=review

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1903
> +    return context->priv->timeZoneOverride.utf8().data();

Hi, this is a use-after-free vulnerability. The CString object is already destroyed when the function returns, so the caller receives a pointer to an invalid buffer. To fix this, please store a CString in the priv struct, not a String.
Comment 72 Yury Semikhatsky 2022-05-11 18:23:02 PDT
Comment on attachment 458694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458694&action=review

>> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1903
>> +    return context->priv->timeZoneOverride.utf8().data();
> 
> Hi, this is a use-after-free vulnerability. The CString object is already destroyed when the function returns, so the caller receives a pointer to an invalid buffer. To fix this, please store a CString in the priv struct, not a String.

Sent a fix to https://bugs.webkit.org/show_bug.cgi?id=240327