Closed Bug 1135281 Opened 6 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: mcomella, Assigned: rnewman)

References

Details

Attachments

(4 files)

Attached file 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).
Flags: needinfo?(rnewman)
<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
Flags: needinfo?(michael.l.comella)
Attached file 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).
Flags: needinfo?(michael.l.comella)
Attached file Stack trace 2
I just received this on my phone. Notably, I cleared data while I had a sync account set up.
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?
Flags: needinfo?(rnewman) → needinfo?(michael.l.comella)
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.
Untested, but this compiles and looks OK to me. Care to give this a shot with your broken profile?
Attachment #8573651 - Flags: review?(michael.l.comella)
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Duplicate of this bug: 1163606
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.
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[])
With the patch applied the crash is gone and sync can finally complete.
Flags: needinfo?(rnewman)
Flags: needinfo?(rnewman)
Attachment #8573651 - Flags: review?(s.kaspari)
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.
Attachment #8573651 - Flags: review?(s.kaspari) → review+
url:        https://hg.mozilla.org/integration/fx-team/rev/7bbb28b68b0dbd8d067ed71c8c4017cd50b5dbaa
changeset:  7bbb28b68b0dbd8d067ed71c8c4017cd50b5dbaa
user:       Richard Newman <rnewman@mozilla.com>
date:       Fri Aug 07 21:43:47 2015 -0700
description:
Bug 1135281 - Correctly bind null parameters in SQLiteBridge.cpp. r=sebastian
Kinda forgot to land this. In it goes!
Flags: needinfo?(michael.l.comella)
https://hg.mozilla.org/mozilla-central/rev/7bbb28b68b0d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Duplicate of this bug: 1177920
You need to log in before you can comment on or make changes to this bug.