Audit MatrixBlobCursor for concurrency errors

RESOLVED FIXED in Firefox 34

Status

()

Firefox for Android
Data Providers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rnewman, Assigned: rnewman, Mentored)

Tracking

Trunk
Firefox 34
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
E.g., `data` being accessed via the RowBuilder and also the consumers of the cursor.
(Assignee)

Comment 1

4 years ago
Created attachment 8464383 [details] [diff] [review]
Part 1: clean up MatrixBlobCursor without addressing hypothetical concurrency issues. v1

Here are the basics: no ReentrantReadWriteLocks etc., but tackles enclosing private member access, adds a bunch of finals, tweaks a few other things.
Attachment #8464383 - Flags: review?(wjohnston)
(Assignee)

Updated

4 years ago
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Comment on attachment 8464383 [details] [diff] [review]
Part 1: clean up MatrixBlobCursor without addressing hypothetical concurrency issues. v1

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

Sorry for the delay.

::: mobile/android/base/sqlite/MatrixBlobCursor.java
@@ +32,5 @@
>   * and MatrixCursor forgot to override it. This was fixed
>   * at some point but old devices are still SOL.
>   * Oh, and everything in MatrixCursor is private instead of
>   * protected, so we need to entirely duplicate it here,
> + * instead of just being able to add the missing method.

If you're going to clean up this comment, I would probably rewrite it since its so... friendly.

@@ +41,5 @@
>   * {@link #newRow()} to add rows. Automatically expands internal capacity
>   * as needed.
>   */
>  public class MatrixBlobCursor extends AbstractCursor {
> +    private static final String LOGTAG = "MatrixBlobCursor";

"GeckoMatrixBlobCursor"

@@ +187,5 @@
>          }
>  
> +        final int end = start + columnCount;
> +        ensureCapacity(end);
> +        final Object[] localData = data;

Add a comment about why your grabbing this reference.
Attachment #8464383 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/0f6e945dfe6c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Comment on attachment 8464383 [details] [diff] [review]
Part 1: clean up MatrixBlobCursor without addressing hypothetical concurrency issues. v1

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

Given that this is Android source, maybe we should file a bug upstream with our patch?

::: mobile/android/base/sqlite/MatrixBlobCursor.java
@@ +343,5 @@
>      @Override
>      protected void finalize() {
>          if (AppConstants.DEBUG_BUILD) {
>              if (!isClosed()) {
> +                Log.e(LOGTAG, "Cursor finalized without being closed", new RuntimeException("stack"));

Does this work? What useful stack do you expect in finalize(), as it's called from the GC? I think this can only work when you record the stack when the Cursor is created, which is what Android's SQLite classes do.
(Assignee)

Comment 6

4 years ago
(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)

> > +                Log.e(LOGTAG, "Cursor finalized without being closed", new RuntimeException("stack"));
> 
> Does this work? What useful stack do you expect in finalize(), as it's
> called from the GC? I think this can only work when you record the stack
> when the Cursor is created, which is what Android's SQLite classes do.

Good point; it depends on the VM's finalizer approach as to exactly what the stack is (I figured better than nothing), but the calling stack would be more useful. Could you file a follow-up?
You need to log in before you can comment on or make changes to this bug.