1003911 - crash in java.lang.IllegalStateException: Couldn''t read row 0, col 0 from CursorWindow. Make sure the Cursor is initialized correctly before accessing data from it. at android.database.CursorWindow.nativeGetBlob(Native Method)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Favicon Handling, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

This bug was filed from the Socorro interface and is 
report bp-67b3733d-f507-4861-9a2a-966382140429.
=============================================================

java.lang.IllegalStateException: Couldn't read row 0, col 0 from CursorWindow.  Make sure the Cursor is initialized correctly before accessing data from it.
	at android.database.CursorWindow.nativeGetBlob(Native Method)
	at android.database.CursorWindow.getBlob(CursorWindow.java:407)
	at android.database.AbstractWindowedCursor.getBlob(AbstractWindowedCursor.java:45)
	at android.database.CursorWrapper.getBlob(CursorWrapper.java:122)
	at org.mozilla.gecko.db.LocalBrowserDB.getFaviconForUrl(LocalBrowserDB.java:729)
	at org.mozilla.gecko.db.BrowserDB.getFaviconForFaviconUrl(BrowserDB.java:262)
	at org.mozilla.gecko.favicons.LoadFaviconTask.doInBackground$2d4c763b(LoadFaviconTask.java:352)
	at org.mozilla.gecko.favicons.LoadFaviconTask.doInBackground$42af7916(LoadFaviconTask.java:42)
	at org.mozilla.gecko.util.UiAsyncTask$BackgroundTaskRunnable.run(UiAsyncTask.java:48)
	at android.os.Handler.handleCallback(Handler.java:605)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:32)

Comment 1

[Richard Newman [:rnewman]]

Yikes.

Comment 2

[Richard Newman [:rnewman]]

Attachment: Don't try to extract null favicons from the database. v1

This should be enough -- don't match rows with null values, then try to turn null into a blob!

Added the same check to thumbnails.

Comment 3

[:Margaret Leibovic]

Comment on attachment 8415369 [details] [diff] [review]
Don't try to extract null favicons from the database. v1

Review of attachment 8415369 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds like a good safety check. But why are we storing null blobs for some URLs?

Comment 4

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #3)
> But why are we storing null blobs for some URLs?

In old code, who knows. In the new favicon cache, that can only happen if we fail to recompress an icon. I'll add a part two which prevents insertion of null data.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Richard - Request uplift as appropriate

Comment 6

[Richard Newman [:rnewman]]

Attachment: Part 2: don't write null favicons or thumbnails into the DB. v1

The obvious null checks, some clarifying comments, and I tidied up the favicon re-encoding code a little to un-invert the conditional and fail more quietly in low-memory conditions, reusing the same failure path.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8416022 [details] [diff] [review]
Part 2: don't write null favicons or thumbnails into the DB. v1

Review of attachment 8416022 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/Tab.java
@@ +699,5 @@
>          setLoadProgressIfLoading(LOAD_PROGRESS_LOADED);
>      }
>  
>      protected void saveThumbnailToDB() {
> +        final BitmapDrawable thumbnail = mThumbnail;

Why do you need to make this local variable?

::: mobile/android/base/favicons/decoders/LoadFaviconResult.java
@@ -33,5 @@
>      }
>  
>      /**
>       * Return a representation of this result suitable for storing in the database.
> -     * For

I wonder what someone was intending to write here :)

Comment 8

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #7)

> Why do you need to make this local variable?

So we only read it once, rather than once in the check and again when we do the write. The member isn't volatile, but every little helps. Might allow the compiler to do better reordering, too.

> > -     * For
> 
> I wonder what someone was intending to write here :)

Srsly! :D

Thanks for the quick review!

Comment 9

[Richard Newman [:rnewman]]

   https://hg.mozilla.org/integration/fx-team/rev/4fbc58906fac
   https://hg.mozilla.org/integration/fx-team/rev/ac0e2abada09

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4fbc58906fac
https://hg.mozilla.org/mozilla-central/rev/ac0e2abada09

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8415369 [details] [diff] [review]
Don't try to extract null favicons from the database. v1

Requesting uplift for both parts.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  New favicon work in 29, but perhaps older, too.

User impact if declined: 
  Crashes if bad data got into the database.

Testing completed (on m-c, etc.): 
  Success case tested locally. Just landed, and will watch for reports. Changes certainly shouldn't make things any worse in the failure case.

Risk to taking this patch (and alternatives if risky): 
  Low: null checks.

String or IDL/UUID changes made by this patch:
  None.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/74545bec98d7
https://hg.mozilla.org/releases/mozilla-aurora/rev/30d08eab404d

https://hg.mozilla.org/releases/mozilla-beta/rev/97392de21322
https://hg.mozilla.org/releases/mozilla-beta/rev/e03ba68c6044

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/97392de21322
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/e03ba68c6044