728928 - Handle orphaned bookmarks if a folder is improperly deleted

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect, P5)

Description

[Lucas Rocha (:lucasr)]

A follow from bug 723841.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

How oten do we think this will happen? Is this a blocker?

Comment 2

[Richard Newman [:rnewman]]

Can't happen from Fennec, because Fennec doesn't yet allow folder deletions.

Orphaning could happen any time a sync involves deleting a folder before its children -- most likely that'll fail due to consistency constraints. That's a follow-up for me.

I don't know exactly which condition Lucas is proposing to resolve here: leftovers in the DB from earlier Fennec builds? "Deletion" in the sense of IS_DELETED = 1 (which won't trigger consistency constraints)? Something else?

Comment 3

[Lucas Rocha (:lucasr)]

Stepping back a bit: I don't think fix is necessary anymore as we guarantee consistency through the foreign key constraint now. And no, there's no leftovers that need fixing. Closing as INVALID. Richard, feel free to re-open if there are any cases where sync would still need this feature.

Comment 4

[Richard Newman [:rnewman]]

I'll handle the Sync side of stuff in Bug 724740.

Lucas, are you sure about consistency? What if I take this:

  folder A    ID 10    Parent 1      deleted = 0
  item B      ID 11    Parent 10     deleted = 0

and set folder A's deleted flag to 1? It's not inconsistent -- there's still a row in the DB with ID 10 -- but item B won't be reachable or visible in the UI despite not being itself deleted.

When the pruner comes along, it'll attempt to delete the folder, but it'll fail because the removal of the row would violate a constraint.

Do you already address this situation?

It's not an issue right now because Fennec can't delete folders yet, but I don't believe it's fair to say that the foreign key constraint addresses this situation, and the bug could rise again when folder deletion becomes possible.

I could imagine that you would want to:

* Recursively mark children of 'deleted' folders as deleted within the CP (unless, of course, the caller moves them first)

or

* Ensure that the pruner deletes bookmarks before folders (giving you a basic level of avoidance), and is prepared to handle a SQL consistency exception without falling over.
* Eventually, do bottom-up pruning, or investigate an alternative deletion mechanism, such as the password manager's deleted items table.

or

* Assert that callers are responsible for recursively deleting children; alter the deletion logic to refuse to delete folders that have non-deleted children.

or

* Really, really, really assert that callers are responsible for this kind of consistency, and do nothing at all to catch problems early.

Thoughts?

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Richard Newman [:rnewman] from comment #4)
> I'll handle the Sync side of stuff in Bug 724740.
> 
> Lucas, are you sure about consistency? What if I take this:
> 
>   folder A    ID 10    Parent 1      deleted = 0
>   item B      ID 11    Parent 10     deleted = 0
> 
> and set folder A's deleted flag to 1? It's not inconsistent -- there's still
> a row in the DB with ID 10 -- but item B won't be reachable or visible in
> the UI despite not being itself deleted.
> 
> When the pruner comes along, it'll attempt to delete the folder, but it'll
> fail because the removal of the row would violate a constraint.
> 
> Do you already address this situation?
> 
> It's not an issue right now because Fennec can't delete folders yet, but I
> don't believe it's fair to say that the foreign key constraint addresses
> this situation, and the bug could rise again when folder deletion becomes
> possible.
> 
> I could imagine that you would want to:
> 
> * Recursively mark children of 'deleted' folders as deleted within the CP
> (unless, of course, the caller moves them first)
> 
> or
> 
> * Ensure that the pruner deletes bookmarks before folders (giving you a
> basic level of avoidance), and is prepared to handle a SQL consistency
> exception without falling over.
> * Eventually, do bottom-up pruning, or investigate an alternative deletion
> mechanism, such as the password manager's deleted items table.
> 
> or
> 
> * Assert that callers are responsible for recursively deleting children;
> alter the deletion logic to refuse to delete folders that have non-deleted
> children.
> 
> or
> 
> * Really, really, really assert that callers are responsible for this kind
> of consistency, and do nothing at all to catch problems early.
> 
> Thoughts?

You're right, I missed the delete-as-update-DELETE=1 case. I think we should just fail if the DELETE=1 update will lead to orphan bookmarks which would be consistent with the actual delete case.

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

Minus on the assumption that this is future work. If this is needed for the initial release please renom and explain why this needs to block beta or release.

Comment 7

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 8

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.