SQLiteDatabase cannot be closed in Android 4.0 or below devices, in apps built by Scala 2.10.x w/ Android 4.1 due to Scala 2.10.x bytecode problem.

scabug commented 11 years ago

This reports that SQLiteDatabase instance cannot be closed due to "NoSuchMethodError:java.lang.NoSuchMethodError: android.database.sqlite.SQLiteClosable.close" in Android 4.0 or below devices if you build a project by Scala2.10.x with Android 4.1 or above library. I believe this is critical problem for all Android developers in Scala.

Whole source codes:

    class DemoActivity extends Activity{
      override def onCreate(savedInstanceState: Bundle) {
        super.onCreate(savedInstanceState)

        new SQLHelperDemo(this).getReadableDatabase.close()

        val textView = new TextView(this)
        textView.setText("Do I still survive?")
        setContentView(textView)
        }
    }
    class SQLHelperDemo(context:Context) extends SQLiteOpenHelper(context,"demo",null,1){
      def onCreate(p1: SQLiteDatabase) {}
      def onUpgrade(p1: SQLiteDatabase, p2: Int, p3: Int) {}
    }

This seems to come from bytecode differences between Scala 2.10.x and Scala 2.9.x as shown below.

###Bytecode extracts
**[Problem]Scala 2.10.0 android-16(Android4.1) bytecodes:**

    >javap -c DemoActivity

    public void onCreate(android.os.Bundle);
    ...
    57:aload_2
    58:invokevirtual#55; //Method android/database/sqlite/SQLiteClosable.close:()V
    61:return
    }

**[Working fine]Scala2.10.x with android-15, or Scala2.9.x with android-16 bytecodes:**

    >javap -c DemoActivity

    public void onCreate(android.os.Bundle);
    ...
        57:aload_2
    58:invokevirtual#55; //Method android/database/sqlite/SQLiteDatabase.close:()V
    61:return
    }

Although SQLiteDatabase has "close()" method in all Android version, "close()" is implemented in SQLiteDatabase class until Android 4.0 while it is inherited from SQLiteClosable which is super class of SQLiteDatabase from Android 4.1.

That's why Scala 2.10.x compiler generates bytecodes with SQLiteClosable.close() which doesn't exist in Android 4.0 or below devices. This leads to "NoSuchMethodError:java.lang.NoSuchMethodError: android.database.sqlite.SQLiteClosable.close". Of note, I find a very similar problem in AbsListView.setChoiceMode and there must be more.

While I'm not sure why this change happens in Scala 2.10.x compiler (maybe ASM difference?), there should be a way to avoid this problem such as compile option.

You can reproduce this issue from my below github repo, that includes more information in README.

https://github.com/taisukeoe/scala_2_10_android_error

scabug commented 11 years ago

Imported From: https://issues.scala-lang.org/browse/SI-7253?orig=1 Reporter: Taisuke Oe (oe_uia) Affected Versions: 2.10.0, 2.10.1

scabug commented 11 years ago

@paulp said: This isn't a bug, not in scala anyway. You can't expect to compile against version 4.1 of a library and deploy against version 4.0, not if they're not binary compatible, which they aren't. You were getting away with it through a combination of bugs and happenstance. As you describe it, the current behavior is correct and the old behavior was wrong.

Why aren't you building against 4.0 to deploy against 4.0?

scabug commented 11 years ago

Taisuke Oe (oe_uia) said: Hi Paul, Thank you very much for your clarification. I should have explained it as background.

To compile against Android 4.1 and deploy against Android 4.0, is the recommended way to use new features only in newer Android 4.1 devices with keeping compatibility with older Android devices such as Android 4.0 in an application. It must be compatible as long as the features aren't used in older Android devices such as 4.0. SQLiteDatabase.close() method is NOT newly added but exisiting from the first publich Android version 1.5 .

That's why there are two types of setting in SDK version in Android Project, targetSDKversion and minSDKversion.

TargetSDKversion is an Android SDK library version to compile against, which is Android 4.1 in this case.
MinSDKversion is a minimun Android version to run the application, which is Android 4.0 or below in this case.

Please find below link about why targetSDKversion often differs from minSDKversion.

http://developer.android.com/intl/ja/guide/topics/manifest/uses-sdk-element.html

I'm not inisiting to say this is a bug in Scala, but I just really want to understand why this compiler behavior change happens and how to avoid this in a practical way.

If you have any questions, please feel free to let me know.

Elarboration by anyone is welcome. Thanks in advance.

scabug commented 11 years ago

@paulp said: Things you might want to try (I can only guess; I'm not an android developer and none of this looks reproducible without time far beyond what I have.)

1) (new SQLHelperDemo(this).getReadableDatabase: SQLiteDatabase).close()

I don't expect this to help, but maybe it infers a weaker type in the chained expression. A stronger version would be { val x: SQLiteDatabase = new SQLHelperDemo(this).getReadableDatabase ; x.close() }

2) scalac -target:jvm-1.5-fjbg. We switched from fjbg to asm between 2.9 and 2.10; I don't think this is asm's doing, but not sure of that either.

scabug commented 11 years ago

@paulp said: Here's the commit where the behavior changed: https://github.com/scala/scala/commit/0bea2ab5f6

It might offer some background on the motivation.

scabug commented 11 years ago

Taisuke Oe (oe_uia) said: Thank you very much for your info / suggestion.

I tried both but neither 1) nor 2) work. They still do invokevirtual SQLiteClosable.close() as follow: 18: invokevirtual #30; //Method android/database/sqlite/SQLiteClosable.close:()V

Now I'm diving into commits history you suggested.

scabug commented 11 years ago

@Blaisorblade said: Paul, I think this bug is valid and not restricted to Android. See below.

Dear Taisuke Oke, could you verify that a corresponding example works with Java, and what bytecode it generates? Does it generate the same bytecode as 2.9?

=== Paul: this bug is IMHO valid because those libraries are binary compatible, according to the Java Language Specification and in practice. Interoperability strongly suggests that also Scala has to treat them as compatible. Maven allows to declare compatible libraries, and SBT trusts that. Finally, this bug potentially applies to many other libraries relying on Java binary compatibility.

The JLS declares that moving a method upward in the class hierarchy is a binary compatible change (http://docs.oracle.com/javase/specs/jls/se5.0/html/binaryComp.html). And we can guess safely that Android developers wouldn't screw up their binary compatibility constraints - you'd see a huge uproar if they did.

Also, to fix #1430, given A <: B, a method foo in A, and code invoking foo on a receiver of static type B, we need to invoke it using descriptor B.foo when the receiver has type B and A is not accessible; B is guaranteed to be accessible in this scenario. It seems that in this case we also need to use descriptor B.foo. Hence, it seems that always generating B.foo in this scenario would solve both bugs. However, the code also checks if an interface is involved - I don't get why, and the direction of the test was flipped (I think) without a clear reason. That's however hard to see, because that commit mixes a refactoring with a bugfix.

It even seems that using B.foo is even required by the JLS when it says that "A reference to a method must be resolved at compile time to a symbolic reference to the erasure (§4.6) of the qualifying type of the invocation" in http://docs.oracle.com/javase/specs/jls/se5.0/html/binaryComp.html.

More in general, MIMA might be getting this wrong as well. Either:

MIMA allows that change - but then it doesn't work because of this commit; or
MIMA forbids this change which should be allowed - and then we're using overly restrictive compatibility constraints, but might be unable to fix that until 2.11 since we shouldn't look for trouble with 2.10.0

scabug commented 11 years ago

@Blaisorblade said: So, Paul, I suggest that applying:

       val useMethodOwner = (
            style != Dynamic
-        || !isInterfaceCall(hostSymbol) && isAccessibleFrom(methodOwner, siteSymbol)
         || hostSymbol.isBottomClass
       )

on line https://github.com/scala/scala/commit/0bea2ab5f6#L9R1190 would fix both bugs. What do you think?

Taisuke, if you manage to test that change on top of that commit and verify that it doesn't break the testsuite, it'd be great. I don't have time to do it myself; but maybe Paul will try it out.

(BTW, I'd favor renaming back isInterfaceCall -> needsInterfaceCall, but that's orthogonal).

scabug commented 11 years ago

@paulp said: It is possible we can change the logic. But first, let's establish whether this is a bug or a feature request, because I am still of the opinion that we are doing nothing wrong.

The JLS and java binary compatibility in general is about backward compatibility. This situation is about forward binary compatibility." The JLS declares that moving a method upward in the class hierarchy is a binary compatible change," yes that's right. That is a statement about the binary compatibility of that artifact. It doesn't say anything about what implications that change might have on other code which is compiled against it.

There are a lot of things which are binary compatible changes, but it doesn't mean the change happens in a bubble and there are no consequences. When you compile against different bytecode, you will often generate different artifacts. This is as true in java as it is in scala.

scabug commented 11 years ago

@JamesIry said: I'd say it's a regression. Invoking against the definition point instead of against the type we're given is very brittle.

scabug commented 11 years ago

@Blaisorblade said:

In short

I claim that if a library if forward binary compatible in Java (like the Android standard library), it should also be compatible in Scala, and we have here a valid counterexample. What Android developers did was valid by the JLS, and what the bug reporter did would have been clearly valid in Java.

I don't know if this is a bug or a feature request, but if the fix is as easy as I suggested, I'm not interested in figuring that out.

In detail

TL;DR.

This issue is indeed about forward binary compatibility. But then, my other JLS quote applies:

"A reference to a method must be resolved at compile time to a symbolic reference to the erasure (§4.6) of the qualifying type of the invocation, plus the erasure of the signature of the method (§8.4.2)."

I won't paste what "qualifying type" means, but in our case that's the type of the receiver (http://docs.oracle.com/javase/specs/jls/se5.0/html/binaryComp.html for the general definition, but I'd recommend to just trust me for a while). Any Java compiler implementing that will also guarantee forward binary compatibility - viceversa, the Android developers can use that JLS clause to prove that their change is forward binary compatible against Java code; yet, this does not extend to Scala code. So I'd say Scalac is wrong.

scabug commented 11 years ago

@paulp said: I've been looking at this; you got me, it's a bug.

trait A { def f(): Int }
trait B1 extends A
abstract class B2 extends A

class C {
  (null: A).f()
  (null: B1).f()
  (null: B2).f()
  // 2.9.3
  //  7: invokeinterface #16,  1           // InterfaceMethod A.f:()I
  // 16: invokeinterface #16,  1           // InterfaceMethod A.f:()I
  // 25: invokeinterface #16,  1           // InterfaceMethod A.f:()I

  // 2.10.1
  //  7: invokeinterface #18,  1           // InterfaceMethod A.f:()I
  // 16: invokeinterface #21,  1           // InterfaceMethod B1.f:()I
  // 25: invokeinterface #18,  1           // InterfaceMethod A.f:()I
}

scabug commented 11 years ago

Taisuke Oe (oe_uia) said: Thank you very much, Paolo. I confirmed corresponding example works with Java and Android 4.1, with producing the same bytecodes (SQLiteDatabase.close) as Scala 2.9.x

   public void onCreate(android.os.Bundle);
     Code:
   ...
     20:invokespecial#24; //Method com/hemplant/java/SQLiteCloseDemo/SQLiteHelperDemo."<init>":(Landroid/content/Context;Ljava/lang/String;Landroid/database/sqlite/SQLiteDatabase$CursorFactory;I)V
     23:invokevirtual#28; //Method com/hemplant/java/SQLiteCloseDemo/SQLiteHelperDemo.getReadableDatabase:()Landroid/database/sqlite/SQLiteDatabase;
     26:invokevirtual#33; //Method android/database/sqlite/SQLiteDatabase.close:()V

I'll test suggested codes on top of that commit and get back to you once I get the result.

scabug commented 11 years ago

@JamesIry said: javac always invokes against the given static type rather than where the method was defined.

scabug commented 11 years ago

@paulp said: Okay, everyone take a good look at this. I was confused during this discussion (in part) because everyone seems to be saying the 2.9 behavior is correct and 2.10 was a regression. It looks like the 2.9 behavior is at least as wrong as 2.10. I'm currently of the opinion that it should look like the Java example to the extent it is possible - I forget what the obstacles are but I'm sure I'll have the joy of rediscovering them.

So if you think the "Java" example here is NOT the ideal, please speak now or forever hold your linkage errors.

trait A { def f(): Int }
trait B1 extends A
abstract class B2 extends A
class B3 extends A { def f(): Int = 1 }
class B4 extends B3

class C {
  (null: A).f()
  (null: B1).f()
  (null: B2).f()
  (null: B3).f()
  (null: B4).f()
}

/*****

2.9.3:

 1: invokespecial #10                 // Method java/lang/Object."<init>":()V
 7: invokeinterface #16,  1           // InterfaceMethod A.f:()I
16: invokeinterface #16,  1           // InterfaceMethod A.f:()I
25: invokeinterface #16,  1           // InterfaceMethod A.f:()I
34: invokevirtual #19                 // Method B3.f:()I
41: invokevirtual #22                 // Method B4.f:()I

2.10.1:

 1: invokespecial #12                 // Method java/lang/Object."<init>":()V
 7: invokeinterface #18,  1           // InterfaceMethod A.f:()I
16: invokeinterface #21,  1           // InterfaceMethod B1.f:()I
25: invokeinterface #18,  1           // InterfaceMethod A.f:()I
34: invokevirtual #24                 // Method B3.f:()I
41: invokevirtual #24                 // Method B3.f:()I

Java:

 4: invokeinterface #7,  1            // InterfaceMethod A.f:()I
14: invokeinterface #8,  1            // InterfaceMethod B1.f:()I
24: invokevirtual #9                  // Method B2.f:()I
32: invokevirtual #10                 // Method B3.f:()I
40: invokevirtual #11                 // Method B4.f:()I

****/

scabug commented 11 years ago

@paulp said: Also, I'm absolutely buried in other stuff I have to do. Pull requests extremely welcome.

scabug commented 11 years ago

@Blaisorblade said (edited on Mar 15, 2013 2:44:32 PM UTC): That's the behavior I expected. UPDATED: the fix I proposed seems to fix this problem, I need to do some regression testing.

scabug commented 11 years ago

@Blaisorblade said: https://github.com/scala/scala/pull/2264

scabug commented 11 years ago

Taisuke Oe (oe_uia) said: Wow, awesome!! Thank you very much!

scabug commented 11 years ago

@Blaisorblade said: The pull request was now merged, so I'm closing this.

scala / bug