<i18n dev> DateFormatSymbols triggers this.clone() in the constructor

[Yoshito Umaoka]

Sato-san,

Yes, it's not thing wrong to check null in the clone() method, and 
that's what I did to avoid this problem. But what we can do there is to 
skip cloning the field in our code only.

With the current Java implementation, I'm pretty sure that it won't 
cause any actual problem, because Java implementation does not call any 
methods on the cached copy. The implementation only reads previously 
initialized symbols (such as weekday name, month names) by directly 
accessing the fields.

However, if the implementation is changed in future and calls any 
DateFormatSymbols' public method, our subclass won't work because our 
class overrides these methods and expects the field (which is supposed 
to be initialized by our class's constructor) is null.

I'm not suggesting that Java DateFormatSymbols should not utilize a 
cache storing symbols initialized from resource bundles. My point is - 
caching an instance of DateFormatSymbols itself could be dangerous 
because of the reason I explained above.

I think the right thing to do is to create a private class object 
holding these symbols (this can be a singleton per Locale) separate from 
DateFormatSymbols - and DateFormatSymbols constructor just copy the 
field to a new DateFormatSymbols 2nd time.

For example, creating a private class:

private static class SymbolsCacheEntry {
     String eras[];
     String months[];
     ....
     String patternChars;
     ....
}

Then actual locale data is once loaded into SymbolsCacheEntry - and 
cached. When DateFormatSymbols constructor is called 2nd time,  
"copyMembers()" copies field values from an instance of 
SymbolsCacheEntry (instead of cached DateFormatSymbols), that is,

private void copyMembers(SymbolsCacheEntry src, DateFormatSymbols dst) {
     dst.eras = Arrays.copyOf(src.eras, src.eras.length);
     ....
}

By doing so, the implementation can avoid premature (and potentially 
dangerous) DateFormatSymbols subclass instance.

Anyway, if Java guarantees that the implementation will never call 
DateFormatSymbols methods (such as getMonths()) in future and just 
directly accessing Java's DateFormatSymbols' own fields, then I'm OK 
with it. (In this case, I would suggest Java team to add some comments 
in the source code, so future maintainer of the Java code can understand 
the risk.)

Thanks,
Yoshito


On 6/5/2015 5:53 PM, Naoto Sato wrote:
> Umaoka-san,
>
> I believe the cloning is needed to return the defensive copy, 
> otherwise an app can mutate the state of the DateFormatSymbols for 
> other apps. One solution could be to not cache the instance if its 
> from SPI, but apparently that will affect the performance. I don't see 
> anything wrong to check if foo is null or not in clone(), since it's a 
> private property to the provider.
>
> Naoto
>
> On 6/5/15 8:31 AM, Yoshito Umaoka wrote:
>> Hi folks,
>>
>> ICU4J implements Java locale service provider interface. I recently
>> received a problem report from our customer. When they upgraded Java
>> version to 8, the provider implementation stopped working because of NPE
>> thrown by our custom DateFormatSymbols subclass. I dug into the problem
>> and found that DateFormatSymbols caches its own instance in the
>> constructor. Our subclass implementation expects a field is always
>> initialized to non-null in the constructor. However, clone() method is
>> called for the super class's constructor, the field is not yet
>> initialized at the point.
>>
>> It looks adding 'null' for the field in clone() method would resolve the
>> immediate problem, but I'm not comfortable that DateFormatSymbols
>> implementation cache a premature instance of my own subclass. If any
>> methods overridden by my own subclass is called on a premature instance,
>> it might cause another issue.
>>
>> Anyway, I filed a bug in the Java bug database with the description
>> below. For meanwhile, adding simple 'null' check in our code would
>> suffice. But I would like to hear Java i18n team's opinion about this
>> issue.
>>
>> Thanks,
>> Yoshito
>>
>>
>> ---------------------------
>> To implement my own locale service provider, I have a class extending
>> java.text.DateFormatSymbols. My custom subclass's constructor implicitly
>> invokes the no-args constructor in java.text.DateFormatSymbols. The
>> constructor calls a private method - initializeData(Locale).
>>
>> It looks the implementation was updated in Java 8 and initializeData is
>> now trying to cache an instance of DateFormatSymbols at the end and
>> calls this.clone().
>>
>> In my own subclass implements clone() method, which copies a field
>> initialized by a constructor in the class. For example,
>>
>> ===============
>> public class MyDateFormatSymbols extends DateFormatSymbols {
>>      private final Foo foo;
>>
>>      public MyDateFormatSymbols(Foo foo) {
>>          if (foo == null) {
>>              this.foo = new Foo();
>>          } else {
>>              this.foo = foo;
>>          }
>>      }
>>
>>      @Override
>>      public Object clone() {
>>          MyDateFormatSymbols mdfs = (MyDateFormatSymbols)super.clone();
>>          mdfs.foo = this.foo.clone();
>>      }
>> }
>> ===============
>>
>> When the constructor MyDateFormatSymbols(Foo) is called, it triggers
>> no-args constructor of the super class - DateFormatSymbols() first. As I
>> explained earlier, Java 8 implementation calls this.clone() in
>> DateFormatSymbpls.initializeData(Locale). At that point, the field foo
>> in my class is not yet initialized, so this.foo.clone() will throw
>> NullPointerException.
>>
>> My own code expects the field 'foo' is always non-null. I could change
>> clone() to check if this.foo is null or not, but I cannot control cached
>> 'premature' instance held by Java DateFormatSymbols. At this moment, it
>> looks the cache is only used for copying field values maintained by
>> DateFormaSymbols itself and never call a method. So, even 'premature'
>> instance of my own subclass instance is referenced by DateFormatSymbols,
>> it won't cause any problems. However, if the Java's implementation is
>> changed to call any DateFormatSymbols method overridden by my own
>> subclass, it may not work (because my subclass expects the field foo is
>> non-null).
>>
>> Such code above did not have any problems with earlier Java releases
>> (Java 6 / 7).
>>
>> In my opinion, this.clone() should not be called in DateFormatSymbols
>> initialization code. Instead, it should create a private container class
>> for these symbols, and cache the object, not DateFormatSymbols itself.