Closed Bug 1153357 Opened 9 years ago Closed 9 years ago

Investigate: MODIFIED record with change flags = NONE

Categories

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

Firefox 38
All
Android
defect
Not set
normal

Tracking

(firefox38 fixed, firefox39 fixed, firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox38 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(1 file, 1 obsolete file)

We saw this in Margaret's DB: a record with no GUID, marked as MODIFIED, but no change flag set. This shouldn't happen.
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.
Attachment #8591249 - Flags: review?(nalexander)
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
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.
Attachment #8591278 - Flags: review?(nalexander)
Attachment #8591249 - Attachment is obsolete: true
Attachment #8591249 - Flags: review?(nalexander)
Tests pass. Basic manual smoketesting (fresh sync to existing account with new local item, deleting, verify on desktop) works.
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?
Attachment #8591278 - Flags: review?(nalexander) → review+
(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 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.
Attachment #8591278 - Flags: approval-mozilla-beta?
Attachment #8591278 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/87ffd9756037
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
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.
Attachment #8591278 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8591278 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: