Closed Bug 1003911 Opened 6 years ago Closed 6 years ago

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)

Categories

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

All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 32
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
firefox32 --- fixed
b2g-v1.4 --- fixed
fennec + ---

People

(Reporter: kbrosnan, Assigned: rnewman)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

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)
Yikes.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
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.
Attachment #8415369 - Flags: review?(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?
Attachment #8415369 - Flags: review?(margaret.leibovic) → review+
(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.
tracking-fennec: ? → +
Richard - Request uplift as appropriate
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.
Attachment #8416022 - Flags: review?(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 :)
Attachment #8416022 - Flags: review?(margaret.leibovic) → review+
(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!
https://hg.mozilla.org/mozilla-central/rev/4fbc58906fac
https://hg.mozilla.org/mozilla-central/rev/ac0e2abada09
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
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.
Attachment #8415369 - Flags: approval-mozilla-beta?
Attachment #8415369 - Flags: approval-mozilla-aurora?
Attachment #8415369 - Flags: approval-mozilla-beta?
Attachment #8415369 - Flags: approval-mozilla-beta+
Attachment #8415369 - Flags: approval-mozilla-aurora?
Attachment #8415369 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.