Closed Bug 1299978 Opened 3 years ago Closed 3 years ago

Ensure deleted bookmark reconciliation uploads the correct records

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: Lina, Assigned: tcsc)

References

Details

(Whiteboard: [sync-tracker])

Attachments

(1 file)

See bug 1274496, comment 40. If a remote record is deleted, but the local record is newer and has a change, we need to upload the local parent, too.

If we do resurrect a bookmark, we also need to do the same for its ancestors, to avoid writing orphans for deleted subtrees (with `removeFoldersContents` or `removeFolderChildren`).
The subtree case is interesting: what happens if you delete a folder with subfolders on another device, then update a bookmark in one of those subfolders locally? In that case, uploading the changed bookmark and its parent is insufficient; we need to reupload the entire subtree, so that the bookmark isn't permanently orphaned.
The more common case is having locally added a bookmark to a remotely deleted folder. 

On iOS we resolve this by putting orphans under their grandparents, walking up the tree until we find a non-deleted folder.

I would not recommend restoring subtrees; I can see the bug reports now…
Priority: -- → P2
Assignee: nobody → tchiovoloni
Priority: P2 → P1
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

Getting this up for feedback. It should have more tests (TPS hardly ever runs...), but I'm more concerned with the general approach and whether or not its actually what we want to do. (Also, if it is, if this is a sane approach for implementing it -- it appears to work, but I'm not sure if we want to move some more of the work into PlacesSyncUtils, etc)

The most concerning bit, to me, is the bit where I avoid re-uploading a changed folder, and my comment should describe more or less why -- we probably want to do so in certain cases (but not most cases?).
Attachment #8796243 - Flags: feedback?(rnewman)
Attachment #8796243 - Flags: feedback?(kcambridge)
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

https://reviewboard.mozilla.org/r/82146/#review80782

We talked about this a bunch on IRC.

I'd like to see a big block comment that outlines the things we want to have happen:

- Goal: don't lose a bookmark that a user has created and not explicitly deleted.
- Goal: don't undelete a bookmark without ensuring that the server structure correctly includes it.
- Approach:
  - Non-folders are deleted before folder deletions, so that when we process folder deletions we know the correct state.
  - Remote deletions always win for folders, but do not result in recursive deletion of children. This is a hack because we're not able to distinguish between value changes and structural changes to folders, and we don't even have the old server record to compare to.
  - When a folder is deleted, its remaining children are moved in order to their closest living ancestor.
  - Remote deletions can lose for non-folders, but only until we handle bookmark restores correctly (removing stale state from the server).

I'm not sure how I feel about adding so much logic to handle reviving, but I accept that some is needed to avoid badness after restoring from a backup.

I would suggest, for clarity, queuing non-folder and folder deletions, applying each in turn.

::: services/sync/modules/engines.js:1383
(Diff revision 1)
>                        "Applying the youngest record.");
> +      if (!remoteIsNewer) {
> +        if (!this._canRevive(item)) {
> +          return true;
> +        }
> +        this._cancelRemoteDelete(item);

For clarity, `return false` here?

::: services/sync/modules/engines/bookmarks.js:786
(Diff revision 1)
>        return;
>      }
>  
>      let guid = BookmarkSpecialIds.syncIDToPlacesGUID(record.id);
> +    let hasLocalChildren = Async.promiseSpinningly(
> +      PlacesSyncUtils.bookmarks.fetchChildGuids(guid)).length > 0;

Does this return 0 for non-folders? We want to delete non-folders immediately.
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

https://reviewboard.mozilla.org/r/82146/#review80786

::: services/sync/modules/engines/bookmarks.js:786
(Diff revision 1)
>        return;
>      }
>  
>      let guid = BookmarkSpecialIds.syncIDToPlacesGUID(record.id);
> +    let hasLocalChildren = Async.promiseSpinningly(
> +      PlacesSyncUtils.bookmarks.fetchChildGuids(guid)).length > 0;

I should say: we want to delete non-folders before folders.

::: services/sync/modules/engines/bookmarks.js:871
(Diff revision 1)
> +          parentGuid: grandparentGuid
> +        })))
> +        .then(() => PlacesSyncUtils.bookmarks.remove(guid, {
> +          preventRemovalOfNonEmptyFolders: true
> +        }))
> +        .then(() => grandparentSyncID)

This is a great place to also return all of the child GUIDs.

::: services/sync/modules/engines/bookmarks.js:878
(Diff revision 1)
> +    });
> +
> +    return Promise.all(promises).then(grandparentGuids =>
> +      // Remove duplicate guids and nulls (which occur if the grandparent didn't
> +      // need modification, or an error occured during deletion), and items that
> +      // we expect to have deleted by now.

You also need to include the GUIDs of the children you had to move -- they need to be reuploaded with their new parentid. The tracker won't have seen the moves because we're in the middle of a sync.
Attachment #8796243 - Flags: feedback?(rnewman)
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

Still needs tests, now all types of deletion are buffered.

I also changed the code deleting pending folders to no longer try to delete them all at the same time (e.g. no more Promise.all), since I don't think this is safe given the moves. This made making it use Task.async clearer.
Attachment #8796243 - Flags: feedback?(rnewman)
Attachment #8796243 - Flags: feedback?(kcambridge)
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

https://reviewboard.mozilla.org/r/82146/#review80782

> I'd like to see a big block comment that outlines the things we want to have happen...

Added, roughly mirroring what you said. (I'm not sure the placement of it is great though -- it's a little tough since the changes are spread out)

> I'm not sure how I feel about adding so much logic to handle reviving, but I accept that some is needed to avoid badness after restoring from a backup.

Well, AIUI most of the logic is useful more generally, e.g. its not hard to come up with scenarios where we do the wrong thing due to recursive delete even if we ignore reviving.

> Does this return 0 for non-folders? We want to delete non-folders immediately.

This code is gone now but yes, non-folders and empty folders were updated immediately. In retrospect, this was probably the wrong call, and handling empty folders with the rest of the folders seems more sensible to me.
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

https://reviewboard.mozilla.org/r/82146/#review81420

f+

::: services/sync/modules/bookmark_utils.js:47
(Diff revision 2)
>      },
>    };
>  });
>  
> +// Ideally this wouldn't be necessary, see bug 1295521
> +const PLACES_MOBILE_GUID = "mobile______";

Now I remember why `placesGUIDToSyncID` doesn't exist: mobile isn't a real root yet, so its GUID is a random string, not `mobile______`. So we end up having to create it just to get its GUID, when we're most likely looking up a non-root. Might be better to just inline the `findMobileRoot` call into `placesGUIDToSyncID`. I hope that bug can land soon, it's blocked on bug 1302901.

::: services/sync/modules/engines.js:1380
(Diff revision 2)
>        // delete. There might be a good reason for that delete and it might be
>        // enexpected for this client to restore that data.
>        this._log.trace("Incoming record is deleted but we had local changes. " +
>                        "Applying the youngest record.");
> -      return remoteIsNewer;
> +      if (!remoteIsNewer) {
> +        if (!this._canRevive(item)) {

Some trace logging here would be great, since we no longer unconditionally take the newest record.

Minor style nit: what do you think about inverting the condition and returning early if `remoteIsNewer`? I missed the `!` on the first couple of reads, so I thought `_canRevive` should return false at first, even though it's correct the way it is.

::: services/sync/modules/engines/bookmarks.js:412
(Diff revision 2)
> +  // server has the folder as deleted, we *would* want to reupload this folder.
> +  // This is mitigated by the fact that we move any undeleted children to the
> +  // grandparent when deleting the parent.
> +  _canRevive(item) {
> +    let placesGuid = BookmarkSpecialIds.syncIDToPlacesGUID(item.id);
> +    let kind = Async.promiseSpinningly(

Using `item.type` (actually the kind, because gratuitous differences) would save a spin here.

::: services/sync/modules/engines/bookmarks.js:428
(Diff revision 2)
> +      this._log.error("Remote delete cancelled for unmodified item: " + item.id);
> +      return;
> +    }
> +    let localID = this._store.idForGUID(item.id);
> +    let localParentID = PlacesUtils.bookmarks.getFolderIdForItem(localID);
> +    let localParentSyncID = this._store.GUIDForId(localParentID);

Ugh, I'm sorry about all the converting back and forth. ETOOMANYIDS.

::: services/sync/modules/engines/bookmarks.js:783
(Diff revision 2)
>        this._log.warn("Refusing to remove special folder " + record.id);
>        return;
>      }
>  
>      let guid = BookmarkSpecialIds.syncIDToPlacesGUID(record.id);
> -    try {
> +    let recordKind = Async.promiseSpinningly(

Ditto, `record.type`.

::: services/sync/modules/engines/bookmarks.js:839
(Diff revision 2)
> +  //
> +  // - Additions, moves, and updates are processed before deletions.
> +  //     - To do this, all deletion operations are buffered during a sync. Folders
> +  //       we plan on deleting have their sync id's stored in `this._foldersToDelete`,
> +  //       and non-folders we plan on deleting have their sync id's stored in
> +  //       `this._atomsToDelete`.

Nice choice on the name! We definitely overuse "record", "item", "bookmark".

::: services/sync/modules/engines/bookmarks.js:845
(Diff revision 2)
> +  //     - The exception to this is the moves that occur to fix the order of bookmark
> +  //       children, which are performed after we process deletions.
> +  // - Non-folders are deleted before folder deletions, so that when we process
> +  //   folder deletions we know the correct state.
> +  // - Remote deletions always win for folders, but do not result in recursive
> +  //   deletion of children. This is a hack because we're not able to distinguish

And, if a folder was deleted remotely, we've already processed the deletes for its children at this point, right?

::: services/sync/modules/engines/bookmarks.js:875
(Diff revision 2)
> +        preventRemovalOfNonEmptyFolders: true
> +      });
> +      this._log.debug(`Removed item ${guid} with type ${info.type}`);
> +    } catch (ex) {
> +      // Likely already removed.
> +      this._log.debug(`Error removing ${record.id}`, ex);

s/record.id/guid

::: services/sync/modules/engines/bookmarks.js:886
(Diff revision 2)
> +      [...this._atomsToDelete.values()]
> +        .map(syncID => this._deleteAtom(
> +          BookmarkSpecialIds.syncIDToPlacesGUID(syncID))));
> +  },
> +
> +  // Returns an array of sync ids that need updates.

I was thinking it might be nice to have this live in PSU, but it's your call. It's really specific to the bookmarks engine's internals, and I guess the return value wouldn't be very specific ("potential changed IDs"?).

::: services/sync/modules/engines/bookmarks.js:919
(Diff revision 2)
> +      yield Promise.all(childGuids.map(child => PlacesSyncUtils.bookmarks.update({
> +        guid: child,
> +        parentGuid: grandparentGuid
> +      })));
> +
> +      // Delete the (now empty) parent

Aside: eventually, it might make sense to ban or buffer bookmark changes made while syncing. This should all happen in one transaction, so that the user doesn't add another bookmark to the folder while we're making it consistent.

Of course, the chances of that happening are probably so remote that we can recover on the next sync if this throws (I think?).

::: services/sync/modules/engines/bookmarks.js:927
(Diff revision 2)
> +      });
> +
> +      // Add children (for parentid) and grandparent (for children list) to set
> +      // of records needing an update, *unless* they're marked for deletion.
> +      if (!this._foldersToDelete.has(grandparentSyncID)) {
> +        needUpdate.add(grandparentSyncID);

Just so I understand: this is in case we're removing a subtree, but we saw the child folder first, right?
Attachment #8796243 - Flags: feedback?(kcambridge) → feedback+
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

https://reviewboard.mozilla.org/r/82146/#review81634

I thought I better get my head around this :)

::: services/sync/modules/engines.js:1372
(Diff revision 2)
>          this._log.trace("Applying incoming delete because the local item " +
>                          "exists and isn't modified.");
>          return true;
>        }
>  
>        // TODO As part of bug 720592, determine whether we should do more here.

ISTM that we are doing exactly what this comment says (ie, this bug is basically a dupe of 720592)? This comment should probably be updated to briefly explain that reconcilliation of deleted items is complicated and needs to be under the control of the individual engines.

::: services/sync/modules/engines/bookmarks.js:395
(Diff revision 2)
> +  _deletePending() {
> +    this._log.debug("About to delete pending items");
> +    // Delete pending items -- See the comment above BookmarkStore's deletePending
> +    let newlyModified = Async.promiseSpinningly(this._store.deletePending());
> +    let now = this._tracker._now();
> +    this._log.debug("Deleted pending items, marking " + newlyModified.length +

this logging seems a little noisy - the one above should probably be removed or .trace. Here, I think .debug("Deleting pending items", newlyModified) is all we need (and does less work in the off-chance .debug logs aren't enabled - but they are by default) and only logs one line in the usual case of no such items.

::: services/sync/modules/engines/bookmarks.js:404
(Diff revision 2)
> +        this._modified.set(modifiedSyncID, { timestamp: now, deleted: false });
> +      }
> +    }
> +  },
> +
> +  // We avoid reviving folder since reviving them properly would require

nit - folder*s*

::: services/sync/modules/engines/bookmarks.js:421
(Diff revision 2)
> +
> +  _cancelRemoteDelete(item) {
> +    // In addition to preventing the deletion of this record (handled by the caller),
> +    // we need to mark the parent of this record for uploading next sync, in order
> +    // to ensure its children array is accurate.
> +    let modifiedTimestamp = this._modified.getModifiedTimestamp(item.id);

consider passing localAge and remoteAge?

::: services/sync/modules/engines/bookmarks.js:430
(Diff revision 2)
> +    }
> +    let localID = this._store.idForGUID(item.id);
> +    let localParentID = PlacesUtils.bookmarks.getFolderIdForItem(localID);
> +    let localParentSyncID = this._store.GUIDForId(localParentID);
> +
> +    if (!this._modified.has(localParentSyncID)) {

is this check needed? ISTM it must always be false given we've checked modifiedTimestamp (and even with that passed in, I think we can make assumptions about null vs a timestamp)

(I also idly wonder what this means to Kit's tracker patch - I guess he just bumps the change counter?)

::: services/sync/modules/engines/bookmarks.js:829
(Diff revision 2)
>      Async.promiseSpinningly(Promise.all(promises));
>    },
>  
> +  // There's some complexity here around pending deletions. Our goals:
> +  //
> +  // - Don't delete any bookmarks a user has created but not explicitly deleted

I know this mainly came from Richard, but I'm just making sure I understand this correctly...

So this line is a little vague - eg, if a user locally deletes a folder, it's ambigious if that qualifies as "explicitly deleted" for the folder children. I think we are trying to say something like "any bookmarks locally created since the last Sync and where this Sync flags the parent as deleted"? Or maybe "Any bookmark that was not a child of the folder at the time the deletion was recorded"?

::: services/sync/modules/engines/bookmarks.js:891
(Diff revision 2)
> +  // Returns an array of sync ids that need updates.
> +  _deletePendingFolders: Task.async(function* _deletePendingFolders() {
> +    // To avoid data loss, we don't want to just delete the folder outright,
> +    // so we buffer folder deletions and process them at the end (now).
> +    //
> +    // At this point, any member in the folder that remains is either a folder

I don't understand why it's important to have non-folders before folders, but not folders-inside-folders before top-level folders? Although I admit I still haven't fully got my head around this patch and I'm done for the night :)
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

https://reviewboard.mozilla.org/r/82146/#review81674

::: services/sync/modules/engines.js:1381
(Diff revision 2)
>        // enexpected for this client to restore that data.
>        this._log.trace("Incoming record is deleted but we had local changes. " +
>                        "Applying the youngest record.");
> -      return remoteIsNewer;
> +      if (!remoteIsNewer) {
> +        if (!this._canRevive(item)) {
> +          return true;

[damn - reviewboard farted - I also added, but reviewboard lost:]

OTOH though, I actually find these semantics difficult to comprehend (ie, I kinda need to understand what those 2 methods do and how they interact to piece things together). Using a single method, say, `this._maybeRevive` makes it clear that there's one place I can go where all will be explained :)
(In reply to Mark Hammond [:markh] from comment #11)

> So this line is a little vague - eg, if a user locally deletes a folder,
> it's ambigious if that qualifies as "explicitly deleted" for the folder
> children. I think we are trying to say something like "any bookmarks locally
> created since the last Sync and where this Sync flags the parent as
> deleted"? Or maybe "Any bookmark that was not a child of the folder at the
> time the deletion was recorded"?

Yeah, pretty much. The key is that the user deleted that folder on some device… and on that device they explicitly deleted the children of that folder on that device at that instant. (As opposed to _implicitly_ deleting all future or elsewhere children of a folder with the same GUID, which is the alternative interpretation.)

Sync will sync aaaalllll of those deletions, which is why I call them 'explicit', and so we shouldn't recursively delete folders, and we can expect to have local changes to the deleted tree and its contents.


> I don't understand why it's important to have non-folders before folders,
> but not folders-inside-folders before top-level folders? Although I admit I
> still haven't fully got my head around this patch and I'm done for the night
> :)

Strictly speaking it should be non-folders first, then folders leaves-to-root. That way we can check for any orphans, collapsing up the tree as we find that the world disagrees with our expectations.

Because we don't construct a tree on desktop, we can't easily do the latter, so a reasonable approach is:

* Delete non-folders, because it makes everything else simpler.
* For every folder we wish to delete:
  * If it contains children that are not in our list of folders still to delete, then we need to rescue those orphans.
  * Anything else in it must be a child folder that's in our list of folders still to delete. Recurse down them applying the same logic at each level of the tree, collecting orphans upward.
* Finally, we've moved all non-deleted children out of each deleted folder, and we can delete all such folders.

The last two stages should occur in a single database transaction, and multiple potential implementation strategies exist.
Most of the time these trees will be shallow, and orphans few, so it's simple enough.

You can see some of this logic here:

https://github.com/mozilla-mobile/firefox-ios/blob/master/Sync/Synchronizers/Bookmarks/ThreeWayTreeMerger.swift#L283
https://github.com/mozilla-mobile/firefox-ios/blob/master/Sync/Synchronizers/Bookmarks/ThreeWayTreeMerger.swift#L382
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

https://reviewboard.mozilla.org/r/82146/#review81420

> Using `item.type` (actually the kind, because gratuitous differences) would save a spin here.

This is guaranteed to be a deleted item from the server, so type is always "item", unfortunately.

> Ditto, `record.type`.

Ditto, it's often type === "item" (the default applyIncoming does `if (record.deleted) this.remove(record);`)

> And, if a folder was deleted remotely, we've already processed the deletes for its children at this point, right?

Yes, most likely.  (Although maybe we could do something better here now that the patch we're buffering deletions of non-folders as well).

> I was thinking it might be nice to have this live in PSU, but it's your call. It's really specific to the bookmarks engine's internals, and I guess the return value wouldn't be very specific ("potential changed IDs"?).

It seems a bit too specific to me. But if theres a performance reason to do so (such as reusing the connection or something) I'd be totally willing to move it.

> Aside: eventually, it might make sense to ban or buffer bookmark changes made while syncing. This should all happen in one transaction, so that the user doesn't add another bookmark to the folder while we're making it consistent.
> 
> Of course, the chances of that happening are probably so remote that we can recover on the next sync if this throws (I think?).

We discussed this some (with :rnewman) online. Ideally, this would be done in a transaction, but after some messing around with it, I think this would be too much work (it would require either refactoring a bunch of places's Bookmarks.jsm or duplicating a bunch of code...)

I'm instead opting to just catch the error, and revive the parent if this happens. It's really very unlikely that this will happen, and we won't have any corruption if we do it this way.

> Just so I understand: this is in case we're removing a subtree, but we saw the child folder first, right?

Yes.
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

https://reviewboard.mozilla.org/r/82146/#review81634

> ISTM that we are doing exactly what this comment says (ie, this bug is basically a dupe of 720592)? This comment should probably be updated to briefly explain that reconcilliation of deleted items is complicated and needs to be under the control of the individual engines.

Nice catch.

> consider passing localAge and remoteAge?

Sorry, I'm not sure I follow why?

> is this check needed? ISTM it must always be false given we've checked modifiedTimestamp (and even with that passed in, I think we can make assumptions about null vs a timestamp)
> 
> (I also idly wonder what this means to Kit's tracker patch - I guess he just bumps the change counter?)

I think so -- note that we're checking the parent here. My concern is overwriting the `deleted` flag.

> I don't understand why it's important to have non-folders before folders, but not folders-inside-folders before top-level folders? Although I admit I still haven't fully got my head around this patch and I'm done for the night :)

The order folders are processed in shouldn't matter, although in an ideal world I think we'd go from the leaves up.
rnewman, markh: Probably the most critical thing to note here (read: the thing that concerns me the most) is that we aren't actually updating these in a DB transaction -- Unfortunately, PlacesUtils isn't set up for calling its functions inside transactions, and making it well behaved when this is going on is unfortunately not trivial (duplicating the queries would probably be simpler, but maintaining correct semantics while doing so results in a lot of code).

This seems to me to be enough of an edge case where that complexity isn't warranted, especially as there's a way to handle that edge case without introducing corruption (e.g. without having the client and server diverge). Adding logic to retry this GUID also occured to me, but ultimately we'd need to do something if our retry failed.

(Perhaps I'm underestimating the importance of this, however, and it's worth the additional complexity)

P.S. :kitcambridge, I didn't assign you as a reviewer since two reviewers is probably enough, and you gave f+. (Hopefully that's not a faux pas of some sort)
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

https://reviewboard.mozilla.org/r/82146/#review82284

::: services/sync/modules/bookmark_utils.js:118
(Diff revision 3)
>  
> +  placesGUIDToSyncID(g) {
> +    if (g in PlacesGUIDToSpecialGUID) {
> +      return PlacesGUIDToSpecialGUID[g];
> +    }
> +    if (BookmarkSpecialIds.findMobileRoot(false) === g) {

Sadly, `findMobileRoot` returns an ID; you'll want to convert `g` to an ID.

::: services/sync/modules/engines/bookmarks.js:397
(Diff revision 3)
> +    let newlyModified = Async.promiseSpinningly(this._store.deletePending());
> +    let now = this._tracker._now();
> +    this._log.debug("Deleted pending items", newlyModified);
> +    for (let modifiedSyncID of newlyModified) {
> +      if (!this._modified.has(modifiedSyncID)) {
> +        this._modified.set(modifiedSyncID, { timestamp: now, deleted: false });

I wonder if we need to add these (and `localParentSyncID` below) to the tracker, too. What happens if Sync is interrupted after we've moved children from the deleted parent, but before (or during) upload. Since we don't add these back, is it possible we'll miss uploading the new children?
(In reply to Thom Chiovoloni [:tcsc] from comment #15)
> > Ditto, `record.type`.
> 
> Ditto, it's often type === "item" (the default applyIncoming does `if
> (record.deleted) this.remove(record);`)

Derp. :-P Sorry for the noise.

> It seems a bit too specific to me. But if theres a performance reason to do
> so (such as reusing the connection or something) I'd be totally willing to
> move it.

Makes sense. There's no performance benefit or connection reuse, so let's keep it here.

(In reply to Thom Chiovoloni [:tcsc] from comment #17)
> P.S. :kitcambridge, I didn't assign you as a reviewer since two reviewers is
> probably enough, and you gave f+. (Hopefully that's not a faux pas of some
> sort)

Not at all! I left a couple of drive-by comments, but I don't think I need to be a reviewer; f+ and two reviewers are plenty.
Status: NEW → ASSIGNED
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

https://reviewboard.mozilla.org/r/82146/#review82284

> I wonder if we need to add these (and `localParentSyncID` below) to the tracker, too. What happens if Sync is interrupted after we've moved children from the deleted parent, but before (or during) upload. Since we don't add these back, is it possible we'll miss uploading the new children?

Not sure. Wouldn't this mean we reupload them in the next sync as well?
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

https://reviewboard.mozilla.org/r/82146/#review84648

I'm a little concerned about how bad Places's one-by-one API will make our perf, but if Mark's happy with it, I'm happy.

::: services/sync/modules/engines.js:1287
(Diff revision 4)
> +  // changed it more recently than the deletion. If we return false, the
> +  // record will be deleted locally. If we return true, we'll reupload the
> +  // record to the server -- any extra work that's needed as part of this
> +  // process should be done at this point (such as mark the record's parent
> +  // for reuploading in the case of bookmarks).
> +  _maybeRevive(remoteItem) {

This should be named `_shouldReviveRemotelyDeletedRecord`, or something similarly descriptive. "maybe" doesn't tell you want returning true or false means.

This is yet another indicator that engines.js should die. It's silly to have all of these stub methods.

::: services/sync/modules/engines.js:1369
(Diff revision 4)
>          return true;
>        }
> +      this._log.trace("Incoming record is deleted but we had local changes.");
>  
> -      // TODO As part of bug 720592, determine whether we should do more here.
> -      // In the case where the local changes are newer, it is quite possible
> +      if (remoteIsNewer) {
> +        this._log.trace("Remote record is newer -- deleting local record.");

Why not move all of this logic into `_maybeRevive`?

::: services/sync/modules/engines/bookmarks.js:794
(Diff revision 4)
> +  //   folder deletions we know the correct state.
> +  // - Remote deletions always win for folders, but do not result in recursive
> +  //   deletion of children. This is a hack because we're not able to distinguish
> +  //   between value changes and structural changes to folders, and we don't even
> +  //   have the old server record to compare to. See `BookmarkEngine`'s
> +  //   `_maybeRevive` method.

Naming tweak needs to apply here, too.

::: services/sync/modules/engines/bookmarks.js:830
(Diff revision 4)
> +  }),
> +
> +  _deletePendingAtoms() {
> +    return Promise.all(
> +      [...this._atomsToDelete.values()]
> +        .map(syncID => this._deleteAtom(syncID)));

`bookmarks.remove` is a bit of a pain point, isn't it? We're doing one async task and three function calls and one database transaction for each pending item. A big remote delete is going to trash perf and hammer the disk, all because there's no `removeAll` API.

::: services/sync/modules/engines/bookmarks.js:841
(Diff revision 4)
> +    // so we buffer folder deletions and process them at the end (now).
> +    //
> +    // At this point, any member in the folder that remains is either a folder
> +    // pending deletion (which we'll get to in this function), or an item that
> +    // should not be deleted. To avoid deleting these items, we move them to
> +    // the parent of the folder we're about to delete first.

"we first move them… about to delete".

::: services/sync/modules/engines/bookmarks.js:855
(Diff revision 4)
> +      // We could avoid some redundant work here by finding the nearest
> +      // grandparent who isn't present in `this._toDelete`...
> +
> +      let grandparentSyncId = this.GUIDForId(
> +        PlacesUtils.bookmarks.getFolderIdForItem(
> +          this.idForGUID(PlacesSyncUtils.bookmarks.syncIdToGuid(syncId))));

Do we really not already have the GUID at this point? Four function calls to get each grandparent? Yikes.

::: services/sync/modules/engines/bookmarks.js:873
(Diff revision 4)
> +        yield PlacesSyncUtils.bookmarks.remove(syncId, {
> +          preventRemovalOfNonEmptyFolders: true
> +        });
> +      } catch (e) {
> +        // Someone added something to this folder between when we got the
> +        // children and now. This is unlikely, but possible. To avoid corruption

Or the database is corrupt. Or something else happened…
Attachment #8796243 - Flags: review?(rnewman) → review+
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

https://reviewboard.mozilla.org/r/82146/#review84648

> This should be named `_shouldReviveRemotelyDeletedRecord`, or something similarly descriptive. "maybe" doesn't tell you want returning true or false means.
> 
> This is yet another indicator that engines.js should die. It's silly to have all of these stub methods.

Fair point, and I've renamed it (OTOH the name there doesn't make it clear that it can/should change state).

> Why not move all of this logic into `_maybeRevive`?

I mean, we could, but I don't see a strong benefit to doing so (its not like this logic can be reused)... 

And we'd either need to duplicate the logic or call the base class's implementation in each subclass, which doesn't seem great to me.

> Do we really not already have the GUID at this point? Four function calls to get each grandparent? Yikes.

We really don't. A aggregate API would be nice, but its a bit tricky since the operations here can change what the result would be.
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

https://reviewboard.mozilla.org/r/82146/#review85414

Sorry for the delay :( This LGTM, especially with the enhanced tracker on the horizon. Thanks.

::: services/sync/modules/engines/bookmarks.js:508
(Diff revision 5)
> +    // In addition to preventing the deletion of this record (handled by the caller),
> +    // we need to mark the parent of this record for uploading next sync, in order
> +    // to ensure its children array is accurate.
> +    let modifiedTimestamp = this._modified.getModifiedTimestamp(item.id);
> +    if (!modifiedTimestamp) {
> +      this._log.error("Remote delete cancelled for unmodified item: " + item.id);

this message looks misleading - we aren't "cancelling" the remote delete when we return false. Also, we should tweak the comment why this is a log.error (ie, that the caller should have already checked the item is modified) - as it reads it sound like it is an expected state.

IOW, something like "we should only ever be called for items marked as locally modified, so something strange is going on - play it safe and don't attempt reviving it."

::: services/sync/modules/engines/bookmarks.js:513
(Diff revision 5)
> +      this._log.error("Remote delete cancelled for unmodified item: " + item.id);
> +      // There's no reason to revive the record if the record isn't actually modified.
> +      return false;
> +    }
> +
> +    let localID = this._store.idForGUID(item.id);

I think another log.trace here indicating we are revising it makes sense.
Attachment #8796243 - Flags: review?(markh) → review+
Comment on attachment 8796243 [details]
Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync

https://reviewboard.mozilla.org/r/82146/#review82284

> Not sure. Wouldn't this mean we reupload them in the next sync as well?

Wouldn't quite work (discussed in irc), opened followup bug 1311837 about this.
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3a971c05a7e3
Reupload parents of revived bookmarks and buffer folder deletion during sync r=markh,rnewman
https://hg.mozilla.org/mozilla-central/rev/3a971c05a7e3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.