Page MenuHomePhabricator

mw.user.options.get('timecorrection')'s offset should be regenerated more often, to prevent daylight savings time (DST) bugs
Open, Needs TriagePublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • In Special:GlobalPreferences, set your timezone to a region that is currently in daylight savings time (e.g. America/Los Angeles on November 5, 2022)
  • Wait for the region to not be in daylight savings time anymore (e.g. America/Los Angeles on November 7, 2022)
  • Open browser console
  • Type mw.user.options.get('timecorrection')

What happens?:

  • 'ZoneInfo|-420|America/Los_Angeles' is displayed in the browser console, which translates to -420 mintues / 60 minutes = -7 hours, which is incorrect.

What should have happened instead?:

  • 'ZoneInfo|-480|America/Los_Angeles' is displayed in the browser console, which translates to -480 mintues / 60 minutes = -8 hours, which is correct.

Software version (skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):

  • Causing the bug T303124
  • Likely caused by the timezone offset being cached somewhere instead of recalculated every time it is called

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Novem_Linguae renamed this task from mw.user.options.get('timecorrection') not accounting for daylight savings time to mw.user.options.get('timecorrection') failing to detect end of daylight savings time.Wed, Nov 16, 5:37 AM
Novem_Linguae renamed this task from mw.user.options.get('timecorrection') failing to detect end of daylight savings time to mw.user.options.get('timecorrection') incorrectly thinks it's daylight savings time.
Novem_Linguae added subscribers: Func, Daimona.

Ideas from off-wiki discussion:

  1. Maybe one of these patches? https://gerrit.wikimedia.org/r/q/project:mediawiki/core+timecorrection. Pinging some recent patch writers @Func @Daimona
  1. Maybe some of our servers/datacenters have an out of date zoneinfo file?

Maybe some of our servers/datacenters have an out of date zoneinfo file?

No, I see the same for CEST and if you set your timezone from scratch it gets it correct.
I suspect that it is https://gerrit.wikimedia.org/r/c/mediawiki/core/+/802798

The offset recalculation is specifically for the switching between DST vs non-DST. offset should always be recalculated as it is specifically NOT constant troughout the year and because timezones change.

I can't reproduce this with the given steps on meta, beta-meta, or my local wiki. In all of them, the user preference has the correct offset. I can reproduce the bug in T303124, but that might be a different thing, and it also can't be caused by patches merged recently.

The offset recalculation is specifically for the switching between DST vs non-DST. offset should always be recalculated as it is specifically NOT constant troughout the year and because timezones change.

Or even better, the time correction setting should not include the offset at all, unless it was set explicitly. There's no point in storing something that is not guaranteed to be accurate unless you recompute it every time you read it; which effectively means the stored value is ignored. See T322352.

One thing I have in mind: UserPreferencesLookup special-cases the timecorrection settings and it will recompute the offset on the fly when reading the preference. Is the same normalization applied before sending the value to JS? Maybe the JS side can only see the value stored in the database, whose offset might be wrong, without applying the normalization. This would also explain why the bug is not reproducible if you change your preferences.

Hypothesis: timezone offset was set during DST weeks ago, and has been cached somewhere, perhaps in localstorage or the SQL database

Test: for someone such as me who has the DST bug, go to preferences, switch to a different timezone, then switch back, forcing a regeneration of the offset

Result: Fix worked. Confirms the hypothesis.

Novem_Linguae renamed this task from mw.user.options.get('timecorrection') incorrectly thinks it's daylight savings time to mw.user.options.get('timecorrection')'s offset should be regenerated more often, to prevent DST bugs.Wed, Nov 16, 9:23 PM
Novem_Linguae renamed this task from mw.user.options.get('timecorrection')'s offset should be regenerated more often, to prevent DST bugs to mw.user.options.get('timecorrection')'s offset should be regenerated more often, to prevent daylight savings time (DST) bugs.

Which is another argument in favour of just removing the offset from the timecorrection preference when in the geographical format (T322352)...

Krinkle added a subscriber: Krinkle.

I'll try to repro and understand the issue. If it's caching related, I might be able to help within RL. Otherwise might require a change to the data structure, in which case that'd probably be best to involve another team as stakeholder. Not sure whom though. Maybe CommTech as being preferences related but seems unfair to burden them with anything that happens to be stored as a preference, or consumer PageTriage/Growth, which could also be unfair as merely being its consumer. It's a very simple preference that's built-in to core and has no explicit owner at this time.

thiemowmde added a subscriber: thiemowmde.

I might be wrong, but as far as I understand there is nothing we can do in the Revision-Slider codebase. Please feel free to ping us again if we can help.

Hmm. strange, i can't locally reproduce this... Wondering if perhaps it's global prefs getting in the way. Will try to set that up locally as well.

Yep. this makes sense. It's GlobalPreferences.

https://gerrit.wikimedia.org/g/mediawiki/core/+/2fb06169bae70aeb4dd9d7873a9571062a3c8830/includes/user/UserOptionsManager.php#622

Makes the correction locally. And then you get:

		// Need to store what we have so far before the hook to prevent
		// infinite recursion if the hook attempts to reload options
		$this->originalOptionsCache[$userKey] = $options;
		$this->queryFlagsUsedForCaching[$userKey] = $queryFlags;
		$this->hookRunner->onLoadUserOptions( $user, $options );
		$this->originalOptionsCache[$userKey] = $options;

Where the onLoadUserOptions hook is what loads the timecorrection option out of the globaluserpreferences store, which thus will NOT be corrected.
We probably find the same issue with the deprecated language code correction that is right above it.

The next question thus becomes... do we let globalprefs do the same correction as core.. or do we move the correction to after the hook has run... But then the hook listeners could theoretically get 'incorrect' information.

See T223002#5843155 for a previous issue with using mw.user.options.get('timecorrection') prior to refactoring (rMW65c955746c59: Split TimeCorrection parser into separate class). For ZoneInfo, the offset in the database is unreliable and the timezone name needs to be used. Just get rid of it (T322352).

See T223002#5843155 for a previous issue with using mw.user.options.get('timecorrection') prior to refactoring (rMW65c955746c59: Split TimeCorrection parser into separate class). For ZoneInfo, the offset in the database is unreliable and the timezone name needs to be used. Just get rid of it (T322352).

I very much agree with this. However, as noted in T303124#8407331, JavaScript pretty much doesn't support geographical time zones, unless you use some library. I firmly believe this is something we should fix in the longer term. But maybe a temporary (tm) hack could be to have globalprefs correct the value like core does.

Change 860646 had a related patch set uploaded (by TheDJ; author: TheDJ):

[mediawiki/extensions/GlobalPreferences@master] GlobalPrefs should apply same options corrections as core

https://gerrit.wikimedia.org/r/860646