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

[Masayoshi Okutsu]

I think what Umaoka-san pointed out is valid. Overrideable methods 
shouldn't be called in a constructor.

Masayoshi

On 6/6/2015 6:53 AM, 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.