Closed Bug 1276090 Opened 8 years ago Closed 8 years ago

Don't increment the bookmarks score during a batch operation

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: markh, Assigned: markh)

References

Details

(Whiteboard: [sync-data-integrity])

Attachments

(1 file, 1 obsolete file)

The bookmarks engine treats every bookmark change as "significant" - even ones that are happening as part of a batch operation. This means our tracker is almost guaranteed to miss changes - a batch operation is almost certainly making multiple changes but we will stop listening after the first.

This probably becomes moot with Kit's patch, but I had this patch in my tree and figured I see what people thought about it in the meantime.

Richard/Kit, can you see any reason this isn't the right thing to do for our current tracker?
Attachment #8757076 - Flags: feedback?(rnewman)
Comment on attachment 8757076 [details] [diff] [review]
0001-Don-t-increment-the-sync-score-during-a-batch-bookma.patch

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

::: services/sync/modules/engines/bookmarks.js
@@ +1375,5 @@
> +    // If we are in a batch update just record what we should increment the
> +    // score by; we avoid starting a Sync during a batch upload as this will
> +    // cause us to miss future notifications from the batch.
> +    if (this._inBatchUpdate) {
> +      this._batchScore += SCORE_INCREMENT_XLARGE;

Every single bump will exceed the sync threshold. Might as well just do nothing and just bump score in `onEndUpdateBatch`, or flip a flag here, instead.

@@ +1532,5 @@
>      PlacesUtils.annotations.removeItemAnnotation(itemId, BookmarkAnnos.PARENT_ANNO);
>    },
>  
> +  onBeginUpdateBatch: function () {
> +    this._inBatchUpdate = true;

Can batches ever nest?

Can we make this a `batchDepth` counter instead, and have the active edge be 1->0?
Attachment #8757076 - Flags: feedback?(rnewman) → feedback+
Assignee: nobody → markh
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Don't increment the bookmarks "score" during a batch operation. → Don't increment the bookmarks score during a batch operation
Whiteboard: [sync-data-integrity]
(In reply to Richard Newman [:rnewman] from comment #1)
> > +    if (this._inBatchUpdate) {
> > +      this._batchScore += SCORE_INCREMENT_XLARGE;
> 
> Every single bump will exceed the sync threshold. Might as well just do
> nothing and just bump score in `onEndUpdateBatch`, or flip a flag here,
> instead.

Yeah, I did the former - unconditionally increment the score at the end of the batch - it doesn't seem worthwhile optimizing for the case of a batch that does nothing.

> @@ +1532,5 @@
> >      PlacesUtils.annotations.removeItemAnnotation(itemId, BookmarkAnnos.PARENT_ANNO);
> >    },
> >  
> > +  onBeginUpdateBatch: function () {
> > +    this._inBatchUpdate = true;
> 
> Can batches ever nest?

The runInBatch implementation tracks the depth and only notifies at the end, so in practice, this does the right thing without taking nested batches into account...

> Can we make this a `batchDepth` counter instead, and have the active edge be
> 1->0?

But yeah, I don't see a good reason to rely on this implementation detail, so I ended up using a batchDepth.

Note a side-effect of this patch is that one existing test that uses PlacesSortFolderByNameTransaction now only increments the score by SCORE_INCREMENT_XLARGE instead of SCORE_INCREMENT_XLARGE*2 due to it using a batch under the covers, but this shouldn't cause any problems.

Patch forthcoming...
Attachment #8757076 - Attachment is obsolete: true
Comment on attachment 8766684 [details]
Bug 1276090 - increment the sync score after a batch bookmark operation.

This patch causes continuous syncing :( The engine does its own batching so every time we finish syncing we bump the score and start again.
Attachment #8766684 - Flags: review?(rnewman)
Oh, that's exciting.
Comment on attachment 8766684 [details]
Bug 1276090 - increment the sync score after a batch bookmark operation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61492/diff/1-2/
Attachment #8766684 - Flags: review?(rnewman)
Comment on attachment 8766684 [details]
Bug 1276090 - increment the sync score after a batch bookmark operation.

https://reviewboard.mozilla.org/r/61492/#review58592

::: services/sync/modules/engines/bookmarks.js:1537
(Diff revision 2)
>  
> -  onBeginUpdateBatch: function () {},
> -  onEndUpdateBatch: function () {},
> +  onBeginUpdateBatch: function () {
> +    ++this._batchDepth;
> +  },
> +  onEndUpdateBatch: function () {
> +    if(--this._batchDepth == 0 && this._batchSawScoreIncrement) {

Nit: space after `if`, use `===`.

So your change is: don't increment as a result of a batch alone, because a batch will occur as an ordinary part of a sync. The score won't be incremented when the tracker is disabled during a sync, so we'll keep re-syncing until an uninterrupted sync occurs. Makes sense.
Attachment #8766684 - Flags: review?(rnewman) → review+
Thanks!

(In reply to Richard Newman [:rnewman] from comment #7)
> use `===`.

While I made that change, FTR, https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style still says "In JavaScript, == is preferred to ==="
Comment on attachment 8766684 [details]
Bug 1276090 - increment the sync score after a batch bookmark operation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61492/diff/2-3/
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/511ae2ae8db6
increment the sync score after a batch bookmark operation. r=rnewman
https://hg.mozilla.org/mozilla-central/rev/511ae2ae8db6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1288877
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: