Closed Bug 1162650 Opened 5 years ago Closed 4 years ago

IS_DELETED check missing while searching for a bookmark in isBookmark

Categories

(Firefox for Android :: Data Providers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: vivek, Assigned: vivek, Mentored)

Details

Attachments

(1 file)

This is a follow up to bug https://bugzilla.mozilla.org/show_bug.cgi?id=822979#c6
While checking if a bookmark exists the condition must also check if that bookmark is not deleted.
rnewman, am I right about this?
Flags: needinfo?(rnewman)
Good question.

I think this is looking at the wrong side of a buggy coin :)

Deleted items only need a GUID and a modified time: they don't need to keep their URL, because Sync only cares about the GUID when processing deletions.

That means they shouldn't be found by isBookmark (because it's doing the lookup by URL), so it doesn't need to check for deletion.


You can see that we clear the row correctly for history:

        debug("Marking history entry as deleted for URI: " + uri);

        ContentValues values = new ContentValues();
        values.put(History.IS_DELETED, 1);

        // Wipe sensitive data.
        values.putNull(History.TITLE);
        values.put(History.URL, "");          // Column is NOT NULL.
        values.put(History.DATE_CREATED, 0);
        values.put(History.DATE_LAST_VISITED, 0);
        values.put(History.VISITS, 0);
        values.put(History.DATE_MODIFIED, System.currentTimeMillis());

but we don't do it for bookmarks:

        debug("Marking bookmarks as deleted for URI: " + uri);

        ContentValues values = new ContentValues();
        values.put(Bookmarks.IS_DELETED, 1);


I don't remember a good reason for this; perhaps Nick or someone else remembers. If we can't find a good reason, that's the approach I'd take: morph this bug to be unrooting and clearing bookmarks on deletion, rather than just flipping the deleted flag.
Flags: needinfo?(rnewman)
Component: General → Data Providers
> but we don't do it for bookmarks:
> 
>         debug("Marking bookmarks as deleted for URI: " + uri);
> 
>         ContentValues values = new ContentValues();
>         values.put(Bookmarks.IS_DELETED, 1);
> 
> 
> I don't remember a good reason for this; perhaps Nick or someone else
> remembers.

Nah, I don't recall anything here.  It's my opinion that having strong column constraints (NOT NULL, for examples) and a pre-check for is_deleted is better than weakening the column constraints so that we can have is_deleted = 1 and URL = NULL, for example.

 If we can't find a good reason, that's the approach I'd take:
> morph this bug to be unrooting and clearing bookmarks on deletion, rather
> than just flipping the deleted flag.

Unrooting, I agree; clearing, less sure.  I'd rather make sure we avoid things marked deleted outright.  See comment above.
(In reply to Nick Alexander :nalexander from comment #3)
> > but we don't do it for bookmarks:
> > 
> >         debug("Marking bookmarks as deleted for URI: " + uri);
> > 
> >         ContentValues values = new ContentValues();
> >         values.put(Bookmarks.IS_DELETED, 1);
> > 
> > 
> > I don't remember a good reason for this; perhaps Nick or someone else
> > remembers.
> 
> Nah, I don't recall anything here.  It's my opinion that having strong
> column constraints (NOT NULL, for examples) and a pre-check for is_deleted
> is better than weakening the column constraints so that we can have
> is_deleted = 1 and URL = NULL, for example.
> 
>  If we can't find a good reason, that's the approach I'd take:
> > morph this bug to be unrooting and clearing bookmarks on deletion, rather
> > than just flipping the deleted flag.
> 
> Unrooting, I agree; clearing, less sure.  I'd rather make sure we avoid
> things marked deleted outright.  See comment above.

vivek: unrooting means to take the branch of the bookmarks tree that is deleted (this might be just the bookmark, or a folder and children, or ...) and move it out into "Unsorted bookmarks" or something like that.  Chop out the bit of the tree just deleted, put it in temporary storage, and then mark it deleted.
(In reply to Nick Alexander :nalexander from comment #3)
> > but we don't do it for bookmarks:
> > 
> >         debug("Marking bookmarks as deleted for URI: " + uri);
> > 
> >         ContentValues values = new ContentValues();
> >         values.put(Bookmarks.IS_DELETED, 1);
> > 
> > 
> > I don't remember a good reason for this; perhaps Nick or someone else
> > remembers.
> 
> Nah, I don't recall anything here.  It's my opinion that having strong
> column constraints (NOT NULL, for examples) and a pre-check for is_deleted
> is better than weakening the column constraints so that we can have
> is_deleted = 1 and URL = NULL, for example.

Re-reading, scratch this.  My suggestion is more likely to make the UI show deleted bookmarks safely; nulling things out will make it clearer there's an issue (either by crashing or showing empty data).

So I concur with rnewman: flip the flag and clear the bookmark's data (save for the GUID).  Sync should delete the record when appropriate.
(In reply to Nick Alexander :nalexander from comment #5)
> (In reply to Nick Alexander :nalexander from comment #3)
> > > but we don't do it for bookmarks:
> > > 
> > >         debug("Marking bookmarks as deleted for URI: " + uri);
> > > 
> > >         ContentValues values = new ContentValues();
> > >         values.put(Bookmarks.IS_DELETED, 1);
> > > 
> > > 
> > > I don't remember a good reason for this; perhaps Nick or someone else
> > > remembers.
> > 
> > Nah, I don't recall anything here.  It's my opinion that having strong
> > column constraints (NOT NULL, for examples) and a pre-check for is_deleted
> > is better than weakening the column constraints so that we can have
> > is_deleted = 1 and URL = NULL, for example.
> 
> Re-reading, scratch this.  My suggestion is more likely to make the UI show
> deleted bookmarks safely; nulling things out will make it clearer there's an
> issue (either by crashing or showing empty data).
> 
> So I concur with rnewman: flip the flag and clear the bookmark's data (save
> for the GUID).  Sync should delete the record when appropriate.

And in this context, unrooting would mean setting the parent to be... "mobile bookmarks"?  rnewman?
Flags: needinfo?(rnewman)
The parent column allows nulls, as does the URL and everything but position and GUID. So:

        values.put(Bookmarks.IS_DELETED, 1);
        values.put(Bookmarks.POSITION, 0);
        values.putNull(Bookmarks.PARENT);
        values.putNull(Bookmarks.URL);
        values.putNull(Bookmarks.TITLE);
        values.putNull(Bookmarks.DESCRIPTION);
        values.putNull(Bookmarks.KEYWORD);
        values.putNull(Bookmarks.TAGS);
        values.putNull(Bookmarks.FAVICON_ID);


After all, this is a deleted bookmark; it doesn't have a URL or a parent.

(Obviously, more work is needed than this for folder deletion; that involves recursion. I don't think we allow folder deletion yet, but I might be wrong.)
Flags: needinfo?(rnewman)
Attached patch 1162650.patchSplinter Review
Unrooted and cleared bookmarks on deletion.
BrowserProvider tests updated to verify this behaviour
Try logs : https://treeherder.mozilla.org/#/jobs?repo=try&revision=76306fbf2d36
Attachment #8615102 - Flags: review?(rnewman)
Assignee: nobody → vivekb.balakrishnan
Status: NEW → ASSIGNED
Comment on attachment 8615102 [details] [diff] [review]
1162650.patch

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

I think this is right!
Attachment #8615102 - Flags: review?(rnewman) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ddc4611e79f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.