Currently, the TimeZone::AdoptDefault said the following "This function is not thread safe. It is an error for multiple threads to concurrently attempt to set the default time zone, or for any thread to attempt to reference the default zone while another thread is setting it."
In chrome, several code call icu::TimeZone::CreateDefault . Some in the application, the other in internally library, such as v8, and other are called inside ICU itself. These are calls in different thread. Chrome also listen to platform API for timezone change, for example, when the user take a flight to a different city, or change the timezone on Mac desktop via System Preference. While the system notify chrome (or other app) the change of the timezone, Chrome, in https://cs.chromium.org/chromium/src/services/device/time_zone_monitor/time_zone_monitor.cc
it call the icu::TimeZone::detectHostTimeZone() to find out the new timezone and call icu::TimeZone::adoptDefault() to inform the ICU the default is now change to a newly detected timezone.
But while that happen, there are no way this timezone monitor will be able to know all the other code running on different thread to add mutex to sync with them, because these calls are in various different places and running on different thread.
Therefore, it cause crash because it is not practical for the AdoptDefault caller to sync with the createDefault caller.. It make much more sense for the AdoptDefault implementation to sync with the createDefault implementation.
We need to change the AdoptDefault call to be thread safe. Currently the problem is
Notice if thread A is calling adoptDefault while thread B is calling createDefault, consider the following
Thread A
Thread B
adoptDefault()
createDefault()
TimeZone *old = DEFAULT_ZONE;
umtx_initOnce(gDefaultZoneInitOnce, initDefault);
DEFAULT_ZONE = zone; delete old;
return (DEFAULT_ZONE != NULL) ? DEFAULT_ZONE->
clone() : NULL;
If the “DEFAULT_ZONE = zone; delete old;” happen between the time of DEFAULT_ZONE-> pointer start to call clone() and before it finish the clone. The cloned copy could be a partially trashed old DEFAULT_ZONE
The return value of B will be a clone of a trashed old DEFAULT_ZONE since the clone could happen while the old got deleted or after it got deleted.
We need to add mutex to ensure the "delete old" and "DEFAULT_ZONE->clone()" won't happen concurrently.
It would be easier to review your proposed changes if you were to make a GitHub pull request.
The approach seems good.
If TimeZone::clone() is simple and quick, you might be able to use the ICU global mutex instead of making a new one. Requires that clone() itself not use the global mutex. If clone() does a lot of work, having a separate mutex, as you propose, is better.
The idiom for creating a new mutex has been updated. Copy an example from the current master branch.
This is an issue we discovered from a mystical crash in chrome. https://bugs.chromium.org/p/chromium/issues/detail?id=950851
Currently, the TimeZone::AdoptDefault said the following
"This function is not thread safe. It is an error for multiple threads to concurrently attempt to set the default time zone, or for any thread to attempt to reference the default zone while another thread is setting it."
In chrome, several code call icu::TimeZone::CreateDefault . Some in the application, the other in internally library, such as v8, and other are called inside ICU itself. These are calls in different thread.
Chrome also listen to platform API for timezone change, for example, when the user take a flight to a different city, or change the timezone on Mac desktop via System Preference. While the system notify chrome (or other app) the change of the timezone, Chrome, in https://cs.chromium.org/chromium/src/services/device/time_zone_monitor/time_zone_monitor.cc
it call the icu::TimeZone::detectHostTimeZone() to find out the new timezone and call icu::TimeZone::adoptDefault() to inform the ICU the default is now change to a newly detected timezone.
But while that happen, there are no way this timezone monitor will be able to know all the other code running on different thread to add mutex to sync with them, because these calls are in various different places and running on different thread.
Therefore, it cause crash because it is not practical for the AdoptDefault caller to sync with the createDefault caller.. It make much more sense for the AdoptDefault implementation to sync with the createDefault implementation.
We need to change the AdoptDefault call to be thread safe. Currently the problem is
Notice if thread A is calling adoptDefault while thread B is calling createDefault, consider the following
Thread A
Thread B
adoptDefault()
createDefault()
TimeZone *old = DEFAULT_ZONE;
umtx_initOnce(gDefaultZoneInitOnce, initDefault);
DEFAULT_ZONE = zone;
delete old;
return (DEFAULT_ZONE != NULL) ? DEFAULT_ZONE->
clone() : NULL;
If the “DEFAULT_ZONE = zone; delete old;” happen between the time of DEFAULT_ZONE-> pointer start to call clone() and before it finish the clone. The cloned copy could be a partially trashed old DEFAULT_ZONE
The return value of B will be a clone of a trashed old DEFAULT_ZONE since the clone could happen while the old got deleted or after it got deleted.
We need to add mutex to ensure the "delete old" and "DEFAULT_ZONE->clone()" won't happen concurrently.
Here is my propose change: