1045916 - Audit MatrixBlobCursor for concurrency errors

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Richard Newman [:rnewman]]

E.g., `data` being accessed via the RowBuilder and also the consumers of the cursor.

Comment 1

[Richard Newman [:rnewman]]

Attachment: 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.

Comment 2

[Wesley Johnston (:wesj)]

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.

Comment 3

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/0f6e945dfe6c

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f6e945dfe6c

Comment 5

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 6

[Richard Newman [:rnewman]]

(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?