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

[Naoto Sato]

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.