1135281 - GetStringUTFChars received null jstring from org.mozilla.gecko.sqlite.MatrixBlobCursor org.mozilla.gecko.sqlite.SQLiteBridge.sqliteCallWithDb(long, java.lang.String, java.lang.String[], long[])

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Stacktrace

<rnewman> mcomella: sounds like there's a null password in the DB
<@rnewman> or null host, or something else Sync assumes is non-null
<@rnewman> SQLiteBridgeContentProvider.java:428
<mcomella> I haven't used about:passwords on this device
<@rnewman> have you saved any passwords?
<mcomella> no
<mcomella> local build, I haven't done much
<@rnewman> well, it's coming from the cursor, so it's reading from the DB
<@rnewman> any chance we're returning some kind of empty row for an empty DB?
<@rnewman> should be easily deduced by inspecting the code
<@rnewman> file with stack, ni me
<@rnewman> I don't trust MatrixBlobCursor

Nabbed the profile too, if you want a stab at it.

I looked at signons.sqlite and all the tables (moz_logins, moz_disabled_logins, moz_disableHosts) are empty (i.e. `select * from <table-name>` return nothing for all three).

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

<rnewman> mcomella: re that DB exception: could you either breakpoint or log, and add to the bug what the arguments were? I'm guessing that a bind value was null, or the query was null.
<@rnewman> hard to get a good stack out of the JNI call
<@rnewman> should be straightforward to write a failing test, for that matter

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: db.query args

Is this the information you needed, Richard?

By the way, the tables are empty in my copied profile (at least in the profile I copied when I initially filed this bug).

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Stack trace 2

I just received this on my phone. Notably, I cleared data while I had a sync account set up.

Comment 4

[Richard Newman [:rnewman]]

So we're back to:

03-05 17:05:25.951 F/art     (24324): art/runtime/check_jni.cc:65] JNI DETECTED ERROR IN APPLICATION: GetStringUTFChars received null jstring
03-05 17:05:25.952 F/art     (24324): art/runtime/check_jni.cc:65]     in call to GetStringUTFChars
03-05 17:05:25.952 F/art     (24324): art/runtime/check_jni.cc:65]     from org.mozilla.gecko.sqlite.MatrixBlobCursor org.mozilla.gecko.sqlite.SQLiteBridge.sqliteCallWithDb(long, java.lang.String, java.lang.String[], long[])

extern "C" NS_EXPORT jobject JNICALL
Java_org_mozilla_gecko_sqlite_SQLiteBridge_sqliteCallWithDb(JNIEnv* jenv, jclass,
                                                            jlong jDb,
                                                            jstring jQuery,
                                                            jobjectArray jParams,
                                                            jlongArray jQueryRes)
{
    JNI_Setup(jenv);

    jobject jCursor = nullptr;
    sqlite3 *db = (sqlite3*)jDb;
    jCursor = sqliteInternalCall(jenv, db, jQuery, jParams, jQueryRes);
    return jCursor;
}

Presumably there's a missing stack from for sqliteInternalCall.

Which has:

    const char* queryStr;
    queryStr = jenv->GetStringUTFChars(jQuery, nullptr);

    rc = f_sqlite3_prepare_v2(db, queryStr, -1, &ppStmt, &pzTail);
    if (rc != SQLITE_OK || ppStmt == nullptr) {
        throwSqliteException(jenv,
            "Can't prepare statement: %s", f_sqlite3_errmsg(db));
        return nullptr;
    }
    jenv->ReleaseStringUTFChars(jQuery, queryStr);

...
    if (jParams != nullptr) {
        // Bind parameters, if any
        if (numPars > 0) {
            for (int i = 0; i < numPars; i++) {

...

                }
                jstring jStringParam = (jstring)jObjectParam;
                const char* paramStr = jenv->GetStringUTFChars(jStringParam, nullptr);


So one of these things is the case:

* We got a null query string.
* We got a null param.
* Something about our get/release process is wrong, and it's manifesting in this way.

(I think we should be using the third argument to GSUTFC, but maybe I'm wrong.)


The query definitely isn't null. Your logging doesn't indicate whether the selectionArgs contents might be null. Could you expand the logging for that?

Comment 5

[Richard Newman [:rnewman]]

Here's my guess.

  private PasswordRecord findExistingRecord(PasswordRecord record) throws NullCursorException, RemoteException {

...

    final String[] whereArgs = new String[] {
      record.hostname,
      record.httpRealm,
      record.formSubmitURL,
      record.usernameField,
      record.passwordField
    };

    try {
      cursor = passwordsHelper.safeQuery(passwordsProvider, ".findRecord", getAllColumns(), WHERE_RECORD_DATA, whereArgs, null);



so yeah, it's quite possible that those whereArgs will contains nulls. And it looks like SQLiteBridge.cpp doesn't handle that correctly (i.e., by not crashing).

It's not clear to me whether we want the semantics of = NULL here, or if we should be explicitly checking and using IS NULL in the constructed query. That might be a second bug, but making SQLiteBridge not die is still necessary.

N.B., you will only get this exception for a debuggable app -- JNI checking isn't enabled by default. That explains why we don't really see this in the wild.

Comment 6

[Richard Newman [:rnewman]]

Attachment: Correctly bind null parameters in SQLiteBridge.cpp. v1

Untested, but this compiles and looks OK to me. Care to give this a shot with your broken profile?

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Unfortunately, my computer died and I'm unable to install new builds to an existing profile – I'll come back to this if it happens again.

Leave NI to play around w/ clearing synced data/removing accounts when I get a chance.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

I'm now seeing exactly this for my builds on the N9:

> F/art     (16129): art/runtime/check_jni.cc:65] JNI DETECTED ERROR IN APPLICATION: GetStringUTFChars received null jstring
> F/art     (16129): art/runtime/check_jni.cc:65]     in call to GetStringUTFChars
> F/art     (16129): art/runtime/check_jni.cc:65]     from org.mozilla.gecko.sqlite.MatrixBlobCursor org.mozilla.gecko.sqlite.SQLiteBridge.sqliteCallWithDb(long, java.lang.String, java.lang.String[], long[])

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

With the patch applied the crash is gone and sync can finally complete.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8573651 [details] [diff] [review]
Correctly bind null parameters in SQLiteBridge.cpp. v1

LGTM. Mostly because of testing the patch. I'm not really experienced with C++ but the patch seems to make sense.

Comment 12

[Richard Newman [:rnewman]]

url:        https://hg.mozilla.org/integration/fx-team/rev/7bbb28b68b0dbd8d067ed71c8c4017cd50b5dbaa
changeset:  7bbb28b68b0dbd8d067ed71c8c4017cd50b5dbaa
user:       Richard Newman <[e-mail]>
date:       Fri Aug 07 21:43:47 2015 -0700
description:
Bug 1135281 - Correctly bind null parameters in SQLiteBridge.cpp. r=sebastian

Comment 13

[Richard Newman [:rnewman]]

Kinda forgot to land this. In it goes!

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7bbb28b68b0d