1153357 - Investigate: MODIFIED record with change flags = NONE

Status: RESOLVED FIXED

(Android Background Services Graveyard :: Reading List Sync, defect)

Description

[Richard Newman [:rnewman]]

We saw this in Margaret's DB: a record with no GUID, marked as MODIFIED, but no change flag set. This shouldn't happen.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Don't set SYNC_STATUS_MODIFIED unless an update touches fields that we sync. v1

This doesn't answer one obvious question: who's calling update on the CP and not providing any fields that flip a flag? But the change is correct, IMO.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Don't set SYNC_STATUS_MODIFIED unless an update touches fields that we sync. v2

This patch does two things:

* It doesn't set MODIFIED at all unless fields are being changed. That's the original bug.
* It only sets MODIFIED in a non-Sync update if the item is marked as SYNCED. NEW items will stay new even if Fennec changes them, until they've been synced.

Not yet tested.

Comment 4

[Richard Newman [:rnewman]]

Tests pass. Basic manual smoketesting (fresh sync to existing account with new local item, deleting, verify on desktop) works.

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8591278 [details] [diff] [review]
Don't set SYNC_STATUS_MODIFIED unless an update touches fields that we sync. v2

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

I think this does what it says on the tin.  Include the comment you gave to me (about two parts) in the commit message.

There are Robocop tests for both DBUtils and RLP.  Simple tests should not be hard... you could file mentored follow-up.

::: mobile/android/base/db/DBUtils.java
@@ +210,5 @@
>      }
>  
>      @RobocopTarget
>      public enum UpdateOperation {
>          ASSIGN,

Wow, this is evil.  Can you add comments to these?  (And check my understanding.)

// ASSIGN turns "foo", value into "foo = ?" and adds value as an argument.
// BITWISE_OR turns "foo", value into "foo |= ?" and add value as an argument.
// EXPRESSION turn "foo", "value" into "foo = value" and treats value as a literal SQL string.  Injection attack alert!

@@ +280,5 @@
>                          sql.append("= ? | ");
>                          sql.append(colName);
>                      }
>                      break;
> +                case EXPRESSION:

It's possible that the op is null (yay Java!), which will NPE the switch.  Boo.

::: mobile/android/base/db/ReadingListProvider.java
@@ +122,4 @@
>              return db.update(TABLE_READING_LIST, values, selection, selectionArgs);
>          }
>  
> +        // Set synced items to MODIFIED; otherwise, leave the sync status untouched.

Is there a reason to pass through the SYNC_STATUS untouched?  It seems like you could set it to SYNC_STATUS_NEW, possibly simplifying the status progression.  Although if we add a new SYNC_STATUS_SYNC_FAILED maybe it gets more complicated?

Comment 6

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #5)

> There are Robocop tests for both DBUtils and RLP.  Simple tests should not
> be hard... you could file mentored follow-up.

Sure.


> Wow, this is evil.  Can you add comments to these?  (And check my
> understanding.)

Sure.

 
> // ASSIGN turns "foo", value into "foo = ?" and adds value as an argument.
> // BITWISE_OR turns "foo", value into "foo |= ?" and add value as an
> argument.
> // EXPRESSION turn "foo", "value" into "foo = value" and treats value as a
> literal SQL string.  Injection attack alert!

Correct. Yes, EXPRESSION (and this mechanism as a whole) is basically a way to declaratively run arbitrary UPDATE queries without deviating too far from the way Android chose to do things.

The alternative is to start a transaction, run a query for values, then construct an awful batch-update with appropriate ContentValues. Really awful.


> possible that the op is null (yay Java!), which will NPE the switch. 
> Boo.

Yeah, programming errors deserve crashes :)

> you could set it to SYNC_STATUS_NEW, possibly simplifying the status
> progression.  Although if we add a new SYNC_STATUS_SYNC_FAILED maybe it gets
> more complicated?

Things can be NEW, MODIFIED, or SYNCED.

If they're NEW and we make a change, we want them to stay that way. If they're SYNCED, set them to MODIFIED. If they're MODIFIED, all of this is a no-op.

So there are two ways to phrase this: set status to modified if not new, or set status to modified if synced. The latter is most specific (as you note, we might add FAILED or similar), so that's what I chose.

The easiest way to express conditional assignment in SQL is a pass-through CASE:

  SET sync_status = CASE sync_status WHEN 3 THEN 1 ELSE sync_status END

because we can't otherwise do conditional set without running two queries or really messing around with subselect and COALESCE.

(Running two queries was my original approach to this, but it's even more confusing.)

Comment 7

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/87ffd9756037

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8591278 [details] [diff] [review]
Don't set SYNC_STATUS_MODIFIED unless an update touches fields that we sync. v2

Approval Request Comment
[Feature/regressing bug #]:
  Initial landing of RL sync.

[User impact if declined]:
  Items modified by background fetches or manual before they're uploaded by syncing will misbehave.

[Describe test coverage new/current, TreeHerder]:
  Waiting for manual testing on Nightly.

[Risks and why]: 
  Some risk, but IMO acceptable given the scope of the error.

[String/UUID change made/needed]:
  None.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87ffd9756037

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8591278 [details] [diff] [review]
Don't set SYNC_STATUS_MODIFIED unless an update touches fields that we sync. v2

Approving for uplift to aurora, since it sounds like we need this fix for Reading List and the risk is limited to RL.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/437caf37b9ae

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8591278 [details] [diff] [review]
Don't set SYNC_STATUS_MODIFIED unless an update touches fields that we sync. v2

We also want that for beta.
Should be in 38 beta 5.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/199b60ec60dc